Replaced comma with Constant Delimiter in Template (OpenFeign#930)

Replaced comma with Constant Delimiter in Template

Fixes OpenFeign#924

Commas were used to identify iterable content, which conflicted when
a comma delimited literal was provided during expansion.  This change
switches commas for semi-colons, which are considered reserved secondary
delimiters in RFC 6750 and should not be used without being pct-encoded.

Should be a safer choice.

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

@@ -105,7 +105,7 @@ String expand(Object variable, boolean encode) {
         for (Object item : ((Iterable) variable)) {
           items.add((encode) ? encode(item) : item.toString());
         }
-        expanded.append(String.join(",", items));
+        expanded.append(String.join(Template.COLLECTION_DELIMITER, items));
       } else {
         expanded.append((encode) ? encode(variable) : variable);
       }

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

@@ -23,7 +23,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 
@@ -32,8 +31,7 @@
  */
 public final class QueryTemplate extends Template {
 
-  public static final String UNDEF = "undef";
-  /* cache a copy of the variables for lookup later */
+  private static final String UNDEF = "undef";
   private List<String> values;
   private final Template name;
   private final CollectionFormat collectionFormat;
@@ -64,7 +62,7 @@ public static QueryTemplate create(String name,
                                      Iterable<String> values,
                                      Charset charset,
                                      CollectionFormat collectionFormat) {
-    if (name == null || name.isEmpty()) {
+    if (Util.isBlank(name)) {
       throw new IllegalArgumentException("name is required.");
     }
 
@@ -82,7 +80,7 @@ public static QueryTemplate create(String name,
     while (iterator.hasNext()) {
       template.append(iterator.next());
       if (iterator.hasNext()) {
-        template.append(",");
+        template.append(COLLECTION_DELIMITER);
       }
     }
 
@@ -181,7 +179,7 @@ private String queryString(String name, String values) {
     }
 
     /* covert the comma separated values into a value query string */
-    List<String> resolved = Arrays.stream(values.split(","))
+    List<String> resolved = Arrays.stream(values.split(COLLECTION_DELIMITER))
         .filter(Objects::nonNull)
         .filter(s -> !UNDEF.equalsIgnoreCase(s))
         .collect(Collectors.toList());

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

@@ -32,6 +32,13 @@
  */
 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;

core/src/test/java/feign/template/QueryTemplateTest.java

@@ -118,4 +118,50 @@ public void expandNameUnresolved() {
     String expanded = template.expand(Collections.singletonMap("name", "firsts"));
     assertThat(expanded).isEqualToIgnoringCase("%7Bparameter%7D=James&%7Bparameter%7D=Jason");
   }
+
+  @Test
+  public void expandSingleValueWithComma() {
+    QueryTemplate template =
+        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");
+  }
+
+  @Test
+  public void expandSingleValueWithPipe() {
+    QueryTemplate template =
+        QueryTemplate.create("collection",
+            Collections.singletonList("{collection}"), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("collection", "one|two|three"));
+    assertThat(expanded).isEqualToIgnoringCase("collection=one%7Ctwo%7Cthree");
+  }
+
+  @Test
+  public void expandSingleValueWithSpace() {
+    QueryTemplate template =
+        QueryTemplate.create("collection",
+            Collections.singletonList("{collection}"), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("collection", "one two three"));
+    assertThat(expanded).isEqualToIgnoringCase("collection=one%20two%20three");
+  }
+
+  @Test
+  public void expandSingleValueWithTab() {
+    QueryTemplate template =
+        QueryTemplate.create("collection",
+            Collections.singletonList("{collection}"), Util.UTF_8);
+    String expanded = template.expand(Collections.singletonMap("collection", "one\ttwo\tthree"));
+    assertThat(expanded).isEqualToIgnoringCase("collection=one%09two%09three");
+  }
+
+  @Test
+  public void expandSingleValueWithJson() {
+    QueryTemplate template =
+        QueryTemplate.create("json", Collections.singletonList("{json}"), Util.UTF_8);
+    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");
+  }
 }