Only create one BoundField instance per field in ReflectiveTypeAdapterFactory (google#2440)

* Only create one BoundField instance per field in ReflectiveTypeAdapterFactory

Instead of creating a BoundField for every possible name of a field (for
SerializedName usage) and then storing for that BoundField whether it is
serialized or deserialized, instead only create one BoundField and then have
a separate Map<String, BoundField> for deserialized fields, and a separate
List<BoundField> for serialized fields.

* Fix indentation

Author: Marcono1234

gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java

@@ -130,9 +130,8 @@ private static <M extends AccessibleObject & Member> void checkAccessible(Object
   }
 
   private BoundField createBoundField(
-      final Gson context, final Field field, final String name,
-      final TypeToken<?> fieldType, boolean serialize, boolean deserialize,
-      final boolean blockInaccessible) {
+      final Gson context, final Field field, final String serializedName,
+      final TypeToken<?> fieldType, final boolean serialize, final boolean blockInaccessible) {
 
     final boolean isPrimitive = Primitives.isPrimitive(fieldType.getRawType());
 
@@ -159,10 +158,9 @@ private BoundField createBoundField(
       // Will never actually be used, but we set it to avoid confusing nullness-analysis tools
       writeTypeAdapter = typeAdapter;
     }
-    return new BoundField(name, field, serialize, deserialize) {
+    return new BoundField(serializedName, field) {
       @Override void write(JsonWriter writer, Object source)
           throws IOException, IllegalAccessException {
-        if (!serialized) return;
         if (blockInaccessible) {
           checkAccessible(source, field);
         }
@@ -173,7 +171,7 @@ private BoundField createBoundField(
           // avoid direct recursion
           return;
         }
-        writer.name(name);
+        writer.name(serializedName);
         writeTypeAdapter.write(writer, fieldValue);
       }
 
@@ -196,13 +194,35 @@ void readIntoField(JsonReader reader, Object target)
     };
   }
 
-  private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type, Class<?> raw,
-                                                 boolean blockInaccessible) {
-    Map<String, BoundField> result = new LinkedHashMap<>();
+  private static class FieldsData {
+    public static final FieldsData EMPTY = new FieldsData(Collections.<String, BoundField>emptyMap(), Collections.<BoundField>emptyList());
+
+    /** Maps from JSON member name to field */
+    public final Map<String, BoundField> deserializedFields;
+    public final List<BoundField> serializedFields;
+
+    public FieldsData(Map<String, BoundField> deserializedFields, List<BoundField> serializedFields) {
+      this.deserializedFields = deserializedFields;
+      this.serializedFields = serializedFields;
+    }
+  }
+
+  private static IllegalArgumentException createDuplicateFieldException(Class<?> declaringType, String duplicateName, Field field1, Field field2) {
+    throw new IllegalArgumentException("Class " + declaringType.getName()
+        + " declares multiple JSON fields named '" + duplicateName + "'; conflict is caused"
+        + " by fields " + ReflectionHelper.fieldToString(field1) + " and " + ReflectionHelper.fieldToString(field2)
+        + "\nSee " + TroubleshootingGuide.createUrl("duplicate-fields"));
+  }
+
+  private FieldsData getBoundFields(Gson context, TypeToken<?> type, Class<?> raw, boolean blockInaccessible) {
     if (raw.isInterface()) {
-      return result;
+      return FieldsData.EMPTY;
     }
 
+    Map<String, BoundField> deserializedFields = new LinkedHashMap<>();
+    // For serialized fields use a Map to track duplicate field names; otherwise this could be a List<BoundField> instead
+    Map<String, BoundField> serializedFields = new LinkedHashMap<>();
+
     Class<?> originalRaw = raw;
     while (raw != Object.class) {
       Field[] fields = raw.getDeclaredFields();
@@ -229,44 +249,47 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
         if (!blockInaccessible) {
           ReflectionHelper.makeAccessible(field);
         }
+
         Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());
         List<String> fieldNames = getFieldNames(field);
-        BoundField previous = null;
-        for (int i = 0, size = fieldNames.size(); i < size; ++i) {
-          String name = fieldNames.get(i);
-          if (i != 0) serialize = false; // only serialize the default name
-          BoundField boundField = createBoundField(context, field, name,
-              TypeToken.get(fieldType), serialize, deserialize, blockInaccessible);
-          BoundField replaced = result.put(name, boundField);
-          if (previous == null) previous = replaced;
+        String serializedName = fieldNames.get(0);
+        BoundField boundField = createBoundField(context, field, serializedName,
+                                                 TypeToken.get(fieldType), serialize, blockInaccessible);
+
+        if (deserialize) {
+          for (String name : fieldNames) {
+            BoundField replaced = deserializedFields.put(name, boundField);
+
+            if (replaced != null) {
+              throw createDuplicateFieldException(originalRaw, name, replaced.field, field);
+            }
+          }
         }
-        if (previous != null) {
-          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)
-              + "\nSee " + TroubleshootingGuide.createUrl("duplicate-fields"));
+
+        if (serialize) {
+          BoundField replaced = serializedFields.put(serializedName, boundField);
+          if (replaced != null) {
+            throw createDuplicateFieldException(originalRaw, serializedName, replaced.field, field);
+          }
         }
       }
       type = TypeToken.get($Gson$Types.resolve(type.getType(), raw, raw.getGenericSuperclass()));
       raw = type.getRawType();
     }
-    return result;
+    return new FieldsData(deserializedFields, new ArrayList<>(serializedFields.values()));
   }
 
   static abstract class BoundField {
-    final String name;
+    /** Name used for serialization (but not for deserialization) */
+    final String serializedName;
     final Field field;
     /** Name of the underlying field */
     final String fieldName;
-    final boolean serialized;
-    final boolean deserialized;
 
-    protected BoundField(String name, Field field, boolean serialized, boolean deserialized) {
-      this.name = name;
+    protected BoundField(String serializedName, Field field) {
+      this.serializedName = serializedName;
       this.field = field;
       this.fieldName = field.getName();
-      this.serialized = serialized;
-      this.deserialized = deserialized;
     }
 
     /** Read this field value from the source, and append its JSON value to the writer */
@@ -288,10 +311,10 @@ protected BoundField(String name, Field field, boolean serialized, boolean deser
    */
   // This class is public because external projects check for this class with `instanceof` (even though it is internal)
   public static abstract class Adapter<T, A> extends TypeAdapter<T> {
-    final Map<String, BoundField> boundFields;
+    private final FieldsData fieldsData;
 
-    Adapter(Map<String, BoundField> boundFields) {
-      this.boundFields = boundFields;
+    Adapter(FieldsData fieldsData) {
+      this.fieldsData = fieldsData;
     }
 
     @Override
@@ -303,7 +326,7 @@ public void write(JsonWriter out, T value) throws IOException {
 
       out.beginObject();
       try {
-        for (BoundField boundField : boundFields.values()) {
+        for (BoundField boundField : fieldsData.serializedFields) {
           boundField.write(out, value);
         }
       } catch (IllegalAccessException e) {
@@ -320,13 +343,14 @@ public T read(JsonReader in) throws IOException {
       }
 
       A accumulator = createAccumulator();
+      Map<String, BoundField> deserializedFields = fieldsData.deserializedFields;
 
       try {
         in.beginObject();
         while (in.hasNext()) {
           String name = in.nextName();
-          BoundField field = boundFields.get(name);
-          if (field == null || !field.deserialized) {
+          BoundField field = deserializedFields.get(name);
+          if (field == null) {
             in.skipValue();
           } else {
             readField(accumulator, in, field);
@@ -356,8 +380,8 @@ abstract void readField(A accumulator, JsonReader in, BoundField field)
   private static final class FieldReflectionAdapter<T> extends Adapter<T, T> {
     private final ObjectConstructor<T> constructor;
 
-    FieldReflectionAdapter(ObjectConstructor<T> constructor, Map<String, BoundField> boundFields) {
-      super(boundFields);
+    FieldReflectionAdapter(ObjectConstructor<T> constructor, FieldsData fieldsData) {
+      super(fieldsData);
       this.constructor = constructor;
     }
 

gson/src/test/java/com/google/gson/functional/ObjectTest.java

@@ -19,6 +19,8 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
+import com.google.gson.ExclusionStrategy;
+import com.google.gson.FieldAttributes;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.google.gson.InstanceCreator;
@@ -171,14 +173,39 @@ private static class Superclass2 {
 
   @Test
   public void testClassWithDuplicateFields() {
+    String expectedMessage = "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"
+        + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#duplicate-fields";
+
+    try {
+      gson.getAdapter(Subclass.class);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+    }
+
+    // Detection should also work properly when duplicate fields exist only for serialization
+    Gson gson = new GsonBuilder()
+        .addDeserializationExclusionStrategy(new ExclusionStrategy() {
+          @Override
+          public boolean shouldSkipField(FieldAttributes f) {
+            // Skip all fields for deserialization
+            return true;
+          }
+
+          @Override
+          public boolean shouldSkipClass(Class<?> clazz) {
+            return false;
+          }
+        })
+        .create();
+
     try {
       gson.getAdapter(Subclass.class);
       fail();
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("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"
-          + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#duplicate-fields");
+      assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
     }
   }