The builder clones itself before enrichment (#2117)

* Enrichment of a clone

---------

Co-authored-by: Marvin Froeder <marvin.froeder@dovetailstudios.com>

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

@@ -65,7 +65,7 @@ private static class LazyInitializedExecutorService {
             });
   }
 
-  public static class AsyncBuilder<C> extends BaseBuilder<AsyncBuilder<C>> {
+  public static class AsyncBuilder<C> extends BaseBuilder<AsyncBuilder<C>, AsyncFeign<C>> {
 
     private AsyncContextSupplier<C> defaultContextSupplier = () -> null;
     private AsyncClient<C> client = new AsyncClient.Default<>(
@@ -190,9 +190,8 @@ public AsyncBuilder<C> invocationHandlerFactory(InvocationHandlerFactory invocat
       return super.invocationHandlerFactory(invocationHandlerFactory);
     }
 
-    public AsyncFeign<C> build() {
-      super.enrich();
-
+    @Override
+    public AsyncFeign<C> internalBuild() {
       AsyncResponseHandler responseHandler =
           (AsyncResponseHandler) Capability.enrich(
               new AsyncResponseHandler(

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

@@ -29,7 +29,7 @@
 import java.util.Objects;
 import java.util.stream.Collectors;
 
-public abstract class BaseBuilder<B extends BaseBuilder<B>> {
+public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Cloneable {
 
   private final B thisB;
 
@@ -230,33 +230,41 @@ public B addCapability(Capability capability) {
     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<Field> getFieldsToEnrich() {
@@ -275,5 +283,9 @@ List<Field> getFieldsToEnrich() {
         .collect(Collectors.toList());
   }
 
+  public final T build() {
+    return enrich().internalBuild();
+  }
 
+  protected abstract T internalBuild();
 }

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 static String configKey(Method method) {
    */
   public abstract <T> T newInstance(Target<T> target);
 
-  public static class Builder extends BaseBuilder<Builder> {
+  public static class Builder extends BaseBuilder<Builder, Feign> {
 
     private Client client = new Client.Default(null, null);
 
@@ -201,9 +201,8 @@ public <T> T target(Target<T> target) {
       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);

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 void checkEnrichTouchesAllAsyncBuilderFields()
     }), 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<Field> fields = enriched.getFieldsToEnrich();
     assertThat(fields).hasSize(expectedFieldsCount);
@@ -46,6 +47,7 @@ private void test(BaseBuilder<?> builder, int expectedFieldsCount)
       }
       assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue)
           .isMock());
+      assertNotSame(builder, enriched);
     }
 
   }

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.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 Builder contract(Contract contract) {
     }
 
     @Override
-    public Feign build() {
+    public Feign internalBuild() {
       return build(null);
     }
 
@@ -148,7 +148,7 @@ public InvocationHandler create(Target target,
         }
       });
       super.contract(new HystrixDelegatingContract(contract));
-      return super.build();
+      return super.internalBuild();
     }
 
     // Covariant overrides to support chaining to new fallback method.

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

@@ -112,7 +112,8 @@ public String toString() {
     }
   }
 
-  public static class CoroutineBuilder<C> extends BaseBuilder<CoroutineBuilder<C>> {
+  public static class CoroutineBuilder<C>
+      extends BaseBuilder<CoroutineBuilder<C>, CoroutineFeign<C>> {
 
     private AsyncContextSupplier<C> defaultContextSupplier = () -> null;
     private AsyncClient<C> client = new AsyncClient.Default<>(
@@ -156,10 +157,9 @@ public <T> T target(Target<T> target, C context) {
       return build().newInstance(target, context);
     }
 
+    @Override
     @SuppressWarnings("unchecked")
-    public CoroutineFeign<C> build() {
-      super.enrich();
-
+    public CoroutineFeign<C> internalBuild() {
       AsyncFeign<C> asyncFeign = (AsyncFeign<C>) AsyncFeign.builder()
           .logLevel(logLevel)
           .client((AsyncClient<Object>) client)

reactive/src/main/java/feign/reactive/ReactiveFeign.java

@@ -42,13 +42,13 @@ public Builder contract(Contract contract) {
      * @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

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 static class Builder extends ReactiveFeign.Builder {
     }
 
     @Override
-    public Feign build() {
+    public Feign internalBuild() {
       super.invocationHandlerFactory(new ReactorInvocationHandlerFactory(scheduler));
-      return super.build();
+      return super.internalBuild();
     }
 
     @Override

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 static class Builder extends ReactiveFeign.Builder {
     private Scheduler scheduler = Schedulers.trampoline();
 
     @Override
-    public Feign build() {
+    public Feign internalBuild() {
       super.invocationHandlerFactory(new RxJavaInvocationHandlerFactory(scheduler));
-      return super.build();
+      return super.internalBuild();
     }
 
     @Override