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

Improve exception message for duplicate field names #2251

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 @@ -139,7 +139,7 @@ private static <M extends AccessibleObject & Member> void checkAccessible(Object
}
}

private ReflectiveTypeAdapterFactory.BoundField createBoundField(
private BoundField createBoundField(
final Gson context, final Field field, final Method accessor, final String name,
final TypeToken<?> fieldType, boolean serialize, boolean deserialize,
final boolean blockInaccessible) {
Expand All @@ -161,7 +161,7 @@ private ReflectiveTypeAdapterFactory.BoundField createBoundField(

@SuppressWarnings("unchecked")
final TypeAdapter<Object> typeAdapter = (TypeAdapter<Object>) mapped;
return new ReflectiveTypeAdapterFactory.BoundField(name, field.getName(), serialize, deserialize) {
return new BoundField(name, field, serialize, deserialize) {
@Override void write(JsonWriter writer, Object source)
throws IOException, IllegalAccessException {
if (!serialized) return;
Expand Down Expand Up @@ -232,7 +232,6 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
return result;
}

Type declaredType = type.getType();
Class<?> originalRaw = raw;
while (raw != Object.class) {
Field[] fields = raw.getDeclaredFields();
Expand Down Expand Up @@ -298,8 +297,9 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
if (previous == null) previous = replaced;
}
if (previous != null) {
throw new IllegalArgumentException(declaredType
+ " declares multiple JSON fields named " + previous.name);
throw new IllegalArgumentException("Class " + originalRaw.getName()
+ " declares multiple JSON fields named '" + previous.name + "'; conflict is caused"
+ " by fields " + ReflectionHelper.fieldToString(previous.field) + " and " + ReflectionHelper.fieldToString(field));
}
}
type = TypeToken.get($Gson$Types.resolve(type.getType(), raw, raw.getGenericSuperclass()));
Expand All @@ -310,14 +310,16 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,

static abstract class BoundField {
final String name;
final Field field;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As side note: Most likely BoundField (or rather its anonymous subclass) already had an implicit Field field since it captured the Field from the enclosing scope:

return new ReflectiveTypeAdapterFactory.BoundField(name, field.getName(), serialize, deserialize) {
@Override void write(JsonWriter writer, Object source)
throws IOException, IllegalAccessException {
if (!serialized) return;
if (blockInaccessible) {
if (accessor == null) {
checkAccessible(source, field);

/** Name of the underlying field */
final String fieldName;
final boolean serialized;
final boolean deserialized;

protected BoundField(String name, String fieldName, boolean serialized, boolean deserialized) {
protected BoundField(String name, Field field, boolean serialized, boolean deserialized) {
this.name = name;
this.fieldName = fieldName;
this.field = field;
this.fieldName = field.getName();
this.serialized = serialized;
this.deserialized = deserialized;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ public static String getAccessibleObjectDescription(AccessibleObject object, boo
String description;

if (object instanceof Field) {
Field field = (Field) object;
description = "field '" + field.getDeclaringClass().getName() + "#" + field.getName() + "'";
description = "field '" + fieldToString((Field) object) + "'";
} else if (object instanceof Method) {
Method method = (Method) object;

Expand All @@ -75,6 +74,14 @@ public static String getAccessibleObjectDescription(AccessibleObject object, boo
return description;
}

/**
* Creates a string representation for a field, omitting modifiers and
* the field type.
*/
public static String fieldToString(Field field) {
return field.getDeclaringClass().getName() + "#" + field.getName();
}

/**
* Creates a string representation for a constructor.
* E.g.: {@code java.lang.String(char[], int, int)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
import com.google.gson.annotations.SerializedName;
import com.google.gson.common.TestTypes.ClassWithSerializedNameFields;
import com.google.gson.common.TestTypes.StringWrapper;

import junit.framework.TestCase;

import java.lang.reflect.Field;
import junit.framework.TestCase;

/**
* Functional tests for naming policies.
Expand Down Expand Up @@ -122,6 +120,12 @@ public void testGsonDuplicateNameUsingSerializedNameFieldNamingPolicySerializati
gson.toJson(target);
fail();
} catch (IllegalArgumentException expected) {
assertEquals(
"Class com.google.gson.functional.NamingPolicyTest$ClassWithDuplicateFields declares multiple JSON fields named 'a';"
+ " conflict is caused by fields com.google.gson.functional.NamingPolicyTest$ClassWithDuplicateFields#a and"
+ " com.google.gson.functional.NamingPolicyTest$ClassWithDuplicateFields#b",
expected.getMessage()
);
}
}

Expand Down
25 changes: 25 additions & 0 deletions gson/src/test/java/com/google/gson/functional/ObjectTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,31 @@ public void testClassWithNoFieldsDeserialization() throws Exception {
assertEquals(expected, target);
}

private static class Subclass extends Superclass1 {
}
private static class Superclass1 extends Superclass2 {
@SuppressWarnings("unused")
String s;
}
private static class Superclass2 {
@SuppressWarnings("unused")
String s;
}

public void testClassWithDuplicateFields() {
try {
gson.getAdapter(Subclass.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals(
"Class com.google.gson.functional.ObjectTest$Subclass declares multiple JSON fields named 's';"
+ " conflict is caused by fields com.google.gson.functional.ObjectTest$Superclass1#s and"
+ " com.google.gson.functional.ObjectTest$Superclass2#s",
e.getMessage()
);
}
}

public void testNestedSerialization() throws Exception {
Nested target = new Nested(new BagOfPrimitives(10, 20, false, "stringValue"),
new BagOfPrimitives(30, 40, true, "stringValue"));
Expand Down