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.

Sign up for GitHub

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
merged 10 commits into from
Aug 21, 2023
Merged

The builder clones itself before enrichment #2117

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a9d8998
Enrichment of a clone
vitalijr2 Jul 3, 2023
52c4d85
Merge branch 'master' into feign-builder-wraps-over-already-wrapped-f…
vitalijr2 Jul 5, 2023
2640ceb
Merge branch 'master' into feign-builder-wraps-over-already-wrapped-f…
vitalijr2 Jul 16, 2023
87ed0af
Alternative internal builder process
velo Aug 21, 2023
dace3b5
Merge branch 'master' of github.com:OpenFeign/feign into feign-builde…
velo Aug 21, 2023
1b6f78f
build(deps): bump io.sundr:sundr-maven-plugin from 0.100.2 to 0.100.3…
dependabot[bot] Aug 14, 2023
227be3e
Alternative internal builder process
velo Aug 21, 2023
2e09730
Merge branch 'feign-builder-wraps-over-already-wrapped-fields' of git…
velo Aug 21, 2023
9981206
Alternative internal builder process
velo Aug 21, 2023
540370d
Alternative internal builder process
velo Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions core/src/main/java/feign/AsyncFeign.java
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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