From 2c00066d4a7a1f1882708166f8b2cbaabe721efa Mon Sep 17 00:00:00 2001 From: Marvin Froeder Date: Tue, 29 Aug 2023 17:45:57 +1200 Subject: [PATCH] Add support for multiple ResponseInterceptors (#1829) * Add support for multiple ResponseInterceptors * Address PR comments --------- Co-authored-by: Marvin Froeder Co-authored-by: Marvin Froeder --- core/src/main/java/feign/AsyncFeign.java | 2 +- .../main/java/feign/AsyncResponseHandler.java | 12 +-- core/src/main/java/feign/BaseBuilder.java | 30 +++++- core/src/main/java/feign/Capability.java | 4 + core/src/main/java/feign/Feign.java | 2 +- .../main/java/feign/InvocationContext.java | 58 ------------ core/src/main/java/feign/ResponseHandler.java | 18 ++-- .../main/java/feign/ResponseInterceptor.java | 93 +++++++++++++++++-- core/src/test/java/feign/BaseBuilderTest.java | 6 +- .../java/feign/kotlin/CoroutineFeign.java | 2 +- 10 files changed, 137 insertions(+), 90 deletions(-) delete mode 100644 core/src/main/java/feign/InvocationContext.java diff --git a/core/src/main/java/feign/AsyncFeign.java b/core/src/main/java/feign/AsyncFeign.java index 6733766d..1ff3b043 100644 --- a/core/src/main/java/feign/AsyncFeign.java +++ b/core/src/main/java/feign/AsyncFeign.java @@ -200,7 +200,7 @@ public final class AsyncFeign { decoder, errorDecoder, dismiss404, - closeAfterDecode, decodeVoid, responseInterceptor), + closeAfterDecode, decodeVoid, responseInterceptorChain()), AsyncResponseHandler.class, capabilities); diff --git a/core/src/main/java/feign/AsyncResponseHandler.java b/core/src/main/java/feign/AsyncResponseHandler.java index 2515e143..242c83ae 100644 --- a/core/src/main/java/feign/AsyncResponseHandler.java +++ b/core/src/main/java/feign/AsyncResponseHandler.java @@ -27,13 +27,11 @@ import java.util.concurrent.CompletableFuture; 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 handleResponse(String configKey, diff --git a/core/src/main/java/feign/BaseBuilder.java b/core/src/main/java/feign/BaseBuilder.java index e8e0ed1d..b30b245b 100644 --- a/core/src/main/java/feign/BaseBuilder.java +++ b/core/src/main/java/feign/BaseBuilder.java @@ -35,7 +35,7 @@ public abstract class BaseBuilder, T> implements Clo protected final List requestInterceptors = new ArrayList<>(); - protected ResponseInterceptor responseInterceptor = ResponseInterceptor.DEFAULT; + protected final List 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 abstract class BaseBuilder, T> implements Clo return thisB; } + /** + * Sets the full set of request interceptors for the builder, overwriting any previous + * interceptors. + */ + public B responseInterceptors(Iterable 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 abstract class BaseBuilder, T> implements Clo } 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); + } + + } diff --git a/core/src/main/java/feign/Capability.java b/core/src/main/java/feign/Capability.java index 417246bd..63fae070 100644 --- a/core/src/main/java/feign/Capability.java +++ b/core/src/main/java/feign/Capability.java @@ -90,6 +90,10 @@ public interface Capability { return responseInterceptor; } + default ResponseInterceptor.Chain enrich(ResponseInterceptor.Chain chain) { + return chain; + } + default Logger enrich(Logger logger) { return logger; } diff --git a/core/src/main/java/feign/Feign.java b/core/src/main/java/feign/Feign.java index 8ef8e54e..b107af9f 100644 --- a/core/src/main/java/feign/Feign.java +++ b/core/src/main/java/feign/Feign.java @@ -205,7 +205,7 @@ public abstract class Feign { public Feign internalBuild() { final ResponseHandler responseHandler = new ResponseHandler(logLevel, logger, decoder, errorDecoder, - dismiss404, closeAfterDecode, decodeVoid, responseInterceptor); + dismiss404, closeAfterDecode, decodeVoid, responseInterceptorChain()); MethodHandler.Factory methodHandlerFactory = new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors, responseHandler, logger, logLevel, propagationPolicy, diff --git a/core/src/main/java/feign/InvocationContext.java b/core/src/main/java/feign/InvocationContext.java deleted file mode 100644 index 30b2459e..00000000 --- a/core/src/main/java/feign/InvocationContext.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Copyright 2012-2023 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 - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License - * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express - * or implied. See the License for the specific language governing permissions and limitations under - * the License. - */ -package feign; - -import static feign.FeignException.errorReading; -import feign.codec.DecodeException; -import feign.codec.Decoder; -import java.io.IOException; -import java.lang.reflect.Type; - -public class InvocationContext { - - private final Decoder decoder; - private final Type returnType; - private final Response response; - - InvocationContext(Decoder decoder, Type returnType, Response response) { - this.decoder = decoder; - this.returnType = returnType; - this.response = response; - } - - public Object proceed() { - try { - return decoder.decode(response, returnType); - } catch (final FeignException e) { - throw e; - } catch (final RuntimeException e) { - throw new DecodeException(response.status(), e.getMessage(), response.request(), e); - } catch (IOException e) { - throw errorReading(response.request(), response, e); - } - } - - public Decoder decoder() { - return decoder; - } - - public Type returnType() { - return returnType; - } - - public Response response() { - return response; - } - -} diff --git a/core/src/main/java/feign/ResponseHandler.java b/core/src/main/java/feign/ResponseHandler.java index 63be2a4b..a9e0c090 100644 --- a/core/src/main/java/feign/ResponseHandler.java +++ b/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,11 +39,11 @@ 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; @@ -51,8 +51,8 @@ public class ResponseHandler { 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 @@ public class ResponseHandler { } 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()); } diff --git a/core/src/main/java/feign/ResponseInterceptor.java b/core/src/main/java/feign/ResponseInterceptor.java index cab991fb..dc949642 100644 --- a/core/src/main/java/feign/ResponseInterceptor.java +++ b/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; + } + + } } diff --git a/core/src/test/java/feign/BaseBuilderTest.java b/core/src/test/java/feign/BaseBuilderTest.java index b513db30..8f6f4dc5 100644 --- a/core/src/test/java/feign/BaseBuilderTest.java +++ b/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 @@ public class BaseBuilderTest { 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) mockedValue).get(0); } assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue) @@ -56,7 +58,7 @@ public class BaseBuilderTest { public void checkEnrichTouchesAllBuilderFields() throws IllegalArgumentException, IllegalAccessException { test(Feign.builder().requestInterceptor(template -> { - }), 12); + }).responseInterceptor((ic, c) -> c.next(ic)), 12); } } diff --git a/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java b/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java index 4e38016e..50f3a0e6 100644 --- a/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java +++ b/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java @@ -172,7 +172,7 @@ public class CoroutineFeign { .queryMapEncoder(queryMapEncoder) .options(options) .requestInterceptors(requestInterceptors) - .responseInterceptor(responseInterceptor) + .responseInterceptors(responseInterceptors) .invocationHandlerFactory(invocationHandlerFactory) .defaultContextSupplier((AsyncContextSupplier) defaultContextSupplier) .methodInfoResolver(methodInfoResolver)