Adding Support for Query Parameter Name Expansion (#841)

* Adding Support for Query Parameter Name Expansion Fixes #838 `QueryTemplate` assumed that all query names were literals. This change adds support for Expressions in Query Parameter names, providing better adherence to RFC 6570. RequestLines such as `@RequestLine("GET /uri?{parameter}={value}")` are now fully expanded whereas before, only `{value}` would be. * Adding Encoding and Resolution Enums for Template Control These new enums replace the boolean values used to control encoding and expression expansion options in Templates * Allow unresolved expressions in Query Parameter Name and Body Template Expressions in Query Parameter names and in a Body Template will now no longer be removed if they are not resolved. For Query Template, this change will prevent invalid query name/value pairs from being generated. For the Body Template, the documentation states that unresolved should be preserved, yet the code did not match.
OpenFeign · Nov 15, 2018 · a69c2af · a69c2af
1 parent afe673e
commit a69c2af
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 28 deletions.
diff --git a/core/src/main/java/feign/template/BodyTemplate.java b/core/src/main/java/feign/template/BodyTemplate.java
@@ -34,7 +34,7 @@ public static BodyTemplate create(String template) {
   }
 
   private BodyTemplate(String value, Charset charset) {
-    super(value, false, false, false, charset);
+    super(value, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.NOT_REQUIRED, false, charset);
   }
 
   @Override

diff --git a/core/src/main/java/feign/template/Expression.java b/core/src/main/java/feign/template/Expression.java
@@ -64,4 +64,9 @@ public String getValue() {
     }
     return "{" + this.name + "}";
   }
+
+  @Override
+  public String toString() {
+    return this.getValue();
+  }
 }
diff --git a/core/src/main/java/feign/template/HeaderTemplate.java b/core/src/main/java/feign/template/HeaderTemplate.java
@@ -79,7 +79,7 @@ public static HeaderTemplate append(HeaderTemplate headerTemplate, Iterable<Stri
    * @param template to parse.
    */
   private HeaderTemplate(String template, String name, Iterable<String> values, Charset charset) {
-    super(template, false, false, false, charset);
+    super(template, ExpansionOptions.REQUIRED, EncodingOptions.NOT_REQUIRED, false, charset);
     this.values =
         StreamSupport.stream(values.spliterator(), false)
             .filter(Util::isNotBlank)

diff --git a/core/src/main/java/feign/template/QueryTemplate.java b/core/src/main/java/feign/template/QueryTemplate.java
@@ -30,7 +30,7 @@ public final class QueryTemplate extends Template {
 
   /* cache a copy of the variables for lookup later */
   private List<String> values;
-  private final String name;
+  private final Template name;
   private final CollectionFormat collectionFormat;
   private boolean pure = false;
 
@@ -115,8 +115,10 @@ private QueryTemplate(
       Iterable<String> values,
       Charset charset,
       CollectionFormat collectionFormat) {
-    super(template, false, true, true, charset);
-    this.name = name;
+    super(template, ExpansionOptions.REQUIRED, EncodingOptions.REQUIRED, true, charset);
+    this.name =
+        new Template(
+            name, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.REQUIRED, false, charset);
     this.collectionFormat = collectionFormat;
     this.values =
         StreamSupport.stream(values.spliterator(), false)
@@ -133,12 +135,12 @@ public List<String> getValues() {
   }
 
   public String getName() {
-    return name;
+    return name.toString();
   }
 
   @Override
   public String toString() {
-    return this.queryString(super.toString());
+    return this.queryString(this.name.toString(), super.toString());
   }
 
   /**
@@ -150,20 +152,21 @@ public String toString() {
    */
   @Override
   public String expand(Map<String, ?> variables) {
-    return this.queryString(super.expand(variables));
+    String name = this.name.expand(variables);
+    return this.queryString(name, super.expand(variables));
   }
 
-  private String queryString(String values) {
+  private String queryString(String name, String values) {
     if (this.pure) {
-      return this.name;
+      return name;
     }
 
     /* covert the comma separated values into a value query string */
     List<String> resolved =
         Arrays.stream(values.split(",")).filter(Util::isNotBlank).collect(Collectors.toList());
 
     if (!resolved.isEmpty()) {
-      return this.collectionFormat.join(this.name, resolved, this.getCharset()).toString();
+      return this.collectionFormat.join(name, resolved, this.getCharset()).toString();
     }
 
     /* nothing to return, all values are unresolved */

diff --git a/core/src/main/java/feign/template/Template.java b/core/src/main/java/feign/template/Template.java
@@ -28,13 +28,13 @@
  * href="https://tools.ietf.org/html/rfc6570">RFC 6570</a>, with some relaxed rules, allowing the
  * concept to be used in areas outside of the uri.
  */
-public abstract class Template {
+public class Template {
 
   private static final Logger logger = Logger.getLogger(Template.class.getName());
   private static final Pattern QUERY_STRING_PATTERN = Pattern.compile("(?<!\\{)(\\?)");
   private final String template;
   private final boolean allowUnresolved;
-  private final boolean encode;
+  private final EncodingOptions encode;
   private final boolean encodeSlash;
   private final Charset charset;
   private final List<TemplateChunk> templateChunks = new ArrayList<>();
@@ -48,12 +48,16 @@ public abstract class Template {
    * @param encodeSlash if slash characters should be encoded.
    */
   Template(
-      String value, boolean allowUnresolved, boolean encode, boolean encodeSlash, Charset charset) {
+      String value,
+      ExpansionOptions allowUnresolved,
+      EncodingOptions encode,
+      boolean encodeSlash,
+      Charset charset) {
     if (value == null) {
       throw new IllegalArgumentException("template is required.");
     }
     this.template = value;
-    this.allowUnresolved = allowUnresolved;
+    this.allowUnresolved = ExpansionOptions.ALLOW_UNRESOLVED == allowUnresolved;
     this.encode = encode;
     this.encodeSlash = encodeSlash;
     this.charset = charset;
@@ -78,7 +82,7 @@ public String expand(Map<String, ?> variables) {
         Expression expression = (Expression) chunk;
         Object value = variables.get(expression.getName());
         if (value != null) {
-          String expanded = expression.expand(value, this.encode);
+          String expanded = expression.expand(value, this.encode.isEncodingRequired());
           if (!this.encodeSlash) {
             logger.fine("Explicit slash decoding specified, decoding all slashes in uri");
             expanded = expanded.replaceAll("\\%2F", "/");
@@ -105,7 +109,7 @@ public String expand(Map<String, ?> variables) {
    * @return the encoded value.
    */
   private String encode(String value) {
-    return this.encode ? UriUtils.encode(value, this.charset) : value;
+    return this.encode.isEncodingRequired() ? UriUtils.encode(value, this.charset) : value;
   }
 
   /**
@@ -116,7 +120,7 @@ private String encode(String value) {
    * @return the encoded value
    */
   private String encode(String value, boolean query) {
-    if (this.encode) {
+    if (this.encode.isEncodingRequired()) {
       return query
           ? UriUtils.queryEncode(value, this.charset)
           : UriUtils.pathEncode(value, this.charset);
@@ -216,15 +220,11 @@ public String toString() {
     return this.templateChunks.stream().map(TemplateChunk::getValue).collect(Collectors.joining());
   }
 
-  public boolean allowUnresolved() {
-    return allowUnresolved;
-  }
-
   public boolean encode() {
-    return encode;
+    return encode.isEncodingRequired();
   }
 
-  public boolean encodeSlash() {
+  boolean encodeSlash() {
     return encodeSlash;
   }
 
@@ -312,4 +312,24 @@ public String next() {
       throw new IllegalStateException("No More Elements");
     }
   }
+
+  public enum EncodingOptions {
+    REQUIRED(true),
+    NOT_REQUIRED(false);
+
+    private boolean shouldEncode;
+
+    EncodingOptions(boolean shouldEncode) {
+      this.shouldEncode = shouldEncode;
+    }
+
+    public boolean isEncodingRequired() {
+      return this.shouldEncode;
+    }
+  }
+
+  public enum ExpansionOptions {
+    ALLOW_UNRESOLVED,
+    REQUIRED
+  }
 }
diff --git a/core/src/main/java/feign/template/UriTemplate.java b/core/src/main/java/feign/template/UriTemplate.java
@@ -70,6 +70,6 @@ public static UriTemplate append(UriTemplate uriTemplate, String fragment) {
    * @param charset to use when encoding.
    */
   private UriTemplate(String template, boolean encodeSlash, Charset charset) {
-    super(template, false, true, encodeSlash, charset);
+    super(template, ExpansionOptions.REQUIRED, EncodingOptions.REQUIRED, encodeSlash, charset);
   }
 }
diff --git a/core/src/test/java/feign/DefaultContractTest.java b/core/src/test/java/feign/DefaultContractTest.java
@@ -132,7 +132,9 @@ public void headersOnMethodAddsContentTypeHeader() throws Exception {
     assertThat(md.template())
         .hasHeaders(
             entry("Content-Type", asList("application/xml")),
-            entry("Content-Length", asList(String.valueOf(md.template().body().length))));
+            entry(
+                "Content-Length",
+                asList(String.valueOf(md.template().requestBody().asBytes().length))));
   }
 
   @Test
@@ -142,7 +144,9 @@ public void headersOnTypeAddsContentTypeHeader() throws Exception {
     assertThat(md.template())
         .hasHeaders(
             entry("Content-Type", asList("application/xml")),
-            entry("Content-Length", asList(String.valueOf(md.template().body().length))));
+            entry(
+                "Content-Length",
+                asList(String.valueOf(md.template().requestBody().asBytes().length))));
   }
 
   @Test
@@ -152,7 +156,9 @@ public void headersContainsWhitespaces() throws Exception {
     assertThat(md.template())
         .hasHeaders(
             entry("Content-Type", asList("application/xml")),
-            entry("Content-Length", asList(String.valueOf(md.template().body().length))));
+            entry(
+                "Content-Length",
+                asList(String.valueOf(md.template().requestBody().asBytes().length))));
   }
 
   @Test

diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -70,4 +70,35 @@ public void collectionFormat() {
     String expanded = template.expand(Collections.emptyMap());
     assertThat(expanded).isEqualToIgnoringCase("name=James,Jason");
   }
+
+  @Test
+  public void expandName() {
+    QueryTemplate template =
+        QueryTemplate.create("{name}", Arrays.asList("James", "Jason"), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("name", "firsts"));
+    assertThat(expanded).isEqualToIgnoringCase("firsts=James&firsts=Jason");
+  }
+
+  @Test
+  public void expandPureParameter() {
+    QueryTemplate template = QueryTemplate.create("{name}", Collections.emptyList(), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("name", "firsts"));
+    assertThat(expanded).isEqualToIgnoringCase("firsts");
+  }
+
+  @Test
+  public void expandPureParameterWithSlash() {
+    QueryTemplate template =
+        QueryTemplate.create("/path/{name}", Collections.emptyList(), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("name", "firsts"));
+    assertThat(expanded).isEqualToIgnoringCase("/path/firsts");
+  }
+
+  @Test
+  public void expandNameUnresolved() {
+    QueryTemplate template =
+        QueryTemplate.create("{parameter}", Arrays.asList("James", "Jason"), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("name", "firsts"));
+    assertThat(expanded).isEqualToIgnoringCase("%7Bparameter%7D=James&%7Bparameter%7D=Jason");
+  }
 }