add doNotCloseAfterDecode flag to Feign builder (OpenFeign#649)

benmanbs · adriancole · commit a2addbf2cae2 · 2018-03-06T07:01:43.000+08:00
This commit adds the `doNotCloseAfterDecode` flag to the Feign builder object. This allows you to lazily evaluate the response in your Decoder, in order to support Iterators or Java 8 Streams. This is a pretty light weight change, to support a do-it-yourself approach to lazy instantiation. Fixes OpenFeign#514.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,6 @@
+### Version 9.6
+* Feign builder now supports flag `doNotCloseAfterDecode` to support lazy iteration of responses.
+
 ### Version 9.5.1
 * When specified, Content-Type header is now included on OkHttp requests lacking a body.
 * Sets empty HttpEntity if apache request body is null.
diff --git a/core/src/main/java/feign/Feign.java b/core/src/main/java/feign/Feign.java
@@ -108,6 +108,7 @@ public static class Builder {
     private InvocationHandlerFactory invocationHandlerFactory =
         new InvocationHandlerFactory.Default();
     private boolean decode404;
+    private boolean closeAfterDecode = true;
 
     public Builder logLevel(Logger.Level logLevel) {
       this.logLevel = logLevel;
@@ -210,6 +211,25 @@ public Builder invocationHandlerFactory(InvocationHandlerFactory invocationHandl
       return this;
     }
 
+    /**
+     * This flag indicates that the response should not be automatically closed
+     * upon completion of decoding the message. This should be set if you plan on
+     * processing the response into a lazy-evaluated construct, such as a
+     * {@link java.util.Iterator}.
+     *
+     * </p>Feign standard decoders do not have built in support for this flag. If
+     * you are using this flag, you MUST also use a custom Decoder, and be sure to
+     * close all resources appropriately somewhere in the Decoder (you can use
+     * {@link Util.ensureClosed} for convenience).
+     *
+     * @since 9.6
+     *
+     */
+    public Builder doNotCloseAfterDecode() {
+      this.closeAfterDecode = false;
+      return this;
+    }
+
     public <T> T target(Class<T> apiType, String url) {
       return target(new HardCodedTarget<T>(apiType, url));
     }
@@ -221,7 +241,7 @@ public <T> T target(Target<T> target) {
     public Feign build() {
       SynchronousMethodHandler.Factory synchronousMethodHandlerFactory =
           new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors, logger,
-                                               logLevel, decode404);
+                                               logLevel, decode404, closeAfterDecode);
       ParseHandlersByName handlersByName =
           new ParseHandlersByName(contract, options, encoder, decoder,
                                   errorDecoder, synchronousMethodHandlerFactory);
diff --git a/core/src/main/java/feign/SynchronousMethodHandler.java b/core/src/main/java/feign/SynchronousMethodHandler.java
@@ -46,12 +46,14 @@ final class SynchronousMethodHandler implements MethodHandler {
   private final Decoder decoder;
   private final ErrorDecoder errorDecoder;
   private final boolean decode404;
+  private final boolean closeAfterDecode;
 
   private SynchronousMethodHandler(Target<?> target, Client client, Retryer retryer,
                                    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 decode404,
+                                   boolean closeAfterDecode) {
     this.target = checkNotNull(target, "target");
     this.client = checkNotNull(client, "client for %s", target);
     this.retryer = checkNotNull(retryer, "retryer for %s", target);
@@ -65,6 +67,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
     this.errorDecoder = checkNotNull(errorDecoder, "errorDecoder for %s", target);
     this.decoder = checkNotNull(decoder, "decoder for %s", target);
     this.decode404 = decode404;
+    this.closeAfterDecode = closeAfterDecode;
   }
 
   @Override
@@ -130,9 +133,11 @@ Object executeAndDecode(RequestTemplate template) throws Throwable {
         if (void.class == metadata.returnType()) {
           return null;
         } else {
+          shouldClose = closeAfterDecode;
           return decode(response);
         }
       } else if (decode404 && response.status() == 404 && void.class != metadata.returnType()) {
+        shouldClose = closeAfterDecode;
         return decode(response);
       } else {
         throw errorDecoder.decode(metadata.configKey(), response);
@@ -178,23 +183,25 @@ static class Factory {
     private final Logger logger;
     private final Logger.Level logLevel;
     private final boolean decode404;
+    private final boolean closeAfterDecode;
 
     Factory(Client client, Retryer retryer, List<RequestInterceptor> requestInterceptors,
-            Logger logger, Logger.Level logLevel, boolean decode404) {
+            Logger logger, Logger.Level logLevel, boolean decode404, boolean closeAfterDecode) {
       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.closeAfterDecode = closeAfterDecode;
     }
 
     public MethodHandler create(Target<?> target, MethodMetadata md,
                                 RequestTemplate.Factory buildTemplateFromArgs,
                                 Options options, Decoder decoder, ErrorDecoder errorDecoder) {
       return new SynchronousMethodHandler(target, client, retryer, requestInterceptors, logger,
                                           logLevel, md, buildTemplateFromArgs, options, decoder,
-                                          errorDecoder, decode404);
+                                          errorDecoder, decode404, closeAfterDecode);
     }
   }
 }
diff --git a/core/src/test/java/feign/FeignBuilderTest.java b/core/src/test/java/feign/FeignBuilderTest.java
@@ -21,10 +21,12 @@
 import org.junit.Rule;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.util.Arrays;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -35,6 +37,8 @@
 import static feign.assertj.MockWebServerAssertions.assertThat;
 import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class FeignBuilderTest {
@@ -242,6 +246,62 @@ public void testDefaultCallingProxiedMethod() throws Exception {
     assertThat(server.takeRequest()).hasPath("/");
   }
 
+  /**
+   * This test ensures that the doNotCloseAfterDecode flag functions.
+   *
+   * It does so by creating a custom Decoder that lazily retrieves the
+   * response body when asked for it and pops the value into an Iterator.
+   *
+   * Without the doNoCloseAfterDecode flag, the test will fail with a
+   * "stream is closed" exception.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testDoNotCloseAfterDecode() throws Exception {
+    server.enqueue(new MockResponse().setBody("success!"));
+
+    String url = "http://localhost:" + server.getPort();
+    Decoder decoder = new Decoder() {
+      @Override
+      public Iterator decode(Response response, Type type) {
+        return new Iterator() {
+          private boolean called = false;
+
+          @Override
+          public boolean hasNext() {
+            return !called;
+          }
+
+          @Override
+          public Object next() {
+            try {
+              return Util.toString(response.body().asReader());
+            } catch (IOException e) {
+              fail(e.getMessage());
+              return null;
+            } finally {
+              Util.ensureClosed(response);
+              called = true;
+            }
+          }
+        };
+      }
+    };
+
+    TestInterface api = Feign.builder()
+            .decoder(decoder)
+            .doNotCloseAfterDecode()
+            .target(TestInterface.class, url);
+    Iterator<String> iterator = api.decodedLazyPost();
+
+    assertTrue(iterator.hasNext());
+    assertEquals("success!", iterator.next());
+    assertFalse(iterator.hasNext());
+
+    assertEquals(1, server.getRequestCount());
+  }
+
   interface TestInterface {
     @RequestLine("GET")
     Response getNoPath();
@@ -257,6 +317,9 @@ interface TestInterface {
 
     @RequestLine("POST /")
     String decodedPost();
+
+    @RequestLine("POST /")
+    Iterator<String> decodedLazyPost();
     
     @RequestLine(value = "GET /api/queues/{vhost}", decodeSlash = false)
     byte[] getQueues(@Param("vhost") String vhost);
