Merge branch 'master' into OpenFeignGH-1319/reserved-characters

benchmark/pom.xml

@@ -30,7 +30,7 @@
     <jmh.version>1.34</jmh.version>
     <rx.netty.version>0.5.3</rx.netty.version>
     <rx.java.version>1.3.8</rx.java.version>
-    <netty.version>4.1.72.Final</netty.version>
+    <netty.version>4.1.74.Final</netty.version>
 
     <main.basedir>${project.basedir}/..</main.basedir>
   </properties>

core/src/main/java/feign/AsyncFeign.java

@@ -68,7 +68,7 @@ public static class AsyncBuilder<C> {
 
     private Decoder decoder = new Decoder.Default();
     private ErrorDecoder errorDecoder = new ErrorDecoder.Default();
-    private boolean decode404;
+    private boolean dismiss404;
     private boolean closeAfterDecode = true;
 
     public AsyncBuilder() {
@@ -104,9 +104,18 @@ public AsyncBuilder<C> decoder(Decoder decoder) {
 
     /**
      * @see Builder#decode404()
+     * @deprecated
      */
     public AsyncBuilder<C> decode404() {
-      this.decode404 = true;
+      this.dismiss404 = true;
+      return this;
+    }
+
+    /**
+     * @see Builder#dismiss404()
+     */
+    public AsyncBuilder<C> dismiss404() {
+      this.dismiss404 = true;
       return this;
     }
 
@@ -256,7 +265,7 @@ protected AsyncFeign(AsyncBuilder<C> asyncBuilder) {
         asyncBuilder.logger,
         asyncBuilder.decoder,
         asyncBuilder.errorDecoder,
-        asyncBuilder.decode404,
+        asyncBuilder.dismiss404,
         asyncBuilder.closeAfterDecode);
 
     asyncBuilder.builder.client(this::stageExecution);

core/src/main/java/feign/AsyncResponseHandler.java

@@ -37,17 +37,17 @@ class AsyncResponseHandler {
 
   private final Decoder decoder;
   private final ErrorDecoder errorDecoder;
-  private final boolean decode404;
+  private final boolean dismiss404;
   private final boolean closeAfterDecode;
 
   AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
-      boolean decode404, boolean closeAfterDecode) {
+      boolean dismiss404, boolean closeAfterDecode) {
     super();
     this.logLevel = logLevel;
     this.logger = logger;
     this.decoder = decoder;
     this.errorDecoder = errorDecoder;
-    this.decode404 = decode404;
+    this.dismiss404 = dismiss404;
     this.closeAfterDecode = closeAfterDecode;
   }
 
@@ -88,7 +88,7 @@ void handleResponse(CompletableFuture<Object> resultFuture,
           shouldClose = closeAfterDecode;
           resultFuture.complete(result);
         }
-      } else if (decode404 && response.status() == 404 && !isVoidType(returnType)) {
+      } else if (dismiss404 && response.status() == 404 && !isVoidType(returnType)) {
         final Object result = decode(response, returnType);
         shouldClose = closeAfterDecode;
         resultFuture.complete(result);

core/src/main/java/feign/Client.java

@@ -20,6 +20,7 @@
 import static feign.Util.checkArgument;
 import static feign.Util.checkNotNull;
 import static feign.Util.isNotBlank;
+import static java.lang.String.CASE_INSENSITIVE_ORDER;
 import static java.lang.String.format;
 import feign.Request.Options;
 import java.io.IOException;
@@ -31,9 +32,9 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Base64;
 import java.util.Collection;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeMap;
 import java.util.zip.DeflaterOutputStream;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
@@ -114,7 +115,7 @@ Response convertResponse(HttpURLConnection connection, Request request) throws I
             connection.getRequestMethod(), connection.getURL()));
       }
 
-      Map<String, Collection<String>> headers = new LinkedHashMap<>();
+      Map<String, Collection<String>> headers = new TreeMap<>(CASE_INSENSITIVE_ORDER);
       for (Map.Entry<String, List<String>> field : connection.getHeaderFields().entrySet()) {
         // response message
         if (field.getKey() != null) {
@@ -130,9 +131,9 @@ Response convertResponse(HttpURLConnection connection, Request request) throws I
       if (status >= 400) {
         stream = connection.getErrorStream();
       } else {
-        if (this.isGzip(connection.getHeaderFields().get(CONTENT_ENCODING))) {
+        if (this.isGzip(headers.get(CONTENT_ENCODING))) {
           stream = new GZIPInputStream(connection.getInputStream());
-        } else if (this.isDeflate(connection.getHeaderFields().get(CONTENT_ENCODING))) {
+        } else if (this.isDeflate(headers.get(CONTENT_ENCODING))) {
           stream = new InflaterInputStream(connection.getInputStream());
         } else {
           stream = connection.getInputStream();

core/src/main/java/feign/Contract.java

@@ -149,8 +149,10 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
       }
 
       if (data.headerMapIndex() != null) {
-        checkMapString("HeaderMap", parameterTypes[data.headerMapIndex()],
-            genericParameterTypes[data.headerMapIndex()]);
+        // check header map parameter for map type
+        if (Map.class.isAssignableFrom(parameterTypes[data.headerMapIndex()])) {
+          checkMapKeys("HeaderMap", genericParameterTypes[data.headerMapIndex()]);
+        }
       }
 
       if (data.queryMapIndex() != null) {
@@ -308,7 +310,6 @@ public Default() {
         checkState(data.queryMapIndex() == null,
             "QueryMap annotation was present on multiple parameters.");
         data.queryMapIndex(paramIndex);
-        data.queryMapEncoded(queryMap.encoded());
       });
       super.registerParameterAnnotation(HeaderMap.class, (queryMap, data, paramIndex) -> {
         checkState(data.headerMapIndex() == null,

core/src/main/java/feign/Feign.java

@@ -110,7 +110,7 @@ public static class Builder {
     private Options options = new Options();
     private InvocationHandlerFactory invocationHandlerFactory =
         new InvocationHandlerFactory.Default();
-    private boolean decode404;
+    private boolean dismiss404;
     private boolean closeAfterDecode = true;
     private ExceptionPropagationPolicy propagationPolicy = NONE;
     private boolean forceDecoding = false;
@@ -180,9 +180,32 @@ public Builder mapAndDecode(ResponseMapper mapper, Decoder decoder) {
      * custom {@link #client(Client) client}.
      *
      * @since 8.12
+     * @deprecated
      */
     public Builder decode404() {
-      this.decode404 = true;
+      this.dismiss404 = true;
+      return this;
+    }
+
+    /**
+     * This flag indicates that the {@link #decoder(Decoder) decoder} should process responses with
+     * 404 status, specifically returning null or empty instead of throwing {@link FeignException}.
+     *
+     * <p/>
+     * All first-party (ex gson) decoders return well-known empty values defined by
+     * {@link Util#emptyValueOf}. To customize further, wrap an existing {@link #decoder(Decoder)
+     * decoder} or make your own.
+     *
+     * <p/>
+     * This flag only works with 404, as opposed to all or arbitrary status codes. This was an
+     * explicit decision: 404 -> empty is safe, common and doesn't complicate redirection, retry or
+     * fallback policy. If your server returns a different status for not-found, correct via a
+     * custom {@link #client(Client) client}.
+     *
+     * @since 11.9
+     */
+    public Builder dismiss404() {
+      this.dismiss404 = true;
       return this;
     }
 
@@ -285,7 +308,7 @@ public Feign build() {
 
       SynchronousMethodHandler.Factory synchronousMethodHandlerFactory =
           new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors, logger,
-              logLevel, decode404, closeAfterDecode, propagationPolicy, forceDecoding);
+              logLevel, dismiss404, closeAfterDecode, propagationPolicy, forceDecoding);
       ParseHandlersByName handlersByName =
           new ParseHandlersByName(contract, options, encoder, decoder, queryMapEncoder,
               errorDecoder, synchronousMethodHandlerFactory);

core/src/main/java/feign/MethodMetadata.java

@@ -29,7 +29,6 @@ public final class MethodMetadata implements Serializable {
   private Integer bodyIndex;
   private Integer headerMapIndex;
   private Integer queryMapIndex;
-  private boolean queryMapEncoded;
   private boolean alwaysEncodeBody;
   private transient Type bodyType;
   private final RequestTemplate template = new RequestTemplate();
@@ -110,15 +109,6 @@ public MethodMetadata queryMapIndex(Integer queryMapIndex) {
     return this;
   }
 
-  public boolean queryMapEncoded() {
-    return queryMapEncoded;
-  }
-
-  public MethodMetadata queryMapEncoded(boolean queryMapEncoded) {
-    this.queryMapEncoded = queryMapEncoded;
-    return this;
-  }
-
   @Experimental
   public boolean alwaysEncodeBody() {
     return alwaysEncodeBody;

core/src/main/java/feign/QueryMap.java

@@ -53,15 +53,21 @@
  * Once this conversion is applied, the query keys and resulting String values follow the same
  * contract as if they were set using {@link RequestTemplate#query(String, String...)}.
  */
-@SuppressWarnings("deprecation")
 @Retention(RUNTIME)
 @java.lang.annotation.Target(PARAMETER)
 public @interface QueryMap {
 
   /**
    * Specifies whether parameter names and values are already encoded.
+   * <p>
+   * Deprecation: there are two options
+   * <ul>
+   * <li>if name or value are already encoded we do nothing;</li>
+   * <li>if name or value are not encoded we encode them.</li>
+   * </ul>
    *
    * @see Param#encoded
+   * @deprecated
    */
   boolean encoded() default false;
 }

core/src/main/java/feign/ReflectiveFeign.java

@@ -239,8 +239,10 @@ public RequestTemplate create(Object[] argv) {
       }
 
       if (metadata.headerMapIndex() != null) {
-        template =
-            addHeaderMapHeaders((Map<String, Object>) argv[metadata.headerMapIndex()], template);
+        // add header map parameters for a resolution of the user pojo object
+        Object value = argv[metadata.headerMapIndex()];
+        Map<String, Object> headerMap = toQueryMap(value);
+        template = addHeaderMapHeaders(headerMap, template);
       }
 
       return template;
@@ -302,27 +304,26 @@ private RequestTemplate addQueryMapQueryParameters(Map<String, Object> queryMap,
       for (Entry<String, Object> currEntry : queryMap.entrySet()) {
         Collection<String> values = new ArrayList<String>();
 
-        boolean encoded = metadata.queryMapEncoded();
         Object currValue = currEntry.getValue();
         if (currValue instanceof Iterable<?>) {
           Iterator<?> iter = ((Iterable<?>) currValue).iterator();
           while (iter.hasNext()) {
             Object nextObject = iter.next();
-            values.add(nextObject == null ? null
-                : encoded ? nextObject.toString()
-                    : UriUtils.encode(nextObject.toString()));
+            values.add(nextObject == null ? null : UriUtils.encode(nextObject.toString()));
           }
         } else if (currValue instanceof Object[]) {
           for (Object value : (Object[]) currValue) {
-            values.add(value == null ? null
-                : encoded ? value.toString() : UriUtils.encode(value.toString()));
+            values.add(value == null ? null : UriUtils.encode(value.toString()));
           }
         } else {
-          values.add(currValue == null ? null
-              : encoded ? currValue.toString() : UriUtils.encode(currValue.toString()));
+          if (currValue != null) {
+            values.add(UriUtils.encode(currValue.toString()));
+          }
         }
 
-        mutable.query(encoded ? currEntry.getKey() : UriUtils.encode(currEntry.getKey()), values);
+        if (values.size() > 0) {
+          mutable.query(UriUtils.encode(currEntry.getKey()), values);
+        }
       }
       return mutable;
     }

core/src/main/java/feign/Request.java

@@ -36,7 +36,7 @@ public enum HttpMethod {
   public enum ProtocolVersion {
     HTTP_1_0("HTTP/1.0"), HTTP_1_1("HTTP/1.1"), HTTP_2("HTTP/2.0"), MOCK;
 
-    String protocolVersion;
+    final String protocolVersion;
 
     ProtocolVersion() {
       protocolVersion = name();

core/src/main/java/feign/SynchronousMethodHandler.java

@@ -51,7 +51,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
       List<RequestInterceptor> requestInterceptors, Logger logger,
       Logger.Level logLevel, MethodMetadata metadata,
       RequestTemplate.Factory buildTemplateFromArgs, Options options,
-      Decoder decoder, ErrorDecoder errorDecoder, boolean decode404,
+      Decoder decoder, ErrorDecoder errorDecoder, boolean dismiss404,
       boolean closeAfterDecode, ExceptionPropagationPolicy propagationPolicy,
       boolean forceDecoding) {
 
@@ -75,7 +75,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
     } else {
       this.decoder = null;
       this.asyncResponseHandler = new AsyncResponseHandler(logLevel, logger, decoder, errorDecoder,
-          decode404, closeAfterDecode);
+          dismiss404, closeAfterDecode);
     }
   }
 
@@ -181,20 +181,20 @@ static class Factory {
     private final List<RequestInterceptor> requestInterceptors;
     private final Logger logger;
     private final Logger.Level logLevel;
-    private final boolean decode404;
+    private final boolean dismiss404;
     private final boolean closeAfterDecode;
     private final ExceptionPropagationPolicy propagationPolicy;
     private final boolean forceDecoding;
 
     Factory(Client client, Retryer retryer, List<RequestInterceptor> requestInterceptors,
-        Logger logger, Logger.Level logLevel, boolean decode404, boolean closeAfterDecode,
+        Logger logger, Logger.Level logLevel, boolean dismiss404, boolean closeAfterDecode,
         ExceptionPropagationPolicy propagationPolicy, boolean forceDecoding) {
       this.client = checkNotNull(client, "client");
       this.retryer = checkNotNull(retryer, "retryer");
       this.requestInterceptors = checkNotNull(requestInterceptors, "requestInterceptors");
       this.logger = checkNotNull(logger, "logger");
       this.logLevel = checkNotNull(logLevel, "logLevel");
-      this.decode404 = decode404;
+      this.dismiss404 = dismiss404;
       this.closeAfterDecode = closeAfterDecode;
       this.propagationPolicy = propagationPolicy;
       this.forceDecoding = forceDecoding;
@@ -208,7 +208,7 @@ public MethodHandler create(Target<?> target,
                                 ErrorDecoder errorDecoder) {
       return new SynchronousMethodHandler(target, client, retryer, requestInterceptors, logger,
           logLevel, md, buildTemplateFromArgs, options, decoder,
-          errorDecoder, decode404, closeAfterDecode, propagationPolicy, forceDecoding);
+          errorDecoder, dismiss404, closeAfterDecode, propagationPolicy, forceDecoding);
     }
   }
 }

core/src/main/java/feign/Util.java

@@ -241,7 +241,7 @@ public static Type resolveLastTypeParameter(Type genericContext, Class<?> supert
    * </ul>
    *
    * <p/>
-   * When {@link Feign.Builder#decode404() decoding HTTP 404 status}, you'll need to teach decoders
+   * When {@link Feign.Builder#dismiss404() decoding HTTP 404 status}, you'll need to teach decoders
    * a default empty value for a type. This method cheaply supports typical types by only looking at
    * the raw type (vs type hierarchy). Decorate for sophistication.
    */

core/src/main/java/feign/codec/Decoder.java

@@ -58,7 +58,7 @@
  * List<Foo>}. <br/>
  * <h3>Note on exception propagation</h3> Exceptions thrown by {@link Decoder}s get wrapped in a
  * {@link DecodeException} unless they are a subclass of {@link FeignException} already, and unless
- * the client was configured with {@link Feign.Builder#decode404()}.
+ * the client was configured with {@link Feign.Builder#dismiss404()}.
  */
 public interface Decoder {
 

core/src/main/java/feign/codec/ErrorDecoder.java

@@ -64,7 +64,7 @@
  * <p/>
  * It is commonly the case that 404 (Not Found) status has semantic value in HTTP apis. While the
  * default behavior is to raise exeception, users can alternatively enable 404 processing via
- * {@link feign.Feign.Builder#decode404()}.
+ * {@link feign.Feign.Builder#dismiss404()}.
  */
 public interface ErrorDecoder {
 

core/src/main/java/feign/codec/StringDecoder.java

@@ -24,7 +24,7 @@ public class StringDecoder implements Decoder {
   @Override
   public Object decode(Response response, Type type) throws IOException {
     Response.Body body = response.body();
-    if (body == null) {
+    if (response.status() == 404 || response.status() == 204 || body == null) {
       return null;
     }
     if (String.class.equals(type)) {

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

@@ -329,7 +329,7 @@ public String next() {
   public enum EncodingOptions {
     REQUIRED(true), NOT_REQUIRED(false);
 
-    private boolean shouldEncode;
+    private final boolean shouldEncode;
 
     EncodingOptions(boolean shouldEncode) {
       this.shouldEncode = shouldEncode;