From bb8aa05b42a023189aa55284df7e0a8cedebb41c Mon Sep 17 00:00:00 2001 From: Alexei KLENIN Date: Sun, 23 Feb 2025 20:03:34 +0100 Subject: [PATCH] FIX: interceptors disapearing on BaseBuilder#clone --- core/src/main/java/feign/BaseBuilder.java | 77 +++++++++------- core/src/test/java/feign/BaseBuilderTest.java | 89 +++++++++++++++++++ 2 files changed, 135 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/feign/BaseBuilder.java b/core/src/main/java/feign/BaseBuilder.java index 754fcd306..2463b9545 100644 --- a/core/src/main/java/feign/BaseBuilder.java +++ b/core/src/main/java/feign/BaseBuilder.java @@ -39,7 +39,6 @@ import java.util.stream.Collectors; public abstract class BaseBuilder, T> implements Cloneable { - protected List requestInterceptors = new ArrayList<>(); protected List responseInterceptors = new ArrayList<>(); protected List methodInterceptors = new ArrayList<>(); @@ -65,44 +64,45 @@ public BaseBuilder() { } @SuppressWarnings("unchecked") - private B thisB() { - return (B) this; - } - public B logLevel(Logger.Level logLevel) { this.logLevel = logLevel; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B contract(Contract contract) { this.contract = contract; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B retryer(Retryer retryer) { this.retryer = retryer; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B logger(Logger logger) { this.logger = logger; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B encoder(Encoder encoder) { this.encoder = encoder; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B decoder(Decoder decoder) { this.decoder = decoder; - return thisB(); + return (B) this; } public B codec(Codec codec) { this.encoder = codec.encoder(); this.decoder = codec.decoder(); - return thisB(); + return (B) this; } /** @@ -115,25 +115,29 @@ public B codec(Codec codec) { * * @since 9.6 */ + @SuppressWarnings("unchecked") public B doNotCloseAfterDecode() { this.closeAfterDecode = false; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B decodeVoid() { this.decodeVoid = true; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B queryMapEncoder(QueryMapEncoder queryMapEncoder) { this.queryMapEncoder = queryMapEncoder; - return thisB(); + return (B) this; } /** Allows to map the response before passing it to the decoder. */ + @SuppressWarnings("unchecked") public B mapAndDecode(ResponseMapper mapper, Decoder decoder) { this.decoder = new ResponseMappingDecoder(mapper, decoder); - return thisB(); + return (B) this; } /** @@ -151,9 +155,10 @@ public B mapAndDecode(ResponseMapper mapper, Decoder decoder) { * * @since 11.9 */ + @SuppressWarnings("unchecked") public B dismiss404() { this.dismiss404 = true; - return thisB(); + return (B) this; } /** @@ -173,55 +178,62 @@ public B dismiss404() { * @deprecated use {@link #dismiss404()} instead. */ @Deprecated + @SuppressWarnings("unchecked") public B decode404() { this.dismiss404 = true; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B errorDecoder(ErrorDecoder errorDecoder) { this.errorDecoder = errorDecoder; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B options(Options options) { this.options = options; - return thisB(); + return (B) this; } /** Adds a single request interceptor to the builder. */ + @SuppressWarnings("unchecked") public B requestInterceptor(RequestInterceptor requestInterceptor) { this.requestInterceptors.add(requestInterceptor); - return thisB(); + return (B) this; } /** * Sets the full set of request interceptors for the builder, overwriting any previous * interceptors. */ + @SuppressWarnings("unchecked") public B requestInterceptors(Iterable requestInterceptors) { this.requestInterceptors.clear(); for (RequestInterceptor requestInterceptor : requestInterceptors) { this.requestInterceptors.add(requestInterceptor); } - return thisB(); + return (B) this; } /** * Sets the full set of request interceptors for the builder, overwriting any previous * interceptors. */ + @SuppressWarnings("unchecked") public B responseInterceptors(Iterable responseInterceptors) { this.responseInterceptors.clear(); for (ResponseInterceptor responseInterceptor : responseInterceptors) { this.responseInterceptors.add(responseInterceptor); } - return thisB(); + return (B) this; } /** Adds a single response interceptor to the builder. */ + @SuppressWarnings("unchecked") public B responseInterceptor(ResponseInterceptor responseInterceptor) { this.responseInterceptors.add(responseInterceptor); - return thisB(); + return (B) this; } /** @@ -232,7 +244,7 @@ public B responseInterceptor(ResponseInterceptor responseInterceptor) { @Experimental public B methodInterceptor(MethodInterceptor methodInterceptor) { this.methodInterceptors.add(methodInterceptor); - return thisB(); + return (B) this; } /** Sets the full set of method interceptors, overwriting any previously configured. */ @@ -242,33 +254,36 @@ public B methodInterceptors(Iterable methodInterceptors) { for (MethodInterceptor methodInterceptor : methodInterceptors) { this.methodInterceptors.add(methodInterceptor); } - return thisB(); + return (B) this; } /** Allows you to override how reflective dispatch works inside of Feign. */ + @SuppressWarnings("unchecked") public B invocationHandlerFactory(InvocationHandlerFactory invocationHandlerFactory) { this.invocationHandlerFactory = invocationHandlerFactory; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B exceptionPropagationPolicy(ExceptionPropagationPolicy propagationPolicy) { this.propagationPolicy = propagationPolicy; - return thisB(); + return (B) this; } + @SuppressWarnings("unchecked") public B addCapability(Capability capability) { this.capabilities.add(capability); - return thisB(); + return (B) this; } @SuppressWarnings("unchecked") B enrich() { if (capabilities.isEmpty()) { - return thisB(); + return (B) this; } try { - B clone = (B) thisB().clone(); + B clone = (B) this.clone(); getFieldsToEnrich() .forEach( diff --git a/core/src/test/java/feign/BaseBuilderTest.java b/core/src/test/java/feign/BaseBuilderTest.java index 169cd5a7d..62131d8b7 100644 --- a/core/src/test/java/feign/BaseBuilderTest.java +++ b/core/src/test/java/feign/BaseBuilderTest.java @@ -101,4 +101,93 @@ public ResponseInterceptor enrich(ResponseInterceptor responseInterceptor) { assertThat(enrichCalls).hasValue(1); assertThat(enrichedBuilder.responseInterceptors).containsExactly(capabilityInterceptor); } + + @Test + void checkCloneDontLooseInterceptors() { + Feign.Builder originalBuilder = + Feign.builder() + .requestInterceptor(new FirstRequestInterceptor()) + .addCapability( + new Capability() { + @Override + public RequestInterceptor enrich(RequestInterceptor requestInterceptor) { + return new DecoratingRequestInterceptor(requestInterceptor); + } + }); + + // There is one interceptor FirstRequestInterceptor + assertThat(originalBuilder.requestInterceptors) + .isNotNull() + .isNotEmpty() + .hasSize(1) + .first() + .isInstanceOf(FirstRequestInterceptor.class); + + Feign.Builder enrichedBuilder = originalBuilder.enrich(); + + // Original builder should have one interceptor FirstRequestInterceptor + assertThat(originalBuilder.requestInterceptors) + .isNotNull() + .isNotEmpty() + .hasSize(1) + .first() + .isInstanceOf(FirstRequestInterceptor.class); + + // enrichedBuilder should have one interceptor DecoratingRequestInterceptor + assertThat(enrichedBuilder.requestInterceptors) + .isNotNull() + .isNotEmpty() + .hasSize(1) + .first() + .isInstanceOf(DecoratingRequestInterceptor.class); + + Feign.Builder enrichedBuilderWithInterceptor = + enrichedBuilder.requestInterceptor(new SecondRequestInterceptor()); + + // Original builder should have one interceptor FirstRequestInterceptor + assertThat(originalBuilder.requestInterceptors) + .isNotNull() + .isNotEmpty() + .hasSize(1) + .first() + .isInstanceOf(FirstRequestInterceptor.class); + + // enrichedBuilder should have two interceptors + assertThat(enrichedBuilder.requestInterceptors).isNotNull().isNotEmpty().hasSize(2); + assertThat(enrichedBuilder.requestInterceptors.get(0)) + .isInstanceOf(DecoratingRequestInterceptor.class); + assertThat(enrichedBuilder.requestInterceptors.get(1)) + .isInstanceOf(SecondRequestInterceptor.class); + + // enrichedBuilderWithInterceptor should have two interceptors + assertThat(enrichedBuilderWithInterceptor.requestInterceptors) + .isNotNull() + .isNotEmpty() + .hasSize(2); + assertThat(enrichedBuilderWithInterceptor.requestInterceptors.get(0)) + .isInstanceOf(DecoratingRequestInterceptor.class); + assertThat(enrichedBuilderWithInterceptor.requestInterceptors.get(1)) + .isInstanceOf(SecondRequestInterceptor.class); + } + + static final class FirstRequestInterceptor implements RequestInterceptor { + @Override + public void apply(final RequestTemplate template) {} + } + + static final class SecondRequestInterceptor implements RequestInterceptor { + @Override + public void apply(final RequestTemplate template) {} + } + + static final class DecoratingRequestInterceptor implements RequestInterceptor { + RequestInterceptor delegate; + + DecoratingRequestInterceptor(RequestInterceptor delegate) { + this.delegate = delegate; + } + + @Override + public void apply(final RequestTemplate template) {} + } }