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.
This commit is contained in:
Éamonn McManus 2022-10-12 17:24:36 -04:00 committed by GitHub
parent a0dc7bfddd
commit 86c35bba30
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 63 additions and 35 deletions

View File

@ -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<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 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
}
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 @@ 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<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();

View File

@ -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);
}

View File

@ -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) {}
}

View File

@ -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;

View File

@ -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