From 86c35bba3091afe70f38598b04ba89b7e8f539f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89amonn=20McManus?= Date: Wed, 12 Oct 2022 17:24:36 -0400 Subject: [PATCH] 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. --- .../bind/ReflectiveTypeAdapterFactory.java | 29 ++++++++----- .../internal/reflect/ReflectionHelper.java | 10 ++--- .../gson/functional/Java17RecordTest.java | 43 +++++++++++++------ .../ReflectiveTypeAdapterFactoryTest.java | 3 +- .../reflect/ReflectionHelperTest.java | 13 +++--- 5 files changed, 63 insertions(+), 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 index 5caeb107..3da14f23 100644 --- 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.Field; 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 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory { List 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 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory { } private static final class RecordAdapter extends Adapter { + static Map, Object> PRIMITIVE_DEFAULTS = primitiveDefaults(); + // The actual record constructor. private final Constructor constructor; // Array of arguments to the constructor, initialized with default values for primitives @@ -417,16 +416,24 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory { // 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, Object> primitiveDefaults() { + Map, 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 index f55b30f5..80df515a 100644 --- 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 @@ package com.google.gson.internal.reflect; 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,7 +160,6 @@ public class ReflectionHelper { 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"); @@ -166,13 +167,12 @@ public class ReflectionHelper { 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 index 95166d82..023bec30 100644 --- a/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java +++ b/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java @@ -16,18 +16,17 @@ 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(); @@ -35,36 +34,54 @@ public final class Java17RecordTest { 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 index 08c92f80..8ee15aa0 100644 --- 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 index 7d0c9833..f5c827e8 100644 --- 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 class ReflectionHelperTest { @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