Improve exception message for duplicate field names (#2251)

4 files changed, 50 insertions, 12 deletions

diff --git a/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java b/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java
index 5ddac50e..4db9bd72 100644
--- a/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java
+++ b/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java
@@ -139,7 +139,7 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
     }
   }
 
-  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) {
@@ -161,7 +161,7 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
 
     @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;
@@ -232,7 +232,6 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
       return result;
     }
 
-    Type declaredType = type.getType();
     Class<?> originalRaw = raw;
     while (raw != Object.class) {
       Field[] fields = raw.getDeclaredFields();
@@ -298,8 +297,9 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
           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()));
@@ -310,14 +310,16 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
 
   static abstract class BoundField {
     final String name;
+    final Field 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;
     }
diff --git a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java
index ac061212..737bb5d4 100644
--- a/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java
+++ b/gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java
@@ -53,8 +53,7 @@ public class ReflectionHelper {
     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;
 
@@ -76,6 +75,14 @@ public class ReflectionHelper {
   }
 
   /**
+   * 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)}
    */
diff --git a/gson/src/test/java/com/google/gson/functional/NamingPolicyTest.java b/gson/src/test/java/com/google/gson/functional/NamingPolicyTest.java
index ab76e649..c19ee460 100644
--- a/gson/src/test/java/com/google/gson/functional/NamingPolicyTest.java
+++ b/gson/src/test/java/com/google/gson/functional/NamingPolicyTest.java
@@ -22,10 +22,8 @@ import com.google.gson.GsonBuilder;
 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.
@@ -122,6 +120,12 @@ public class NamingPolicyTest extends TestCase {
       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()
+      );
     }
   }
 
diff --git a/gson/src/test/java/com/google/gson/functional/ObjectTest.java b/gson/src/test/java/com/google/gson/functional/ObjectTest.java
index bed5b598..cf851e06 100644
--- a/gson/src/test/java/com/google/gson/functional/ObjectTest.java
+++ b/gson/src/test/java/com/google/gson/functional/ObjectTest.java
@@ -145,6 +145,31 @@ public class ObjectTest extends TestCase {
     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"));