Correct Encoding and restore decodeSlash in QueryTemplate (#1160)

Fixes #1156 Collection Format was encoding query string values unnecessarily due to changes introduced in #1138 and #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. * Restoring decodeSlash in QueryTemplate * Correcting Readme with regards to decodeSlash usage
OpenFeign · Jan 18, 2020 · 819b2df · 819b2df
1 parent 49e1373
commit 819b2df
Show file tree

Hide file tree

Showing 32 changed files with 98 additions and 81 deletions.
diff --git a/README.md b/README.md
@@ -216,8 +216,8 @@ http://localhost:8080/test
 See [Advanced Usage](#advanced-usage) for more examples.
 
 > **What about slashes? `/`**
->
-> @RequestLine and @QueryMap Templates do encode slash `/` characters by default.  To change this behavior, set the `decodeSlash` property on the `@RequestLine` to `true`.  
+> 
+> @RequestLine templates do not encode slash `/` characters by default.  To change this behavior, set the `decodeSlash` property on the `@RequestLine` to `false`.  
 
 > **What about plus? `+`**
 >

diff --git a/benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java b/benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.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
@@ -20,17 +20,12 @@
 import feign.Logger.Level;
 import feign.Response;
 import feign.Retryer;
-import io.reactivex.netty.protocol.http.HttpHandlerNames;
 import io.reactivex.netty.protocol.http.server.HttpServer;
-import io.reactivex.netty.protocol.http.server.HttpServerRequest;
-import io.reactivex.netty.protocol.http.server.HttpServerResponse;
-import io.reactivex.netty.protocol.http.server.RequestHandler;
 import java.io.IOException;
 import java.util.concurrent.TimeUnit;
 import io.netty.buffer.ByteBuf;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
-import okhttp3.internal.http.HttpHeaders;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.BenchmarkMode;
 import org.openjdk.jmh.annotations.Fork;
@@ -42,7 +37,6 @@
 import org.openjdk.jmh.annotations.State;
 import org.openjdk.jmh.annotations.TearDown;
 import org.openjdk.jmh.annotations.Warmup;
-import rx.Observable;
 
 @Measurement(iterations = 5, time = 1)
 @Warmup(iterations = 10, time = 1)

diff --git a/core/src/main/java/feign/Client.java b/core/src/main/java/feign/Client.java
@@ -19,7 +19,6 @@
 import static feign.Util.ENCODING_GZIP;
 import static feign.Util.checkArgument;
 import static feign.Util.checkNotNull;
-import static feign.Util.isBlank;
 import static feign.Util.isNotBlank;
 import static java.lang.String.format;
 import java.io.IOException;

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
@@ -327,6 +327,13 @@ public RequestTemplate decodeSlash(boolean decodeSlash) {
     this.decodeSlash = decodeSlash;
     this.uriTemplate =
         UriTemplate.create(this.uriTemplate.toString(), !this.decodeSlash, this.charset);
+    if (!this.queries.isEmpty()) {
+      this.queries.replaceAll((key, queryTemplate) -> QueryTemplate.create(
+          /* replace the current template with new ones honoring the decode value */
+          queryTemplate.getName(), queryTemplate.getValues(), charset, collectionFormat,
+          decodeSlash));
+
+    }
     return this;
   }
 
@@ -636,9 +643,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/Types.java b/core/src/main/java/feign/Types.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
@@ -21,7 +21,6 @@
 import java.lang.reflect.TypeVariable;
 import java.lang.reflect.WildcardType;
 import java.util.Arrays;
-import java.util.Map;
 import java.util.NoSuchElementException;
 
 /**

diff --git a/core/src/main/java/feign/Util.java b/core/src/main/java/feign/Util.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
@@ -37,7 +37,6 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Predicate;

diff --git a/core/src/main/java/feign/stream/StreamDecoder.java b/core/src/main/java/feign/stream/StreamDecoder.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
@@ -21,7 +21,6 @@
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.util.Iterator;
-import java.util.Spliterator;
 import java.util.Spliterators;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;

diff --git a/core/src/main/java/feign/template/Expression.java b/core/src/main/java/feign/template/Expression.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,7 +13,6 @@
  */
 package feign.template;
 
-import feign.CollectionFormat;
 import java.util.Optional;
 import java.util.regex.Pattern;
 

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(), charset);
+    } 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
@@ -19,8 +19,6 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
-import feign.Request.HttpMethod;
-import feign.template.UriUtils;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -476,6 +474,24 @@ public void slashShouldNotBeAppendedForMatrixParams() {
         .uri("/path;key1=value1;key2=value2", true);
 
     assertThat(template.url()).isEqualTo("/path;key1=value1;key2=value2");
+  }
 
+  @Test
+  public void encodedReservedPreserveSlash() {
+    RequestTemplate template = new RequestTemplate();
+    template.uri("/get?url={url}");
+    template.method(HttpMethod.GET);
+    template = template.resolve(Collections.singletonMap("url", "https://www.google.com"));
+    assertThat(template.url()).isEqualToIgnoringCase("/get?url=https%3A//www.google.com");
+  }
+
+  @Test
+  public void encodedReservedEncodeSlash() {
+    RequestTemplate template = new RequestTemplate();
+    template.uri("/get?url={url}");
+    template.decodeSlash(false);
+    template.method(HttpMethod.GET);
+    template = template.resolve(Collections.singletonMap("url", "https://www.google.com"));
+    assertThat(template.url()).isEqualToIgnoringCase("/get?url=https%3A%2F%2Fwww.google.com");
   }
 }
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/assertj/RecordedRequestAssert.java b/core/src/test/java/feign/assertj/RecordedRequestAssert.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,7 +15,6 @@
 
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
 import okhttp3.Headers;
 import okhttp3.mockwebserver.RecordedRequest;
 import org.assertj.core.api.AbstractAssert;

diff --git a/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java b/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java
@@ -28,7 +28,6 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
-import org.springframework.util.StringUtils;
 
 @SuppressWarnings("deprecation")
 public class DefaultErrorDecoderTest {

diff --git a/core/src/test/java/feign/stream/StreamDecoderTest.java b/core/src/test/java/feign/stream/StreamDecoderTest.java
@@ -14,7 +14,6 @@
 package feign.stream;
 
 import com.fasterxml.jackson.core.type.TypeReference;
-import com.fasterxml.jackson.databind.ObjectMapper;
 import feign.Feign;
 import feign.Request;
 import feign.Request.HttpMethod;