FIX: interceptors disapearing on BaseBuilder#clone

Author: hosuaby

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

@@ -32,9 +32,6 @@
 import java.util.stream.Collectors;
 
 public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Cloneable {
-
-  private final B thisB;
-
   protected final List<RequestInterceptor> requestInterceptors = new ArrayList<>();
   protected final List<ResponseInterceptor> responseInterceptors = new ArrayList<>();
   protected Logger.Level logLevel = Logger.Level.NONE;
@@ -56,37 +53,42 @@ public abstract class BaseBuilder<B extends BaseBuilder<B, T>, 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<RequestInterceptor> 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<ResponseInterceptor> 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<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"))
         // skip primitive types
         .filter(field -> !field.getType().isPrimitive())
         // skip enumerations

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) {}
+  }
 }