All decoders follow rule: if a status is 404 it returns empty or nul…

…l value (OpenFeign#1597)

* All decoders follow rule: if status 404 it returns empty or null value

* Replace decode404 with dismiss404

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/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/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/test/java/feign/AsyncFeignTest.java

@@ -583,12 +583,12 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingExceptionGetWrappedInDecode404Mode() throws Throwable {
+  public void decodingExceptionGetWrappedInDismiss404Mode() throws Throwable {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(DecodeException.class);
     thrown.expectCause(isA(NoSuchElementException.class));;
 
-    TestInterfaceAsync api = new TestInterfaceAsyncBuilder().decode404().decoder(new Decoder() {
+    TestInterfaceAsync api = new TestInterfaceAsyncBuilder().dismiss404().decoder(new Decoder() {
       @Override
       public Object decode(Response response, Type type) throws IOException {
         assertEquals(404, response.status());
@@ -600,11 +600,11 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Throwable {
+  public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Throwable {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(IllegalArgumentException.class);
 
-    TestInterfaceAsync api = new TestInterfaceAsyncBuilder().decode404()
+    TestInterfaceAsync api = new TestInterfaceAsyncBuilder().dismiss404()
         .errorDecoder(new IllegalArgumentExceptionOn404())
         .target("http://localhost:" + server.getPort());
 
@@ -1010,8 +1010,8 @@ TestInterfaceAsyncBuilder errorDecoder(ErrorDecoder errorDecoder) {
       return this;
     }
 
-    TestInterfaceAsyncBuilder decode404() {
-      delegate.decode404();
+    TestInterfaceAsyncBuilder dismiss404() {
+      delegate.dismiss404();
       return this;
     }
 

core/src/test/java/feign/FeignBuilderTest.java

@@ -66,7 +66,7 @@ public void testDefaults() throws Exception {
 
   /** Shows exception handling isn't required to coerce 404 to null or empty */
   @Test
-  public void testDecode404() {
+  public void testDismiss404() {
     server.enqueue(new MockResponse().setResponseCode(404));
     server.enqueue(new MockResponse().setResponseCode(404));
     server.enqueue(new MockResponse().setResponseCode(404));
@@ -75,7 +75,7 @@ public void testDecode404() {
     server.enqueue(new MockResponse().setResponseCode(400));
 
     String url = "http://localhost:" + server.getPort();
-    TestInterface api = Feign.builder().decode404().target(TestInterface.class, url);
+    TestInterface api = Feign.builder().dismiss404().target(TestInterface.class, url);
 
     assertThat(api.getQueues("/")).isEmpty(); // empty, not null!
     assertThat(api.decodedLazyPost().hasNext()).isFalse(); // empty, not null!

core/src/test/java/feign/FeignTest.java

@@ -723,13 +723,13 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingExceptionGetWrappedInDecode404Mode() throws Exception {
+  public void decodingExceptionGetWrappedInDismiss404Mode() throws Exception {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(DecodeException.class);
     thrown.expectCause(isA(NoSuchElementException.class));;
 
     TestInterface api = new TestInterfaceBuilder()
-        .decode404()
+        .dismiss404()
         .decoder(new Decoder() {
           @Override
           public Object decode(Response response, Type type) throws IOException {
@@ -741,12 +741,12 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Exception {
+  public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Exception {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(IllegalArgumentException.class);
 
     TestInterface api = new TestInterfaceBuilder()
-        .decode404()
+        .dismiss404()
         .errorDecoder(new IllegalArgumentExceptionOn404())
         .target("http://localhost:" + server.getPort());
     api.queryMap(Collections.<String, Object>emptyMap());
@@ -1168,8 +1168,8 @@ TestInterfaceBuilder errorDecoder(ErrorDecoder errorDecoder) {
       return this;
     }
 
-    TestInterfaceBuilder decode404() {
-      delegate.decode404();
+    TestInterfaceBuilder dismiss404() {
+      delegate.dismiss404();
       return this;
     }
 

core/src/test/java/feign/FeignUnderAsyncTest.java

@@ -582,13 +582,13 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingExceptionGetWrappedInDecode404Mode() throws Exception {
+  public void decodingExceptionGetWrappedInDismiss404Mode() throws Exception {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(DecodeException.class);
     thrown.expectCause(isA(NoSuchElementException.class));;
 
     TestInterface api = new TestInterfaceBuilder()
-        .decode404()
+        .dismiss404()
         .decoder(new Decoder() {
           @Override
           public Object decode(Response response, Type type) throws IOException {
@@ -600,12 +600,12 @@ public Object decode(Response response, Type type) throws IOException {
   }
 
   @Test
-  public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Exception {
+  public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Exception {
     server.enqueue(new MockResponse().setResponseCode(404));
     thrown.expect(IllegalArgumentException.class);
 
     TestInterface api = new TestInterfaceBuilder()
-        .decode404()
+        .dismiss404()
         .errorDecoder(new IllegalArgumentExceptionOn404())
         .target("http://localhost:" + server.getPort());
     api.queryMap(Collections.<String, Object>emptyMap());
@@ -1000,8 +1000,8 @@ TestInterfaceBuilder errorDecoder(ErrorDecoder errorDecoder) {
       return this;
     }
 
-    TestInterfaceBuilder decode404() {
-      delegate.decode404();
+    TestInterfaceBuilder dismiss404() {
+      delegate.dismiss404();
       return this;
     }
 

core/src/test/java/feign/optionals/OptionalDecoderTests.java

@@ -40,7 +40,7 @@ public void simple404OptionalTest() throws IOException, InterruptedException {
     server.enqueue(new MockResponse().setBody("foo"));
 
     final OptionalInterface api = Feign.builder()
-        .decode404()
+        .dismiss404()
         .decoder(new OptionalDecoder(new Decoder.Default()))
         .target(OptionalInterface.class, server.url("/").toString());
 

gson/src/main/java/feign/gson/GsonDecoder.java

@@ -21,6 +21,7 @@
 import java.lang.reflect.Type;
 import java.util.Collections;
 import feign.Response;
+import feign.Util;
 import feign.codec.Decoder;
 import static feign.Util.UTF_8;
 import static feign.Util.ensureClosed;
@@ -43,6 +44,8 @@ public GsonDecoder(Gson gson) {
 
   @Override
   public Object decode(Response response, Type type) throws IOException {
+    if (response.status() == 404 || response.status() == 204)
+      return Util.emptyValueOf(type);
     if (response.body() == null)
       return null;
     Reader reader = response.body().asReader(UTF_8);

gson/src/test/java/feign/gson/GsonCodecTest.java

@@ -226,15 +226,15 @@ public void customEncoder() {
         + "]");
   }
 
-  /** Enabled via {@link feign.Feign.Builder#decode404()} */
+  /** Enabled via {@link feign.Feign.Builder#dismiss404()} */
   @Test
-  public void notFoundDecodesToNull() throws Exception {
+  public void notFoundDecodesToEmpty() throws Exception {
     Response response = Response.builder()
         .status(404)
         .reason("NOT FOUND")
         .headers(Collections.emptyMap())
         .request(Request.create(HttpMethod.GET, "/api", Collections.emptyMap(), null, Util.UTF_8))
         .build();
-    assertThat((byte[]) new GsonDecoder().decode(response, byte[].class)).isNull();
+    assertThat((byte[]) new GsonDecoder().decode(response, byte[].class)).isEmpty();
   }
 }