Small adjustments to the new record code. (#2219)

* Small adjustments to the new record code. * Replace wildcard imports with single imports. * Enable `Java17RecordTest` and fix its many previously-hidden problems. * Use a `Map` to get primitive zero values rather than a potentially-expensive reflective trick. * Apply some automated code fixes. * Address review comments.
google · Oct 12, 2022 · 86c35bb · 86c35bb
1 parent a0dc7bf
commit 86c35bb
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 35 deletions.
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
@@ -43,7 +43,6 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
-import java.lang.reflect.Array;
 import java.util.Arrays;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -93,9 +92,7 @@ private List<String> getFieldNames(Field f) {
 
     List<String> fieldNames = new ArrayList<>(alternates.length + 1);
     fieldNames.add(serializedName);
-    for (String alternate : alternates) {
-      fieldNames.add(alternate);
-    }
+    Collections.addAll(fieldNames, alternates);
     return fieldNames;
   }
 
@@ -394,6 +391,8 @@ T finalize(T accumulator) {
   }
 
   private static final class RecordAdapter<T> extends Adapter<T, Object[]> {
+    static Map<Class<?>, Object> PRIMITIVE_DEFAULTS = primitiveDefaults();
+
     // The actual record constructor.
     private final Constructor<? super T> constructor;
     // Array of arguments to the constructor, initialized with default values for primitives
@@ -417,16 +416,24 @@ private static final class RecordAdapter<T> extends Adapter<T, Object[]> {
       // we create an Object[] where all primitives are initialized to non-null values.
       constructorArgsDefaults = new Object[parameterTypes.length];
       for (int i = 0; i < parameterTypes.length; i++) {
-        if (parameterTypes[i].isPrimitive()) {
-          // Voodoo magic, we create a new instance of this primitive type using reflection via an
-          // array. The array has 1 element, that of course will be initialized to the primitives
-          // default value. We then retrieve this value back from the array to get the properly
-          // initialized default value for the primitve type.
-          constructorArgsDefaults[i] = Array.get(Array.newInstance(parameterTypes[i], 1), 0);
-        }
+        // This will correctly be null for non-primitive types:
+        constructorArgsDefaults[i] = PRIMITIVE_DEFAULTS.get(parameterTypes[i]);
       }
     }
 
+    private static Map<Class<?>, Object> primitiveDefaults() {
+      Map<Class<?>, Object> zeroes = new HashMap<>();
+      zeroes.put(byte.class, (byte) 0);
+      zeroes.put(short.class, (short) 0);
+      zeroes.put(int.class, 0);
+      zeroes.put(long.class, 0L);
+      zeroes.put(float.class, 0F);
+      zeroes.put(double.class, 0D);
+      zeroes.put(char.class, '\0');
+      zeroes.put(boolean.class, false);
+      return zeroes;
+    }
+
     @Override
     Object[] createAccumulator() {
       return constructorArgsDefaults.clone();

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
@@ -2,8 +2,10 @@
 
 import com.google.gson.JsonIOException;
 import com.google.gson.internal.GsonBuildConfig;
-
-import java.lang.reflect.*;
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 
 public class ReflectionHelper {
 
@@ -158,21 +160,19 @@ private static class RecordSupportedHelper extends RecordHelper {
     private final Method getRecordComponents;
     private final Method getName;
     private final Method getType;
-    private final Method getAccessor;
 
     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");
-      getAccessor = recordComponentType.getMethod("getAccessor");
     }
 
     @Override
     boolean isRecord(Class<?> raw) {
       try {
-        return Boolean.class.cast(isRecord.invoke(raw)).booleanValue();
+        return (boolean) isRecord.invoke(raw);
       } catch (ReflectiveOperationException e) {
         throw createExceptionForRecordReflectionException(e);
       }

diff --git a/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java b/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java
@@ -16,55 +16,72 @@
 package com.google.gson.functional;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThrows;
 
 import com.google.gson.Gson;
 import com.google.gson.annotations.SerializedName;
 import java.util.Objects;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
-@Ignore // Disabled until record support is added
 public final class Java17RecordTest {
   private final Gson gson = new Gson();
 
   @Test
   public void testFirstNameIsChosenForSerialization() {
     MyRecord target = new MyRecord("v1", "v2");
     // Ensure name1 occurs exactly once, and name2 and name3 don't appear
-    assertEquals("{\"name\":\"v1\",\"name1\":\"v2\"}", gson.toJson(target));
+    assertEquals("{\"name\":\"modified-v1\",\"name1\":\"v2\"}", gson.toJson(target));
   }
 
   @Test
   public void testMultipleNamesDeserializedCorrectly() {
-    assertEquals("v1", gson.fromJson("{'name':'v1'}", MyRecord.class).a);
+    assertEquals("modified-v1", gson.fromJson("{'name':'v1'}", MyRecord.class).a);
 
     // Both name1 and name2 gets deserialized to b
-    assertEquals("v11", gson.fromJson("{'name1':'v11'}", MyRecord.class).b);
-    assertEquals("v2", gson.fromJson("{'name2':'v2'}", MyRecord.class).b);
-    assertEquals("v3", gson.fromJson("{'name3':'v3'}", MyRecord.class).b);
+    assertEquals("v11", gson.fromJson("{'name': 'v1', 'name1':'v11'}", MyRecord.class).b);
+    assertEquals("v2", gson.fromJson("{'name': 'v1', 'name2':'v2'}", MyRecord.class).b);
+    assertEquals("v3", gson.fromJson("{'name': 'v1', 'name3':'v3'}", MyRecord.class).b);
   }
 
   @Test
   public void testMultipleNamesInTheSameString() {
     // The last value takes precedence
-    assertEquals("v3", gson.fromJson("{'name1':'v1','name2':'v2','name3':'v3'}", MyRecord.class).b);
+    assertEquals("v3",
+        gson.fromJson("{'name': 'foo', 'name1':'v1','name2':'v2','name3':'v3'}", MyRecord.class).b);
   }
 
   @Test
   public void testConstructorRuns() {
-    assertThrows(NullPointerException.class,
-        () -> gson.fromJson("{'name1': null, 'name2': null", MyRecord.class));
+    assertEquals(new MyRecord(null, null),
+        gson.fromJson("{'name1': null, 'name2': null}", MyRecord.class));
   }
 
-  private static record MyRecord(
+  @Test
+  public void testPrimitiveDefaultValues() {
+    RecordWithPrimitives expected = new RecordWithPrimitives("s", (byte) 0, (short) 0, 0, 0, 0, 0, '\0', false);
+    assertEquals(expected, gson.fromJson("{'aString': 's'}", RecordWithPrimitives.class));
+  }
+
+  @Test
+  public void testPrimitiveNullValues() {
+    RecordWithPrimitives expected = new RecordWithPrimitives("s", (byte) 0, (short) 0, 0, 0, 0, 0, '\0', false);
+    // TODO(eamonnmcmanus): consider forbidding null for primitives
+    String s = "{'aString': 's', 'aByte': null, 'aShort': null, 'anInt': null, 'aLong': null, 'aFloat': null, 'aDouble': null, 'aChar': null, 'aBoolean': null}";
+    assertEquals(expected, gson.fromJson(s, RecordWithPrimitives.class));
+  }
+
+  public record MyRecord(
       @SerializedName("name") String a,
       @SerializedName(value = "name1", alternate = {"name2", "name3"}) String b) {
-    MyRecord {
-      Objects.requireNonNull(a);
+    public MyRecord {
+      a = "modified-" + a;
     }
   }
+
+  public record RecordWithPrimitives(
+      String aString, byte aByte, short aShort, int anInt, long aLong, float aFloat, double aDouble, char aChar, boolean aBoolean) {}
 }
diff --git a/gson/src/test/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactoryTest.java b/gson/src/test/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactoryTest.java
@@ -1,6 +1,7 @@
 package com.google.gson.internal.bind;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;

diff --git a/gson/src/test/java/com/google/gson/internal/reflect/ReflectionHelperTest.java b/gson/src/test/java/com/google/gson/internal/reflect/ReflectionHelperTest.java
@@ -1,6 +1,9 @@
 package com.google.gson.internal.reflect;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -76,10 +79,10 @@ public String getName() {
 
     @Override
     public boolean equals(Object o) {
-      if (this == o) return true;
-      if (o == null || getClass() != o.getClass()) return false;
-      PrincipalImpl principal = (PrincipalImpl) o;
-      return Objects.equals(name, principal.name);
+      if (o instanceof PrincipalImpl) {
+        return Objects.equals(name, ((PrincipalImpl) o).name);
+      }
+      return false;
     }
 
     @Override