From bdaf6088b4d924b8e016aea3cd946ccde68a9342 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 22 Aug 2023 20:50:11 +0200 Subject: [PATCH] Avoid `equals` usage in `getDelegateAdapter` & minor other changes Previously `getDelegateAdapter` called `factories.contains(skipPast)`, but unlike the other comparisons which check for reference equality, that would have used the `equals` method. This could lead to spurious "GSON cannot serialize ..." exceptions if two factory instances compared equal, but the one provided as `skipPast` had not been registered yet. --- gson/src/main/java/com/google/gson/Gson.java | 23 +++++++++------- ...onAdapterAnnotationTypeAdapterFactory.java | 26 +++++++++++-------- .../test/java/com/google/gson/GsonTest.java | 20 ++++++++++++++ .../JsonAdapterAnnotationOnClassesTest.java | 22 +++++++++------- .../JsonAdapterAnnotationOnFieldsTest.java | 12 ++++++--- 5 files changed, 69 insertions(+), 34 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 49aea28a95..4ffda1ee7f 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -631,16 +631,16 @@ public TypeAdapter getAdapter(TypeToken type) { * StatsTypeAdapterFactory stats = new StatsTypeAdapterFactory(); * Gson gson = new GsonBuilder().registerTypeAdapterFactory(stats).create(); * // Call gson.toJson() and fromJson methods on objects - * System.out.println("Num JSON reads" + stats.numReads); - * System.out.println("Num JSON writes" + stats.numWrites); + * System.out.println("Num JSON reads: " + stats.numReads); + * System.out.println("Num JSON writes: " + stats.numWrites); * } * Note that this call will skip all factories registered before {@code skipPast}. In case of * multiple TypeAdapterFactories registered it is up to the caller of this function to insure * that the order of registration does not prevent this method from reaching a factory they * would expect to reply from this call. - * Note that since you can not override type adapter factories for String and Java primitive - * types, our stats factory will not count the number of String or primitives that will be - * read or written. + * Note that since you can not override the type adapter factories for some types, see + * {@link GsonBuilder#registerTypeAdapter(Type, Object)}, our stats factory will not count + * the number of instances of those types that will be read or written. * *

If {@code skipPast} is a factory which has neither been registered on the {@link GsonBuilder} * nor specified with the {@link JsonAdapter @JsonAdapter} annotation on a class, then this @@ -660,11 +660,8 @@ public TypeAdapter getDelegateAdapter(TypeAdapterFactory skipPast, TypeTo Objects.requireNonNull(skipPast, "skipPast must not be null"); Objects.requireNonNull(type, "type must not be null"); - if (jsonAdapterFactory.isClassJsonAdapterFactory(type.getRawType(), skipPast)) { + if (jsonAdapterFactory.isClassJsonAdapterFactory(type, skipPast)) { skipPast = jsonAdapterFactory; - } else if (!factories.contains(skipPast)) { - // Probably a factory from @JsonAdapter on a field - return getAdapter(type); } boolean skipPastFound = false; @@ -681,7 +678,13 @@ public TypeAdapter getDelegateAdapter(TypeAdapterFactory skipPast, TypeTo return candidate; } } - throw new IllegalArgumentException("GSON cannot serialize " + type); + + if (skipPastFound) { + throw new IllegalArgumentException("GSON cannot serialize " + type); + } else { + // Probably a factory from @JsonAdapter on a field + return getAdapter(type); + } } /** diff --git a/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java b/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java index 573de3324d..b444a4bd61 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java +++ b/gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java @@ -54,9 +54,10 @@ private static class DummyTypeAdapterFactory implements TypeAdapterFactory { private static final TypeAdapterFactory TREE_TYPE_FIELD_DUMMY_FACTORY = new DummyTypeAdapterFactory(); private final ConstructorConstructor constructorConstructor; + /** - * For a class, if it is annotated with {@code @JsonAdapter} and refers to a {@code TypeAdapterFactory} - * stores the factory instance, in case it has been requested already. + * For a class, if it is annotated with {@code @JsonAdapter} and refers to a {@link TypeAdapterFactory}, + * stores the factory instance in case it has been requested already. * Has to be a {@link ConcurrentMap} because {@link Gson} guarantees to be thread-safe. */ // Note: In case these strong reference to TypeAdapterFactory instances are considered @@ -121,16 +122,12 @@ TypeAdapter getTypeAdapter(ConstructorConstructor constructorConstructor, Gso ? (JsonDeserializer) instance : null; + // Uses dummy factory instances because TreeTypeAdapter needs a 'skipPast' factory for `Gson.getDelegateAdapter` + // call and has to differentiate there whether TreeTypeAdapter was created for @JsonAdapter on class or field TypeAdapterFactory skipPast; if (isClassAnnotation) { - // Use dummy `skipPast` value and put it into factory map; otherwise TreeTypeAdapter's call to - // `Gson.getDelegateAdapter` would cause infinite recursion because it would keep returning the - // adapter specified by @JsonAdapter skipPast = TREE_TYPE_CLASS_DUMMY_FACTORY; - adapterFactoryMap.put(type.getRawType(), skipPast); } else { - // Use dummy `skipPast` value, but don't put it into factory map; this way `Gson.getDelegateAdapter` - // will return regular adapter for field type without actually skipping past any factory skipPast = TREE_TYPE_FIELD_DUMMY_FACTORY; } @SuppressWarnings({ "unchecked", "rawtypes" }) @@ -154,12 +151,19 @@ TypeAdapter getTypeAdapter(ConstructorConstructor constructorConstructor, Gso /** * Returns whether {@code factory} is a type adapter factory created for {@code @JsonAdapter} - * placed on {@code rawType}. + * placed on {@code type}. */ - public boolean isClassJsonAdapterFactory(Class rawType, TypeAdapterFactory factory) { - Objects.requireNonNull(rawType); + public boolean isClassJsonAdapterFactory(TypeToken type, TypeAdapterFactory factory) { + Objects.requireNonNull(type); Objects.requireNonNull(factory); + if (factory == TREE_TYPE_CLASS_DUMMY_FACTORY) { + return true; + } + + // Using raw type to match behavior of `create(Gson, TypeToken)` above + Class rawType = type.getRawType(); + TypeAdapterFactory existingFactory = adapterFactoryMap.get(rawType); if (existingFactory != null) { // Checks for reference equality, like it is done by `Gson.getDelegateAdapter` diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index d8598ec953..47c3836da5 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -310,6 +310,18 @@ class DummyFactory implements TypeAdapterFactory { public TypeAdapter create(Gson gson, TypeToken type) { return (TypeAdapter) adapter; } + + // Override equals to verify that reference equality check is performed by Gson, + // and this method is ignored + @Override + public boolean equals(Object obj) { + return obj instanceof DummyFactory && ((DummyFactory) obj).adapter.equals(adapter); + } + + @Override + public int hashCode() { + return adapter.hashCode(); + } } DummyAdapter adapter1 = new DummyAdapter(1); @@ -334,6 +346,14 @@ public TypeAdapter create(Gson gson, TypeToken type) { assertThat(gson.getDelegateAdapter(factory2, type)).isEqualTo(adapter1); // Default Gson adapter should be returned assertThat(gson.getDelegateAdapter(factory1, type)).isNotInstanceOf(DummyAdapter.class); + + DummyFactory factory1Eq = new DummyFactory(adapter1); + // Verify that test setup is correct + assertThat(factory1.equals(factory1Eq)).isTrue(); + // Should only consider reference equality and ignore that custom `equals` method considers + // factories to be equal, therefore returning `adapter2` which came from `factory2` instead + // of skipping past `factory1` + assertThat(gson.getDelegateAdapter(factory1Eq, type)).isEqualTo(adapter2); } @Test diff --git a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java index e5aa70f4f0..d540f6e7d2 100644 --- a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java +++ b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java @@ -283,17 +283,21 @@ private static final class D { */ @Test public void testDelegatingAdapterFactory() { - WithDelegatingFactory deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class); + @SuppressWarnings("unchecked") + WithDelegatingFactory deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class); assertThat(deserialized.f).isEqualTo("de"); - WithDelegatingFactory serialized = new WithDelegatingFactory("se"); + deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", new TypeToken>() {}); + assertThat(deserialized.f).isEqualTo("de"); + + WithDelegatingFactory serialized = new WithDelegatingFactory<>("se"); assertThat(new Gson().toJson(serialized)).isEqualTo("{\"custom\":{\"f\":\"se\"}}"); } @JsonAdapter(WithDelegatingFactory.Factory.class) - private static class WithDelegatingFactory { - String f; + private static class WithDelegatingFactory { + T f; - WithDelegatingFactory(String f) { + WithDelegatingFactory(T f) { this.f = f; } @@ -396,10 +400,10 @@ public void testDelegating_SameFactoryClass() { .create(); // Should use both factories, and therefore have `{"custom": ... }` twice - WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"custom\":{\"f\":\"de\"}}}", WithDelegatingFactory.class); + WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"custom\":{\"f\":\"de\"}}}", WithDelegatingFactory.class); assertThat(deserialized.f).isEqualTo("de"); - WithDelegatingFactory serialized = new WithDelegatingFactory("se"); + WithDelegatingFactory serialized = new WithDelegatingFactory<>("se"); assertThat(gson.toJson(serialized)).isEqualTo("{\"custom\":{\"custom\":{\"f\":\"se\"}}}"); } @@ -425,10 +429,10 @@ public void testDelegating_SameFactoryInstance() { // or not, it can only work based on the `skipPast` factory, so if the same factory instance is used // the one registered with `GsonBuilder.registerTypeAdapterFactory` actually skips past the @JsonAdapter // one, so the JSON string is `{"custom": ...}` instead of `{"custom":{"custom":...}}` - WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class); + WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class); assertThat(deserialized.f).isEqualTo("de"); - WithDelegatingFactory serialized = new WithDelegatingFactory("se"); + WithDelegatingFactory serialized = new WithDelegatingFactory<>("se"); assertThat(gson.toJson(serialized)).isEqualTo("{\"custom\":{\"f\":\"se\"}}"); } diff --git a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnFieldsTest.java b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnFieldsTest.java index 226d3d6aa6..6601b2d36b 100644 --- a/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnFieldsTest.java +++ b/gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnFieldsTest.java @@ -477,17 +477,21 @@ public TypeAdapter create(Gson gson, TypeToken type) { */ @Test public void testDelegatingAdapterFactory() { - WithDelegatingFactory deserialized = new Gson().fromJson("{\"f\":\"test\"}", WithDelegatingFactory.class); + @SuppressWarnings("unchecked") + WithDelegatingFactory deserialized = new Gson().fromJson("{\"f\":\"test\"}", WithDelegatingFactory.class); + assertThat(deserialized.f).isEqualTo("test-custom"); + + deserialized = new Gson().fromJson("{\"f\":\"test\"}", new TypeToken>() {}); assertThat(deserialized.f).isEqualTo("test-custom"); - WithDelegatingFactory serialized = new WithDelegatingFactory(); + WithDelegatingFactory serialized = new WithDelegatingFactory<>(); serialized.f = "value"; assertThat(new Gson().toJson(serialized)).isEqualTo("{\"f\":\"value-custom\"}"); } - private static class WithDelegatingFactory { + private static class WithDelegatingFactory { @SuppressWarnings("SameNameButDifferent") // suppress Error Prone warning; should be clear that `Factory` refers to nested class @JsonAdapter(Factory.class) - String f; + T f; static class Factory implements TypeAdapterFactory { @SuppressWarnings("unchecked")