Implements possibility to disable @Param url encoding (#465)

CHANGELOG.md

@@ -1,5 +1,6 @@
 ### Version 9.3
 * Adds `FallbackFactory`, allowing access to the cause of a Hystrix fallback
+* Adds support for encoded parameters via `@Param(encoded = true)`
 
 ### Version 9.2
 * Adds Hystrix `SetterFactory` to customize group and command keys

core/src/main/java/feign/Contract.java

@@ -249,19 +249,18 @@ public interface Contract {
       for (Annotation annotation : annotations) {
         Class<? extends Annotation> annotationType = annotation.annotationType();
         if (annotationType == Param.class) {
-          String name = ((Param) annotation).value();
-          checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.",
-              paramIndex);
+          Param paramAnnotation = (Param) annotation;
+          String name = paramAnnotation.value();
+          checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.", paramIndex);
           nameParam(data, name, paramIndex);
-          if (annotationType == Param.class) {
-            Class<? extends Param.Expander> expander = ((Param) annotation).expander();
-            if (expander != Param.ToStringExpander.class) {
-              data.indexToExpanderClass().put(paramIndex, expander);
-            }
+          Class<? extends Param.Expander> expander = paramAnnotation.expander();
+          if (expander != Param.ToStringExpander.class) {
+            data.indexToExpanderClass().put(paramIndex, expander);
           }
+          data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
           isHttpAnnotation = true;
           String varName = '{' + name + '}';
-          if (data.template().url().indexOf(varName) == -1 &&
+          if (!data.template().url().contains(varName) &&
               !searchMapValuesContainsSubstring(data.template().queries(), varName) &&
               !searchMapValuesContainsSubstring(data.template().headers(), varName)) {
             data.formParams().add(name);
@@ -289,7 +288,7 @@ public interface Contract {
 
       for (Collection<String> entry : values) {
         for (String value : entry) {
-          if (value.indexOf(search) != -1) {
+          if (value.contains(search)) {
             return true;
           }
         }

core/src/main/java/feign/MethodMetadata.java

@@ -42,6 +42,7 @@ public final class MethodMetadata implements Serializable {
       new LinkedHashMap<Integer, Collection<String>>();
   private Map<Integer, Class<? extends Expander>> indexToExpanderClass =
       new LinkedHashMap<Integer, Class<? extends Expander>>();
+  private Map<Integer, Boolean> indexToEncoded = new LinkedHashMap<Integer, Boolean>();
   private transient Map<Integer, Expander> indexToExpander;
 
   MethodMetadata() {
@@ -140,6 +141,10 @@ public final class MethodMetadata implements Serializable {
     return indexToName;
   }
 
+  public Map<Integer, Boolean> indexToEncoded() {
+    return indexToEncoded;
+  }
+
   /**
    * If {@link #indexToExpander} is null, classes here will be instantiated by newInstance.
    */

core/src/main/java/feign/Param.java

@@ -38,6 +38,14 @@ public @interface Param {
    */
   Class<? extends Expander> expander() default ToStringExpander.class;
 
+  /**
+   * Specifies whether argument is already encoded
+   * The value is ignored for headers (headers are never encoded)
+   *
+   * @see QueryMap#encoded
+   */
+  boolean encoded() default false;
+
   interface Expander {
 
     /**

core/src/main/java/feign/QueryMap.java

@@ -60,6 +60,10 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME;
 @Retention(RUNTIME)
 @java.lang.annotation.Target(PARAMETER)
 public @interface QueryMap {
-    /** Specifies whether parameter names and values are already encoded. */
+    /**
+     * Specifies whether parameter names and values are already encoded.
+     *
+     * @see Param#encoded
+     */
     boolean encoded() default false;
 }

core/src/main/java/feign/ReflectiveFeign.java

@@ -291,7 +291,15 @@ public class ReflectiveFeign extends Feign {
 
     protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable,
                                       Map<String, Object> variables) {
-      return mutable.resolve(variables);
+      // Resolving which variable names are already encoded using their indices
+      Map<String, Boolean> variableToEncoded = new LinkedHashMap<String, Boolean>();
+      for (Entry<Integer, Boolean> entry : metadata.indexToEncoded().entrySet()) {
+        Collection<String> names = metadata.indexToName().get(entry.getKey());
+        for (String name : names) {
+          variableToEncoded.put(name, entry.getValue());
+        }
+      }
+      return mutable.resolve(variables, variableToEncoded);
     }
   }
 

core/src/main/java/feign/RequestTemplate.java

@@ -117,7 +117,7 @@ public final class RequestTemplate implements Serializable {
     // skip expansion if there's no valid variables set. ex. {a} is the
     // first valid
     if (checkNotNull(template, "template").length() < 3) {
-      return template.toString();
+      return template;
     }
     checkNotNull(variables, "variables for %s", template);
 
@@ -200,21 +200,29 @@ public final class RequestTemplate implements Serializable {
     map.put(key, values);
   }
 
+  /** {@link #resolve(Map, Map)}, which assumes no parameter is encoded */
+  public RequestTemplate resolve(Map<String, ?> unencoded) {
+    return resolve(unencoded, Collections.<String, Boolean>emptyMap());
+  }
+
   /**
    * Resolves any template parameters in the requests path, query, or headers against the supplied
    * unencoded arguments. <br> <br><br><b>relationship to JAXRS 2.0</b><br> <br> This call is
    * similar to {@code javax.ws.rs.client.WebTarget.resolveTemplates(templateValues, true)} , except
    * that the template values apply to any part of the request, not just the URL
    */
-  public RequestTemplate resolve(Map<String, ?> unencoded) {
-    replaceQueryValues(unencoded);
+  RequestTemplate resolve(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {
+    replaceQueryValues(unencoded, alreadyEncoded);
     Map<String, String> encoded = new LinkedHashMap<String, String>();
     for (Entry<String, ?> entry : unencoded.entrySet()) {
-      encoded.put(entry.getKey(), urlEncode(String.valueOf(entry.getValue())));
+      final String key = entry.getKey();
+      final Object objectValue = entry.getValue();
+      String encodedValue = encodeValueIfNotEncoded(key, objectValue, alreadyEncoded);
+      encoded.put(key, encodedValue);
     }
     String resolvedUrl = expand(url.toString(), encoded).replace("+", "%20");
     if (decodeSlash) {
-    	resolvedUrl = resolvedUrl.replace("%2F", "/");
+      resolvedUrl = resolvedUrl.replace("%2F", "/");
     }
     url = new StringBuilder(resolvedUrl);
 
@@ -235,13 +243,21 @@ public final class RequestTemplate implements Serializable {
     return this;
   }
 
+  private String encodeValueIfNotEncoded(String key, Object objectValue, Map<String, Boolean> alreadyEncoded) {
+    String value = String.valueOf(objectValue);
+    final Boolean isEncoded = alreadyEncoded.get(key);
+    if (isEncoded == null || !isEncoded) {
+      value = urlEncode(value);
+    }
+    return value;
+  }
+
   /* roughly analogous to {@code javax.ws.rs.client.Target.request()}. */
   public Request request() {
     Map<String, Collection<String>> safeCopy = new LinkedHashMap<String, Collection<String>>();
     safeCopy.putAll(headers);
     return Request.create(
-        method,
-        new StringBuilder(url).append(queryLine()).toString(),
+        method, url + queryLine(),
         Collections.unmodifiableMap(safeCopy),
         body, charset
     );
@@ -589,11 +605,16 @@ public final class RequestTemplate implements Serializable {
     return request().toString();
   }
 
+  /** {@link #replaceQueryValues(Map, Map)}, which assumes no parameter is encoded */
+  public void replaceQueryValues(Map<String, ?> unencoded) {
+    replaceQueryValues(unencoded, Collections.<String, Boolean>emptyMap());
+  }
+
   /**
    * Replaces query values which are templated with corresponding values from the {@code unencoded}
    * map. Any unresolved queries are removed.
    */
-  public void replaceQueryValues(Map<String, ?> unencoded) {
+  void replaceQueryValues(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {
     Iterator<Entry<String, Collection<String>>> iterator = queries.entrySet().iterator();
     while (iterator.hasNext()) {
       Entry<String, Collection<String>> entry = iterator.next();
@@ -610,10 +631,12 @@ public final class RequestTemplate implements Serializable {
           }
           if (variableValue instanceof Iterable) {
             for (Object val : Iterable.class.cast(variableValue)) {
-              values.add(urlEncode(String.valueOf(val)));
+              String encodedValue = encodeValueIfNotEncoded(entry.getKey(), val, alreadyEncoded);
+              values.add(encodedValue);
             }
           } else {
-            values.add(urlEncode(String.valueOf(variableValue)));
+            String encodedValue = encodeValueIfNotEncoded(entry.getKey(), variableValue, alreadyEncoded);
+            values.add(encodedValue);
           }
         } else {
           values.add(value);

core/src/test/java/feign/FeignTest.java

@@ -646,6 +646,18 @@ public class FeignTest {
         .hasBody(expectedRequest);
   }
 
+  @Test
+  public void encodedQueryParam() throws Exception {
+    server.enqueue(new MockResponse());
+
+    TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
+
+    api.encodedQueryParam("5.2FSi+");
+
+    assertThat(server.takeRequest())
+            .hasPath("/?trim=5.2FSi+");
+  }
+
   interface TestInterface {
 
     @RequestLine("POST /")
@@ -704,6 +716,9 @@ public class FeignTest {
     @RequestLine("GET /?name={name}")
     void queryMapWithQueryParams(@Param("name") String name, @QueryMap Map<String, Object> queryMap);
 
+    @RequestLine("GET /?trim={trim}")
+    void encodedQueryParam(@Param(value = "trim", encoded = true) String trim);
+
     class DateToMillis implements Param.Expander {
 
       @Override

core/src/test/java/feign/RequestTemplateTest.java

@@ -36,21 +36,20 @@ public class RequestTemplateTest {
   /**
    * Avoid depending on guava solely for map literals.
    */
-  private static Map<String, Object> mapOf(String key, Object val) {
-    Map<String, Object> result = new LinkedHashMap<String, Object>();
+  private static <K, V> Map<K, V> mapOf(K key, V val) {
+    Map<K, V> result = new LinkedHashMap<K, V>();
     result.put(key, val);
     return result;
   }
 
-  private static Map<String, Object> mapOf(String k1, Object v1, String k2, Object v2) {
-    Map<String, Object> result = mapOf(k1, v1);
+  private static <K, V> Map<K, V> mapOf(K k1, V v1, K k2, V v2) {
+    Map<K, V> result = mapOf(k1, v1);
     result.put(k2, v2);
     return result;
   }
 
-  private static Map<String, Object> mapOf(String k1, Object v1, String k2, Object v2, String k3,
-                                           Object v3) {
-    Map<String, Object> result = mapOf(k1, v1, k2, v2);
+  private static <K, V> Map<K, V> mapOf(K k1, V v1, K k2, V v2, K k3, V v3) {
+    Map<K, V> result = mapOf(k1, v1, k2, v2);
     result.put(k3, v3);
     return result;
   }