From c5870141eb285fdd21417f2bf47e435bc27c279e Mon Sep 17 00:00:00 2001
From: Kevin Davis <kdavisk6@gmail.com>
Date: Thu, 26 Dec 2019 14:21:49 -0500
Subject: [PATCH 1/5] Ensure Iterable values are encoded before template
 expansion

Fixes #1123, Fixes #1133, Fixes #1102, Fixes #1028

Ensures that all expressions are fully-encoded before being
manipulated during template expansion.  This allows parameters
to include reserved values and result in properly encoded
results.

Additionally, `Iterable` values are now handled in accordance
with RFC 6570 allowing for the specified `CollectionFormat` to
be applied and empty parameters to be expanded correctly as this
is the main use case that exhibited this issue.
---
 .../src/main/java/feign/CollectionFormat.java |   8 +-
 core/src/main/java/feign/RequestTemplate.java |   5 +-
 .../main/java/feign/template/Expression.java  |   2 +-
 .../main/java/feign/template/Expressions.java |  55 ++++--
 .../java/feign/template/QueryTemplate.java    | 112 ++++++-----
 .../main/java/feign/template/Template.java    | 107 ++++-------
 .../main/java/feign/template/UriUtils.java    | 176 ++++++------------
 .../test/java/feign/RequestTemplateTest.java  |   4 +-
 .../feign/template/QueryTemplateTest.java     |  13 +-
 .../java/feign/template/UriUtilsTest.java     |  71 ++-----
 10 files changed, 229 insertions(+), 324 deletions(-)

diff --git a/core/src/main/java/feign/CollectionFormat.java b/core/src/main/java/feign/CollectionFormat.java
index 14909da41..5623c5aa6 100644
--- a/core/src/main/java/feign/CollectionFormat.java
+++ b/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;
diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java
index bf089181c..facf969f2 100644
--- a/core/src/main/java/feign/RequestTemplate.java
+++ b/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
diff --git a/core/src/main/java/feign/template/Expression.java b/core/src/main/java/feign/template/Expression.java
index 3040aa1c3..f4852df8c 100644
--- a/core/src/main/java/feign/template/Expression.java
+++ b/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;
 
 /**
diff --git a/core/src/main/java/feign/template/Expressions.java b/core/src/main/java/feign/template/Expressions.java
index 96dfe02dc..87592621a 100644
--- a/core/src/main/java/feign/template/Expressions.java
+++ b/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;
@@ -42,7 +39,7 @@ public final class Expressions {
         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);
@@ -77,7 +74,7 @@ public static Expression create(final String value, final FragmentType type) {
       }
     }
 
-    return new SimpleExpression(variableName, variablePattern, type);
+    return new SimpleExpression(variableName, variablePattern);
   }
 
   private static String stripBraces(String expression) {
@@ -96,26 +93,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 +118,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();
+    }
   }
 }
diff --git a/core/src/main/java/feign/template/QueryTemplate.java b/core/src/main/java/feign/template/QueryTemplate.java
index c67735272..19e267c7d 100644
--- a/core/src/main/java/feign/template/QueryTemplate.java
+++ b/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 */
diff --git a/core/src/main/java/feign/template/Template.java b/core/src/main/java/feign/template/Template.java
index da58f0958..de7efb3fc 100644
--- a/core/src/main/java/feign/template/Template.java
+++ b/core/src/main/java/feign/template/Template.java
@@ -13,15 +13,12 @@
  */
 package feign.template;
 
-import feign.Util;
-import feign.template.UriUtils.FragmentType;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -32,13 +29,6 @@
  */
 public class Template {
 
-  /*
-   * special delimiter for collection based expansion, in an attempt to avoid accidental splitting
-   * for resolved values. semi-colon was chosen because it is a reserved character that must be
-   * pct-encoded and should not appear unencoded.
-   */
-  static final String COLLECTION_DELIMITER = ";";
-
   private static final Logger logger = Logger.getLogger(Template.class.getName());
   private static final Pattern QUERY_STRING_PATTERN = Pattern.compile("(?<!\\{)(\\?)");
   private final String template;
@@ -82,37 +72,53 @@ public String expand(Map<String, ?> variables) {
     }
 
     /* resolve all expressions within the template */
-    StringBuilder resolved = new StringBuilder();
+    StringBuilder resolved = null;
     for (TemplateChunk chunk : this.templateChunks) {
+      String expanded;
       if (chunk instanceof Expression) {
-        String resolvedExpression = this.resolveExpression((Expression) chunk, variables);
-        if (resolvedExpression != null) {
-          resolved.append(resolvedExpression);
-        }
+        expanded = this.resolveExpression((Expression) chunk, variables);
       } else {
         /* chunk is a literal value */
-        resolved.append(chunk.getValue());
+        expanded = chunk.getValue();
+      }
+      if (expanded == null) {
+        continue;
+      }
+
+      /* append it to the result */
+      if (resolved == null) {
+        resolved = new StringBuilder();
       }
+      resolved.append(expanded);
     }
+
+    if (resolved == null) {
+      /* entire template is unresolved */
+      return null;
+    }
+
     return resolved.toString();
   }
 
-  protected String resolveExpression(Expression expression, Map<String, ?> variables) {
+  protected String resolveExpression(
+                                     Expression expression,
+                                     Map<String, ?> variables) {
     String resolved = null;
     Object value = variables.get(expression.getName());
     if (value != null) {
-      String expanded = expression.expand(value, this.encode.isEncodingRequired());
-      if (Util.isNotBlank(expanded)) {
-        if (this.encodeSlash) {
+      String expanded = expression.expand(
+          value, this.encode.isEncodingRequired());
+      if (expanded != null) {
+        if (!this.encodeSlash) {
           logger.fine("Explicit slash decoding specified, decoding all slashes in uri");
-          expanded = expanded.replaceAll("/", "%2F");
+          expanded = expanded.replaceAll("%2F", "/");
         }
         resolved = expanded;
       }
     } else {
       if (this.allowUnresolved) {
         /* unresolved variables are treated as literals */
-        resolved = encode(expression.toString());
+        resolved = encodeLiteral(expression.toString());
       }
     }
     return resolved;
@@ -124,24 +130,8 @@ protected String resolveExpression(Expression expression, Map<String, ?> variabl
    * @param value to encode.
    * @return the encoded value.
    */
-  private String encode(String value) {
-    return this.encode.isEncodingRequired() ? UriUtils.encode(value, this.charset) : value;
-  }
-
-  /**
-   * Uri Encode the value.
-   *
-   * @param value to encode
-   * @param query indicating this value is on a query string.
-   * @return the encoded value
-   */
-  private String encode(String value, boolean query) {
-    if (this.encode.isEncodingRequired()) {
-      return query ? UriUtils.queryEncode(value, this.charset)
-          : UriUtils.pathEncode(value, this.charset);
-    } else {
-      return value;
-    }
+  private String encodeLiteral(String value) {
+    return this.encodeLiteral() ? UriUtils.encode(value, this.charset, true) : value;
   }
 
   /**
@@ -183,34 +173,17 @@ public boolean isLiteral() {
    * Parse the template into {@link TemplateChunk}s.
    */
   private void parseTemplate() {
-    /*
-     * query string and path literals have different reserved characters and different encoding
-     * requirements. to ensure compliance with RFC 6570, we'll need to encode query literals
-     * differently from path literals. let's look at the template to see if it contains a query
-     * string and if so, keep track of where it starts.
-     */
-    Matcher queryStringMatcher = QUERY_STRING_PATTERN.matcher(this.template);
-    if (queryStringMatcher.find()) {
-      /*
-       * the template contains a query string, split the template into two parts, the path and query
-       */
-      String path = this.template.substring(0, queryStringMatcher.start());
-      String query = this.template.substring(queryStringMatcher.end() - 1);
-      this.parseFragment(path, false);
-      this.parseFragment(query, true);
-    } else {
-      /* parse the entire template */
-      this.parseFragment(this.template, false);
-    }
+
+    /* parse the entire template */
+    this.parseFragment(this.template);
   }
 
   /**
    * Parse a template fragment.
    *
    * @param fragment to parse
-   * @param query if the fragment is part of a query string.
    */
-  private void parseFragment(String fragment, boolean query) {
+  private void parseFragment(String fragment) {
     ChunkTokenizer tokenizer = new ChunkTokenizer(fragment);
 
     while (tokenizer.hasNext()) {
@@ -218,18 +191,14 @@ private void parseFragment(String fragment, boolean query) {
       String chunk = tokenizer.next();
 
       if (chunk.startsWith("{")) {
-        /* it's an expression, defer encoding until resolution */
-        FragmentType type = (query) ? FragmentType.QUERY : FragmentType.PATH_SEGMENT;
-
-        Expression expression = Expressions.create(chunk, type);
+        Expression expression = Expressions.create(chunk);
         if (expression == null) {
-          this.templateChunks.add(Literal.create(encode(chunk, query)));
+          this.templateChunks.add(Literal.create(this.encodeLiteral(chunk)));
         } else {
           this.templateChunks.add(expression);
         }
       } else {
-        /* it's a literal, pct-encode it */
-        this.templateChunks.add(Literal.create(encode(chunk, query)));
+        this.templateChunks.add(Literal.create(this.encodeLiteral(chunk)));
       }
     }
   }
@@ -240,7 +209,7 @@ public String toString() {
         .map(TemplateChunk::getValue).collect(Collectors.joining());
   }
 
-  public boolean encode() {
+  public boolean encodeLiteral() {
     return encode.isEncodingRequired();
   }
 
diff --git a/core/src/main/java/feign/template/UriUtils.java b/core/src/main/java/feign/template/UriUtils.java
index 1fd17dcc8..379f5bffd 100644
--- a/core/src/main/java/feign/template/UriUtils.java
+++ b/core/src/main/java/feign/template/UriUtils.java
@@ -23,9 +23,6 @@
 
 public class UriUtils {
 
-
-  // private static final String QUERY_RESERVED_CHARACTERS = "=";
-  // private static final String PATH_RESERVED_CHARACTERS = "/=@:!$&\'(),;~";
   private static final Pattern PCT_ENCODED_PATTERN = Pattern.compile("%[0-9A-Fa-f][0-9A-Fa-f]");
 
   /**
@@ -34,8 +31,14 @@ public class UriUtils {
    * @param value to check.
    * @return {@literal true} if the value is already pct-encoded
    */
-  public static boolean isEncoded(String value) {
-    return PCT_ENCODED_PATTERN.matcher(value).matches();
+  public static boolean isEncoded(String value, Charset charset) {
+    for (byte b : value.getBytes(charset)) {
+      if (!isUnreserved((char) b) && b != '%') {
+        /* break if there are any unreserved character */
+        return false;
+      }
+    }
+    return PCT_ENCODED_PATTERN.matcher(value).find();
   }
 
   /**
@@ -45,7 +48,7 @@ public static boolean isEncoded(String value) {
    * @return the encoded value.
    */
   public static String encode(String value) {
-    return encodeReserved(value, FragmentType.URI, Util.UTF_8);
+    return encodeChunk(value, Util.UTF_8, false);
   }
 
   /**
@@ -56,7 +59,15 @@ public static String encode(String value) {
    * @return the encoded value.
    */
   public static String encode(String value, Charset charset) {
-    return encodeReserved(value, FragmentType.URI, charset);
+    return encodeChunk(value, charset, false);
+  }
+
+  public static String encode(String value, boolean allowReservedCharacters) {
+    return encodeInternal(value, Util.UTF_8, allowReservedCharacters);
+  }
+
+  public static String encode(String value, Charset charset, boolean allowReservedCharacters) {
+    return encodeInternal(value, charset, allowReservedCharacters);
   }
 
   /**
@@ -76,47 +87,6 @@ public static String decode(String value, Charset charset) {
     }
   }
 
-  /**
-   * Uri Encode a Path Fragment.
-   *
-   * @param path containing the path fragment.
-   * @param charset to use.
-   * @return the encoded path fragment.
-   */
-  public static String pathEncode(String path, Charset charset) {
-    return encodeReserved(path, FragmentType.PATH_SEGMENT, charset);
-
-    /*
-     * path encoding is not equivalent to query encoding, there are few differences, namely dealing
-     * with spaces, !, ', (, ), and ~ characters. we will need to manually process those values.
-     */
-    // return encoded.replaceAll("\\+", "%20");
-  }
-
-  /**
-   * Uri Encode a Query Fragment.
-   *
-   * @param query containing the query fragment
-   * @param charset to use.
-   * @return the encoded query fragment.
-   */
-  public static String queryEncode(String query, Charset charset) {
-    return encodeReserved(query, FragmentType.QUERY, charset);
-
-    /* spaces will be encoded as 'plus' symbols here, we want them pct-encoded */
-    // return encoded.replaceAll("\\+", "%20");
-  }
-
-  /**
-   * Uri Encode a Query Parameter name or value.
-   *
-   * @param queryParam containing the query parameter.
-   * @param charset to use.
-   * @return the encoded query fragment.
-   */
-  public static String queryParamEncode(String queryParam, Charset charset) {
-    return encodeReserved(queryParam, FragmentType.QUERY_PARAM, charset);
-  }
 
   /**
    * Determines if the provided uri is an absolute uri.
@@ -134,16 +104,17 @@ public static boolean isAbsolute(String uri) {
    * ignored.
    *
    * @param value inspect.
-   * @param type identifying which uri fragment rules to apply.
    * @param charset to use.
    * @return a new String with the reserved characters preserved.
    */
-  public static String encodeReserved(String value, FragmentType type, Charset charset) {
+  public static String encodeInternal(String value,
+                                      Charset charset,
+                                      boolean allowReservedCharacters) {
     /* value is encoded, we need to split it up and skip the parts that are already encoded */
     Matcher matcher = PCT_ENCODED_PATTERN.matcher(value);
 
     if (!matcher.find()) {
-      return encodeChunk(value, type, charset);
+      return encodeChunk(value, charset, true);
     }
 
     int length = value.length();
@@ -154,7 +125,7 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
       String before = value.substring(index, matcher.start());
 
       /* encode it */
-      encoded.append(encodeChunk(before, type, charset));
+      encoded.append(encodeChunk(before, charset, allowReservedCharacters));
 
       /* append the encoded value */
       encoded.append(matcher.group());
@@ -165,7 +136,7 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
 
     /* append the rest of the string */
     String tail = value.substring(index, length);
-    encoded.append(encodeChunk(tail, type, charset));
+    encoded.append(encodeChunk(tail, charset, allowReservedCharacters));
     return encoded.toString();
   }
 
@@ -173,16 +144,18 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
    * Encode a Uri Chunk, ensuring that all reserved characters are also encoded.
    *
    * @param value to encode.
-   * @param type identifying which uri fragment rules to apply.
    * @param charset to use.
    * @return an encoded uri chunk.
    */
-  private static String encodeChunk(String value, FragmentType type, Charset charset) {
+  private static String encodeChunk(String value, Charset charset, boolean allowReserved) {
+    if (isEncoded(value, charset)) {
+      return value;
+    }
+
     byte[] data = value.getBytes(charset);
     ByteArrayOutputStream encoded = new ByteArrayOutputStream();
-
     for (byte b : data) {
-      if (type.isAllowed(b)) {
+      if (isUnreserved(b) || (isReserved(b) && allowReserved)) {
         encoded.write(b);
       } else {
         /* percent encode the byte */
@@ -206,79 +179,36 @@ private static void pctEncode(byte data, ByteArrayOutputStream bos) {
     bos.write(hex2);
   }
 
-  enum FragmentType {
 
-    URI {
-      @Override
-      boolean isAllowed(int c) {
-        return isUnreserved(c);
-      }
-    },
-    RESERVED {
-      @Override
-      boolean isAllowed(int c) {
-        return isUnreserved(c) || isReserved(c);
-      }
-    },
-    PATH_SEGMENT {
-      @Override
-      boolean isAllowed(int c) {
-        return this.isPchar(c) || (c == '/');
-      }
-    },
-    QUERY {
-      @Override
-      boolean isAllowed(int c) {
-        /* although plus signs are allowed, their use is inconsistent. force encoding */
-        if (c == '+') {
-          return false;
-        }
-
-        return this.isPchar(c) || c == '/' || c == '?';
-      }
-    },
-    QUERY_PARAM {
-      @Override
-      boolean isAllowed(int c) {
-        /* explicitly encode equals, ampersands, questions */
-        if (c == '=' || c == '&' || c == '?') {
-          return false;
-        }
-        return QUERY.isAllowed(c);
-      }
-    };
-
-    abstract boolean isAllowed(int c);
 
-    protected boolean isAlpha(int c) {
-      return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z');
-    }
-
-    protected boolean isDigit(int c) {
-      return (c >= '0' && c <= '9');
-    }
+  private static boolean isAlpha(int c) {
+    return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z');
+  }
 
-    protected boolean isGenericDelimiter(int c) {
-      return (c == ':') || (c == '/') || (c == '?') || (c == '#') || (c == '[') || (c == ']')
-          || (c == '@');
-    }
+  private static boolean isDigit(int c) {
+    return (c >= '0' && c <= '9');
+  }
 
-    protected boolean isSubDelimiter(int c) {
-      return (c == '!') || (c == '$') || (c == '&') || (c == '\'') || (c == '(') || (c == ')')
-          || (c == '*') || (c == '+') || (c == ',') || (c == ';') || (c == '=');
-    }
+  private static boolean isGenericDelimiter(int c) {
+    return (c == ':') || (c == '/') || (c == '?') || (c == '#') || (c == '[') || (c == ']')
+        || (c == '@');
+  }
 
-    protected boolean isUnreserved(int c) {
-      return this.isAlpha(c) || this.isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~';
-    }
+  private static boolean isSubDelimiter(int c) {
+    return (c == '!') || (c == '$') || (c == '&') || (c == '\'') || (c == '(') || (c == ')')
+        || (c == '*') || (c == '+') || (c == ',') || (c == ';') || (c == '=');
+  }
 
-    protected boolean isReserved(int c) {
-      return this.isGenericDelimiter(c) || this.isSubDelimiter(c);
-    }
+  private static boolean isUnreserved(int c) {
+    return isAlpha(c) || isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~';
+  }
 
-    protected boolean isPchar(int c) {
-      return this.isUnreserved(c) || this.isSubDelimiter(c) || c == ':' || c == '@';
-    }
+  private static boolean isReserved(int c) {
+    return isGenericDelimiter(c) || isSubDelimiter(c);
+  }
 
+  private boolean isPchar(int c) {
+    return isUnreserved(c) || isSubDelimiter(c) || c == ':' || c == '@';
   }
+
 }
diff --git a/core/src/test/java/feign/RequestTemplateTest.java b/core/src/test/java/feign/RequestTemplateTest.java
index 3d64e3f20..4831a33d1 100644
--- a/core/src/test/java/feign/RequestTemplateTest.java
+++ b/core/src/test/java/feign/RequestTemplateTest.java
@@ -172,7 +172,7 @@ public void resolveTemplateWithMixedCollectionFormatsByQuery() {
         "values[]", Arrays.asList("1", "2")));
 
     assertThat(template.url())
-        .isEqualToIgnoringCase("/api/collections?keys=one&keys=two&values%5B%5D=1,2");
+        .isEqualToIgnoringCase("/api/collections?keys=one&keys=two&values%5B%5D=1%2C2");
   }
 
   @Test
@@ -418,7 +418,7 @@ public void encodedQueryWithUnsafeCharactersMixedWithUnencoded() {
     assertThat(template.queryLine()).isEqualTo("?params%5B%5D=not%20encoded&params%5B%5D=encoded");
     Map<String, Collection<String>> queries = template.queries();
     assertThat(queries).containsKey("params[]");
-    assertThat(queries.get("params[]")).contains("encoded").contains("not encoded");
+    assertThat(queries.get("params[]")).contains("encoded").contains("not%20encoded");
   }
 
   @SuppressWarnings("unchecked")
diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
index ffa3cb620..7c91f1f15 100644
--- a/core/src/test/java/feign/template/QueryTemplateTest.java
+++ b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -30,6 +30,15 @@ public void templateToQueryString() {
     assertThat(template.toString()).isEqualToIgnoringCase("name=Bob&name=James&name=Jason");
   }
 
+  @Test
+  public void expandCollectionWithBlanks() {
+    QueryTemplate template =
+        QueryTemplate.create("people", Collections.singletonList("{people}"), Util.UTF_8);
+    String expanded = template.expand(
+        Collections.singletonMap("people", Arrays.asList("", "Jason", "James")));
+    assertThat(expanded).isEqualToIgnoringCase("people=&people=Jason&people=James");
+  }
+
   @Test
   public void expandSingleValue() {
     QueryTemplate template =
@@ -125,7 +134,7 @@ public void expandSingleValueWithComma() {
         QueryTemplate.create("collection",
             Collections.singletonList("{collection}"), Util.UTF_8);
     String expanded = template.expand(Collections.singletonMap("collection", "one,two,three"));
-    assertThat(expanded).isEqualToIgnoringCase("collection=one,two,three");
+    assertThat(expanded).isEqualToIgnoringCase("collection=one%2Ctwo%2Cthree");
   }
 
   @Test
@@ -162,7 +171,7 @@ public void expandSingleValueWithJson() {
     String expanded = template.expand(
         Collections.singletonMap("json", "{\"name\":\"feign\",\"version\": \"10\"}"));
     assertThat(expanded).isEqualToIgnoringCase(
-        "json=%7B%22name%22:%22feign%22,%22version%22:%20%2210%22%7D");
+        "json=%7B%22name%22%3A%22feign%22%2C%22version%22%3A%20%2210%22%7D");
   }
 
 
diff --git a/core/src/test/java/feign/template/UriUtilsTest.java b/core/src/test/java/feign/template/UriUtilsTest.java
index 353d19935..53d1964e8 100644
--- a/core/src/test/java/feign/template/UriUtilsTest.java
+++ b/core/src/test/java/feign/template/UriUtilsTest.java
@@ -16,78 +16,29 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
-import java.nio.charset.Charset;
 import org.junit.Test;
 
 public class UriUtilsTest {
 
-
-  /**
-   * Verify that values outside of the allowed characters in a path segment are pct-encoded. The
-   * list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
-   */
-  @Test
-  public void pctEncodeReservedPathCharacters() {
-    /*
-     * the only not allowed characters are the general delimiters, with the exception of slash,
-     * question, colon and at
-     */
-    String reservedPath = "/api/user@host:port#section[a-z]/data";
-    String reservedPathEncoded = UriUtils.pathEncode(reservedPath, UTF_8);
-
-    /*
-     * the result should be the path, with the slash and question retained, but all other special
-     * characters encoded
-     */
-    assertThat(reservedPathEncoded).isEqualTo("/api/user@host:port%23section%5Ba-z%5D/data");
-
-  }
-
   /**
-   * Verify that the list of allowed characters in a path segment are not pct-encoded, as they don't
-   * have to be. The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
+   * pct-encode a String, ensuring that all reserved characters are encoded.
    */
   @Test
-  public void ensureApprovedPathParametersAreNotEncoded() {
-    /* the approved list is 'any pchar' */
-    String pchar =
-        "abcdefghijklmnopqrstuvwxyZABCDEFGHIJKLMNOPQRSTUVWXYZZ0123456789_-~.!$&'()*+,;=:@%2B";
-    String pathEncoded = UriUtils.pathEncode(pchar, UTF_8);
-
-    /* the result here should be nothing has been changed */
-    assertThat(pathEncoded).isEqualTo(pchar);
-
+  public void pctEncode() {
+    String queryParameterValue = "firstName=James;lastName=Bond;location=England&Britain?";
+    assertThat(UriUtils.encode(queryParameterValue, UTF_8))
+        .isEqualToIgnoringCase(
+            "firstName%3DJames%3BlastName%3DBond%3Blocation%3DEngland%26Britain%3F");
   }
 
-  /**
-   * Verify that when a full query string is provided, only unapproved characters are pct-encoded.
-   * This differs from {@link UriUtils#queryParamEncode(String, Charset)} in that this method
-   * adheres to specification exactly, whereas the other is more restrictive.
-   *
-   * The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
-   */
-  @Test
-  public void pctEncodeQueryString() {
-    String query = "?name=James Bond&occupation=Spy&location=Great Britain!";
-    assertThat(UriUtils.queryEncode(query, UTF_8))
-        .isEqualToIgnoringCase("?name=James%20Bond&occupation=Spy&location=Great%20Britain!");
-  }
 
   /**
-   * Verify that a value meant for a query string parameter is pct-encoded using the defined query
-   * set of allowed characters, including equals, ampersands and question marks as in Feign, we
-   * manage the creation of the key value pair.
-   *
-   * The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
+   * pct-encode preserving reserved characters.
    */
   @Test
-  public void pctEncodeQueryParameterValue() {
-    String queryParameterValue = "firstName=James;lastName=Bond;location=England&Britain?";
-    assertThat(UriUtils.queryParamEncode(queryParameterValue, UTF_8))
-        .isEqualToIgnoringCase("firstName%3DJames;lastName%3DBond;location%3DEngland%26Britain%3F");
+  public void pctEncodeWithReservedCharacters() {
+    String withReserved = "/api/user@host:port#section[a-z]/data";
+    String encoded = UriUtils.encode(withReserved, UTF_8, true);
+    assertThat(encoded).isEqualTo("/api/user@host:port#section[a-z]/data");
   }
 }

From d3eb9cf3bd9f8e2a5711100c91ce33f44a54b0b0 Mon Sep 17 00:00:00 2001
From: Kevin Davis <kdavisk6@gmail.com>
Date: Fri, 27 Dec 2019 08:47:53 -0500
Subject: [PATCH 2/5] Ensure all brackets are decoded in JSON based Body
 Templates (#1140)

* Ensure all brackets are decoded in JSON based Body Templates

Fixes #1129

When JSON is detected in a Body Template, all start and end tokens
that may have been pct-encoded are decoded, ensuring that the
expanded result is valid JSON.
---
 .../java/feign/template/BodyTemplate.java     | 11 ++-----
 .../java/feign/template/BodyTemplateTest.java | 32 +++++++++++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)
 create mode 100644 core/src/test/java/feign/template/BodyTemplateTest.java

diff --git a/core/src/main/java/feign/template/BodyTemplate.java b/core/src/main/java/feign/template/BodyTemplate.java
index f4c32c3e0..19f879eb9 100644
--- a/core/src/main/java/feign/template/BodyTemplate.java
+++ b/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;
   }
diff --git a/core/src/test/java/feign/template/BodyTemplateTest.java b/core/src/test/java/feign/template/BodyTemplateTest.java
new file mode 100644
index 000000000..540230708
--- /dev/null
+++ b/core/src/test/java/feign/template/BodyTemplateTest.java
@@ -0,0 +1,32 @@
+/**
+ * Copyright 2012-2019 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign.template;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.Collections;
+import org.junit.Test;
+
+public class BodyTemplateTest {
+
+  @Test
+  public void bodyTemplatesSupportJsonOnlyWhenEncoded() {
+    String bodyTemplate =
+        "%7B\"resize\": %7B\"method\": \"fit\",\"width\": {size},\"height\": {size}%7D%7D";
+    BodyTemplate template = BodyTemplate.create(bodyTemplate);
+    String expanded = template.expand(Collections.singletonMap("size", "100"));
+    assertThat(expanded)
+        .isEqualToIgnoringCase(
+            "{\"resize\": {\"method\": \"fit\",\"width\": 100,\"height\": 100}}");
+  }
+}

From 033de93ccac0db19d31676d49dacc1b570c747e6 Mon Sep 17 00:00:00 2001
From: Kevin Davis <kdavisk6@gmail.com>
Date: Fri, 27 Dec 2019 09:47:03 -0500
Subject: [PATCH 3/5] Remove Template Expression naming restrictions (#1139)

Fixes #1036

Relaxed the regular expression used to determine if an expression
is valid to support additional expression variable names.  We will
no longer restrict what an expression name can be.
---
 .../main/java/feign/template/Expressions.java | 19 ++++++++++++++-----
 .../feign/template/QueryTemplateTest.java     | 11 +++++++++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/feign/template/Expressions.java b/core/src/main/java/feign/template/Expressions.java
index 96dfe02dc..2410cf8aa 100644
--- a/core/src/main/java/feign/template/Expressions.java
+++ b/core/src/main/java/feign/template/Expressions.java
@@ -38,7 +38,8 @@ 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);
   }
 
@@ -70,10 +71,18 @@ 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;
       }
     }
 
diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
index ffa3cb620..4d7cb5e2b 100644
--- a/core/src/test/java/feign/template/QueryTemplateTest.java
+++ b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -176,4 +176,15 @@ public void expandCollectionValueWithBrackets() {
     /* brackets will be pct-encoded */
     assertThat(expanded).isEqualToIgnoringCase("collection%5B%5D=1,2");
   }
+
+  @Test
+  public void expandCollectionValueWithDollar() {
+    QueryTemplate template =
+        QueryTemplate.create("$collection", Collections.singletonList("{$collection}"),
+            Util.UTF_8, CollectionFormat.CSV);
+    String expanded = template.expand(Collections.singletonMap("$collection",
+        Arrays.asList("1", "2")));
+    /* brackets will be pct-encoded */
+    assertThat(expanded).isEqualToIgnoringCase("$collection=1,2");
+  }
 }

From caa9b13dfb9967215e262238d8086590810acebf Mon Sep 17 00:00:00 2001
From: Kevin Davis <kdavisk6@gmail.com>
Date: Thu, 26 Dec 2019 14:21:49 -0500
Subject: [PATCH 4/5] Ensure Iterable values are encoded before template
 expansion

Fixes #1123, Fixes #1133, Fixes #1102, Fixes #1028

Ensures that all expressions are fully-encoded before being
manipulated during template expansion.  This allows parameters
to include reserved values and result in properly encoded
results.

Additionally, `Iterable` values are now handled in accordance
with RFC 6570 allowing for the specified `CollectionFormat` to
be applied and empty parameters to be expanded correctly as this
is the main use case that exhibited this issue.
---
 .../src/main/java/feign/CollectionFormat.java |   8 +-
 core/src/main/java/feign/RequestTemplate.java |   5 +-
 .../main/java/feign/template/Expression.java  |   2 +-
 .../main/java/feign/template/Expressions.java |  55 ++++--
 .../java/feign/template/QueryTemplate.java    | 112 ++++++-----
 .../main/java/feign/template/Template.java    | 107 ++++-------
 .../main/java/feign/template/UriUtils.java    | 176 ++++++------------
 .../test/java/feign/RequestTemplateTest.java  |   4 +-
 .../feign/template/QueryTemplateTest.java     |  13 +-
 .../java/feign/template/UriUtilsTest.java     |  71 ++-----
 10 files changed, 229 insertions(+), 324 deletions(-)

diff --git a/core/src/main/java/feign/CollectionFormat.java b/core/src/main/java/feign/CollectionFormat.java
index 14909da41..5623c5aa6 100644
--- a/core/src/main/java/feign/CollectionFormat.java
+++ b/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;
diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java
index bf089181c..facf969f2 100644
--- a/core/src/main/java/feign/RequestTemplate.java
+++ b/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
diff --git a/core/src/main/java/feign/template/Expression.java b/core/src/main/java/feign/template/Expression.java
index 3040aa1c3..f4852df8c 100644
--- a/core/src/main/java/feign/template/Expression.java
+++ b/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;
 
 /**
diff --git a/core/src/main/java/feign/template/Expressions.java b/core/src/main/java/feign/template/Expressions.java
index 2410cf8aa..b67541eff 100644
--- a/core/src/main/java/feign/template/Expressions.java
+++ b/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;
@@ -43,7 +40,7 @@ public final class Expressions {
         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);
@@ -86,7 +83,7 @@ public static Expression create(final String value, final FragmentType type) {
       }
     }
 
-    return new SimpleExpression(variableName, variablePattern, type);
+    return new SimpleExpression(variableName, variablePattern);
   }
 
   private static String stripBraces(String expression) {
@@ -105,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);
       }
@@ -137,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();
+    }
   }
 }
diff --git a/core/src/main/java/feign/template/QueryTemplate.java b/core/src/main/java/feign/template/QueryTemplate.java
index c67735272..19e267c7d 100644
--- a/core/src/main/java/feign/template/QueryTemplate.java
+++ b/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 */
diff --git a/core/src/main/java/feign/template/Template.java b/core/src/main/java/feign/template/Template.java
index da58f0958..de7efb3fc 100644
--- a/core/src/main/java/feign/template/Template.java
+++ b/core/src/main/java/feign/template/Template.java
@@ -13,15 +13,12 @@
  */
 package feign.template;
 
-import feign.Util;
-import feign.template.UriUtils.FragmentType;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -32,13 +29,6 @@
  */
 public class Template {
 
-  /*
-   * special delimiter for collection based expansion, in an attempt to avoid accidental splitting
-   * for resolved values. semi-colon was chosen because it is a reserved character that must be
-   * pct-encoded and should not appear unencoded.
-   */
-  static final String COLLECTION_DELIMITER = ";";
-
   private static final Logger logger = Logger.getLogger(Template.class.getName());
   private static final Pattern QUERY_STRING_PATTERN = Pattern.compile("(?<!\\{)(\\?)");
   private final String template;
@@ -82,37 +72,53 @@ public String expand(Map<String, ?> variables) {
     }
 
     /* resolve all expressions within the template */
-    StringBuilder resolved = new StringBuilder();
+    StringBuilder resolved = null;
     for (TemplateChunk chunk : this.templateChunks) {
+      String expanded;
       if (chunk instanceof Expression) {
-        String resolvedExpression = this.resolveExpression((Expression) chunk, variables);
-        if (resolvedExpression != null) {
-          resolved.append(resolvedExpression);
-        }
+        expanded = this.resolveExpression((Expression) chunk, variables);
       } else {
         /* chunk is a literal value */
-        resolved.append(chunk.getValue());
+        expanded = chunk.getValue();
+      }
+      if (expanded == null) {
+        continue;
+      }
+
+      /* append it to the result */
+      if (resolved == null) {
+        resolved = new StringBuilder();
       }
+      resolved.append(expanded);
     }
+
+    if (resolved == null) {
+      /* entire template is unresolved */
+      return null;
+    }
+
     return resolved.toString();
   }
 
-  protected String resolveExpression(Expression expression, Map<String, ?> variables) {
+  protected String resolveExpression(
+                                     Expression expression,
+                                     Map<String, ?> variables) {
     String resolved = null;
     Object value = variables.get(expression.getName());
     if (value != null) {
-      String expanded = expression.expand(value, this.encode.isEncodingRequired());
-      if (Util.isNotBlank(expanded)) {
-        if (this.encodeSlash) {
+      String expanded = expression.expand(
+          value, this.encode.isEncodingRequired());
+      if (expanded != null) {
+        if (!this.encodeSlash) {
           logger.fine("Explicit slash decoding specified, decoding all slashes in uri");
-          expanded = expanded.replaceAll("/", "%2F");
+          expanded = expanded.replaceAll("%2F", "/");
         }
         resolved = expanded;
       }
     } else {
       if (this.allowUnresolved) {
         /* unresolved variables are treated as literals */
-        resolved = encode(expression.toString());
+        resolved = encodeLiteral(expression.toString());
       }
     }
     return resolved;
@@ -124,24 +130,8 @@ protected String resolveExpression(Expression expression, Map<String, ?> variabl
    * @param value to encode.
    * @return the encoded value.
    */
-  private String encode(String value) {
-    return this.encode.isEncodingRequired() ? UriUtils.encode(value, this.charset) : value;
-  }
-
-  /**
-   * Uri Encode the value.
-   *
-   * @param value to encode
-   * @param query indicating this value is on a query string.
-   * @return the encoded value
-   */
-  private String encode(String value, boolean query) {
-    if (this.encode.isEncodingRequired()) {
-      return query ? UriUtils.queryEncode(value, this.charset)
-          : UriUtils.pathEncode(value, this.charset);
-    } else {
-      return value;
-    }
+  private String encodeLiteral(String value) {
+    return this.encodeLiteral() ? UriUtils.encode(value, this.charset, true) : value;
   }
 
   /**
@@ -183,34 +173,17 @@ public boolean isLiteral() {
    * Parse the template into {@link TemplateChunk}s.
    */
   private void parseTemplate() {
-    /*
-     * query string and path literals have different reserved characters and different encoding
-     * requirements. to ensure compliance with RFC 6570, we'll need to encode query literals
-     * differently from path literals. let's look at the template to see if it contains a query
-     * string and if so, keep track of where it starts.
-     */
-    Matcher queryStringMatcher = QUERY_STRING_PATTERN.matcher(this.template);
-    if (queryStringMatcher.find()) {
-      /*
-       * the template contains a query string, split the template into two parts, the path and query
-       */
-      String path = this.template.substring(0, queryStringMatcher.start());
-      String query = this.template.substring(queryStringMatcher.end() - 1);
-      this.parseFragment(path, false);
-      this.parseFragment(query, true);
-    } else {
-      /* parse the entire template */
-      this.parseFragment(this.template, false);
-    }
+
+    /* parse the entire template */
+    this.parseFragment(this.template);
   }
 
   /**
    * Parse a template fragment.
    *
    * @param fragment to parse
-   * @param query if the fragment is part of a query string.
    */
-  private void parseFragment(String fragment, boolean query) {
+  private void parseFragment(String fragment) {
     ChunkTokenizer tokenizer = new ChunkTokenizer(fragment);
 
     while (tokenizer.hasNext()) {
@@ -218,18 +191,14 @@ private void parseFragment(String fragment, boolean query) {
       String chunk = tokenizer.next();
 
       if (chunk.startsWith("{")) {
-        /* it's an expression, defer encoding until resolution */
-        FragmentType type = (query) ? FragmentType.QUERY : FragmentType.PATH_SEGMENT;
-
-        Expression expression = Expressions.create(chunk, type);
+        Expression expression = Expressions.create(chunk);
         if (expression == null) {
-          this.templateChunks.add(Literal.create(encode(chunk, query)));
+          this.templateChunks.add(Literal.create(this.encodeLiteral(chunk)));
         } else {
           this.templateChunks.add(expression);
         }
       } else {
-        /* it's a literal, pct-encode it */
-        this.templateChunks.add(Literal.create(encode(chunk, query)));
+        this.templateChunks.add(Literal.create(this.encodeLiteral(chunk)));
       }
     }
   }
@@ -240,7 +209,7 @@ public String toString() {
         .map(TemplateChunk::getValue).collect(Collectors.joining());
   }
 
-  public boolean encode() {
+  public boolean encodeLiteral() {
     return encode.isEncodingRequired();
   }
 
diff --git a/core/src/main/java/feign/template/UriUtils.java b/core/src/main/java/feign/template/UriUtils.java
index 1fd17dcc8..379f5bffd 100644
--- a/core/src/main/java/feign/template/UriUtils.java
+++ b/core/src/main/java/feign/template/UriUtils.java
@@ -23,9 +23,6 @@
 
 public class UriUtils {
 
-
-  // private static final String QUERY_RESERVED_CHARACTERS = "=";
-  // private static final String PATH_RESERVED_CHARACTERS = "/=@:!$&\'(),;~";
   private static final Pattern PCT_ENCODED_PATTERN = Pattern.compile("%[0-9A-Fa-f][0-9A-Fa-f]");
 
   /**
@@ -34,8 +31,14 @@ public class UriUtils {
    * @param value to check.
    * @return {@literal true} if the value is already pct-encoded
    */
-  public static boolean isEncoded(String value) {
-    return PCT_ENCODED_PATTERN.matcher(value).matches();
+  public static boolean isEncoded(String value, Charset charset) {
+    for (byte b : value.getBytes(charset)) {
+      if (!isUnreserved((char) b) && b != '%') {
+        /* break if there are any unreserved character */
+        return false;
+      }
+    }
+    return PCT_ENCODED_PATTERN.matcher(value).find();
   }
 
   /**
@@ -45,7 +48,7 @@ public static boolean isEncoded(String value) {
    * @return the encoded value.
    */
   public static String encode(String value) {
-    return encodeReserved(value, FragmentType.URI, Util.UTF_8);
+    return encodeChunk(value, Util.UTF_8, false);
   }
 
   /**
@@ -56,7 +59,15 @@ public static String encode(String value) {
    * @return the encoded value.
    */
   public static String encode(String value, Charset charset) {
-    return encodeReserved(value, FragmentType.URI, charset);
+    return encodeChunk(value, charset, false);
+  }
+
+  public static String encode(String value, boolean allowReservedCharacters) {
+    return encodeInternal(value, Util.UTF_8, allowReservedCharacters);
+  }
+
+  public static String encode(String value, Charset charset, boolean allowReservedCharacters) {
+    return encodeInternal(value, charset, allowReservedCharacters);
   }
 
   /**
@@ -76,47 +87,6 @@ public static String decode(String value, Charset charset) {
     }
   }
 
-  /**
-   * Uri Encode a Path Fragment.
-   *
-   * @param path containing the path fragment.
-   * @param charset to use.
-   * @return the encoded path fragment.
-   */
-  public static String pathEncode(String path, Charset charset) {
-    return encodeReserved(path, FragmentType.PATH_SEGMENT, charset);
-
-    /*
-     * path encoding is not equivalent to query encoding, there are few differences, namely dealing
-     * with spaces, !, ', (, ), and ~ characters. we will need to manually process those values.
-     */
-    // return encoded.replaceAll("\\+", "%20");
-  }
-
-  /**
-   * Uri Encode a Query Fragment.
-   *
-   * @param query containing the query fragment
-   * @param charset to use.
-   * @return the encoded query fragment.
-   */
-  public static String queryEncode(String query, Charset charset) {
-    return encodeReserved(query, FragmentType.QUERY, charset);
-
-    /* spaces will be encoded as 'plus' symbols here, we want them pct-encoded */
-    // return encoded.replaceAll("\\+", "%20");
-  }
-
-  /**
-   * Uri Encode a Query Parameter name or value.
-   *
-   * @param queryParam containing the query parameter.
-   * @param charset to use.
-   * @return the encoded query fragment.
-   */
-  public static String queryParamEncode(String queryParam, Charset charset) {
-    return encodeReserved(queryParam, FragmentType.QUERY_PARAM, charset);
-  }
 
   /**
    * Determines if the provided uri is an absolute uri.
@@ -134,16 +104,17 @@ public static boolean isAbsolute(String uri) {
    * ignored.
    *
    * @param value inspect.
-   * @param type identifying which uri fragment rules to apply.
    * @param charset to use.
    * @return a new String with the reserved characters preserved.
    */
-  public static String encodeReserved(String value, FragmentType type, Charset charset) {
+  public static String encodeInternal(String value,
+                                      Charset charset,
+                                      boolean allowReservedCharacters) {
     /* value is encoded, we need to split it up and skip the parts that are already encoded */
     Matcher matcher = PCT_ENCODED_PATTERN.matcher(value);
 
     if (!matcher.find()) {
-      return encodeChunk(value, type, charset);
+      return encodeChunk(value, charset, true);
     }
 
     int length = value.length();
@@ -154,7 +125,7 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
       String before = value.substring(index, matcher.start());
 
       /* encode it */
-      encoded.append(encodeChunk(before, type, charset));
+      encoded.append(encodeChunk(before, charset, allowReservedCharacters));
 
       /* append the encoded value */
       encoded.append(matcher.group());
@@ -165,7 +136,7 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
 
     /* append the rest of the string */
     String tail = value.substring(index, length);
-    encoded.append(encodeChunk(tail, type, charset));
+    encoded.append(encodeChunk(tail, charset, allowReservedCharacters));
     return encoded.toString();
   }
 
@@ -173,16 +144,18 @@ public static String encodeReserved(String value, FragmentType type, Charset cha
    * Encode a Uri Chunk, ensuring that all reserved characters are also encoded.
    *
    * @param value to encode.
-   * @param type identifying which uri fragment rules to apply.
    * @param charset to use.
    * @return an encoded uri chunk.
    */
-  private static String encodeChunk(String value, FragmentType type, Charset charset) {
+  private static String encodeChunk(String value, Charset charset, boolean allowReserved) {
+    if (isEncoded(value, charset)) {
+      return value;
+    }
+
     byte[] data = value.getBytes(charset);
     ByteArrayOutputStream encoded = new ByteArrayOutputStream();
-
     for (byte b : data) {
-      if (type.isAllowed(b)) {
+      if (isUnreserved(b) || (isReserved(b) && allowReserved)) {
         encoded.write(b);
       } else {
         /* percent encode the byte */
@@ -206,79 +179,36 @@ private static void pctEncode(byte data, ByteArrayOutputStream bos) {
     bos.write(hex2);
   }
 
-  enum FragmentType {
 
-    URI {
-      @Override
-      boolean isAllowed(int c) {
-        return isUnreserved(c);
-      }
-    },
-    RESERVED {
-      @Override
-      boolean isAllowed(int c) {
-        return isUnreserved(c) || isReserved(c);
-      }
-    },
-    PATH_SEGMENT {
-      @Override
-      boolean isAllowed(int c) {
-        return this.isPchar(c) || (c == '/');
-      }
-    },
-    QUERY {
-      @Override
-      boolean isAllowed(int c) {
-        /* although plus signs are allowed, their use is inconsistent. force encoding */
-        if (c == '+') {
-          return false;
-        }
-
-        return this.isPchar(c) || c == '/' || c == '?';
-      }
-    },
-    QUERY_PARAM {
-      @Override
-      boolean isAllowed(int c) {
-        /* explicitly encode equals, ampersands, questions */
-        if (c == '=' || c == '&' || c == '?') {
-          return false;
-        }
-        return QUERY.isAllowed(c);
-      }
-    };
-
-    abstract boolean isAllowed(int c);
 
-    protected boolean isAlpha(int c) {
-      return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z');
-    }
-
-    protected boolean isDigit(int c) {
-      return (c >= '0' && c <= '9');
-    }
+  private static boolean isAlpha(int c) {
+    return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z');
+  }
 
-    protected boolean isGenericDelimiter(int c) {
-      return (c == ':') || (c == '/') || (c == '?') || (c == '#') || (c == '[') || (c == ']')
-          || (c == '@');
-    }
+  private static boolean isDigit(int c) {
+    return (c >= '0' && c <= '9');
+  }
 
-    protected boolean isSubDelimiter(int c) {
-      return (c == '!') || (c == '$') || (c == '&') || (c == '\'') || (c == '(') || (c == ')')
-          || (c == '*') || (c == '+') || (c == ',') || (c == ';') || (c == '=');
-    }
+  private static boolean isGenericDelimiter(int c) {
+    return (c == ':') || (c == '/') || (c == '?') || (c == '#') || (c == '[') || (c == ']')
+        || (c == '@');
+  }
 
-    protected boolean isUnreserved(int c) {
-      return this.isAlpha(c) || this.isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~';
-    }
+  private static boolean isSubDelimiter(int c) {
+    return (c == '!') || (c == '$') || (c == '&') || (c == '\'') || (c == '(') || (c == ')')
+        || (c == '*') || (c == '+') || (c == ',') || (c == ';') || (c == '=');
+  }
 
-    protected boolean isReserved(int c) {
-      return this.isGenericDelimiter(c) || this.isSubDelimiter(c);
-    }
+  private static boolean isUnreserved(int c) {
+    return isAlpha(c) || isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~';
+  }
 
-    protected boolean isPchar(int c) {
-      return this.isUnreserved(c) || this.isSubDelimiter(c) || c == ':' || c == '@';
-    }
+  private static boolean isReserved(int c) {
+    return isGenericDelimiter(c) || isSubDelimiter(c);
+  }
 
+  private boolean isPchar(int c) {
+    return isUnreserved(c) || isSubDelimiter(c) || c == ':' || c == '@';
   }
+
 }
diff --git a/core/src/test/java/feign/RequestTemplateTest.java b/core/src/test/java/feign/RequestTemplateTest.java
index 3d64e3f20..4831a33d1 100644
--- a/core/src/test/java/feign/RequestTemplateTest.java
+++ b/core/src/test/java/feign/RequestTemplateTest.java
@@ -172,7 +172,7 @@ public void resolveTemplateWithMixedCollectionFormatsByQuery() {
         "values[]", Arrays.asList("1", "2")));
 
     assertThat(template.url())
-        .isEqualToIgnoringCase("/api/collections?keys=one&keys=two&values%5B%5D=1,2");
+        .isEqualToIgnoringCase("/api/collections?keys=one&keys=two&values%5B%5D=1%2C2");
   }
 
   @Test
@@ -418,7 +418,7 @@ public void encodedQueryWithUnsafeCharactersMixedWithUnencoded() {
     assertThat(template.queryLine()).isEqualTo("?params%5B%5D=not%20encoded&params%5B%5D=encoded");
     Map<String, Collection<String>> queries = template.queries();
     assertThat(queries).containsKey("params[]");
-    assertThat(queries.get("params[]")).contains("encoded").contains("not encoded");
+    assertThat(queries.get("params[]")).contains("encoded").contains("not%20encoded");
   }
 
   @SuppressWarnings("unchecked")
diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
index 4d7cb5e2b..604e70698 100644
--- a/core/src/test/java/feign/template/QueryTemplateTest.java
+++ b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -30,6 +30,15 @@ public void templateToQueryString() {
     assertThat(template.toString()).isEqualToIgnoringCase("name=Bob&name=James&name=Jason");
   }
 
+  @Test
+  public void expandCollectionWithBlanks() {
+    QueryTemplate template =
+        QueryTemplate.create("people", Collections.singletonList("{people}"), Util.UTF_8);
+    String expanded = template.expand(
+        Collections.singletonMap("people", Arrays.asList("", "Jason", "James")));
+    assertThat(expanded).isEqualToIgnoringCase("people=&people=Jason&people=James");
+  }
+
   @Test
   public void expandSingleValue() {
     QueryTemplate template =
@@ -125,7 +134,7 @@ public void expandSingleValueWithComma() {
         QueryTemplate.create("collection",
             Collections.singletonList("{collection}"), Util.UTF_8);
     String expanded = template.expand(Collections.singletonMap("collection", "one,two,three"));
-    assertThat(expanded).isEqualToIgnoringCase("collection=one,two,three");
+    assertThat(expanded).isEqualToIgnoringCase("collection=one%2Ctwo%2Cthree");
   }
 
   @Test
@@ -162,7 +171,7 @@ public void expandSingleValueWithJson() {
     String expanded = template.expand(
         Collections.singletonMap("json", "{\"name\":\"feign\",\"version\": \"10\"}"));
     assertThat(expanded).isEqualToIgnoringCase(
-        "json=%7B%22name%22:%22feign%22,%22version%22:%20%2210%22%7D");
+        "json=%7B%22name%22%3A%22feign%22%2C%22version%22%3A%20%2210%22%7D");
   }
 
 
diff --git a/core/src/test/java/feign/template/UriUtilsTest.java b/core/src/test/java/feign/template/UriUtilsTest.java
index 353d19935..53d1964e8 100644
--- a/core/src/test/java/feign/template/UriUtilsTest.java
+++ b/core/src/test/java/feign/template/UriUtilsTest.java
@@ -16,78 +16,29 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
-import java.nio.charset.Charset;
 import org.junit.Test;
 
 public class UriUtilsTest {
 
-
-  /**
-   * Verify that values outside of the allowed characters in a path segment are pct-encoded. The
-   * list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
-   */
-  @Test
-  public void pctEncodeReservedPathCharacters() {
-    /*
-     * the only not allowed characters are the general delimiters, with the exception of slash,
-     * question, colon and at
-     */
-    String reservedPath = "/api/user@host:port#section[a-z]/data";
-    String reservedPathEncoded = UriUtils.pathEncode(reservedPath, UTF_8);
-
-    /*
-     * the result should be the path, with the slash and question retained, but all other special
-     * characters encoded
-     */
-    assertThat(reservedPathEncoded).isEqualTo("/api/user@host:port%23section%5Ba-z%5D/data");
-
-  }
-
   /**
-   * Verify that the list of allowed characters in a path segment are not pct-encoded, as they don't
-   * have to be. The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
+   * pct-encode a String, ensuring that all reserved characters are encoded.
    */
   @Test
-  public void ensureApprovedPathParametersAreNotEncoded() {
-    /* the approved list is 'any pchar' */
-    String pchar =
-        "abcdefghijklmnopqrstuvwxyZABCDEFGHIJKLMNOPQRSTUVWXYZZ0123456789_-~.!$&'()*+,;=:@%2B";
-    String pathEncoded = UriUtils.pathEncode(pchar, UTF_8);
-
-    /* the result here should be nothing has been changed */
-    assertThat(pathEncoded).isEqualTo(pchar);
-
+  public void pctEncode() {
+    String queryParameterValue = "firstName=James;lastName=Bond;location=England&Britain?";
+    assertThat(UriUtils.encode(queryParameterValue, UTF_8))
+        .isEqualToIgnoringCase(
+            "firstName%3DJames%3BlastName%3DBond%3Blocation%3DEngland%26Britain%3F");
   }
 
-  /**
-   * Verify that when a full query string is provided, only unapproved characters are pct-encoded.
-   * This differs from {@link UriUtils#queryParamEncode(String, Charset)} in that this method
-   * adheres to specification exactly, whereas the other is more restrictive.
-   *
-   * The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
-   */
-  @Test
-  public void pctEncodeQueryString() {
-    String query = "?name=James Bond&occupation=Spy&location=Great Britain!";
-    assertThat(UriUtils.queryEncode(query, UTF_8))
-        .isEqualToIgnoringCase("?name=James%20Bond&occupation=Spy&location=Great%20Britain!");
-  }
 
   /**
-   * Verify that a value meant for a query string parameter is pct-encoded using the defined query
-   * set of allowed characters, including equals, ampersands and question marks as in Feign, we
-   * manage the creation of the key value pair.
-   *
-   * The list of approved characters used are those listed in <a
-   * href="https://tools.ietf.org/html/rfc3986#appendix-A>RFC 3986 Appendix A</a>
+   * pct-encode preserving reserved characters.
    */
   @Test
-  public void pctEncodeQueryParameterValue() {
-    String queryParameterValue = "firstName=James;lastName=Bond;location=England&Britain?";
-    assertThat(UriUtils.queryParamEncode(queryParameterValue, UTF_8))
-        .isEqualToIgnoringCase("firstName%3DJames;lastName%3DBond;location%3DEngland%26Britain%3F");
+  public void pctEncodeWithReservedCharacters() {
+    String withReserved = "/api/user@host:port#section[a-z]/data";
+    String encoded = UriUtils.encode(withReserved, UTF_8, true);
+    assertThat(encoded).isEqualTo("/api/user@host:port#section[a-z]/data");
   }
 }

From ded6a217072586f213263d80be4f121321a7302c Mon Sep 17 00:00:00 2001
From: Kevin Davis <kevin.davis@venmo.com>
Date: Fri, 27 Dec 2019 10:12:41 -0500
Subject: [PATCH 5/5] Correcting Test Cases

---
 core/src/test/java/feign/template/QueryTemplateTest.java | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
index 604e70698..85131bfff 100644
--- a/core/src/test/java/feign/template/QueryTemplateTest.java
+++ b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -193,7 +193,7 @@ public void expandCollectionValueWithDollar() {
             Util.UTF_8, CollectionFormat.CSV);
     String expanded = template.expand(Collections.singletonMap("$collection",
         Arrays.asList("1", "2")));
-    /* brackets will be pct-encoded */
-    assertThat(expanded).isEqualToIgnoringCase("$collection=1,2");
+    /* dollar will be pct-encoded */
+    assertThat(expanded).isEqualToIgnoringCase("%24collection=1,2");
   }
 }