From fc6daef95fdde3aaf9d4ba10f36331876c6eead2 Mon Sep 17 00:00:00 2001 From: Vitalij Berdinskih Date: Tue, 22 Aug 2023 00:37:27 +0300 Subject: [PATCH] The builder clones itself before enrichment (#2117) * Enrichment of a clone --------- Co-authored-by: Marvin Froeder --- core/src/main/java/feign/AsyncFeign.java | 7 +-- core/src/main/java/feign/BaseBuilder.java | 56 +++++++++++-------- core/src/main/java/feign/Feign.java | 9 ++- core/src/test/java/feign/BaseBuilderTest.java | 6 +- .../main/java/feign/hystrix/HystrixFeign.java | 10 ++-- .../java/feign/kotlin/CoroutineFeign.java | 8 +-- .../java/feign/reactive/ReactiveFeign.java | 4 +- .../java/feign/reactive/ReactorFeign.java | 12 ++-- .../main/java/feign/reactive/RxJavaFeign.java | 10 ++-- 9 files changed, 67 insertions(+), 55 deletions(-) diff --git a/core/src/main/java/feign/AsyncFeign.java b/core/src/main/java/feign/AsyncFeign.java index 52798a0e..6733766d 100644 --- a/core/src/main/java/feign/AsyncFeign.java +++ b/core/src/main/java/feign/AsyncFeign.java @@ -65,7 +65,7 @@ public final class AsyncFeign { }); } - public static class AsyncBuilder extends BaseBuilder> { + public static class AsyncBuilder extends BaseBuilder, AsyncFeign> { private AsyncContextSupplier defaultContextSupplier = () -> null; private AsyncClient client = new AsyncClient.Default<>( @@ -190,9 +190,8 @@ public final class AsyncFeign { return super.invocationHandlerFactory(invocationHandlerFactory); } - public AsyncFeign build() { - super.enrich(); - + @Override + public AsyncFeign internalBuild() { AsyncResponseHandler responseHandler = (AsyncResponseHandler) Capability.enrich( new AsyncResponseHandler( diff --git a/core/src/main/java/feign/BaseBuilder.java b/core/src/main/java/feign/BaseBuilder.java index f6134e75..e8e0ed1d 100644 --- a/core/src/main/java/feign/BaseBuilder.java +++ b/core/src/main/java/feign/BaseBuilder.java @@ -29,7 +29,7 @@ import java.util.List; import java.util.Objects; import java.util.stream.Collectors; -public abstract class BaseBuilder> { +public abstract class BaseBuilder, T> implements Cloneable { private final B thisB; @@ -230,33 +230,41 @@ public abstract class BaseBuilder> { return thisB; } - protected B enrich() { + @SuppressWarnings("unchecked") + B enrich() { if (capabilities.isEmpty()) { return thisB; } - getFieldsToEnrich().forEach(field -> { - field.setAccessible(true); - try { - final Object originalValue = field.get(thisB); - final Object enriched; - if (originalValue instanceof List) { - Type ownerType = ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0]; - enriched = ((List) originalValue).stream() - .map(value -> Capability.enrich(value, (Class) ownerType, capabilities)) - .collect(Collectors.toList()); - } else { - enriched = Capability.enrich(originalValue, field.getType(), capabilities); + try { + B clone = (B) thisB.clone(); + + getFieldsToEnrich().forEach(field -> { + field.setAccessible(true); + try { + final Object originalValue = field.get(clone); + final Object enriched; + if (originalValue instanceof List) { + Type ownerType = + ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0]; + enriched = ((List) originalValue).stream() + .map(value -> Capability.enrich(value, (Class) ownerType, capabilities)) + .collect(Collectors.toList()); + } else { + enriched = Capability.enrich(originalValue, field.getType(), capabilities); + } + field.set(clone, enriched); + } catch (IllegalArgumentException | IllegalAccessException e) { + throw new RuntimeException("Unable to enrich field " + field, e); + } finally { + field.setAccessible(false); } - field.set(thisB, enriched); - } catch (IllegalArgumentException | IllegalAccessException e) { - throw new RuntimeException("Unable to enrich field " + field, e); - } finally { - field.setAccessible(false); - } - }); + }); - return thisB; + return clone; + } catch (CloneNotSupportedException e) { + throw new AssertionError(e); + } } List getFieldsToEnrich() { @@ -275,5 +283,9 @@ public abstract class BaseBuilder> { .collect(Collectors.toList()); } + public final T build() { + return enrich().internalBuild(); + } + protected abstract T internalBuild(); } diff --git a/core/src/main/java/feign/Feign.java b/core/src/main/java/feign/Feign.java index 0b4d100f..8ef8e54e 100644 --- a/core/src/main/java/feign/Feign.java +++ b/core/src/main/java/feign/Feign.java @@ -13,12 +13,12 @@ */ package feign; +import feign.InvocationHandlerFactory.MethodHandler; import feign.Request.Options; import feign.Target.HardCodedTarget; import feign.codec.Decoder; import feign.codec.Encoder; import feign.codec.ErrorDecoder; -import feign.InvocationHandlerFactory.MethodHandler; import java.io.IOException; import java.lang.reflect.Method; import java.lang.reflect.Type; @@ -92,7 +92,7 @@ public abstract class Feign { */ public abstract T newInstance(Target target); - public static class Builder extends BaseBuilder { + public static class Builder extends BaseBuilder { private Client client = new Client.Default(null, null); @@ -201,9 +201,8 @@ public abstract class Feign { return build().newInstance(target); } - public Feign build() { - super.enrich(); - + @Override + public Feign internalBuild() { final ResponseHandler responseHandler = new ResponseHandler(logLevel, logger, decoder, errorDecoder, dismiss404, closeAfterDecode, decodeVoid, responseInterceptor); diff --git a/core/src/test/java/feign/BaseBuilderTest.java b/core/src/test/java/feign/BaseBuilderTest.java index 8d6cf80c..b513db30 100644 --- a/core/src/test/java/feign/BaseBuilderTest.java +++ b/core/src/test/java/feign/BaseBuilderTest.java @@ -14,6 +14,7 @@ package feign; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.RETURNS_MOCKS; import java.lang.reflect.Field; @@ -30,10 +31,10 @@ public class BaseBuilderTest { }), 14); } - private void test(BaseBuilder builder, int expectedFieldsCount) + private void test(BaseBuilder builder, int expectedFieldsCount) throws IllegalArgumentException, IllegalAccessException { Capability mockingCapability = Mockito.mock(Capability.class, RETURNS_MOCKS); - BaseBuilder enriched = builder.addCapability(mockingCapability).enrich(); + BaseBuilder enriched = builder.addCapability(mockingCapability).enrich(); List fields = enriched.getFieldsToEnrich(); assertThat(fields).hasSize(expectedFieldsCount); @@ -46,6 +47,7 @@ public class BaseBuilderTest { } assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue) .isMock()); + assertNotSame(builder, enriched); } } diff --git a/hystrix/src/main/java/feign/hystrix/HystrixFeign.java b/hystrix/src/main/java/feign/hystrix/HystrixFeign.java index 9a65a958..5f277dad 100644 --- a/hystrix/src/main/java/feign/hystrix/HystrixFeign.java +++ b/hystrix/src/main/java/feign/hystrix/HystrixFeign.java @@ -14,9 +14,6 @@ package feign.hystrix; import com.netflix.hystrix.HystrixCommand; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; -import java.util.Map; import feign.Client; import feign.Contract; import feign.Feign; @@ -30,6 +27,9 @@ import feign.Target; import feign.codec.Decoder; import feign.codec.Encoder; import feign.codec.ErrorDecoder; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.util.Map; /** * Allows Feign interfaces to return HystrixCommand or rx.Observable or rx.Single objects. Also @@ -133,7 +133,7 @@ public final class HystrixFeign { } @Override - public Feign build() { + public Feign internalBuild() { return build(null); } @@ -148,7 +148,7 @@ public final class HystrixFeign { } }); super.contract(new HystrixDelegatingContract(contract)); - return super.build(); + return super.internalBuild(); } // Covariant overrides to support chaining to new fallback method. diff --git a/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java b/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java index 093d069a..4e38016e 100644 --- a/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java +++ b/kotlin/src/main/java/feign/kotlin/CoroutineFeign.java @@ -112,7 +112,8 @@ public class CoroutineFeign { } } - public static class CoroutineBuilder extends BaseBuilder> { + public static class CoroutineBuilder + extends BaseBuilder, CoroutineFeign> { private AsyncContextSupplier defaultContextSupplier = () -> null; private AsyncClient client = new AsyncClient.Default<>( @@ -156,10 +157,9 @@ public class CoroutineFeign { return build().newInstance(target, context); } + @Override @SuppressWarnings("unchecked") - public CoroutineFeign build() { - super.enrich(); - + public CoroutineFeign internalBuild() { AsyncFeign asyncFeign = (AsyncFeign) AsyncFeign.builder() .logLevel(logLevel) .client((AsyncClient) client) diff --git a/reactive/src/main/java/feign/reactive/ReactiveFeign.java b/reactive/src/main/java/feign/reactive/ReactiveFeign.java index 910d7ec3..f8e8ef86 100644 --- a/reactive/src/main/java/feign/reactive/ReactiveFeign.java +++ b/reactive/src/main/java/feign/reactive/ReactiveFeign.java @@ -42,13 +42,13 @@ abstract class ReactiveFeign { * @return a new Feign Instance. */ @Override - public Feign build() { + public Feign internalBuild() { if (!(this.contract instanceof ReactiveDelegatingContract)) { super.contract(new ReactiveDelegatingContract(this.contract)); } else { super.contract(this.contract); } - return super.build(); + return super.internalBuild(); } @Override diff --git a/reactive/src/main/java/feign/reactive/ReactorFeign.java b/reactive/src/main/java/feign/reactive/ReactorFeign.java index 5d40ffaa..13435d1b 100644 --- a/reactive/src/main/java/feign/reactive/ReactorFeign.java +++ b/reactive/src/main/java/feign/reactive/ReactorFeign.java @@ -14,13 +14,13 @@ package feign.reactive; import feign.Feign; -import reactor.core.scheduler.Scheduler; -import reactor.core.scheduler.Schedulers; +import feign.InvocationHandlerFactory; +import feign.Target; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.util.Map; -import feign.InvocationHandlerFactory; -import feign.Target; +import reactor.core.scheduler.Scheduler; +import reactor.core.scheduler.Schedulers; public class ReactorFeign extends ReactiveFeign { @@ -41,9 +41,9 @@ public class ReactorFeign extends ReactiveFeign { } @Override - public Feign build() { + public Feign internalBuild() { super.invocationHandlerFactory(new ReactorInvocationHandlerFactory(scheduler)); - return super.build(); + return super.internalBuild(); } @Override diff --git a/reactive/src/main/java/feign/reactive/RxJavaFeign.java b/reactive/src/main/java/feign/reactive/RxJavaFeign.java index b81ac41f..955be354 100644 --- a/reactive/src/main/java/feign/reactive/RxJavaFeign.java +++ b/reactive/src/main/java/feign/reactive/RxJavaFeign.java @@ -13,14 +13,14 @@ */ package feign.reactive; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; -import java.util.Map; import feign.Feign; import feign.InvocationHandlerFactory; import feign.Target; import io.reactivex.Scheduler; import io.reactivex.schedulers.Schedulers; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.util.Map; public class RxJavaFeign extends ReactiveFeign { @@ -33,9 +33,9 @@ public class RxJavaFeign extends ReactiveFeign { private Scheduler scheduler = Schedulers.trampoline(); @Override - public Feign build() { + public Feign internalBuild() { super.invocationHandlerFactory(new RxJavaInvocationHandlerFactory(scheduler)); - return super.build(); + return super.internalBuild(); } @Override