Correct Encoding and restore decodeSlash in QueryTemplate

Fixes OpenFeign#1156 Collection Format was encoding query string values unnecessarily due to changes introduced in OpenFeign#1138 and OpenFeign#1139 that encode template values before appending them to the query string. In addition, `decodeSlash` flags that were accidentally removed, have been restored in QueryTemplate.
kdavisk6 · Jan 17, 2020 · 53527ea · 53527ea
1 parent 496c70e
commit 53527ea
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 34 deletions.
diff --git a/core/src/main/java/feign/CollectionFormat.java b/core/src/main/java/feign/CollectionFormat.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 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
@@ -77,7 +77,7 @@ public CharSequence join(String field, Collection<String> values, Charset charse
         builder.append(UriUtils.encode(field, charset));
         if (value != null) {
           builder.append('=');
-          builder.append(UriUtils.encode(value, charset));
+          builder.append(value);
         }
       } else {
         // delimited with a separator character
@@ -87,8 +87,8 @@ public CharSequence join(String field, Collection<String> values, Charset charse
         if (value == null) {
           continue;
         }
-        builder.append(valueCount++ == 0 ? "=" : separator);
-        builder.append(UriUtils.encode(value, charset));
+        builder.append(valueCount++ == 0 ? "=" : UriUtils.encode(separator, charset));
+        builder.append(value);
       }
     }
     return builder;

diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java
@@ -636,9 +636,9 @@ private RequestTemplate appendQuery(String name,
     /* create a new query template out of the information here */
     this.queries.compute(name, (key, queryTemplate) -> {
       if (queryTemplate == null) {
-        return QueryTemplate.create(name, values, this.charset, collectionFormat);
+        return QueryTemplate.create(name, values, this.charset, collectionFormat, this.decodeSlash);
       } else {
-        return QueryTemplate.append(queryTemplate, values, collectionFormat);
+        return QueryTemplate.append(queryTemplate, values, collectionFormat, this.decodeSlash);
       }
     });
     return this;

diff --git a/core/src/main/java/feign/template/QueryTemplate.java b/core/src/main/java/feign/template/QueryTemplate.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 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
@@ -13,6 +13,10 @@
  */
 package feign.template;
 
+import feign.CollectionFormat;
+import feign.Util;
+import feign.template.Template.EncodingOptions;
+import feign.template.Template.ExpansionOptions;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -24,10 +28,6 @@
 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.
@@ -49,7 +49,14 @@ public final class QueryTemplate {
    * @return a QueryTemplate.
    */
   public static QueryTemplate create(String name, Iterable<String> values, Charset charset) {
-    return create(name, values, charset, CollectionFormat.EXPLODED);
+    return create(name, values, charset, CollectionFormat.EXPLODED, true);
+  }
+
+  public static QueryTemplate create(String name,
+                                     Iterable<String> values,
+                                     Charset charset,
+                                     CollectionFormat collectionFormat) {
+    return create(name, values, charset, collectionFormat, true);
   }
 
   /**
@@ -59,12 +66,14 @@ public static QueryTemplate create(String name, Iterable<String> values, Charset
    * @param values in the template.
    * @param charset for the template.
    * @param collectionFormat to use.
+   * @param decodeSlash if slash characters should be decoded
    * @return a QueryTemplate
    */
   public static QueryTemplate create(String name,
                                      Iterable<String> values,
                                      Charset charset,
-                                     CollectionFormat collectionFormat) {
+                                     CollectionFormat collectionFormat,
+                                     boolean decodeSlash) {
     if (Util.isBlank(name)) {
       throw new IllegalArgumentException("name is required.");
     }
@@ -78,7 +87,7 @@ public static QueryTemplate create(String name,
         .filter(Util::isNotBlank)
         .collect(Collectors.toList());
 
-    return new QueryTemplate(name, remaining, charset, collectionFormat);
+    return new QueryTemplate(name, remaining, charset, collectionFormat, decodeSlash);
   }
 
   /**
@@ -90,13 +99,14 @@ public static QueryTemplate create(String name,
    */
   public static QueryTemplate append(QueryTemplate queryTemplate,
                                      Iterable<String> values,
-                                     CollectionFormat collectionFormat) {
+                                     CollectionFormat collectionFormat,
+                                     boolean decodeSlash) {
     List<String> queryValues = new ArrayList<>(queryTemplate.getValues());
     queryValues.addAll(StreamSupport.stream(values.spliterator(), false)
         .filter(Util::isNotBlank)
         .collect(Collectors.toList()));
     return create(queryTemplate.getName(), queryValues, StandardCharsets.UTF_8,
-        collectionFormat);
+        collectionFormat, decodeSlash);
   }
 
   /**
@@ -110,10 +120,11 @@ private QueryTemplate(
       String name,
       Iterable<String> values,
       Charset charset,
-      CollectionFormat collectionFormat) {
+      CollectionFormat collectionFormat,
+      boolean decodeSlash) {
     this.values = new CopyOnWriteArrayList<>();
     this.name = new Template(name, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.REQUIRED,
-        false, charset);
+        !decodeSlash, charset);
     this.collectionFormat = collectionFormat;
 
     /* parse each value into a template chunk for resolution later */
@@ -128,7 +139,7 @@ private QueryTemplate(
               value,
               ExpansionOptions.REQUIRED,
               EncodingOptions.REQUIRED,
-              false,
+              !decodeSlash,
               charset));
     }
 

diff --git a/core/src/main/java/feign/template/UriUtils.java b/core/src/main/java/feign/template/UriUtils.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 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
@@ -15,6 +15,7 @@
 
 import feign.Util;
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
 import java.nio.charset.Charset;
@@ -153,16 +154,21 @@ private static String encodeChunk(String value, Charset charset, boolean allowRe
     }
 
     byte[] data = value.getBytes(charset);
-    ByteArrayOutputStream encoded = new ByteArrayOutputStream();
-    for (byte b : data) {
-      if (isUnreserved(b) || (isReserved(b) && allowReserved)) {
-        encoded.write(b);
-      } else {
-        /* percent encode the byte */
-        pctEncode(b, encoded);
+    try (ByteArrayOutputStream bos = new ByteArrayOutputStream()) {
+      for (byte b : data) {
+        if (isUnreserved((char) b)) {
+          bos.write(b);
+        } else if (isReserved((char) b) && allowReserved) {
+          bos.write(b);
+        } else {
+          pctEncode(b, bos);
+        }
       }
+      return new String(bos.toByteArray());
+    } catch (IOException ioe) {
+      throw new IllegalStateException("Error occurred during encoding of the uri: "
+          + ioe.getMessage(), ioe);
     }
-    return new String(encoded.toByteArray());
   }
 
   /**

diff --git a/core/src/test/java/feign/RequestTemplateTest.java b/core/src/test/java/feign/RequestTemplateTest.java
@@ -476,6 +476,15 @@ public void slashShouldNotBeAppendedForMatrixParams() {
         .uri("/path;key1=value1;key2=value2", true);
 
     assertThat(template.url()).isEqualTo("/path;key1=value1;key2=value2");
+  }
 
+  @Test
+  public void encodedReserved() {
+    RequestTemplate template = new RequestTemplate();
+    template.uri("/get?url={url}");
+    template.method(HttpMethod.GET);
+    template = template.resolve(Collections.singletonMap("url", "https://www.google.com"));
+    Request request = template.request();
+    assertThat(request).isNotNull();
   }
 }
diff --git a/core/src/test/java/feign/TargetTest.java b/core/src/test/java/feign/TargetTest.java
@@ -41,7 +41,7 @@ public void baseCaseQueryParamsArePercentEncoded() throws InterruptedException {
 
     Feign.builder().target(TestQuery.class, baseUrl).get("slash/foo", "slash/bar");
 
-    assertThat(server.takeRequest()).hasPath("/default/slash/foo?query=slash%2Fbar");
+    assertThat(server.takeRequest()).hasPath("/default/slash/foo?query=slash/bar");
   }
 
   /**

diff --git a/core/src/test/java/feign/template/QueryTemplateTest.java b/core/src/test/java/feign/template/QueryTemplateTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 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
@@ -93,7 +93,7 @@ public void collectionFormat() {
         QueryTemplate
             .create("name", Arrays.asList("James", "Jason"), Util.UTF_8, CollectionFormat.CSV);
     String expanded = template.expand(Collections.emptyMap());
-    assertThat(expanded).isEqualToIgnoringCase("name=James,Jason");
+    assertThat(expanded).isEqualToIgnoringCase("name=James%2CJason");
   }
 
   @Test
@@ -183,7 +183,7 @@ public void expandCollectionValueWithBrackets() {
     String expanded = template.expand(Collections.singletonMap("collection[]",
         Arrays.asList("1", "2")));
     /* brackets will be pct-encoded */
-    assertThat(expanded).isEqualToIgnoringCase("collection%5B%5D=1,2");
+    assertThat(expanded).isEqualToIgnoringCase("collection%5B%5D=1%2C2");
   }
 
   @Test
@@ -194,6 +194,6 @@ public void expandCollectionValueWithDollar() {
     String expanded = template.expand(Collections.singletonMap("$collection",
         Arrays.asList("1", "2")));
     /* dollar will be pct-encoded */
-    assertThat(expanded).isEqualToIgnoringCase("%24collection=1,2");
+    assertThat(expanded).isEqualToIgnoringCase("%24collection=1%2C2");
   }
 }
diff --git a/core/src/test/java/feign/template/UriTemplateTest.java b/core/src/test/java/feign/template/UriTemplateTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 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
@@ -291,4 +291,12 @@ public void testLiteralTemplateWithQueryString() {
     String expanded = uriTemplate.expand(Collections.emptyMap());
     assertThat(expanded).isEqualToIgnoringCase("https://api.example.com?wsdl");
   }
+
+  @Test
+  public void encodeReserved() {
+    String template = "/get?url={url}";
+    UriTemplate uriTemplate = UriTemplate.create(template, Util.UTF_8);
+    String expanded = uriTemplate.expand(Collections.singletonMap("url", "https://www.google.com"));
+    assertThat(expanded).isEqualToIgnoringCase("/get?url=https%3A%2F%2Fwww.google.com");
+  }
 }