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;
 
@@ -117,19 +120,22 @@ public <T> TypeAdapter<T> create(Gson gson, final TypeToken<T> type) {
     // 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 +160,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 +184,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
@@ -224,18 +238,33 @@ 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 logic for dealing with statics. For deserialization the field is ignored.
+          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 (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,26 +476,26 @@ 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
+                + field.fieldName
                 + ", unable to determine which argument in the constructor the field corresponds"
-                + " to. This is unexpected behaviour, as we expect the RecordComponents to have the"
+                + " 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);

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

@@ -24,65 +24,81 @@ 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 {
-    makeAccessible("field '" + field.getDeclaringClass().getName() + "#" + field.getName() + "'", field);
-  }
-
-  /**
-   * Tries making the constructor accessible, wrapping any thrown exception in a {@link JsonIOException}
-   * with descriptive message.
-   *
-   * @param constructor constructor to make accessible
-   * @throws JsonIOException if making the constructor accessible fails
-   */
-  public static void makeAccessible(Constructor<?> constructor) throws JsonIOException {
-    makeAccessible(
-            "constructor " + constructor + " in " + constructor.getDeclaringClass().getName(),
-            constructor
-    );
-  }
-
   /**
    * Internal implementation of making an {@link AccessibleObject} accessible.
    *
-   * @param description describe what we are attempting to make accessible
    * @param object the object that {@link AccessibleObject#setAccessible(boolean)} should be called on.
    * @throws JsonIOException if making the object accessible fails
    */
-  private static void makeAccessible(String description, AccessibleObject object) throws JsonIOException {
+  public static void makeAccessible(AccessibleObject object) throws JsonIOException {
     try {
       object.setAccessible(true);
     } catch (Exception exception) {
-      throw new JsonIOException("Failed making " + description + "' accessible; either change its visibility "
-              + "or write a custom TypeAdapter for its declaring type", exception);
+      String description = getAccessibleObjectDescription(object, false);
+      throw new JsonIOException("Failed making " + description + " accessible; either increase its visibility "
+              + "or write a custom TypeAdapter for its declaring type.", exception);
     }
   }
 
+  /**
+   * Returns a short string describing the {@link AccessibleObject} in a human-readable way.
+   *
+   * @param object object to describe
+   * @param uppercaseFirstLetter whether the first letter of the description should be uppercased
+   */
+  public static String getAccessibleObjectDescription(AccessibleObject object, boolean uppercaseFirstLetter) {
+    String description;
+
+    if (object instanceof Field) {
+      Field field = (Field) object;
+      description = "field '" + field.getDeclaringClass().getName() + "#" + field.getName() + "'";
+    } else if (object instanceof Method) {
+      Method method = (Method) object;
+
+      StringBuilder methodSignatureBuilder = new StringBuilder(method.getName());
+      appendExecutableParameters(method, methodSignatureBuilder);
+      String methodSignature = methodSignatureBuilder.toString();
+
+      description = "method '" + method.getDeclaringClass().getName() + "#" + methodSignature + "'";
+    } else if (object instanceof Constructor) {
+      description = "constructor '" + constructorToString((Constructor<?>) object) + "'";
+    } else {
+      description = "<unknown AccessibleObject> " + object.toString();
+    }
+
+    if (uppercaseFirstLetter && Character.isLowerCase(description.charAt(0))) {
+      description = Character.toUpperCase(description.charAt(0)) + description.substring(1);
+    }
+    return description;
+  }
+
   /**
    * 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();
+      .append(constructor.getDeclaringClass().getSimpleName());
+    appendExecutableParameters(constructor, stringBuilder);
+
+    return stringBuilder.toString();
+  }
+
+  // Note: Ideally parameter type would be java.lang.reflect.Executable, but that was added in Java 8
+  private static void appendExecutableParameters(AccessibleObject executable, StringBuilder stringBuilder) {
+    stringBuilder.append('(');
+
+    Class<?>[] parameters = (executable instanceof Method) ? ((Method) executable).getParameterTypes()
+        : ((Constructor<?>) executable).getParameterTypes();
     for (int i = 0; i < parameters.length; i++) {
       if (i > 0) {
         stringBuilder.append(", ");
       }
       stringBuilder.append(parameters[i].getSimpleName());
     }
 
-    return stringBuilder.append(')').toString();
+    stringBuilder.append(')');
   }
 
   /**
@@ -99,7 +115,7 @@ public static String tryMakeAccessible(Constructor<?> constructor) {
       return null;
     } catch (Exception exception) {
       return "Failed making constructor '" + constructorToString(constructor) + "' accessible; "
-          + "either change its visibility or write a custom InstanceCreator or TypeAdapter for its declaring type: "
+          + "either increase its visibility or write a custom InstanceCreator or TypeAdapter for its declaring type: "
           // Include the message since it might contain more detailed information
           + exception.getMessage();
     }
@@ -138,7 +154,7 @@ public static RuntimeException createExceptionForRecordReflectionException(
             + "(Gson " + GsonBuildConfig.VERSION + "). "
             + "To support Java records, reflection is utilized to read out information "
             + "about records. All these invocations happens after it is established "
-            + "that records exists in the JVM. This exception is unexpected behaviour.",
+            + "that records exist in the JVM. This exception is unexpected behavior.",
             exception);
   }
 
@@ -164,9 +180,10 @@ private static class RecordSupportedHelper extends RecordHelper {
     private RecordSupportedHelper() throws NoSuchMethodException {
       isRecord = Class.class.getMethod("isRecord");
       getRecordComponents = Class.class.getMethod("getRecordComponents");
-      Class<?> recordComponentType = getRecordComponents.getReturnType().getComponentType();
-      getName = recordComponentType.getMethod("getName");
-      getType = recordComponentType.getMethod("getType");
+      // Class java.lang.reflect.RecordComponent
+      Class<?> classRecordComponent = getRecordComponents.getReturnType().getComponentType();
+      getName = classRecordComponent.getMethod("getName");
+      getType = classRecordComponent.getMethod("getType");
     }
 
     @Override