Ensure Iterable values are encoded before template expansion

core/src/main/java/feign/CollectionFormat.java

@@ -74,21 +74,21 @@ public CharSequence join(String field, Collection<String> values, Charset charse
       if (separator == null) {
         // exploded
         builder.append(valueCount++ == 0 ? "" : "&");
-        builder.append(UriUtils.queryEncode(field, charset));
+        builder.append(UriUtils.encode(field, charset));
         if (value != null) {
           builder.append('=');
-          builder.append(UriUtils.queryEncode(value, charset));
+          builder.append(UriUtils.encode(value, charset));
         }
       } else {
         // delimited with a separator character
         if (builder.length() == 0) {
-          builder.append(UriUtils.queryEncode(field, charset));
+          builder.append(UriUtils.encode(field, charset));
         }
         if (value == null) {
           continue;
         }
         builder.append(valueCount++ == 0 ? "=" : separator);
-        builder.append(UriUtils.queryEncode(value, charset));
+        builder.append(UriUtils.encode(value, charset));
       }
     }
     return builder;

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

@@ -164,7 +164,10 @@ public RequestTemplate resolve(Map<String, ?> variables) {
       this.uriTemplate = UriTemplate.create("", !this.decodeSlash, this.charset);
     }
 
-    uri.append(this.uriTemplate.expand(variables));
+    String expanded = this.uriTemplate.expand(variables);
+    if (expanded != null) {
+      uri.append(expanded);
+    }
 
     /*
      * for simplicity, combine the queries into the uri and use the resulting uri to seed the

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

@@ -50,14 +50,9 @@ private BodyTemplate(String value, Charset charset) {
   public String expand(Map<String, ?> variables) {
     String expanded = super.expand(variables);
     if (this.json) {
-      /* decode only the first and last character */
-      StringBuilder sb = new StringBuilder();
-      sb.append(JSON_TOKEN_START);
-      sb.append(expanded,
-          expanded.indexOf(JSON_TOKEN_START_ENCODED) + JSON_TOKEN_START_ENCODED.length(),
-          expanded.lastIndexOf(JSON_TOKEN_END_ENCODED));
-      sb.append(JSON_TOKEN_END);
-      return sb.toString();
+      /* restore all start and end tokens */
+      expanded = expanded.replaceAll(JSON_TOKEN_START_ENCODED, JSON_TOKEN_START);
+      expanded = expanded.replaceAll(JSON_TOKEN_END_ENCODED, JSON_TOKEN_END);
     }
     return expanded;
   }

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

@@ -13,8 +13,8 @@
  */
 package feign.template;
 
+import feign.CollectionFormat;
 import java.util.Optional;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 /**

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

@@ -13,16 +13,13 @@
  */
 package feign.template;
 
-import feign.Util;
-import feign.template.UriUtils.FragmentType;
-import java.util.ArrayList;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import feign.Util;
 
 public final class Expressions {
   private static Map<Pattern, Class<? extends Expression>> expressions;
@@ -38,11 +35,12 @@ public final class Expressions {
      *
      * see https://tools.ietf.org/html/rfc6570#section-2.3 for more information.
      */
-    expressions.put(Pattern.compile("(\\w[-\\w.\\[\\]]*[ ]*)(:(.+))?"),
+
+    expressions.put(Pattern.compile("^([+#./;?&]?)(.*)$"),
         SimpleExpression.class);
   }
 
-  public static Expression create(final String value, final FragmentType type) {
+  public static Expression create(final String value) {
 
     /* remove the start and end braces */
     final String expression = stripBraces(value);
@@ -70,14 +68,22 @@ public static Expression create(final String value, final FragmentType type) {
     Matcher matcher = expressionPattern.matcher(expression);
     if (matcher.matches()) {
       /* we have a valid variable expression, extract the name from the first group */
-      variableName = matcher.group(1).trim();
-      if (matcher.group(2) != null && matcher.group(3) != null) {
-        /* this variable contains an optional pattern */
-        variablePattern = matcher.group(3);
+      variableName = matcher.group(2).trim();
+      if (variableName.contains(":")) {
+        /* split on the colon */
+        String[] parts = variableName.split(":");
+        variableName = parts[0];
+        variablePattern = parts[1];
+      }
+
+      /* look for nested expressions */
+      if (variableName.contains("{")) {
+        /* nested, literal */
+        return null;
       }
     }
 
-    return new SimpleExpression(variableName, variablePattern, type);
+    return new SimpleExpression(variableName, variablePattern);
   }
 
   private static String stripBraces(String expression) {
@@ -96,26 +102,19 @@ private static String stripBraces(String expression) {
    */
   static class SimpleExpression extends Expression {
 
-    private final FragmentType type;
-
-    SimpleExpression(String expression, String pattern, FragmentType type) {
+    SimpleExpression(String expression, String pattern) {
       super(expression, pattern);
-      this.type = type;
     }
 
     String encode(Object value) {
-      return UriUtils.encodeReserved(value.toString(), type, Util.UTF_8);
+      return UriUtils.encode(value.toString(), Util.UTF_8);
     }
 
     @Override
     String expand(Object variable, boolean encode) {
       StringBuilder expanded = new StringBuilder();
       if (Iterable.class.isAssignableFrom(variable.getClass())) {
-        List<String> items = new ArrayList<>();
-        for (Object item : ((Iterable) variable)) {
-          items.add((encode) ? encode(item) : item.toString());
-        }
-        expanded.append(String.join(Template.COLLECTION_DELIMITER, items));
+        expanded.append(this.expandIterable((Iterable<?>) variable));
       } else {
         expanded.append((encode) ? encode(variable) : variable);
       }
@@ -128,5 +127,38 @@ String expand(Object variable, boolean encode) {
       }
       return result;
     }
+
+
+    private String expandIterable(Iterable<?> values) {
+      StringBuilder result = new StringBuilder();
+      for (Object value : values) {
+        if (value == null) {
+          /* skip */
+          continue;
+        }
+
+        /* expand the value */
+        String expanded = this.encode(value);
+        if (expanded.isEmpty()) {
+          /* always append the separator */
+          result.append(",");
+        } else {
+          if (result.length() != 0) {
+            if (!result.toString().equalsIgnoreCase(",")) {
+              result.append(",");
+            }
+          }
+          result.append(expanded);
+        }
+      }
+
+      if (result.length() == 0) {
+        /* completely unresolved */
+        return null;
+      }
+
+      /* return the expanded value */
+      return result.toString();
+    }
   }
 }

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

@@ -13,26 +13,29 @@
  */
 package feign.template;
 
-import feign.CollectionFormat;
-import feign.Util;
 import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Iterator;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
+import feign.CollectionFormat;
+import feign.Util;
+import feign.template.Template.EncodingOptions;
+import feign.template.Template.ExpansionOptions;
 
 /**
  * Template for a Query String parameter.
  */
-public final class QueryTemplate extends Template {
+public final class QueryTemplate {
 
   private static final String UNDEF = "undef";
-  private List<String> values;
+  private List<Template> values;
   private final Template name;
   private final CollectionFormat collectionFormat;
   private boolean pure = false;
@@ -75,16 +78,7 @@ public static QueryTemplate create(String name,
         .filter(Util::isNotBlank)
         .collect(Collectors.toList());
 
-    StringBuilder template = new StringBuilder();
-    Iterator<String> iterator = remaining.iterator();
-    while (iterator.hasNext()) {
-      template.append(iterator.next());
-      if (iterator.hasNext()) {
-        template.append(COLLECTION_DELIMITER);
-      }
-    }
-
-    return new QueryTemplate(template.toString(), name, remaining, charset, collectionFormat);
+    return new QueryTemplate(name, remaining, charset, collectionFormat);
   }
 
   /**
@@ -101,31 +95,43 @@ public static QueryTemplate append(QueryTemplate queryTemplate,
     queryValues.addAll(StreamSupport.stream(values.spliterator(), false)
         .filter(Util::isNotBlank)
         .collect(Collectors.toList()));
-    return create(queryTemplate.getName(), queryValues, queryTemplate.getCharset(),
+    return create(queryTemplate.getName(), queryValues, StandardCharsets.UTF_8,
         collectionFormat);
   }
 
   /**
    * Create a new Query Template.
    *
-   * @param template for the Query String.
    * @param name of the query parameter.
    * @param values for the parameter.
    * @param collectionFormat to use.
    */
   private QueryTemplate(
-      String template,
       String name,
       Iterable<String> values,
       Charset charset,
       CollectionFormat collectionFormat) {
-    super(template, ExpansionOptions.REQUIRED, EncodingOptions.REQUIRED, true, charset);
+    this.values = new CopyOnWriteArrayList<>();
     this.name = new Template(name, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.REQUIRED,
         false, charset);
     this.collectionFormat = collectionFormat;
-    this.values = StreamSupport.stream(values.spliterator(), false)
-        .filter(Util::isNotBlank)
-        .collect(Collectors.toList());
+
+    /* parse each value into a template chunk for resolution later */
+    for (String value : values) {
+      if (value.isEmpty()) {
+        /* skip */
+        continue;
+      }
+
+      this.values.add(
+          new Template(
+              value,
+              ExpansionOptions.REQUIRED,
+              EncodingOptions.REQUIRED,
+              false,
+              charset));
+    }
+
     if (this.values.isEmpty()) {
       /* in this case, we have a pure parameter */
       this.pure = true;
@@ -134,7 +140,17 @@ private QueryTemplate(
   }
 
   public List<String> getValues() {
-    return values;
+    return Collections.unmodifiableList(this.values.stream()
+        .map(Template::toString)
+        .collect(Collectors.toList()));
+  }
+
+  public List<String> getVariables() {
+    List<String> variables = new ArrayList<>();
+    for (Template template : this.values) {
+      variables.addAll(template.getVariables());
+    }
+    return Collections.unmodifiableList(variables);
   }
 
   public String getName() {
@@ -143,7 +159,7 @@ public String getName() {
 
   @Override
   public String toString() {
-    return this.queryString(this.name.toString(), super.toString());
+    return this.queryString(this.name.toString(), this.getValues());
   }
 
   /**
@@ -153,39 +169,43 @@ public String toString() {
    * @param variables containing the values for expansion.
    * @return the expanded template.
    */
-  @Override
   public String expand(Map<String, ?> variables) {
     String name = this.name.expand(variables);
-    return this.queryString(name, super.expand(variables));
-  }
 
-  @Override
-  protected String resolveExpression(Expression expression, Map<String, ?> variables) {
-    if (variables.containsKey(expression.getName())) {
-      if (variables.get(expression.getName()) == null) {
-        /* explicit undefined */
-        return UNDEF;
+    if (this.pure) {
+      return name;
+    }
+
+    List<String> expanded = new ArrayList<>();
+    for (Template template : this.values) {
+      String result = template.expand(variables);
+      if (result == null) {
+        continue;
+      }
+
+      /*
+       * check for an iterable result, and if one is there, we need to split it into individual
+       * values
+       */
+      if (result.contains(",")) {
+        /* we need to split it */
+        expanded.addAll(Arrays.asList(result.split(",")));
+      } else {
+        expanded.add(result);
       }
-      return super.resolveExpression(expression, variables);
     }
 
-    /* mark the variable as undefined */
-    return UNDEF;
+    return this.queryString(name, Collections.unmodifiableList(expanded));
   }
 
-  private String queryString(String name, String values) {
+
+  private String queryString(String name, List<String> values) {
     if (this.pure) {
       return name;
     }
 
-    /* covert the comma separated values into a value query string */
-    List<String> resolved = Arrays.stream(values.split(COLLECTION_DELIMITER))
-        .filter(Objects::nonNull)
-        .filter(s -> !UNDEF.equalsIgnoreCase(s))
-        .collect(Collectors.toList());
-
-    if (!resolved.isEmpty()) {
-      return this.collectionFormat.join(name, resolved, this.getCharset()).toString();
+    if (!values.isEmpty()) {
+      return this.collectionFormat.join(name, values, StandardCharsets.UTF_8).toString();
     }
 
     /* nothing to return, all values are unresolved */