Fix GraphAdapterBuilder#addType(Type) delegating to previous creator
#2811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When `addType(Type, InstanceCreator)` is used, it adds the creator to the `instanceCreators` map. However, previously that map was also used by the `constructorConstructor`. So when `addType(Type)` was used afterwards, instead of actually obtaining the default creator, it obtained the previously registered creator.
Marcono1234
commented
Feb 20, 2025
Comment on lines
-159
to
+207
| assertThat(jesse.company).isEqualTo(company); | ||
| assertThat(jesse.company).isSameInstanceAs(company); | ||
| Employee joel = company.employees.get(1); | ||
| assertThat(joel.name).isEqualTo("Joel"); | ||
| assertThat(joel.company).isEqualTo(company); | ||
| assertThat(joel.company).isSameInstanceAs(company); |
There was a problem hiding this comment.
Not directly related to the main changes of this pull request, but checking for reference equality here is more correct.
eamonnmcmanus
approved these changes
Feb 21, 2025
|
Have fixed the merge conflict, and included a small fix for when |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -121,6 +121,8 @@ | |||
| * @param gsonBuilder the {@code GsonBuilder} on which to register the graph adapter | |||
| */ | |||
| public void registerOn(GsonBuilder gsonBuilder) { | |||
| // Create copy to allow reusing GraphAdapterBuilder without affecting adapter factory | |||
| Map<Type, InstanceCreator<?>> instanceCreators = new HashMap<>(this.instanceCreators); | |||
Check notice
Code scanning / CodeQL
Possible confusion of local and field Note
There was a problem hiding this comment.
Intentionally shadowing this.instanceCreators, to avoid accidentally referring to it instead of the copy again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Fix
GraphAdapterBuilder#addType(Type)delegating to previous creatorDescription
When
addType(Type, InstanceCreator)is used, it adds the creator to theinstanceCreatorsmap. However, previously that map was also used by theconstructorConstructor.So when
addType(Type)was used afterwards, instead of actually obtaining the default creator fromconstructorConstructor, it obtained the previously registered creator. That does not seem intended to me.Side note: Passing
instanceCreatorsto theConstructorConstructorconstructor was done in 1d9e86e. But I am not sure if this was done intentionally, or just to solve a compilation error due to the removal of the no-argsConstructorConstructorconstructor in 040bae3 (?).Checklist
This is automatically checked by
mvn verify, but can also be checked on its own usingmvn spotless:check.Style violations can be fixed using
mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors