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

Author: kdavisk6

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? `+`**
 >

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)

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;

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;

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;

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;
 
 /**

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;

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;

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;
 

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));
     }