The builder clones itself before enrichment #2117
Conversation
| @@ -186,30 +186,32 @@ public AsyncBuilder<C> invocationHandlerFactory(InvocationHandlerFactory invocat | |||
| } | |||
|
|
|||
| public AsyncFeign<C> build() { | |||
| super.enrich(); | |||
| AsyncBuilder<C> enrichedBuilder = super.enrich(); | |||
There was a problem hiding this comment.
My major concern is that in the future, someone will access a field directly, forget the enrichBuilder prefix
What if, we move the builder body to a new method, and the we invoke the "internalBuild()" in the enriched instance?
|
WalkthroughThe changes primarily focus on refactoring the Changes
|
…r-wraps-over-already-wrapped-fields
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments generated: 1
Files selected for processing (6)
- core/src/main/java/feign/AsyncFeign.java (2 hunks)
- core/src/main/java/feign/BaseBuilder.java (3 hunks)
- core/src/main/java/feign/Feign.java (3 hunks)
- core/src/test/java/feign/BaseBuilderTest.java (3 hunks)
- hystrix/src/main/java/feign/hystrix/HystrixFeign.java (3 hunks)
- kotlin/src/main/java/feign/kotlin/CoroutineFeign.java (2 hunks)
LGTM comments (Suppressed): 4
core/src/test/java/feign/BaseBuilderTest.java (3)
17-17: The import
org.junit.Assert.assertNotSamehas been added. This is used to assert that two objects do not refer to the same object, which is useful for testing the cloning functionality in this context.34-37: The method signature of
test()has been updated to take aBaseBuilder<?, ?>instead ofBaseBuilder<?>. This change reflects the update in theBaseBuilderclass where it now takes two type parameters. Theenrichedvariable is now assigned the result of callingaddCapability(mockingCapability).enrich()on the builder, which is expected due to the changes in theBaseBuilderclass.50-50: An assertion has been added to verify that the original builder and the enriched builder are not the same object. This confirms that the
enrich()method creates a clone of the builder before enrichment, as intended by the changes in theBaseBuilderclass.core/src/main/java/feign/BaseBuilder.java (1)
- 32-32: The class now implements the
Cloneableinterface. This is a good practice when you want to create a copy of an object without affecting the original one, which seems to be the case here as per the PR summary. However, it's important to note that theclone()method in Java does a shallow copy of the object. If your class has complex objects (i.e., objects that contain other objects), you might need to override theclone()method and implement deep copying.
Additional comments (Suppressed): 11
kotlin/src/main/java/feign/kotlin/CoroutineFeign.java (2)
115-116: The class
CoroutineBuildernow extendsBaseBuilderwith two type parameters instead of one. Ensure that all instances ofCoroutineBuilderthroughout the codebase have been updated to match this new signature.160-162: The
build()method has been replaced byinternalBuild(). This change is consistent with the changes described in the PR summary. However, ensure that all calls toCoroutineBuilder.build()have been updated toCoroutineBuilder.internalBuild().hystrix/src/main/java/feign/hystrix/HystrixFeign.java (3)
16-19: The import statements have been rearranged. Ensure that this change does not affect the functionality of the code and is consistent with the project's coding style guide.
30-32: The import statements have been moved. This should not affect the functionality of the code, but ensure it aligns with the project's coding style guide.
136-138: The
build()method has been renamed tointernalBuild(). Make sure all calls to this method throughout the codebase have been updated to match the new method name.core/src/main/java/feign/Feign.java (3)
16-21: The import statements have been rearranged. Ensure that this change doesn't violate the project's code style guide. If there is no specific rule about the order of import statements, this change should be fine.
95-95: The
Builderclass now extendsBaseBuilder<Builder, Feign>. This change seems to be part of a larger refactoring effort to make the builder pattern more robust and flexible. Make sure all references to theBuilderclass in the codebase are updated accordingly.204-205: The
build()method has been replaced withinternalBuild(). This change is consistent with the PR summary, which mentions that the logic previously in thebuild()method is now ininternalBuild(). However, ensure that all calls tobuild()throughout the codebase have been updated to match the new method name.core/src/main/java/feign/AsyncFeign.java (2)
68-68: The
AsyncBuilderclass now extendsBaseBuilderwith two type parameters instead of one. This change is consistent with the changes described in the PR summary and should not cause any issues as long as all references toAsyncBuilderin the codebase are updated accordingly.193-194: The
build()method has been replaced withinternalBuild(), which is called by the newbuild()method inBaseBuilder. The logic insideinternalBuild()remains the same as the oldbuild()method, except that it no longer callsenrich(). This is becauseenrich()is now called on a cloned instance of the builder in the newbuild()method inBaseBuilder. This change is consistent with the PR summary and should not cause any issues as long as all calls tobuild()in the codebase are updated to reflect this change.core/src/main/java/feign/BaseBuilder.java (1)
- 286-290: The
build()method now callsenrich()on a cloned instance of the builder, followed byinternalBuild(). This is a good design choice as it ensures that the original builder remains unmodified whenbuild()is called. However, make sure that all subclasses ofBaseBuilderhave implemented theinternalBuild()method, as it's now abstract and must be provided by any concrete subclass.
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @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); | ||
| } |
There was a problem hiding this comment.
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:
-
The
CloneNotSupportedExceptionis being caught and rethrown as anAssertionError. This could be misleading becauseAssertionErroris usually thrown to indicate that an assertion has failed. It would be better to wrap theCloneNotSupportedExceptionin a custom unchecked exception that indicates a failure to clone the builder. -
The
IllegalArgumentExceptionandIllegalAccessExceptionare being caught and rethrown as aRuntimeExceptionwith 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.
…OpenFeign#2151) Bumps [io.sundr:sundr-maven-plugin](https://github.com/sundrio/sundrio) from 0.100.2 to 0.100.3. - [Commits](https://github.com/sundrio/sundrio/commits) --- updated-dependencies: - dependency-name: io.sundr:sundr-maven-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…hub.com:radio-rogal/feign into feign-builder-wraps-over-already-wrapped-fields
There was a problem hiding this comment.
Actionable comments generated: 0
Files selected for processing (1)
- hystrix/src/main/java/feign/hystrix/HystrixFeign.java (4 hunks)
Files skipped from review due to trivial changes (1)
- hystrix/src/main/java/feign/hystrix/HystrixFeign.java
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Actionable comments generated: 0
Files selected for processing (1)
- reactive/src/main/java/feign/reactive/ReactiveFeign.java (1 hunks)
Files skipped from review due to trivial changes (1)
- reactive/src/main/java/feign/reactive/ReactiveFeign.java
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
There was a problem hiding this comment.
Actionable comments generated: 0
Files selected for processing (2)
- reactive/src/main/java/feign/reactive/ReactorFeign.java (2 hunks)
- reactive/src/main/java/feign/reactive/RxJavaFeign.java (2 hunks)
LGTM comments (Suppressed): 1
reactive/src/main/java/feign/reactive/ReactorFeign.java (1)
- 17-23: The import statements have been rearranged. While this doesn't affect the functionality of the code, it's a good practice to keep related imports grouped together for better readability and maintainability.
Additional comments (Suppressed): 3
reactive/src/main/java/feign/reactive/RxJavaFeign.java (2)
16-23: The import statements have been rearranged. Ensure that this change does not violate any project-specific style guides or conventions.
36-38: The
build()method has been renamed tointernalBuild(). This is consistent with the changes described in the PR summary. However, ensure that all calls to this method throughout the codebase have been updated to match the new method name.- public Feign build() { + public Feign internalBuild() {reactive/src/main/java/feign/reactive/ReactorFeign.java (1)
- 44-46: The
internalBuild()method has been updated to call the parent class'sinternalBuild()method instead ofbuild(). This change is consistent with the PR summary and seems to be part of the effort to prevent wrapping already wrapped fields. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
|
Oh, AI rules world :) |
I have tried to resolve #1961: now when the builder makes a new *Feign it wraps own fields, on second call it wraps already wrapped fields. I replace the link to itself builder
thisBand own fields by a clone and its fields.Summary by CodeRabbit
Refactor:
internalBuild()across various classes in the Feign library, improving code modularity and maintainability.BaseBuilderclass with cloning capabilities to enrich builder objects, enhancing code consistency and logic.BaseBuilderclass.