Adding URI segment specific encoding (#882)

* Adding URI segment specific encoding

Fixes #879

URI encoding introduced in Feign 10.x was refactored to be more in line
with URI and URI Template specifications respectively.  One change was to
ensure that certain reserved characters were not encoded incorrectly.

The result was that path segment specific reserved characters were being
preserved on the query string as well.  This change updates the `UriTemplate`
and `Expression` classes to recognize the segment of the URI that is being processed
and apply the segment specific encoding correctly.

One important change regarding the `+` sign.  Per the URI specification, a `+` sign
is allowed in both the path and query segments of a URI, however, handling of
the symbol on the query can be inconsistent.  In some legacy systems, the `+` is
equivalent to the a space.  Feign takes the approach of modern systems, where a
`+` symbol should not reprsent a space and is explicitly encoded as `%2B` when
found on a query string.

If you wish to use `+` as a space, then use the literal ` ` character or encode
the value directly as `%20`

Author: kdavisk6

README.md

@@ -113,6 +113,14 @@ See [Advanced Usage](#advanced-usage) for more examples.
 >
 > `@RequestLine` and `@QueryMap` templates do not encode slash `/` characters by default.  To change this behavior, set the `decodeSlash` property on the `@RequestLine` to `false`.  
 
+> **What about plus? `+`**
+>
+> Per the URI specification, a `+` sign is allowed in both the path and query segments of a URI, however, handling of
+> the symbol on the query can be inconsistent.  In some legacy systems, the `+` is equivalent to the a space.  Feign takes the approach of modern systems, where a
+> `+` symbol should not represent a space and is explicitly encoded as `%2B` when found on a query string.
+>
+> If you wish to use `+` as a space, then use the literal ` ` character or encode the value directly as `%20`
+ 
 ##### Custom Expansion
 
 The `@Param` annotation has an optional property `expander` allowing for complete control over the individual parameter's expansion.

core/src/main/java/feign/template/Expressions.java

@@ -14,6 +14,7 @@
 package feign.template;
 
 import feign.Util;
+import feign.template.UriUtils.FragmentType;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -28,11 +29,10 @@ public final class Expressions {
 
   static {
     expressions = new LinkedHashMap<>();
-    expressions.put(Pattern.compile("\\+(\\w[-\\w.]*[ ]*)(:(.+))?"), ReservedExpression.class);
     expressions.put(Pattern.compile("(\\w[-\\w.]*[ ]*)(:(.+))?"), SimpleExpression.class);
   }
 
-  public static Expression create(final String value) {
+  public static Expression create(final String value, final FragmentType type) {
 
     /* remove the start and end braces */
     final String expression = stripBraces(value);
@@ -53,7 +53,6 @@ public static Expression create(final String value) {
 
     Entry<Pattern, Class<? extends Expression>> matchedExpression = matchedExpressionEntry.get();
     Pattern expressionPattern = matchedExpression.getKey();
-    Class<? extends Expression> expressionType = matchedExpression.getValue();
 
     /* create a new regular expression matcher for the expression */
     String variableName = null;
@@ -68,7 +67,7 @@ public static Expression create(final String value) {
       }
     }
 
-    return new SimpleExpression(variableName, variablePattern);
+    return new SimpleExpression(variableName, variablePattern, type);
   }
 
   private static String stripBraces(String expression) {
@@ -81,36 +80,21 @@ private static String stripBraces(String expression) {
     return expression;
   }
 
-  /**
-   * Expression that does not encode reserved characters. This expression adheres to RFC 6570
-   * <a href="https://tools.ietf.org/html/rfc6570#section-3.2.3">Reserved Expansion (Level 2)</a>
-   * specification.
-   */
-  public static class ReservedExpression extends SimpleExpression {
-    private final String RESERVED_CHARACTERS = ":/?#[]@!$&\'()*+,;=";
-
-    ReservedExpression(String expression, String pattern) {
-      super(expression, pattern);
-    }
-
-    @Override
-    String encode(Object value) {
-      return UriUtils.encodeReserved(value.toString(), RESERVED_CHARACTERS, Util.UTF_8);
-    }
-  }
-
   /**
    * Expression that adheres to Simple String Expansion as outlined in <a
    * href="https://tools.ietf.org/html/rfc6570#section-3.2.2>Simple String Expansion (Level 1)</a>
    */
   static class SimpleExpression extends Expression {
 
-    SimpleExpression(String expression, String pattern) {
+    private final FragmentType type;
+
+    SimpleExpression(String expression, String pattern, FragmentType type) {
       super(expression, pattern);
+      this.type = type;
     }
 
     String encode(Object value) {
-      return UriUtils.encode(value.toString(), Util.UTF_8);
+      return UriUtils.encodeReserved(value.toString(), type, Util.UTF_8);
     }
 
     @Override

core/src/main/java/feign/template/Template.java

@@ -13,6 +13,7 @@
  */
 package feign.template;
 
+import feign.template.UriUtils.FragmentType;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.List;
@@ -80,9 +81,9 @@ public String expand(Map<String, ?> variables) {
         Object value = variables.get(expression.getName());
         if (value != null) {
           String expanded = expression.expand(value, this.encode.isEncodingRequired());
-          if (!this.encodeSlash) {
+          if (this.encodeSlash) {
             logger.fine("Explicit slash decoding specified, decoding all slashes in uri");
-            expanded = expanded.replaceAll("\\%2F", "/");
+            expanded = expanded.replaceAll("/", "%2F");
           }
           resolved.append(expanded);
         } else {
@@ -200,7 +201,9 @@ private void parseFragment(String fragment, boolean query) {
 
       if (chunk.startsWith("{")) {
         /* it's an expression, defer encoding until resolution */
-        Expression expression = Expressions.create(chunk);
+        FragmentType type = (query) ? FragmentType.QUERY : FragmentType.PATH_SEGMENT;
+
+        Expression expression = Expressions.create(chunk, type);
         if (expression == null) {
           this.templateChunks.add(Literal.create(encode(chunk, query)));
         } else {