Add support for multiple ResponseInterceptors (#1829)

* Add support for multiple ResponseInterceptors

* Address PR comments

---------

Co-authored-by: Marvin Froeder <marvin.froeder@police.govt.nz>
Co-authored-by: Marvin Froeder <velobr@gmail.com>

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

@@ -200,7 +200,7 @@ public AsyncFeign<C> internalBuild() {
                   decoder,
                   errorDecoder,
                   dismiss404,
-                  closeAfterDecode, decodeVoid, responseInterceptor),
+                  closeAfterDecode, decodeVoid, responseInterceptorChain()),
               AsyncResponseHandler.class,
               capabilities);
 

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

@@ -27,13 +27,11 @@
 class AsyncResponseHandler {
   private final ResponseHandler responseHandler;
 
-  AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder,
-      ErrorDecoder errorDecoder, boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
-      ResponseInterceptor responseInterceptor) {
-    this.responseHandler = new ResponseHandler(
-        logLevel, logger, decoder,
-        errorDecoder, dismiss404, closeAfterDecode, decodeVoid,
-        responseInterceptor);
+  AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
+      boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
+      ResponseInterceptor.Chain executionChain) {
+    this.responseHandler = new ResponseHandler(logLevel, logger, decoder, errorDecoder, dismiss404,
+        closeAfterDecode, decodeVoid, executionChain);
   }
 
   public CompletableFuture<Object> handleResponse(String configKey,

core/src/main/java/feign/BaseBuilder.java

@@ -35,7 +35,7 @@ public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Clo
 
   protected final List<RequestInterceptor> requestInterceptors =
       new ArrayList<>();
-  protected ResponseInterceptor responseInterceptor = ResponseInterceptor.DEFAULT;
+  protected final List<ResponseInterceptor> responseInterceptors = new ArrayList<>();
   protected Logger.Level logLevel = Logger.Level.NONE;
   protected Contract contract = new Contract.Default();
   protected Retryer retryer = new Retryer.Default();
@@ -203,11 +203,23 @@ public B requestInterceptors(Iterable<RequestInterceptor> requestInterceptors) {
     return thisB;
   }
 
+  /**
+   * Sets the full set of request interceptors for the builder, overwriting any previous
+   * interceptors.
+   */
+  public B responseInterceptors(Iterable<ResponseInterceptor> responseInterceptors) {
+    this.responseInterceptors.clear();
+    for (ResponseInterceptor responseInterceptor : responseInterceptors) {
+      this.responseInterceptors.add(responseInterceptor);
+    }
+    return thisB;
+  }
+
   /**
    * Adds a single response interceptor to the builder.
    */
   public B responseInterceptor(ResponseInterceptor responseInterceptor) {
-    this.responseInterceptor = responseInterceptor;
+    this.responseInterceptors.add(responseInterceptor);
     return thisB;
   }
 
@@ -288,4 +300,18 @@ public final T build() {
   }
 
   protected abstract T internalBuild();
+
+  protected ResponseInterceptor.Chain responseInterceptorChain() {
+    ResponseInterceptor.Chain endOfChain =
+        ResponseInterceptor.Chain.DEFAULT;
+    ResponseInterceptor.Chain executionChain = this.responseInterceptors.stream()
+        .reduce(ResponseInterceptor::andThen)
+        .map(interceptor -> interceptor.apply(endOfChain))
+        .orElse(endOfChain);
+
+    return (ResponseInterceptor.Chain) Capability.enrich(executionChain,
+        ResponseInterceptor.Chain.class, capabilities);
+  }
+
+
 }

core/src/main/java/feign/Capability.java

@@ -90,6 +90,10 @@ default ResponseInterceptor enrich(ResponseInterceptor responseInterceptor) {
     return responseInterceptor;
   }
 
+  default ResponseInterceptor.Chain enrich(ResponseInterceptor.Chain chain) {
+    return chain;
+  }
+
   default Logger enrich(Logger logger) {
     return logger;
   }

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

@@ -205,7 +205,7 @@ public <T> T target(Target<T> target) {
     public Feign internalBuild() {
       final ResponseHandler responseHandler =
           new ResponseHandler(logLevel, logger, decoder, errorDecoder,
-              dismiss404, closeAfterDecode, decodeVoid, responseInterceptor);
+              dismiss404, closeAfterDecode, decodeVoid, responseInterceptorChain());
       MethodHandler.Factory<Object> methodHandlerFactory =
           new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors,
               responseHandler, logger, logLevel, propagationPolicy,

core/src/main/java/feign/ResponseHandler.java

@@ -13,13 +13,13 @@
  */
 package feign;
 
+import static feign.FeignException.errorReading;
+import static feign.Util.ensureClosed;
 import feign.Logger.Level;
 import feign.codec.Decoder;
 import feign.codec.ErrorDecoder;
 import java.io.IOException;
 import java.lang.reflect.Type;
-import static feign.FeignException.errorReading;
-import static feign.Util.ensureClosed;
 
 /**
  * The response handler that is used to provide synchronous support on top of standard response
@@ -39,20 +39,20 @@ public class ResponseHandler {
 
   private final boolean decodeVoid;
 
-  private final ResponseInterceptor responseInterceptor;
+  private final ResponseInterceptor.Chain executionChain;
 
-  public ResponseHandler(Level logLevel, Logger logger, Decoder decoder,
-      ErrorDecoder errorDecoder, boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
-      ResponseInterceptor responseInterceptor) {
+  public ResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
+      boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
+      ResponseInterceptor.Chain executionChain) {
     super();
     this.logLevel = logLevel;
     this.logger = logger;
     this.decoder = decoder;
     this.errorDecoder = errorDecoder;
     this.dismiss404 = dismiss404;
     this.closeAfterDecode = closeAfterDecode;
-    this.responseInterceptor = responseInterceptor;
     this.decodeVoid = decodeVoid;
+    this.executionChain = executionChain;
   }
 
   public Object handleResponse(String configKey,
@@ -122,8 +122,8 @@ private Object decode(Response response, Type type) throws IOException {
     }
 
     try {
-      final Object result = responseInterceptor.aroundDecode(
-          new InvocationContext(decoder, type, response));
+      final Object result = executionChain.next(
+          new ResponseInterceptor.Context(decoder, type, response));
       if (closeAfterDecode) {
         ensureClosed(response.body());
       }

core/src/main/java/feign/ResponseInterceptor.java

@@ -13,26 +13,101 @@
  */
 package feign;
 
+import static feign.FeignException.errorReading;
+import feign.codec.DecodeException;
+import feign.codec.Decoder;
 import java.io.IOException;
+import java.lang.reflect.Type;
 import java.util.function.Function;
 
 /**
- * Zero or One {@code ResponseInterceptor} may be configured for purposes such as verify or modify
- * headers of response, verify the business status of decoded object. Once interceptors are applied,
- * {@link ResponseInterceptor#aroundDecode(Response, Function)} is called around decode method
- * called
+ * Interceptor for purposes such as verify or modify headers of response, verify the business status
+ * of decoded object. Once interceptors are applied,
+ * {@link ResponseInterceptor#intercept(Response, Function)} is called around decode method called
  */
 public interface ResponseInterceptor {
 
-  ResponseInterceptor DEFAULT = InvocationContext::proceed;
-
   /**
-   * Called for response around decode, must either manually invoke
-   * {@link InvocationContext#proceed} or manually create a new response object
+   * Called for response around decode, must either manually invoke {@link Chain#next(Context)} or
+   * manually create a new response object
    *
    * @param invocationContext information surrounding the response been decoded
    * @return decoded response
    */
-  Object aroundDecode(InvocationContext invocationContext) throws IOException;
+  Object intercept(Context context, Chain chain) throws IOException;
+
+  /**
+   * Return a new {@link ResponseInterceptor} that invokes the current interceptor first and then
+   * the one that is passed in.
+   * 
+   * @param nextInterceptor the interceptor to delegate to after the current
+   * @return a new interceptor that chains the two
+   */
+  default ResponseInterceptor andThen(ResponseInterceptor nextInterceptor) {
+    return (ic, chain) -> intercept(ic,
+        nextContext -> nextInterceptor.intercept(nextContext, chain));
+  }
+
+  /**
+   * Contract for delegation to the rest of the chain.
+   */
+  public interface Chain {
+    Chain DEFAULT = ic -> {
+      try {
+        return ic.decoder().decode(ic.response(), ic.returnType());
+      } catch (final FeignException e) {
+        throw e;
+      } catch (final RuntimeException e) {
+        throw new DecodeException(ic.response().status(), e.getMessage(),
+            ic.response().request(), e);
+      } catch (IOException e) {
+        throw errorReading(ic.response().request(), ic.response(), e);
+      }
+    };
+
+    /**
+     * Delegate to the rest of the chain to execute the request.
+     * 
+     * @param context the request to execute the {@link Chain} .
+     * @return the response
+     */
+    Object next(Context context) throws IOException;
+  }
+
+  /**
+   * Apply this interceptor to the given {@code Chain} resulting in an intercepted chain.
+   * 
+   * @param chain the chain to add interception around
+   * @return a new chain instance
+   */
+  default Chain apply(Chain chain) {
+    return request -> intercept(request, chain);
+  }
+
+  public class Context {
+
+    private final Decoder decoder;
+    private final Type returnType;
+    private final Response response;
+
+    Context(Decoder decoder, Type returnType, Response response) {
+      this.decoder = decoder;
+      this.returnType = returnType;
+      this.response = response;
+    }
+
+    public Decoder decoder() {
+      return decoder;
+    }
+
+    public Type returnType() {
+      return returnType;
+    }
+
+    public Response response() {
+      return response;
+    }
+
+  }
 
 }

core/src/test/java/feign/BaseBuilderTest.java

@@ -28,7 +28,7 @@ public class BaseBuilderTest {
   public void checkEnrichTouchesAllAsyncBuilderFields()
       throws IllegalArgumentException, IllegalAccessException {
     test(AsyncFeign.builder().requestInterceptor(template -> {
-    }), 14);
+    }).responseInterceptor((ic, c) -> c.next(ic)), 14);
   }
 
   private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
@@ -43,6 +43,8 @@ private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
       field.setAccessible(true);
       Object mockedValue = field.get(enriched);
       if (mockedValue instanceof List) {
+        assertThat((List) mockedValue).withFailMessage("Enriched list missing contents %s", field)
+            .isNotEmpty();
         mockedValue = ((List<Object>) mockedValue).get(0);
       }
       assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue)
@@ -56,7 +58,7 @@ private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
   public void checkEnrichTouchesAllBuilderFields()
       throws IllegalArgumentException, IllegalAccessException {
     test(Feign.builder().requestInterceptor(template -> {
-    }), 12);
+    }).responseInterceptor((ic, c) -> c.next(ic)), 12);
   }
 
 }

kotlin/src/main/java/feign/kotlin/CoroutineFeign.java

@@ -172,7 +172,7 @@ public CoroutineFeign<C> internalBuild() {
           .queryMapEncoder(queryMapEncoder)
           .options(options)
           .requestInterceptors(requestInterceptors)
-          .responseInterceptor(responseInterceptor)
+          .responseInterceptors(responseInterceptors)
           .invocationHandlerFactory(invocationHandlerFactory)
           .defaultContextSupplier((AsyncContextSupplier<Object>) defaultContextSupplier)
           .methodInfoResolver(methodInfoResolver)