Merge pull request #3449 from hutiefang76/codex/openfeign-2782-builder-clone

Fix fluent methods on cloned builders

Author: velo

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

@@ -40,8 +40,6 @@
 
 public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Cloneable {
 
-  private final B thisB;
-
   protected List<RequestInterceptor> requestInterceptors = new ArrayList<>();
   protected List<ResponseInterceptor> responseInterceptors = new ArrayList<>();
   protected List<MethodInterceptor> methodInterceptors = new ArrayList<>();
@@ -64,43 +62,47 @@ public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Clo
 
   public BaseBuilder() {
     super();
-    thisB = (B) this;
+  }
+
+  @SuppressWarnings("unchecked")
+  private B thisB() {
+    return (B) this;
   }
 
   public B logLevel(Logger.Level logLevel) {
     this.logLevel = logLevel;
-    return thisB;
+    return thisB();
   }
 
   public B contract(Contract contract) {
     this.contract = contract;
-    return thisB;
+    return thisB();
   }
 
   public B retryer(Retryer retryer) {
     this.retryer = retryer;
-    return thisB;
+    return thisB();
   }
 
   public B logger(Logger logger) {
     this.logger = logger;
-    return thisB;
+    return thisB();
   }
 
   public B encoder(Encoder encoder) {
     this.encoder = encoder;
-    return thisB;
+    return thisB();
   }
 
   public B decoder(Decoder decoder) {
     this.decoder = decoder;
-    return thisB;
+    return thisB();
   }
 
   public B codec(Codec codec) {
     this.encoder = codec.encoder();
     this.decoder = codec.decoder();
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -115,23 +117,23 @@ public B codec(Codec codec) {
    */
   public B doNotCloseAfterDecode() {
     this.closeAfterDecode = false;
-    return thisB;
+    return thisB();
   }
 
   public B decodeVoid() {
     this.decodeVoid = true;
-    return thisB;
+    return thisB();
   }
 
   public B queryMapEncoder(QueryMapEncoder queryMapEncoder) {
     this.queryMapEncoder = queryMapEncoder;
-    return thisB;
+    return thisB();
   }
 
   /** Allows to map the response before passing it to the decoder. */
   public B mapAndDecode(ResponseMapper mapper, Decoder decoder) {
     this.decoder = new ResponseMappingDecoder(mapper, decoder);
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -151,7 +153,7 @@ public B mapAndDecode(ResponseMapper mapper, Decoder decoder) {
    */
   public B dismiss404() {
     this.dismiss404 = true;
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -173,23 +175,23 @@ public B dismiss404() {
   @Deprecated
   public B decode404() {
     this.dismiss404 = true;
-    return thisB;
+    return thisB();
   }
 
   public B errorDecoder(ErrorDecoder errorDecoder) {
     this.errorDecoder = errorDecoder;
-    return thisB;
+    return thisB();
   }
 
   public B options(Options options) {
     this.options = options;
-    return thisB;
+    return thisB();
   }
 
   /** Adds a single request interceptor to the builder. */
   public B requestInterceptor(RequestInterceptor requestInterceptor) {
     this.requestInterceptors.add(requestInterceptor);
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -201,7 +203,7 @@ public B requestInterceptors(Iterable<RequestInterceptor> requestInterceptors) {
     for (RequestInterceptor requestInterceptor : requestInterceptors) {
       this.requestInterceptors.add(requestInterceptor);
     }
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -213,13 +215,13 @@ public B responseInterceptors(Iterable<ResponseInterceptor> responseInterceptors
     for (ResponseInterceptor responseInterceptor : responseInterceptors) {
       this.responseInterceptors.add(responseInterceptor);
     }
-    return thisB;
+    return thisB();
   }
 
   /** Adds a single response interceptor to the builder. */
   public B responseInterceptor(ResponseInterceptor responseInterceptor) {
     this.responseInterceptors.add(responseInterceptor);
-    return thisB;
+    return thisB();
   }
 
   /**
@@ -230,7 +232,7 @@ public B responseInterceptor(ResponseInterceptor responseInterceptor) {
   @Experimental
   public B methodInterceptor(MethodInterceptor methodInterceptor) {
     this.methodInterceptors.add(methodInterceptor);
-    return thisB;
+    return thisB();
   }
 
   /** Sets the full set of method interceptors, overwriting any previously configured. */
@@ -240,33 +242,33 @@ public B methodInterceptors(Iterable<MethodInterceptor> methodInterceptors) {
     for (MethodInterceptor methodInterceptor : methodInterceptors) {
       this.methodInterceptors.add(methodInterceptor);
     }
-    return thisB;
+    return thisB();
   }
 
   /** Allows you to override how reflective dispatch works inside of Feign. */
   public B invocationHandlerFactory(InvocationHandlerFactory invocationHandlerFactory) {
     this.invocationHandlerFactory = invocationHandlerFactory;
-    return thisB;
+    return thisB();
   }
 
   public B exceptionPropagationPolicy(ExceptionPropagationPolicy propagationPolicy) {
     this.propagationPolicy = propagationPolicy;
-    return thisB;
+    return thisB();
   }
 
   public B addCapability(Capability capability) {
     this.capabilities.add(capability);
-    return thisB;
+    return thisB();
   }
 
   @SuppressWarnings("unchecked")
   B enrich() {
     if (capabilities.isEmpty()) {
-      return thisB;
+      return thisB();
     }
 
     try {
-      B clone = (B) thisB.clone();
+      B clone = (B) thisB().clone();
 
       getFieldsToEnrich()
           .forEach(
@@ -367,8 +369,6 @@ List<Field> 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"))
         // interceptor lists are enriched per-element then as a whole via custom types
         .filter(field -> !Objects.equals(field.getName(), "requestInterceptors"))
         .filter(field -> !Objects.equals(field.getName(), "responseInterceptors"))

core/src/test/java/feign/BaseBuilderTest.java

@@ -19,6 +19,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.mockito.Mockito.RETURNS_MOCKS;
 
+import feign.codec.Decoder;
 import java.lang.reflect.Field;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -64,6 +65,22 @@ void checkEnrichTouchesAllBuilderFields()
         Feign.builder().requestInterceptor(_ -> {}).responseInterceptor((ic, c) -> c.next(ic)), 10);
   }
 
+  @Test
+  void clonedBuilderFluentMethodsReturnClone() throws CloneNotSupportedException {
+    CloneableFeignBuilder original = new CloneableFeignBuilder();
+    Feign.Builder clone = original.copy();
+
+    assertThat(clone.requestInterceptor(_ -> {})).isSameAs(clone);
+    assertThat(clone.decoder(Mockito.mock(Decoder.class))).isSameAs(clone);
+  }
+
+  private static final class CloneableFeignBuilder extends Feign.Builder {
+
+    Feign.Builder copy() throws CloneNotSupportedException {
+      return (Feign.Builder) super.clone();
+    }
+  }
+
   @Test
   void capabilityCanProvideResponseInterceptorWhenNoneConfigured() {
     AtomicInteger enrichCalls = new AtomicInteger();