From 8a5d6ea8e35eb2bc2bd2b90bd812132536e6cfa9 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 21 Jul 2023 00:09:06 +0200 Subject: [PATCH] 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 for deserialized fields, and a separate List for serialized fields. --- .../bind/ReflectiveTypeAdapterFactory.java | 106 +++++++++++------- .../google/gson/functional/ObjectTest.java | 35 +++++- 2 files changed, 96 insertions(+), 45 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 6a43b817c9..54b3c7e5a7 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 @@ -142,9 +142,8 @@ private static void checkAccessible(Object } 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) { + final Gson context, final Field field, final Method accessor, final String serializedName, + final TypeToken fieldType, final boolean serialize, final boolean blockInaccessible) { final boolean isPrimitive = Primitives.isPrimitive(fieldType.getRawType()); @@ -171,10 +170,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) { if (accessor == null) { checkAccessible(source, field); @@ -200,7 +198,7 @@ private BoundField createBoundField( // avoid direct recursion return; } - writer.name(name); + writer.name(serializedName); writeTypeAdapter.write(writer, fieldValue); } @@ -233,13 +231,35 @@ void readIntoField(JsonReader reader, Object target) }; } - private Map getBoundFields(Gson context, TypeToken type, Class raw, - boolean blockInaccessible, boolean isRecord) { - Map result = new LinkedHashMap<>(); + private static class FieldsData { + public static final FieldsData EMPTY = new FieldsData(Collections.emptyMap(), Collections.emptyList()); + + /** Maps from JSON member name to field */ + public final Map deserializedFields; + public final List serializedFields; + + public FieldsData(Map deserializedFields, List 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, boolean isRecord) { if (raw.isInterface()) { - return result; + return FieldsData.EMPTY; } + Map deserializedFields = new LinkedHashMap<>(); + // For serialized fields use a Map to track duplicate field names; otherwise this could be a List instead + Map serializedFields = new LinkedHashMap<>(); + Class originalRaw = raw; while (raw != Object.class) { Field[] fields = raw.getDeclaredFields(); @@ -293,44 +313,47 @@ private Map getBoundFields(Gson context, TypeToken type, if (!blockInaccessible && accessor == null) { ReflectionHelper.makeAccessible(field); } + Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType()); List 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, accessor, 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, accessor, 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 */ @@ -358,10 +381,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 extends TypeAdapter { - final Map boundFields; + private final FieldsData fieldsData; - Adapter(Map boundFields) { - this.boundFields = boundFields; + Adapter(FieldsData fieldsData) { + this.fieldsData = fieldsData; } @Override @@ -373,7 +396,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) { @@ -390,13 +413,14 @@ public T read(JsonReader in) throws IOException { } A accumulator = createAccumulator(); + Map 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); @@ -426,8 +450,8 @@ abstract void readField(A accumulator, JsonReader in, BoundField field) private static final class FieldReflectionAdapter extends Adapter { private final ObjectConstructor constructor; - FieldReflectionAdapter(ObjectConstructor constructor, Map boundFields) { - super(boundFields); + FieldReflectionAdapter(ObjectConstructor constructor, FieldsData fieldsData) { + super(fieldsData); this.constructor = constructor; } @@ -458,8 +482,8 @@ private static final class RecordAdapter extends Adapter { // Map from component names to index into the constructors arguments. private final Map componentIndices = new HashMap<>(); - RecordAdapter(Class raw, Map boundFields, boolean blockInaccessible) { - super(boundFields); + RecordAdapter(Class raw, FieldsData fieldsData, boolean blockInaccessible) { + super(fieldsData); constructor = ReflectionHelper.getCanonicalRecordConstructor(raw); if (blockInaccessible) { 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 b0de9da48d..230cd0fa52 100644 --- a/gson/src/test/java/com/google/gson/functional/ObjectTest.java +++ b/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); } }