Refactoring RequestTemplate to RFC6570 (#778)

* Refactoring RequestTemplate to RFC6570 This change refactors `RequestTemplate` in an attempt to adhere to the [RFC-6570 - URI Template](https://tools.ietf.org/html/rfc6570) specification more closely. The reason for this is to reduce the amount of inconsistency between `@Param`, `@QueryMap`, `@Header`, `@HeaderMap`, and `@Body` template expansion. First, `RequestTemplate` now delegates uri, header, query, and body template parsing to `UriTemplate`, `HeaderTemplate`, `QueryTemplate`, and `BodyTemplate` respectively. These components are all variations on a `Template`. `UriTemplate` adheres to RFC 6570 explicitly and supports Level 1 (Simple String) variable expansion. Unresolved variables are ignored and removed from the uri. This includes query parameter pairs. All literal and expanded variables are pct-encoded according to the Charset provided in the `RequestTemplate`. `HeaderTemplate` supports Level 1 (Simple String) variable expansion. Unresolved variables are ignored. Empty headers are removed. No encoding is performed. `QueryTemplate` is a subset of a `UriTemplate` and reacts in the same way. Unresolved pairs are ignored and not present on the final template. All literals and expanded variables are pct-encoded according to the Charset provided. `BodyTemplate` supports Level 1 (Simple String) variable expansion. Unresolved variables produce empty strings. Values are not encoded. All remaining customizations, including custom encoders, collection format expansion and charset encoding are still supportted and made backward compatible. Finally, a number of inconsistent methods on `RequestTemplate` have been deprecated for public use and all deprecated usage throughout the library has been replaced.
OpenFeign · Sep 23, 2018 · 2d761cb · 2d761cb
1 parent d7cc9b6
commit 2d761cb
Show file tree

Hide file tree

Showing 61 changed files with 3,039 additions and 1,014 deletions.
diff --git a/README.md b/README.md
diff --git a/benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java b/benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java
@@ -15,6 +15,7 @@
 
 import com.fasterxml.jackson.core.type.TypeReference;
 import feign.Request;
+import feign.Request.HttpMethod;
 import feign.Response;
 import feign.Util;
 import feign.codec.Decoder;
@@ -79,7 +80,7 @@ public void buildResponse() {
     response = Response.builder()
         .status(200)
         .reason("OK")
-        .request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
+        .request(Request.create(HttpMethod.GET, "/", Collections.emptyMap(), null, Util.UTF_8))
         .headers(Collections.emptyMap())
         .body(carsJson(Integer.valueOf(size)), Util.UTF_8)
         .build();

diff --git a/core/src/main/java/feign/CollectionFormat.java b/core/src/main/java/feign/CollectionFormat.java
@@ -13,6 +13,8 @@
  */
 package feign;
 
+import feign.template.UriUtils;
+import java.nio.charset.Charset;
 import java.util.Collection;
 
 /**
@@ -61,31 +63,32 @@ public enum CollectionFormat {
    *
    * @param field The field name corresponding to these values.
    * @param values A collection of value strings for the given field.
+   * @param charset to encode the sequence
    * @return The formatted char sequence of the field and joined values. If the value collection is
    *         empty, an empty char sequence will be returned.
    */
-  CharSequence join(String field, Collection<String> values) {
+  public CharSequence join(String field, Collection<String> values, Charset charset) {
     StringBuilder builder = new StringBuilder();
     int valueCount = 0;
     for (String value : values) {
       if (separator == null) {
         // exploded
         builder.append(valueCount++ == 0 ? "" : "&");
-        builder.append(field);
+        builder.append(UriUtils.queryEncode(field, charset));
         if (value != null) {
           builder.append('=');
-          builder.append(value);
+          builder.append(UriUtils.queryEncode(value, charset));
         }
       } else {
         // delimited with a separator character
         if (builder.length() == 0) {
-          builder.append(field);
+          builder.append(UriUtils.queryEncode(field, charset));
         }
         if (value == null) {
           continue;
         }
         builder.append(valueCount++ == 0 ? "=" : separator);
-        builder.append(value);
+        builder.append(UriUtils.queryEncode(value, charset));
       }
     }
     return builder;

diff --git a/core/src/main/java/feign/Contract.java b/core/src/main/java/feign/Contract.java
@@ -13,6 +13,9 @@
  */
 package feign;
 
+import static feign.Util.checkState;
+import static feign.Util.emptyToNull;
+import feign.Request.HttpMethod;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
@@ -24,8 +27,8 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import static feign.Util.checkState;
+import java.util.regex.Matcher;
-import static feign.Util.emptyToNull;
+import java.util.regex.Pattern;
 
 /**
  * Defines what annotations and values are valid on interfaces.
@@ -65,7 +68,7 @@ public List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
             metadata.configKey());
         result.put(metadata.configKey(), metadata);
       }
-      return new ArrayList<MethodMetadata>(result.values());
+      return new ArrayList<>(result.values());
     }
 
     /**
@@ -209,6 +212,9 @@ protected void nameParam(MethodMetadata data, String name, int i) {
   }
 
   class Default extends BaseContract {
+
+    static final Pattern REQUEST_LINE_PATTERN = Pattern.compile("^([A-Z]+)[ ]*(.*)$");
+
     @Override
     protected void processAnnotationOnClass(MethodMetadata data, Class<?> targetType) {
       if (targetType.isAnnotationPresent(Headers.class)) {
@@ -231,23 +237,16 @@ protected void processAnnotationOnMethod(MethodMetadata data,
         String requestLine = RequestLine.class.cast(methodAnnotation).value();
         checkState(emptyToNull(requestLine) != null,
             "RequestLine annotation was empty on method %s.", method.getName());
-        if (requestLine.indexOf(' ') == -1) {
+
-          checkState(requestLine.indexOf('/') == -1,
+        Matcher requestLineMatcher = REQUEST_LINE_PATTERN.matcher(requestLine);
-              "RequestLine annotation didn't start with an HTTP verb on method %s.",
+        if (!requestLineMatcher.find()) {
-              method.getName());
+          throw new IllegalStateException(String.format(
-          data.template().method(requestLine);
+              "RequestLine annotation didn't start with an HTTP verb on method %s",
-          return;
+              method.getName()));
-        }
-        data.template().method(requestLine.substring(0, requestLine.indexOf(' ')));
-        if (requestLine.indexOf(' ') == requestLine.lastIndexOf(' ')) {
-          // no HTTP version is ok
-          data.template().append(requestLine.substring(requestLine.indexOf(' ') + 1));
         } else {
-          // skip HTTP version
+          data.template().method(HttpMethod.valueOf(requestLineMatcher.group(1)));
-          data.template().append(
+          data.template().uri(requestLineMatcher.group(2));
-              requestLine.substring(requestLine.indexOf(' ') + 1, requestLine.lastIndexOf(' ')));
         }
-
         data.template().decodeSlash(RequestLine.class.cast(methodAnnotation).decodeSlash());
         data.template()
             .collectionFormat(RequestLine.class.cast(methodAnnotation).collectionFormat());
@@ -288,10 +287,7 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data,
           }
           data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
           isHttpAnnotation = true;
-          String varName = '{' + name + '}';
+          if (!data.template().hasRequestVariable(name)) {
-          if (!data.template().url().contains(varName) &&
-              !searchMapValuesContainsSubstring(data.template().queries(), varName) &&
-              !searchMapValuesContainsSubstring(data.template().headers(), varName)) {
             data.formParams().add(name);
           }
         } else if (annotationType == QueryMap.class) {
@@ -310,24 +306,6 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data,
       return isHttpAnnotation;
     }
 
-    private static <K, V> boolean searchMapValuesContainsSubstring(Map<K, Collection<String>> map,
-                                                                   String search) {
-      Collection<Collection<String>> values = map.values();
-      if (values == null) {
-        return false;
-      }
-
-      for (Collection<String> entry : values) {
-        for (String value : entry) {
-          if (value.contains(search)) {
-            return true;
-          }
-        }
-      }
-
-      return false;
-    }
-
     private static Map<String, Collection<String>> toMap(String[] input) {
       Map<String, Collection<String>> result =
           new LinkedHashMap<String, Collection<String>>(input.length);

diff --git a/core/src/main/java/feign/ReflectiveFeign.java b/core/src/main/java/feign/ReflectiveFeign.java
@@ -13,6 +13,7 @@
  */
 package feign;
 
+import feign.template.UriUtils;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
@@ -199,11 +200,11 @@ private BuildTemplateByResolvingArgs(MethodMetadata metadata, QueryMapEncoder qu
 
     @Override
     public RequestTemplate create(Object[] argv) {
-      RequestTemplate mutable = new RequestTemplate(metadata.template());
+      RequestTemplate mutable = RequestTemplate.from(metadata.template());
       if (metadata.urlIndex() != null) {
         int urlIndex = metadata.urlIndex();
         checkArgument(argv[urlIndex] != null, "URI parameter %s was null", urlIndex);
-        mutable.insert(0, String.valueOf(argv[urlIndex]));
+        mutable.target(String.valueOf(argv[urlIndex]));
       }
       Map<String, Object> varBuilder = new LinkedHashMap<String, Object>();
       for (Entry<Integer, Collection<String>> entry : metadata.indexToName().entrySet()) {
@@ -300,31 +301,22 @@ private RequestTemplate addQueryMapQueryParameters(Map<String, Object> queryMap,
             Object nextObject = iter.next();
             values.add(nextObject == null ? null
                 : encoded ? nextObject.toString()
-                    : RequestTemplate.urlEncode(nextObject.toString()));
+                    : UriUtils.encode(nextObject.toString()));
           }
         } else {
           values.add(currValue == null ? null
-              : encoded ? currValue.toString() : RequestTemplate.urlEncode(currValue.toString()));
+              : encoded ? currValue.toString() : UriUtils.encode(currValue.toString()));
         }
 
-        mutable.query(true,
+        mutable.query(encoded ? currEntry.getKey() : UriUtils.encode(currEntry.getKey()), values);
-            encoded ? currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
       }
       return mutable;
     }
 
     protected RequestTemplate resolve(Object[] argv,
                                       RequestTemplate mutable,
                                       Map<String, Object> variables) {
-      // Resolving which variable names are already encoded using their indices
+      return mutable.resolve(variables);
-      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);
     }
   }
 

diff --git a/core/src/main/java/feign/RequestLine.java b/core/src/main/java/feign/RequestLine.java
@@ -18,54 +18,10 @@
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 /**
- * Expands the request-line supplied in the {@code value}, permitting path and query variables, or
+ * Expands the uri template supplied in the {@code value}, permitting path and query variables, or
- * just the http method. <br>
+ * just the http method. Templates should conform to
- * 
+ * <a href="https://tools.ietf.org/html/rfc6570">RFC 6570</a>. Support is limited to Simple String
- * <pre>
+ * expansion and Reserved Expansion (Level 1 and Level 2) expressions.
- * ...
- * &#64;RequestLine("POST /servers")
- * ...
- *
- * &#64;RequestLine("GET /servers/{serverId}?count={count}")
- * void get(&#64;Param("serverId") String serverId, &#64;Param("count") int count);
- * ...
- *
- * &#64;RequestLine("GET")
- * Response getNext(URI nextLink);
- * ...
- * </pre>
- * 
- * HTTP version suffix is optional, but permitted. There are no guarantees this version will impact
- * that sent by the client. <br>
- * 
- * <pre>
- * &#64;RequestLine("POST /servers HTTP/1.1")
- * ...
- * </pre>
- * 
- * <br>
- * <strong>Note:</strong> Query params do not overwrite each other. All queries with the same name
- * will be included in the request. <br>
- * <br>
- * <b>Relationship to JAXRS</b><br>
- * <br>
- * The following two forms are identical. <br>
- * Feign:
- * 
- * <pre>
- * &#64;RequestLine("GET /servers/{serverId}?count={count}")
- * void get(&#64;Param("serverId") String serverId, &#64;Param("count") int count);
- * ...
- * </pre>
- * 
- * <br>
- * JAX-RS:
- * 
- * <pre>
- * &#64;GET &#64;Path("/servers/{serverId}")
- * void get(&#64;PathParam("serverId") String serverId, &#64;QueryParam("count") int count);
- * ...
- * </pre>
  */
 @java.lang.annotation.Target(METHOD)
 @Retention(RUNTIME)