Adjust Record adapter and extend test coverage

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

@@ -19,6 +19,7 @@
 import com.google.gson.FieldNamingStrategy;
 import com.google.gson.Gson;
 import com.google.gson.JsonIOException;
+import com.google.gson.JsonParseException;
 import com.google.gson.JsonSyntaxException;
 import com.google.gson.ReflectionAccessFilter;
 import com.google.gson.ReflectionAccessFilter.FilterResult;
@@ -38,16 +39,18 @@
 import com.google.gson.stream.JsonToken;
 import com.google.gson.stream.JsonWriter;
 import java.io.IOException;
+import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
-import java.util.Arrays;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
-import java.util.LinkedHashMap;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -108,28 +111,30 @@ public <T> TypeAdapter<T> create(Gson gson, final TypeToken<T> type) {
         ReflectionAccessFilterHelper.getFilterResult(reflectionFilters, raw);
     if (filterResult == FilterResult.BLOCK_ALL) {
       throw new JsonIOException(
-          "ReflectionAccessFilter does not permit using reflection for "
-              + raw
+          "ReflectionAccessFilter does not permit using reflection for " + raw
               + ". Register a TypeAdapter for this type or adjust the access filter.");
     }
     boolean blockInaccessible = filterResult == FilterResult.BLOCK_INACCESSIBLE;
 
     // If the type is actually a Java Record, we need to use the RecordAdapter instead. This will always be false
     // on JVMs that do not support records.
     if (ReflectionHelper.isRecord(raw)) {
-      return new RecordAdapter<>(raw, getBoundFields(gson, type, raw, true, true));
+      @SuppressWarnings("unchecked")
+      TypeAdapter<T> adapter = (TypeAdapter<T>) new RecordAdapter<>(raw,
+          getBoundFields(gson, type, raw, blockInaccessible, true), blockInaccessible);
+      return adapter;
     }
 
     ObjectConstructor<T> constructor = constructorConstructor.get(type);
     return new FieldReflectionAdapter<>(constructor, getBoundFields(gson, type, raw, blockInaccessible, false));
   }
 
-  private static void checkAccessible(Object object, Field field) {
-    if (!ReflectionAccessFilterHelper.canAccess(field, Modifier.isStatic(field.getModifiers()) ? null : object)) {
-      throw new JsonIOException("Field '" + field.getDeclaringClass().getName() + "#"
-          + field.getName() + "' is not accessible and ReflectionAccessFilter does not "
-          + "permit making it accessible. Register a TypeAdapter for the declaring type "
-          + "or adjust the access filter.");
+  private static <M extends AccessibleObject & Member> void checkAccessible(Object object, M member) {
+    if (!ReflectionAccessFilterHelper.canAccess(member, Modifier.isStatic(member.getModifiers()) ? null : object)) {
+      String memberDescription = ReflectionHelper.getAccessibleObjectDescription(member, true);
+      throw new JsonIOException(memberDescription + " is not accessible and ReflectionAccessFilter does not"
+          + " permit making it accessible. Register a TypeAdapter for the declaring type, adjust the"
+          + " access filter or increase the visibility of the element and its declaring type.");
     }
   }
 
@@ -154,8 +159,14 @@ private ReflectiveTypeAdapterFactory.BoundField createBoundField(
       @Override void write(JsonWriter writer, Object source)
           throws IOException, ReflectiveOperationException {
         if (!serialized) return;
-        if (blockInaccessible && accessor == null) {
-          checkAccessible(source, field);
+        if (blockInaccessible) {
+          if (accessor == null) {
+            checkAccessible(source, field);
+          } else {
+            // Note: This check might actually be redundant because access check for canonical
+            // constructor should have failed already
+            checkAccessible(source, accessor);
+          }
         }
 
         Object fieldValue = (accessor != null)
@@ -172,11 +183,13 @@ private ReflectiveTypeAdapterFactory.BoundField createBoundField(
       }
 
       @Override
-      void readIntoArray(JsonReader reader, int index, Object[] target) throws IOException {
+      void readIntoArray(JsonReader reader, int index, Object[] target) throws IOException, JsonParseException {
         Object fieldValue = typeAdapter.read(reader);
-        if (fieldValue != null || !isPrimitive) {
-          target[index] = fieldValue;
+        if (fieldValue == null && isPrimitive) {
+          throw new JsonParseException("null is not allowed as value for record component '" + fieldName + "'"
+              + " of primitive type; at path " + reader.getPath());
         }
+        target[index] = fieldValue;
       }
 
       @Override
@@ -209,9 +222,9 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
       if (raw != originalRaw && fields.length > 0) {
         FilterResult filterResult = ReflectionAccessFilterHelper.getFilterResult(reflectionFilters, raw);
         if (filterResult == FilterResult.BLOCK_ALL) {
-          throw new JsonIOException("ReflectionAccessFilter does not permit using reflection for "
-              + raw + " (supertype of " + originalRaw + "). Register a TypeAdapter for this type "
-              + "or adjust the access filter.");
+          throw new JsonIOException("ReflectionAccessFilter does not permit using reflection for " + raw
+              + " (supertype of " + originalRaw + "). Register a TypeAdapter for this type"
+              + " or adjust the access filter.");
         }
         blockInaccessible = filterResult == FilterResult.BLOCK_INACCESSIBLE;
       }
@@ -224,18 +237,34 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
         }
         // The accessor method is only used for records. If the type is a record, we will read out values
         // via its accessor method instead of via reflection. This way we will bypass the accessible restrictions
-        // If there is a static field on a record, there will not be an accessor. Instead we will use the default
-        // field logic for dealing with statics.
         Method accessor = null;
-        if (isRecord && !Modifier.isStatic(field.getModifiers())) {
-          accessor = ReflectionHelper.getAccessor(raw, field);
+        if (isRecord) {
+          // If there is a static field on a record, there will not be an accessor. Instead we will use the default
+          // field serialization logic, but for deserialization the field is excluded for simplicity. Note that Gson
+          // ignores static fields by default, but GsonBuilder.excludeFieldsWithModifiers can overwrite this.
+          if (Modifier.isStatic(field.getModifiers())) {
+            deserialize = false;
+          } else {
+            accessor = ReflectionHelper.getAccessor(raw, field);
+            // If blockInaccessible, skip and perform access check later
+            if (!blockInaccessible) {
+              ReflectionHelper.makeAccessible(accessor);
+            }
+
+            // @SerializedName can be placed on accessor method, but it is not supported there
+            // If field and method have annotation it is not easily possible to determine if accessor method
+            // is implicit and has inherited annotation, or if it is explicitly declared with custom annotation
+            if (accessor.getAnnotation(SerializedName.class) != null
+                && field.getAnnotation(SerializedName.class) == null) {
+              String methodDescription = ReflectionHelper.getAccessibleObjectDescription(accessor, false);
+              throw new JsonIOException("@SerializedName on " + methodDescription + " is not supported");
+            }
+          }
         }
 
-        // If blockInaccessible, skip and perform access check later. When constructing a BoundedField for a Record
-        // field, blockInaccessible is always true, thus makeAccessible will never get called. This is not an issue
-        // though, as we will use the accessor method instead for reading record fields, and the constructor for
-        // writing fields.
-        if (!blockInaccessible) {
+        // If blockInaccessible, skip and perform access check later
+        // For Records if the accessor method is used the field does not have to be made accessible
+        if (!blockInaccessible && accessor == null) {
           ReflectionHelper.makeAccessible(field);
         }
         Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());
@@ -278,7 +307,7 @@ protected BoundField(String name, String fieldName, boolean serialized, boolean
     abstract void write(JsonWriter writer, Object source) throws IOException, ReflectiveOperationException;
 
     /** Read the value into the target array, used to provide constructor arguments for records */
-    abstract void readIntoArray(JsonReader reader, int index, Object[] target) throws IOException;
+    abstract void readIntoArray(JsonReader reader, int index, Object[] target) throws IOException, JsonParseException;
 
     /** Read the value from the reader, and set it on the corresponding field on target via reflection */
     abstract void readIntoField(JsonReader reader, Object target) throws IOException, IllegalAccessException;
@@ -297,10 +326,11 @@ protected BoundField(String name, String fieldName, boolean serialized, boolean
    * @param <T> type of objects that this Adapter creates.
    * @param <A> type of accumulator used to build the deserialization result.
    */
+  // This class is public because external projects check for this class with `instanceof` (even though it is internal)
   public static abstract class Adapter<T, A> extends TypeAdapter<T> {
-    protected final Map<String, BoundField> boundFields;
+    final Map<String, BoundField> boundFields;
 
-    protected Adapter(Map<String, BoundField> boundFields) {
+    Adapter(Map<String, BoundField> boundFields) {
       this.boundFields = boundFields;
     }
 
@@ -356,7 +386,7 @@ public T read(JsonReader in) throws IOException {
     /** Create the Object that will be used to collect each field value */
     abstract A createAccumulator();
     /**
-     * Read a single BoundedField into the accumulator. The JsonReader will be pointed at the
+     * Read a single BoundField into the accumulator. The JsonReader will be pointed at the
      * start of the value for the BoundField to read from.
      */
     abstract void readField(A accumulator, JsonReader in, BoundField field)
@@ -391,20 +421,25 @@ T finalize(T accumulator) {
   }
 
   private static final class RecordAdapter<T> extends Adapter<T, Object[]> {
-    static Map<Class<?>, Object> PRIMITIVE_DEFAULTS = primitiveDefaults();
+    static final Map<Class<?>, Object> PRIMITIVE_DEFAULTS = primitiveDefaults();
 
-    // The actual record constructor.
-    private final Constructor<? super T> constructor;
+    // The canonical constructor of the record
+    private final Constructor<T> constructor;
     // Array of arguments to the constructor, initialized with default values for primitives
     private final Object[] constructorArgsDefaults;
     // Map from component names to index into the constructors arguments.
     private final Map<String, Integer> componentIndices = new HashMap<>();
 
-    RecordAdapter(Class<? super T> raw, Map<String, BoundField> boundFields) {
+    RecordAdapter(Class<T> raw, Map<String, BoundField> boundFields, boolean blockInaccessible) {
       super(boundFields);
-      this.constructor = ReflectionHelper.getCanonicalRecordConstructor(raw);
-      // Ensure the constructor is accessible
-      ReflectionHelper.makeAccessible(this.constructor);
+      constructor = ReflectionHelper.getCanonicalRecordConstructor(raw);
+
+      if (blockInaccessible) {
+        checkAccessible(null, constructor);
+      } else {
+        // Ensure the constructor is accessible
+        ReflectionHelper.makeAccessible(constructor);
+      }
 
       String[] componentNames = ReflectionHelper.getRecordComponentNames(raw);
       for (int i = 0; i < componentNames.length; i++) {
@@ -441,29 +476,28 @@ Object[] createAccumulator() {
 
     @Override
     void readField(Object[] accumulator, JsonReader in, BoundField field) throws IOException {
-      Integer fieldIndex = componentIndices.get(field.fieldName);
-      if (fieldIndex == null) {
+      // Obtain the component index from the name of the field backing it
+      Integer componentIndex = componentIndices.get(field.fieldName);
+      if (componentIndex == null) {
         throw new IllegalStateException(
-            "Could not find the index in the constructor "
-                + constructor
-                + " for field with name "
-                + field.name
-                + ", unable to determine which argument in the constructor the field corresponds"
-                + " to. This is unexpected behaviour, as we expect the RecordComponents to have the"
+            "Could not find the index in the constructor " + ReflectionHelper.constructorToString(constructor)
+                + " for field with name " + field.fieldName + ","
+                + " unable to determine which argument in the constructor the field corresponds"
+                + " to. This is unexpected behavior, as we expect the RecordComponents to have the"
                 + " same names as the fields in the Java class, and that the order of the"
-                + " RecordComponents is the same as the order of the canonical arguments.");
+                + " RecordComponents is the same as the order of the canonical constructor parameters.");
       }
-      field.readIntoArray(in, fieldIndex, accumulator);
+      field.readIntoArray(in, componentIndex, accumulator);
     }
 
     @Override
-    @SuppressWarnings("unchecked")
     T finalize(Object[] accumulator) {
       try {
-        return (T) constructor.newInstance(accumulator);
+        return constructor.newInstance(accumulator);
       } catch (ReflectiveOperationException e) {
         throw new RuntimeException(
-            "Failed to invoke " + constructor + " with args " + Arrays.toString(accumulator), e);
+            "Failed to invoke constructor " + ReflectionHelper.constructorToString(constructor)
+            + " with args " + Arrays.toString(accumulator), e);
       }
     }
   }