From 46ab704221608fb6318d110f1b0c2abca73a9ea2 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Mon, 29 Jan 2024 17:21:04 +0100 Subject: [PATCH] Support serializing anonymous and local class with custom adapter (#2498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Support serializing anonymous and local class with custom adapter * Fix formatting and fix switched 'expected' and 'actual' in EnumTest * Minor code improvements --------- Co-authored-by: Éamonn McManus --- .../com/google/gson/internal/Excluder.java | 60 ++++++++--------- .../bind/ReflectiveTypeAdapterFactory.java | 27 +++++++- .../internal/reflect/ReflectionHelper.java | 10 +++ ...ExposeAnnotationExclusionStrategyTest.java | 30 ++++++--- .../gson/InnerClassExclusionStrategyTest.java | 28 ++++++-- .../gson/VersionExclusionStrategyTest.java | 57 +++++++++++------ .../com/google/gson/functional/EnumTest.java | 19 +++--- .../google/gson/functional/ObjectTest.java | 64 ++++++++++++++++++- 8 files changed, 221 insertions(+), 74 deletions(-) diff --git a/gson/src/main/java/com/google/gson/internal/Excluder.java b/gson/src/main/java/com/google/gson/internal/Excluder.java index b3d2288a..9a6ba9db 100644 --- a/gson/src/main/java/com/google/gson/internal/Excluder.java +++ b/gson/src/main/java/com/google/gson/internal/Excluder.java @@ -24,6 +24,7 @@ import com.google.gson.TypeAdapterFactory; import com.google.gson.annotations.Expose; import com.google.gson.annotations.Since; import com.google.gson.annotations.Until; +import com.google.gson.internal.reflect.ReflectionHelper; import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonWriter; @@ -109,18 +110,20 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { @Override public TypeAdapter create(final Gson gson, final TypeToken type) { Class rawType = type.getRawType(); - boolean excludeClass = excludeClassChecks(rawType); - final boolean skipSerialize = excludeClass || excludeClassInStrategy(rawType, true); - final boolean skipDeserialize = excludeClass || excludeClassInStrategy(rawType, false); + final boolean skipSerialize = excludeClass(rawType, true); + final boolean skipDeserialize = excludeClass(rawType, false); if (!skipSerialize && !skipDeserialize) { return null; } return new TypeAdapter() { - /** The delegate is lazily created because it may not be needed, and creating it may fail. */ - private TypeAdapter delegate; + /** + * The delegate is lazily created because it may not be needed, and creating it may fail. + * Field has to be {@code volatile} because {@link Gson} guarantees to be thread-safe. + */ + private volatile TypeAdapter delegate; @Override public T read(JsonReader in) throws IOException { @@ -141,6 +144,8 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { } private TypeAdapter delegate() { + // A race might lead to `delegate` being assigned by multiple threads but the last + // assignment will stick TypeAdapter d = delegate; return d != null ? d : (delegate = gson.getDelegateAdapter(Excluder.this, type)); } @@ -168,11 +173,7 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { } } - if (!serializeInnerClasses && isInnerClass(field.getType())) { - return true; - } - - if (isAnonymousOrNonStaticLocal(field.getType())) { + if (excludeClass(field.getType(), serialize)) { return true; } @@ -189,7 +190,8 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { return false; } - private boolean excludeClassChecks(Class clazz) { + // public for unit tests; can otherwise be private + public boolean excludeClass(Class clazz, boolean serialize) { if (version != Excluder.IGNORE_VERSIONS && !isValidVersion(clazz.getAnnotation(Since.class), clazz.getAnnotation(Until.class))) { return true; @@ -199,14 +201,24 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { return true; } - return isAnonymousOrNonStaticLocal(clazz); - } + /* + * Exclude anonymous and local classes because they can have synthetic fields capturing enclosing + * values which makes serialization and deserialization unreliable. + * Don't exclude anonymous enum subclasses because enum types have a built-in adapter. + * + * Exclude only for deserialization; for serialization allow because custom adapter might be + * used; if no custom adapter exists reflection-based adapter otherwise excludes value. + * + * Cannot allow deserialization reliably here because some custom adapters like Collection adapter + * fall back to creating instances using Unsafe, which would likely lead to runtime exceptions + * for anonymous and local classes if they capture values. + */ + if (!serialize + && !Enum.class.isAssignableFrom(clazz) + && ReflectionHelper.isAnonymousOrNonStaticLocal(clazz)) { + return true; + } - public boolean excludeClass(Class clazz, boolean serialize) { - return excludeClassChecks(clazz) || excludeClassInStrategy(clazz, serialize); - } - - private boolean excludeClassInStrategy(Class clazz, boolean serialize) { List list = serialize ? serializationStrategies : deserializationStrategies; for (ExclusionStrategy exclusionStrategy : list) { if (exclusionStrategy.shouldSkipClass(clazz)) { @@ -216,18 +228,8 @@ public final class Excluder implements TypeAdapterFactory, Cloneable { return false; } - private static boolean isAnonymousOrNonStaticLocal(Class clazz) { - return !Enum.class.isAssignableFrom(clazz) - && !isStatic(clazz) - && (clazz.isAnonymousClass() || clazz.isLocalClass()); - } - private static boolean isInnerClass(Class clazz) { - return clazz.isMemberClass() && !isStatic(clazz); - } - - private static boolean isStatic(Class clazz) { - return (clazz.getModifiers() & Modifier.STATIC) != 0; + return clazz.isMemberClass() && !ReflectionHelper.isStatic(clazz); } private boolean isValidVersion(Since since, Until until) { 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 90869c63..79e93bcc 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 @@ -78,7 +78,7 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory { } private boolean includeField(Field f, boolean serialize) { - return !excluder.excludeClass(f.getType(), serialize) && !excluder.excludeField(f, serialize); + return !excluder.excludeField(f, serialize); } /** first element holds the default name */ @@ -110,6 +110,31 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory { return null; // it's a primitive! } + // Don't allow using reflection on anonymous and local classes because synthetic fields for + // captured enclosing values make this unreliable + if (ReflectionHelper.isAnonymousOrNonStaticLocal(raw)) { + // This adapter just serializes and deserializes null, ignoring the actual values + // This is done for backward compatibility; troubleshooting-wise it might be better to throw + // exceptions + return new TypeAdapter() { + @Override + public T read(JsonReader in) throws IOException { + in.skipValue(); + return null; + } + + @Override + public void write(JsonWriter out, T value) throws IOException { + out.nullValue(); + } + + @Override + public String toString() { + return "AnonymousOrNonStaticLocalClassAdapter"; + } + }; + } + FilterResult filterResult = ReflectionAccessFilterHelper.getFilterResult(reflectionFilters, raw); if (filterResult == FilterResult.BLOCK_ALL) { 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 ded1e689..7ee75bde 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 @@ -23,6 +23,7 @@ import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; public class ReflectionHelper { @@ -146,6 +147,15 @@ public class ReflectionHelper { stringBuilder.append(')'); } + public static boolean isStatic(Class clazz) { + return Modifier.isStatic(clazz.getModifiers()); + } + + /** Returns whether the class is anonymous or a non-static local class. */ + public static boolean isAnonymousOrNonStaticLocal(Class clazz) { + return !isStatic(clazz) && (clazz.isAnonymousClass() || clazz.isLocalClass()); + } + /** * Tries making the constructor accessible, returning an exception message if this fails. * diff --git a/gson/src/test/java/com/google/gson/ExposeAnnotationExclusionStrategyTest.java b/gson/src/test/java/com/google/gson/ExposeAnnotationExclusionStrategyTest.java index 0e929f04..8738e5b9 100644 --- a/gson/src/test/java/com/google/gson/ExposeAnnotationExclusionStrategyTest.java +++ b/gson/src/test/java/com/google/gson/ExposeAnnotationExclusionStrategyTest.java @@ -31,38 +31,48 @@ import org.junit.Test; public class ExposeAnnotationExclusionStrategyTest { private Excluder excluder = Excluder.DEFAULT.excludeFieldsWithoutExposeAnnotation(); + private void assertIncludesClass(Class c) { + assertThat(excluder.excludeClass(c, true)).isFalse(); + assertThat(excluder.excludeClass(c, false)).isFalse(); + } + + private void assertIncludesField(Field f) { + assertThat(excluder.excludeField(f, true)).isFalse(); + assertThat(excluder.excludeField(f, false)).isFalse(); + } + + private void assertExcludesField(Field f) { + assertThat(excluder.excludeField(f, true)).isTrue(); + assertThat(excluder.excludeField(f, false)).isTrue(); + } + @Test public void testNeverSkipClasses() { - assertThat(excluder.excludeClass(MockObject.class, true)).isFalse(); - assertThat(excluder.excludeClass(MockObject.class, false)).isFalse(); + assertIncludesClass(MockObject.class); } @Test public void testSkipNonAnnotatedFields() throws Exception { Field f = createFieldAttributes("hiddenField"); - assertThat(excluder.excludeField(f, true)).isTrue(); - assertThat(excluder.excludeField(f, false)).isTrue(); + assertExcludesField(f); } @Test public void testSkipExplicitlySkippedFields() throws Exception { Field f = createFieldAttributes("explicitlyHiddenField"); - assertThat(excluder.excludeField(f, true)).isTrue(); - assertThat(excluder.excludeField(f, false)).isTrue(); + assertExcludesField(f); } @Test public void testNeverSkipExposedAnnotatedFields() throws Exception { Field f = createFieldAttributes("exposedField"); - assertThat(excluder.excludeField(f, true)).isFalse(); - assertThat(excluder.excludeField(f, false)).isFalse(); + assertIncludesField(f); } @Test public void testNeverSkipExplicitlyExposedAnnotatedFields() throws Exception { Field f = createFieldAttributes("explicitlyExposedField"); - assertThat(excluder.excludeField(f, true)).isFalse(); - assertThat(excluder.excludeField(f, false)).isFalse(); + assertIncludesField(f); } @Test diff --git a/gson/src/test/java/com/google/gson/InnerClassExclusionStrategyTest.java b/gson/src/test/java/com/google/gson/InnerClassExclusionStrategyTest.java index aa854b6e..f768c501 100644 --- a/gson/src/test/java/com/google/gson/InnerClassExclusionStrategyTest.java +++ b/gson/src/test/java/com/google/gson/InnerClassExclusionStrategyTest.java @@ -32,28 +32,48 @@ public class InnerClassExclusionStrategyTest { public StaticNestedClass staticNestedClass = new StaticNestedClass(); private Excluder excluder = Excluder.DEFAULT.disableInnerClassSerialization(); + private void assertIncludesClass(Class c) { + assertThat(excluder.excludeClass(c, true)).isFalse(); + assertThat(excluder.excludeClass(c, false)).isFalse(); + } + + private void assertExcludesClass(Class c) { + assertThat(excluder.excludeClass(c, true)).isTrue(); + assertThat(excluder.excludeClass(c, false)).isTrue(); + } + + private void assertIncludesField(Field f) { + assertThat(excluder.excludeField(f, true)).isFalse(); + assertThat(excluder.excludeField(f, false)).isFalse(); + } + + private void assertExcludesField(Field f) { + assertThat(excluder.excludeField(f, true)).isTrue(); + assertThat(excluder.excludeField(f, false)).isTrue(); + } + @Test public void testExcludeInnerClassObject() { Class clazz = innerClass.getClass(); - assertThat(excluder.excludeClass(clazz, true)).isTrue(); + assertExcludesClass(clazz); } @Test public void testExcludeInnerClassField() throws Exception { Field f = getClass().getField("innerClass"); - assertThat(excluder.excludeField(f, true)).isTrue(); + assertExcludesField(f); } @Test public void testIncludeStaticNestedClassObject() { Class clazz = staticNestedClass.getClass(); - assertThat(excluder.excludeClass(clazz, true)).isFalse(); + assertIncludesClass(clazz); } @Test public void testIncludeStaticNestedClassField() throws Exception { Field f = getClass().getField("staticNestedClass"); - assertThat(excluder.excludeField(f, true)).isFalse(); + assertIncludesField(f); } @SuppressWarnings("ClassCanBeStatic") diff --git a/gson/src/test/java/com/google/gson/VersionExclusionStrategyTest.java b/gson/src/test/java/com/google/gson/VersionExclusionStrategyTest.java index 2c644964..5c2b22ca 100644 --- a/gson/src/test/java/com/google/gson/VersionExclusionStrategyTest.java +++ b/gson/src/test/java/com/google/gson/VersionExclusionStrategyTest.java @@ -22,6 +22,7 @@ import com.google.errorprone.annotations.Keep; import com.google.gson.annotations.Since; import com.google.gson.annotations.Until; import com.google.gson.internal.Excluder; +import java.lang.reflect.Field; import org.junit.Test; /** @@ -32,44 +33,64 @@ import org.junit.Test; public class VersionExclusionStrategyTest { private static final double VERSION = 5.0D; + private static void assertIncludesClass(Excluder excluder, Class c) { + assertThat(excluder.excludeClass(c, true)).isFalse(); + assertThat(excluder.excludeClass(c, false)).isFalse(); + } + + private static void assertExcludesClass(Excluder excluder, Class c) { + assertThat(excluder.excludeClass(c, true)).isTrue(); + assertThat(excluder.excludeClass(c, false)).isTrue(); + } + + private static void assertIncludesField(Excluder excluder, Field f) { + assertThat(excluder.excludeField(f, true)).isFalse(); + assertThat(excluder.excludeField(f, false)).isFalse(); + } + + private static void assertExcludesField(Excluder excluder, Field f) { + assertThat(excluder.excludeField(f, true)).isTrue(); + assertThat(excluder.excludeField(f, false)).isTrue(); + } + @Test public void testSameVersion() throws Exception { Excluder excluder = Excluder.DEFAULT.withVersion(VERSION); - assertThat(excluder.excludeClass(MockClassSince.class, true)).isFalse(); - assertThat(excluder.excludeField(MockClassSince.class.getField("someField"), true)).isFalse(); + assertIncludesClass(excluder, MockClassSince.class); + assertIncludesField(excluder, MockClassSince.class.getField("someField")); // Until version is exclusive - assertThat(excluder.excludeClass(MockClassUntil.class, true)).isTrue(); - assertThat(excluder.excludeField(MockClassUntil.class.getField("someField"), true)).isTrue(); + assertExcludesClass(excluder, MockClassUntil.class); + assertExcludesField(excluder, MockClassUntil.class.getField("someField")); - assertThat(excluder.excludeClass(MockClassBoth.class, true)).isFalse(); - assertThat(excluder.excludeField(MockClassBoth.class.getField("someField"), true)).isFalse(); + assertIncludesClass(excluder, MockClassBoth.class); + assertIncludesField(excluder, MockClassBoth.class.getField("someField")); } @Test public void testNewerVersion() throws Exception { Excluder excluder = Excluder.DEFAULT.withVersion(VERSION + 5); - assertThat(excluder.excludeClass(MockClassSince.class, true)).isFalse(); - assertThat(excluder.excludeField(MockClassSince.class.getField("someField"), true)).isFalse(); + assertIncludesClass(excluder, MockClassSince.class); + assertIncludesField(excluder, MockClassSince.class.getField("someField")); - assertThat(excluder.excludeClass(MockClassUntil.class, true)).isTrue(); - assertThat(excluder.excludeField(MockClassUntil.class.getField("someField"), true)).isTrue(); + assertExcludesClass(excluder, MockClassUntil.class); + assertExcludesField(excluder, MockClassUntil.class.getField("someField")); - assertThat(excluder.excludeClass(MockClassBoth.class, true)).isTrue(); - assertThat(excluder.excludeField(MockClassBoth.class.getField("someField"), true)).isTrue(); + assertExcludesClass(excluder, MockClassBoth.class); + assertExcludesField(excluder, MockClassBoth.class.getField("someField")); } @Test public void testOlderVersion() throws Exception { Excluder excluder = Excluder.DEFAULT.withVersion(VERSION - 5); - assertThat(excluder.excludeClass(MockClassSince.class, true)).isTrue(); - assertThat(excluder.excludeField(MockClassSince.class.getField("someField"), true)).isTrue(); + assertExcludesClass(excluder, MockClassSince.class); + assertExcludesField(excluder, MockClassSince.class.getField("someField")); - assertThat(excluder.excludeClass(MockClassUntil.class, true)).isFalse(); - assertThat(excluder.excludeField(MockClassUntil.class.getField("someField"), true)).isFalse(); + assertIncludesClass(excluder, MockClassUntil.class); + assertIncludesField(excluder, MockClassUntil.class.getField("someField")); - assertThat(excluder.excludeClass(MockClassBoth.class, true)).isTrue(); - assertThat(excluder.excludeField(MockClassBoth.class.getField("someField"), true)).isTrue(); + assertExcludesClass(excluder, MockClassBoth.class); + assertExcludesField(excluder, MockClassBoth.class.getField("someField")); } @Since(VERSION) diff --git a/gson/src/test/java/com/google/gson/functional/EnumTest.java b/gson/src/test/java/com/google/gson/functional/EnumTest.java index aadf1b4f..835fb508 100644 --- a/gson/src/test/java/com/google/gson/functional/EnumTest.java +++ b/gson/src/test/java/com/google/gson/functional/EnumTest.java @@ -127,10 +127,13 @@ public class EnumTest { assertThat(gson.toJson(EnumSet.allOf(Roshambo.class))) .isEqualTo("[\"ROCK\",\"PAPER\",\"SCISSORS\"]"); assertThat(gson.fromJson("\"ROCK\"", Roshambo.class)).isEqualTo(Roshambo.ROCK); - assertThat(EnumSet.allOf(Roshambo.class)) - .isEqualTo( - gson.fromJson( - "[\"ROCK\",\"PAPER\",\"SCISSORS\"]", new TypeToken>() {}.getType())); + Set deserialized = + gson.fromJson("[\"ROCK\",\"PAPER\",\"SCISSORS\"]", new TypeToken<>() {}); + assertThat(deserialized).isEqualTo(EnumSet.allOf(Roshambo.class)); + + // A bit contrived, but should also work if explicitly deserializing using anonymous enum + // subclass + assertThat(gson.fromJson("\"ROCK\"", Roshambo.ROCK.getClass())).isEqualTo(Roshambo.ROCK); } @Test @@ -145,11 +148,9 @@ public class EnumTest { assertThat(gson.toJson(EnumSet.allOf(Roshambo.class))) .isEqualTo("[\"123ROCK\",\"123PAPER\",\"123SCISSORS\"]"); assertThat(gson.fromJson("\"123ROCK\"", Roshambo.class)).isEqualTo(Roshambo.ROCK); - assertThat(EnumSet.allOf(Roshambo.class)) - .isEqualTo( - gson.fromJson( - "[\"123ROCK\",\"123PAPER\",\"123SCISSORS\"]", - new TypeToken>() {}.getType())); + Set deserialized = + gson.fromJson("[\"123ROCK\",\"123PAPER\",\"123SCISSORS\"]", new TypeToken<>() {}); + assertThat(deserialized).isEqualTo(EnumSet.allOf(Roshambo.class)); } @Test 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 5e684ee8..d0f7e38d 100644 --- a/gson/src/test/java/com/google/gson/functional/ObjectTest.java +++ b/gson/src/test/java/com/google/gson/functional/ObjectTest.java @@ -24,10 +24,13 @@ import com.google.gson.FieldAttributes; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.InstanceCreator; +import com.google.gson.JsonDeserializationContext; +import com.google.gson.JsonDeserializer; import com.google.gson.JsonElement; import com.google.gson.JsonIOException; import com.google.gson.JsonObject; import com.google.gson.JsonParseException; +import com.google.gson.JsonPrimitive; import com.google.gson.JsonSerializationContext; import com.google.gson.JsonSerializer; import com.google.gson.common.TestTypes.ArrayOfObjects; @@ -381,11 +384,14 @@ public class ObjectTest { // empty anonymous class })) .isEqualTo("null"); + + class Local {} + assertThat(gson.toJson(new Local())).isEqualTo("null"); } @Test public void testAnonymousLocalClassesCustomSerialization() { - gson = + Gson gson = new GsonBuilder() .registerTypeHierarchyAdapter( ClassWithNoFields.class, @@ -393,7 +399,7 @@ public class ObjectTest { @Override public JsonElement serialize( ClassWithNoFields src, Type typeOfSrc, JsonSerializationContext context) { - return new JsonObject(); + return new JsonPrimitive("custom-value"); } }) .create(); @@ -403,7 +409,59 @@ public class ObjectTest { new ClassWithNoFields() { // empty anonymous class })) - .isEqualTo("null"); + .isEqualTo("\"custom-value\""); + + class Local {} + gson = + new GsonBuilder() + .registerTypeAdapter( + Local.class, + new JsonSerializer() { + @Override + public JsonElement serialize( + Local src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive("custom-value"); + } + }) + .create(); + assertThat(gson.toJson(new Local())).isEqualTo("\"custom-value\""); + } + + @Test + public void testAnonymousLocalClassesCustomDeserialization() { + Gson gson = + new GsonBuilder() + .registerTypeHierarchyAdapter( + ClassWithNoFields.class, + new JsonDeserializer() { + @Override + public ClassWithNoFields deserialize( + JsonElement json, Type typeOfT, JsonDeserializationContext context) { + return new ClassWithNoFields(); + } + }) + .create(); + + assertThat(gson.fromJson("{}", ClassWithNoFields.class)).isNotNull(); + Class anonymousClass = new ClassWithNoFields() {}.getClass(); + // Custom deserializer is ignored + assertThat(gson.fromJson("{}", anonymousClass)).isNull(); + + class Local {} + gson = + new GsonBuilder() + .registerTypeAdapter( + Local.class, + new JsonDeserializer() { + @Override + public Local deserialize( + JsonElement json, Type typeOfT, JsonDeserializationContext context) { + throw new AssertionError("should not be called"); + } + }) + .create(); + // Custom deserializer is ignored + assertThat(gson.fromJson("{}", Local.class)).isNull(); } @Test