Support serializing anonymous and local class with custom adapter

google · Sep 23, 2023 · 72922a6 · 72922a6
1 parent ff401f1
commit 72922a6
Show file tree

Hide file tree

Showing 8 changed files with 212 additions and 84 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
@@ -24,6 +24,7 @@
 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 Excluder withExclusionStrategy(ExclusionStrategy exclusionStrategy,
 
   @Override public <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> 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<T>() {
-      /** The delegate is lazily created because it may not be needed, and creating it may fail. */
-      private TypeAdapter<T> 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<T> delegate;
 
       @Override public T read(JsonReader in) throws IOException {
         if (skipDeserialize) {
@@ -139,6 +142,7 @@ public Excluder withExclusionStrategy(ExclusionStrategy exclusionStrategy,
       }
 
       private TypeAdapter<T> delegate() {
+        // A race might lead to `delegate` being assigned by multiple threads but the last assignment will stick
         TypeAdapter<T> d = delegate;
         return d != null
             ? d
@@ -168,11 +172,7 @@ public boolean excludeField(Field field, boolean serialize) {
       }
     }
 
-    if (!serializeInnerClasses && isInnerClass(field.getType())) {
-      return true;
-    }
-
-    if (isAnonymousOrNonStaticLocal(field.getType())) {
+    if (excludeClass(field.getType(), serialize)) {
       return true;
     }
 
@@ -189,44 +189,43 @@ public boolean excludeField(Field field, boolean serialize) {
     return false;
   }
 
-  private boolean excludeClassChecks(Class<?> clazz) {
-      if (version != Excluder.IGNORE_VERSIONS && !isValidVersion(clazz.getAnnotation(Since.class), clazz.getAnnotation(Until.class))) {
-          return true;
-      }
-
-      if (!serializeInnerClasses && isInnerClass(clazz)) {
-          return true;
-      }
-
-      return isAnonymousOrNonStaticLocal(clazz);
-  }
-
+  // public for unit tests; can otherwise be private
   public boolean excludeClass(Class<?> clazz, boolean serialize) {
-      return excludeClassChecks(clazz) ||
-              excludeClassInStrategy(clazz, serialize);
-  }
+    if (version != Excluder.IGNORE_VERSIONS && !isValidVersion(clazz.getAnnotation(Since.class), clazz.getAnnotation(Until.class))) {
+      return true;
+    }
 
-  private boolean excludeClassInStrategy(Class<?> clazz, boolean serialize) {
-      List<ExclusionStrategy> list = serialize ? serializationStrategies : deserializationStrategies;
-      for (ExclusionStrategy exclusionStrategy : list) {
-          if (exclusionStrategy.shouldSkipClass(clazz)) {
-              return true;
-          }
-      }
-      return false;
-  }
+    if (!serializeInnerClasses && isInnerClass(clazz)) {
+      return true;
+    }
 
-  private boolean isAnonymousOrNonStaticLocal(Class<?> clazz) {
-    return !Enum.class.isAssignableFrom(clazz) && !isStatic(clazz)
-        && (clazz.isAnonymousClass() || clazz.isLocalClass());
-  }
+    /*
+     * 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;
+    }
 
-  private boolean isInnerClass(Class<?> clazz) {
-    return clazz.isMemberClass() && !isStatic(clazz);
+    List<ExclusionStrategy> list = serialize ? serializationStrategies : deserializationStrategies;
+    for (ExclusionStrategy exclusionStrategy : list) {
+      if (exclusionStrategy.shouldSkipClass(clazz)) {
+        return true;
+      }
+    }
+    return false;
   }
 
-  private boolean isStatic(Class<?> clazz) {
-    return (clazz.getModifiers() & Modifier.STATIC) != 0;
+  private static boolean isInnerClass(Class<?> clazz) {
+    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
@@ -78,7 +78,7 @@ public ReflectiveTypeAdapterFactory(ConstructorConstructor constructorConstructo
   }
 
   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,25 @@ public <T> TypeAdapter<T> create(Gson gson, final TypeToken<T> type) {
       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<T>() {
+        @Override
+        public T read(JsonReader in) throws IOException {
+          in.skipValue();
+          return null;
+        }
+
+        @Override
+        public void write(JsonWriter out, T value) throws IOException {
+          out.nullValue();
+        }
+      };
+    }
+
     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
@@ -23,6 +23,7 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 
 public class ReflectionHelper {
 
@@ -139,6 +140,17 @@ private static void appendExecutableParameters(AccessibleObject executable, Stri
     stringBuilder.append(')');
   }
 
+  public static boolean isStatic(Class<?> clazz) {
+    return (clazz.getModifiers() & Modifier.STATIC) != 0;
+  }
+
+  /**
+   * 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
@@ -31,38 +31,48 @@
 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
@@ -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
@@ -22,6 +22,7 @@
 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 @@
 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)