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

Fix GraphAdapterBuilder#addType(Type) delegating to previous creator #2811

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
public GraphAdapterBuilder() {
this.instanceCreators = new HashMap<>();
this.constructorConstructor =
new ConstructorConstructor(instanceCreators, true, Collections.emptyList());
new ConstructorConstructor(Collections.emptyMap(), true, Collections.emptyList());
}

/**
Expand Down Expand Up @@ -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

Potentially confusing name: method
registerOn
also refers to field
instanceCreators
(as this.instanceCreators).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Intentionally shadowing this.instanceCreators, to avoid accidentally referring to it instead of the copy again.

Factory factory = new Factory(instanceCreators);
gsonBuilder.registerTypeAdapterFactory(factory);
for (Map.Entry<Type, InstanceCreator<?>> entry : instanceCreators.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,51 @@ public void testDeserializationDirectSelfReference() {
assertThat(suicide.beats).isSameInstanceAs(suicide);
}

@Test
public void testAddTypeCustomInstanceCreator() {
GsonBuilder gsonBuilder = new GsonBuilder();
new GraphAdapterBuilder()
.addType(Company.class, type -> new Company("custom"))
.addType(Employee.class)
.registerOn(gsonBuilder);
Gson gson = gsonBuilder.create();

Company company =
gson.fromJson(
"{'0x1':{'employees':['0x2']},'0x2':{'name':'Jesse','company':'0x1'}}", Company.class);
assertThat(company.name).isEqualTo("custom");
Employee employee = company.employees.get(0);
assertThat(employee.name).isEqualTo("Jesse");
assertThat(employee.company).isSameInstanceAs(company);
}

@Test
public void testAddTypeOverwrite() {
GsonBuilder gsonBuilder = new GsonBuilder();
new GraphAdapterBuilder()
.addType(Company.class, type -> new Company("custom"))
// Overwrite Company creator with different custom one
.addType(Company.class, type -> new Company("custom-2"))
.addType(Employee.class)
.registerOn(gsonBuilder);
Gson gson = gsonBuilder.create();

Company company = gson.fromJson("{'0x1':{}}", Company.class);
assertThat(company.name).isEqualTo("custom-2");

gsonBuilder = new GsonBuilder();
new GraphAdapterBuilder()
.addType(Company.class, type -> new Company("custom"))
// Overwrite Company creator with default one
.addType(Company.class)
.addType(Employee.class)
.registerOn(gsonBuilder);
gson = gsonBuilder.create();

company = gson.fromJson("{'0x1':{}}", Company.class);
assertThat(company.name).isNull();
}

@Test
public void testSerializeListOfLists() {
Type listOfListsType = new TypeToken<List<List<?>>>() {}.getType();
Expand Down Expand Up @@ -156,10 +201,37 @@ public void testDeserializationWithMultipleTypes() {
assertThat(company.name).isEqualTo("Google");
Employee jesse = company.employees.get(0);
assertThat(jesse.name).isEqualTo("Jesse");
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);
Comment on lines -159 to +207
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not directly related to the main changes of this pull request, but checking for reference equality here is more correct.

}

@Test
public void testBuilderReuse() {
GsonBuilder gsonBuilder = new GsonBuilder();
GraphAdapterBuilder graphAdapterBuilder =
new GraphAdapterBuilder()
.addType(Company.class, type -> new Company("custom"))
.addType(Employee.class);
graphAdapterBuilder.registerOn(gsonBuilder);
Gson gson = gsonBuilder.create();

Company company = gson.fromJson("{'0x1':{}}", Company.class);
assertThat(company.name).isEqualTo("custom");

GsonBuilder gsonBuilder2 = new GsonBuilder();
// Reuse builder and overwrite creator
graphAdapterBuilder.addType(Company.class, type -> new Company("custom-2"));
graphAdapterBuilder.registerOn(gsonBuilder2);
Gson gson2 = gsonBuilder2.create();

company = gson2.fromJson("{'0x1':{}}", Company.class);
assertThat(company.name).isEqualTo("custom-2");

// But first adapter should not have been affected
company = gson.fromJson("{'0x1':{}}", Company.class);
assertThat(company.name).isEqualTo("custom");
}

static class Roshambo {
Expand Down