From f4a78925fdeddba75df071dd8215a51a74b4e651 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 | 73 +++++++++------ core/src/test/java/feign/BaseBuilderTest.java | 89 +++++++++++++++++++ 2 files changed, 133 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/feign/BaseBuilder.java b/core/src/main/java/feign/BaseBuilder.java index d2549e0a7b..f42f2ab657 100644 --- a/core/src/main/java/feign/BaseBuilder.java +++ b/core/src/main/java/feign/BaseBuilder.java @@ -32,9 +32,6 @@ import java.util.stream.Collectors; public abstract class BaseBuilder, T> implements Cloneable { - - private final B thisB; - protected final List requestInterceptors = new ArrayList<>(); protected final List responseInterceptors = new ArrayList<>(); protected Logger.Level logLevel = Logger.Level.NONE; @@ -56,37 +53,42 @@ public abstract class BaseBuilder, T> implements Clo public BaseBuilder() { super(); - thisB = (B) this; } + @SuppressWarnings("unchecked") 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; } /** @@ -99,25 +101,29 @@ public B decoder(Decoder decoder) { * * @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; } /** @@ -135,9 +141,10 @@ public B mapAndDecode(ResponseMapper mapper, Decoder decoder) { * * @since 11.9 */ + @SuppressWarnings("unchecked") public B dismiss404() { this.dismiss404 = true; - return thisB; + return (B) this; } /** @@ -157,81 +164,91 @@ 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; } /** 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( @@ -274,8 +291,6 @@ List getFieldsToEnrich() { .filter(field -> !field.isSynthetic()) // and capabilities itself .filter(field -> !Objects.equals(field.getName(), "capabilities")) - // and thisB helper field - .filter(field -> !Objects.equals(field.getName(), "thisB")) // skip primitive types .filter(field -> !field.getType().isPrimitive()) // skip enumerations diff --git a/core/src/test/java/feign/BaseBuilderTest.java b/core/src/test/java/feign/BaseBuilderTest.java index 297f0e3bd3..0ee8990102 100644 --- a/core/src/test/java/feign/BaseBuilderTest.java +++ b/core/src/test/java/feign/BaseBuilderTest.java @@ -67,4 +67,93 @@ void checkEnrichTouchesAllBuilderFields() .responseInterceptor((ic, c) -> c.next(ic)), 12); } + + @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) {} + } }