Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The builder clones itself before enrichment #2117

Merged
7 changes: 3 additions & 4 deletions core/src/main/java/feign/AsyncFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<>(
Expand Down Expand Up @@ -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(
Expand Down
56 changes: 34 additions & 22 deletions core/src/main/java/feign/BaseBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Comment on lines +233 to +267
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

The enrich() method has been updated to clone the builder before enriching it. This is a good approach to avoid mutating the original builder. However, there are some potential issues with exception handling:

  1. The CloneNotSupportedException is being caught and rethrown as an AssertionError. This could be misleading because AssertionError is usually thrown to indicate that an assertion has failed. It would be better to wrap the CloneNotSupportedException in a custom unchecked exception that indicates a failure to clone the builder.

  2. The IllegalArgumentException and IllegalAccessException are being caught and rethrown as a RuntimeException with a message indicating a failure to enrich a field. This is fine, but it would be more informative to include the name of the field in the error message.

Here's how you can improve the exception handling:

- throw new RuntimeException("Unable to enrich field " + field, e);
+ throw new RuntimeException("Unable to enrich field " + field.getName(), e);

- throw new AssertionError(e);
+ throw new BuilderCloneException("Failed to clone the builder", e);

You'll need to define the BuilderCloneException class, which could simply extend RuntimeException.

}

List<Field> getFieldsToEnrich() {
Expand All @@ -275,5 +283,9 @@ List<Field> getFieldsToEnrich() {
.collect(Collectors.toList());
}

public final T build() {
return enrich().internalBuild();
}

protected abstract T internalBuild();
}
9 changes: 4 additions & 5 deletions core/src/main/java/feign/Feign.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions core/src/test/java/feign/BaseBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -46,6 +47,7 @@ private void test(BaseBuilder<?> builder, int expectedFieldsCount)
}
assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue)
.isMock());
assertNotSame(builder, enriched);
}

}
Expand Down
10 changes: 5 additions & 5 deletions hystrix/src/main/java/feign/hystrix/HystrixFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -133,7 +133,7 @@ public Builder contract(Contract contract) {
}

@Override
public Feign build() {
public Feign internalBuild() {
return build(null);
}

Expand All @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions kotlin/src/main/java/feign/kotlin/CoroutineFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<>(
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions reactive/src/main/java/feign/reactive/ReactiveFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down