Fix failing to serialize Collection or Map with inaccessible constructor

gson/pom.xml

@@ -31,6 +31,21 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>3.0.0-M5</version>
+        <configuration>
+          <!-- Deny illegal access, this is required for ReflectionAccessTest -->
+          <!-- Requires Java >= 9; Important: In case future Java versions 
+            don't support this flag anymore, don't remove it unless CI also runs with 
+            that Java version. Ideally would use toolchain to specify that this should 
+            run with e.g. Java 11, but Maven toolchain requirements (unlike Gradle ones) 
+            don't seem to be portable (every developer would have to set up toolchain 
+            configuration locally). -->
+          <argLine>--illegal-access=deny</argLine>
+        </configuration>
+      </plugin>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-javadoc-plugin</artifactId>

gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java

@@ -40,15 +40,14 @@
 
 import com.google.gson.InstanceCreator;
 import com.google.gson.JsonIOException;
-import com.google.gson.internal.reflect.ReflectionAccessor;
+import com.google.gson.internal.reflect.ReflectionHelper;
 import com.google.gson.reflect.TypeToken;
 
 /**
  * Returns a function that can construct an instance of a requested type.
  */
 public final class ConstructorConstructor {
   private final Map<Type, InstanceCreator<?>> instanceCreators;
-  private final ReflectionAccessor accessor = ReflectionAccessor.getInstance();
 
   public ConstructorConstructor(Map<Type, InstanceCreator<?>> instanceCreators) {
     this.instanceCreators = instanceCreators;
@@ -97,33 +96,53 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
   }
 
   private <T> ObjectConstructor<T> newDefaultConstructor(Class<? super T> rawType) {
+    final Constructor<? super T> constructor;
     try {
-      final Constructor<? super T> constructor = rawType.getDeclaredConstructor();
-      if (!constructor.isAccessible()) {
-        accessor.makeAccessible(constructor);
-      }
+      constructor = rawType.getDeclaredConstructor();
+    } catch (NoSuchMethodException e) {
+      return null;
+    }
+
+    final String exceptionMessage = ReflectionHelper.tryMakeAccessible(constructor);
+    if (exceptionMessage != null) {
+      /*
+       * Create ObjectConstructor which throws exception.
+       * This keeps backward compatibility (compared to returning `null` which
+       * would then choose another way of creating object).
+       * And it supports types which are only serialized but not deserialized
+       * (compared to directly throwing exception here), e.g. when runtime type
+       * of object is inaccessible, but compile-time type is accessible.
+       */
       return new ObjectConstructor<T>() {
-        @SuppressWarnings("unchecked") // T is the same raw type as is requested
-        @Override public T construct() {
-          try {
-            Object[] args = null;
-            return (T) constructor.newInstance(args);
-          } catch (InstantiationException e) {
-            // TODO: JsonParseException ?
-            throw new RuntimeException("Failed to invoke " + constructor + " with no args", e);
-          } catch (InvocationTargetException e) {
-            // TODO: don't wrap if cause is unchecked!
-            // TODO: JsonParseException ?
-            throw new RuntimeException("Failed to invoke " + constructor + " with no args",
-                e.getTargetException());
-          } catch (IllegalAccessException e) {
-            throw new AssertionError(e);
-          }
+        @Override
+        public T construct() {
+          // New exception is created every time to avoid keeping reference
+          // to exception with potentially long stack trace, causing a
+          // memory leak
+          throw new JsonIOException(exceptionMessage);
         }
       };
-    } catch (NoSuchMethodException e) {
-      return null;
     }
+
+    return new ObjectConstructor<T>() {
+      @SuppressWarnings("unchecked") // T is the same raw type as is requested
+      @Override public T construct() {
+        try {
+          Object[] args = null;
+          return (T) constructor.newInstance(args);
+        } catch (InstantiationException e) {
+          // TODO: JsonParseException ?
+          throw new RuntimeException("Failed to invoke " + constructor + " with no args", e);
+        } catch (InvocationTargetException e) {
+          // TODO: don't wrap if cause is unchecked!
+          // TODO: JsonParseException ?
+          throw new RuntimeException("Failed to invoke " + constructor + " with no args",
+              e.getTargetException());
+        } catch (IllegalAccessException e) {
+          throw new AssertionError(e);
+        }
+      }
+    };
   }
 
   /**

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

@@ -28,7 +28,7 @@
 import com.google.gson.internal.Excluder;
 import com.google.gson.internal.ObjectConstructor;
 import com.google.gson.internal.Primitives;
-import com.google.gson.internal.reflect.ReflectionAccessor;
+import com.google.gson.internal.reflect.ReflectionHelper;
 import com.google.gson.reflect.TypeToken;
 import com.google.gson.stream.JsonReader;
 import com.google.gson.stream.JsonToken;
@@ -50,7 +50,6 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
   private final FieldNamingStrategy fieldNamingPolicy;
   private final Excluder excluder;
   private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory;
-  private final ReflectionAccessor accessor = ReflectionAccessor.getInstance();
 
   public ReflectiveTypeAdapterFactory(ConstructorConstructor constructorConstructor,
       FieldNamingStrategy fieldNamingPolicy, Excluder excluder,
@@ -156,7 +155,7 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
         if (!serialize && !deserialize) {
           continue;
         }
-        accessor.makeAccessible(field);
+        ReflectionHelper.makeAccessible(field);
         Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());
         List<String> fieldNames = getFieldNames(field);
         BoundField previous = null;

gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java

@@ -0,0 +1,71 @@
+package com.google.gson.internal.reflect;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+
+import com.google.gson.JsonIOException;
+
+public class ReflectionHelper {
+  private ReflectionHelper() { }
+
+  /**
+   * Tries making the field accessible, wrapping any thrown exception in a
+   * {@link JsonIOException} with descriptive message.
+   *
+   * @param field
+   *    Field to make accessible
+   * @throws JsonIOException
+   *    If making the field accessible fails
+   */
+  public static void makeAccessible(Field field) throws JsonIOException {
+    try {
+      field.setAccessible(true);
+    } catch (Exception exception) {
+      throw new JsonIOException("Failed making field '" + field.getDeclaringClass().getName() + "#"
+          + field.getName() + "' accessible; either change its visibility or write a custom "
+          + "TypeAdapter for its declaring type", exception);
+    }
+  }
+
+  /**
+   * Creates a string representation for a constructor.
+   * E.g.: {@code java.lang.String#String(char[], int, int)}
+   */
+  private static String constructorToString(Constructor<?> constructor) {
+    StringBuilder stringBuilder = new StringBuilder(constructor.getDeclaringClass().getName())
+      .append('#')
+      .append(constructor.getDeclaringClass().getSimpleName())
+      .append('(');
+    Class<?>[] parameters = constructor.getParameterTypes();
+    for (int i = 0; i < parameters.length; i++) {
+      if (i > 0) {
+        stringBuilder.append(", ");
+      }
+      stringBuilder.append(parameters[i].getSimpleName());
+    }
+
+    return stringBuilder.append(")").toString();
+  }
+
+  /**
+   * Tries making the constructor accessible, returning an exception message
+   * if this fails.
+   *
+   * @param constructor
+   *    Constructor to make accessible
+   * @return
+   *    Exception message; {@code null} if successful, non-{@code null} if
+   *    unsuccessful
+   */
+  public static String tryMakeAccessible(Constructor<?> constructor) {
+    try {
+      constructor.setAccessible(true);
+      return null;
+    } catch (Exception exception) {
+      return "Failed making constructor '" + constructorToString(constructor) + "' accessible; "
+          + "either change its visibility or write a custom InstanceCreator for its declaring type: "
+          // Include the message since it might contain more detailed information
+          + exception.getMessage();
+    }
+  }
+}

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

@@ -181,7 +181,6 @@ public void testNullSerialization() throws Exception {
     testNullSerializationAndDeserialization(Date.class);
     testNullSerializationAndDeserialization(GregorianCalendar.class);
     testNullSerializationAndDeserialization(Calendar.class);
-    testNullSerializationAndDeserialization(Enum.class);
     testNullSerializationAndDeserialization(Class.class);
   }