Support Java Records when present in JVM. #2197

staale · 2022-09-22T10:59:22Z

Added a TypeAdapterFactory that deals specifcally with Java 17 Records. It uses reflection to detect if the JVM supports records, and from there accesses the RecordComponent array to serialize and deserialize objects.

This new TypeAdapterFactory will only be added when Records are actually supported on the JVM.

Fixes #1794 Added a TypeAdapterFactory that deals specifcally with Java 17 Records. It uses reflection to detect if the JVM supports records, and from there accesses the RecordComponent array to serialize and deserialize objects. This new TypeAdapterFactory will only be added when Records are actually supported on the JVM.

eamonnmcmanus

Thanks for doing this! It's great to have it.

Ideally we would use a multi-release jar file and some build magic, so you wouldn't have to do all that reflection. But that supposes quite a lot of mechanism that we don't have. Meanwhile the reflective approach is way better than what we have, which is nothing.

I have a lot of detailed comments, mostly minor. Some high-level comments:

Could you run google-java-format over RecordTypeAdapterFactory? That's much easier than me quibbling over the various divergences from Google Java Style. You might want to make the change I requested to use <ul> in a javadoc comment first, because otherwise g-j-f will make a mess of that comment.
Ideally we'd have some sort of test. Perhaps it could do assumeTrue(RecordTypeAdapterFactory.SUPPORTS_RECORD_TYPES) and then use reflection to make an instance of UnixDomainPrincipal, which seems to be the only record type in the Java 17 API. If we can serialize and deserialize that, that's at least basic test coverage. Later we can think about ways to test more things, like @SerializedName, but let's at least do the UnixDomainPrincipal thing here.

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

Marcono1234

Thanks also from my side!

My initial approach would have been to 'hack' this into ReflectiveTypeAdapterFactory to avoid duplicating much of its logic, but on the other hand that might have been very messy. However, I had not actually tried that yet and I think your approach is fine as well. We just need to pay attention in the future that ReflectiveTypeAdapterFactory and this newly added RecordTypeAdapterFactory don't diverge.

I am not a direct member of this project so feel free to consider my comments mostly suggestions (though I hope @eamonnmcmanus agrees with most of them).

Feel free to work on them one by one; you don't have to resolve all at once.

Marcono1234 · 2022-09-22T19:49:44Z

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

+
+    /**
+     * Internal helper class to manage access to Class.isRecord, and RecordComponent instances. Since this compiles
+     * on Java 8, we need to use reflection to handle RecordComponent instances.


Gson currently compiles against Java 7. However, maybe it would be best to reword it to prevent it from becoming outdated again. Maybe something like this?

Suggested change

* on Java 8, we need to use reflection to handle RecordComponent instances.

* on Java versions which don't have records, we need to use reflection to handle RecordComponent instances.

Marcono1234 · 2022-09-22T19:50:55Z

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

+        private final Method getAnnotation;
+
+        /**
+         * Create a new RecordHelper to handle reflection for RecordComponent on Java 17.


Suggested change

* Create a new RecordHelper to handle reflection for RecordComponent on Java 17.

* Create a new RecordHelper to handle reflection for RecordComponent.

Marcono1234 · 2022-09-22T19:54:04Z

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

+import com.google.gson.stream.JsonWriter;
+
+/**
+ * {@link RecordFieldFactory} to create adapters for Java 17 records.


Suggested change

* {@link RecordFieldFactory} to create adapters for Java 17 records.

* {@link RecordFieldFactory} to create adapters for Java 16 records.

(see https://www.oracle.com/java/technologies/javase/16-relnote-issues.html#JDK-8246771)

Records were already available as preview feature in Java 15; not sure if that requires any special handling here, or if we just assume that either:

no one is using Java 15 anymore since it is not LTS

users who enable preview features are aware that they might not work properly with libraries (such as Gson)

If the code here doesn't work with Java 15 and --enable-preview, I think that is absolutely fine. People shouldn't be using that configuration in production.

Marcono1234 · 2022-09-22T19:56:13Z

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

+
+        boolean isRecord(TypeToken<?> type) {
+            try {
+                return isRecord != null && Boolean.TRUE.equals(isRecord.invoke(type));


The null check seems redundant; the constructor already made sure it is not null

Suggested change

return isRecord != null && Boolean.TRUE.equals(isRecord.invoke(type));

return Boolean.TRUE.equals(isRecord.invoke(type));

Marcono1234 · 2022-09-22T19:59:08Z

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

+            }
+        }
+
+        boolean isRecord(TypeToken<?> type) {


Shouldn't the parameter type be Class? Otherwise the invoke call below will fail.

Marcono1234 · 2022-09-22T20:47:59Z

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

+        }
+    }
+
+    public static class DeSerializeAccessException extends RuntimeException {


Not sure if it is a good idea to introduce these exception types here. Arguably Gson has a bad exception hierarchy, but creating separate specific exceptions here might not make it better. TypeAdapterReflectionException is fine though in my opinion.

I think for the case where this exception is currently thrown (constructor.newInstance), it would also be acceptable to throw a TypeAdapterReflectionException instead.

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

Marcono1234 · 2022-09-22T20:56:32Z

gson/src/main/java/com/google/gson/Gson.java

@@ -332,9 +333,17 @@ public Gson() {
    this.jsonAdapterFactory = new JsonAdapterAnnotationTypeAdapterFactory(constructorConstructor);
    factories.add(jsonAdapterFactory);
    factories.add(TypeAdapters.ENUM_FACTORY);
+
+    // If we are on Java 17, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,


Suggested change

// If we are on Java 17, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,

// If we are on Java 16 or newer, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,

Marcono1234 · 2022-09-22T21:00:13Z

gson/src/main/java/com/google/gson/Gson.java

+    // If we are on Java 17, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,
+    // so that we intercept all Record types first.
+    if (RecordTypeAdapterFactory.SUPPORTS_RECORD_TYPES) {
+      factories.add(new RecordTypeAdapterFactory(this.excluder, constructorConstructor, jsonAdapterFactory));


For consistency:

Suggested change

factories.add(new RecordTypeAdapterFactory(this.excluder, constructorConstructor, jsonAdapterFactory));

factories.add(new RecordTypeAdapterFactory(excluder, constructorConstructor, jsonAdapterFactory));

Marcono1234 · 2022-09-22T21:06:52Z

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

+            JsonAdapter jsonAdapter = RECORD_HELPER.getAnnotation(recordComponent, JsonAdapter.class);
+            TypeAdapter<?> typeAdapter;
+            if (jsonAdapter == null) {
+                typeAdapter = gson.getAdapter(fieldType);


Similar to ReflectiveTypeAdapterFactory this should probably wrap the adapter in a TypeAdapterRuntimeTypeWrapper.

Marcono1234 · 2022-09-23T13:54:57Z

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

+        }
+
+        private RecordField createRecordField(Field field, Object recordComponent) {
+            TypeToken<?> fieldType = TypeToken.get(RECORD_HELPER.getGenericType(recordComponent));


Probably has to resolve the field type against the parameterized record type (in case the record type is generic), see ReflectiveTypeAdapterFactory code:

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

Line 205 in 2154e46

Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());

Marcono1234 · 2022-09-23T14:05:16Z

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

+            // present, we need to respect it. This might also mean that multiple keys acts as aliases
+            // when reading Json.
+            SerializedName serializedNameAnnotation =
+                    RECORD_HELPER.getAnnotation(recordComponent, SerializedName.class);


Not sure if that actually works. SerializedName does not have RECORD_COMPONENT as target (and cannot have it as target if we want to still compile against Java 7).

However, you can obtain the annotation from the private field because it is propagated, see Java Language Specification:

A component field is annotated with the annotations, if any, that appear on the corresponding record component and whose annotation interfaces are applicable in the field declaration context

Oh, that's unfortunate. It's still worth adding support for records, but I suppose we won't be able to serialize their members using a name other than their own. One possible workaround would be to look for an annotation on the property method:

record Foo(int bar) { @SerializedName("baz") int bar() { return bar; } }

Another possibility would be to remove the @Target restriction on @SerializedName, but that would be a bit sad.

I'm not sure I understand the comment about obtaining the annotation from the private field. If we could get @SerializedName onto the record component then it would indeed be copied onto the field, but how would we do that?

Anyway, I'm fine with this initial version not supporting names other than the Java name of the record component.

This is all making me think that we should have tests that run on Java ≥16, presumably using Maven profiles. Again that doesn't have to be in this PR, though perhaps you could add a test and then just exclude it for now. The exclusion could look like this and go here.

If we could get @SerializedName onto the record component then it would indeed be copied onto the field, but how would we do that?

Java allows to place @SerializedName on record components because it has FIELD as target. It won't be available on the component using reflection then, but on the backing field.
Just try it out (e.g. with jshell):

import java.lang.annotation.*; @Target(ElementType.FIELD) @Retention(RetentionPolicy.RUNTIME) @interface MyAnnotation {} record R(@MyAnnotation int i) {} R.class.getRecordComponents()[0].getAnnotations() // empty array R.class.getDeclaredFields()[0].getAnnotations() // Annotation[1] { @MyAnnotation() }

The annotation is also propagated to the accessor method if it has METHOD as target, however only if the accessor is implicit. If the user explicitly defines the accessor method the annotation won't be propagated. So the more reliable approach would be to obtain the annotation from the field.

Oh, well that's OK. We should do that.

I've now configured the GitHub actions so that builds are done on both JDK 11 and JDK 17. So if we add a reflection-based test it should actually run on JDK 17. Ideally we would instead add a test that has actual records in it, and arrange for that to be excluded on Java 11. That would allow us to test the @SerializedName logic.

Later if desired some of the tests from my gson-record-type-adapter-factory can be used. Though not all test cases are relevant for this implementation here.

Marcono1234 · 2022-09-23T16:39:39Z

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

+            String serializedName;
+            String[] deserializedNames;
+            if (serializedNameAnnotation == null) {
+                serializedName = RECORD_HELPER.getName(recordComponent);


Maybe this should pass the private field to the FieldNamingStrategy to behave similarly to ReflectiveTypeAdapterFactory.

Marcono1234 · 2022-09-23T16:42:11Z

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

+            // Yes -> Not supported, we return null as per the TypeAdapterFactory contract
+            return null;
+        }
+


Probably this should also check the ReflectionAccessFilter:

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

Lines 104 to 108 in 2154e46

FilterResult filterResult = ReflectionAccessFilterHelper.getFilterResult(reflectionFilters, raw);

if (filterResult == FilterResult.BLOCK_ALL) {

throw new JsonIOException("ReflectionAccessFilter does not permit using reflection for "

+ raw + ". Register a TypeAdapter for this type or adjust the access filter.");

}

For example a user might only permit reflection based serialization of classes in a model package but disallow (accidental) serialization of classes in an implementation package.

Marcono1234 · 2022-09-23T16:47:12Z

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

+            // There is no method in the Java API to do the equivialent, instead we rely on the constructor and
+            // recordComponent order to be the same. This construct matches the StackOverflow answer here:
+            // https://stackoverflow.com/a/67127067
+            Constructor<T> recordConstructor = (Constructor<T>) type.getRawType().getConstructor(componentTypes);


The implicit canonical constructor has the same visibility as the record class, see https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.10.4.
So maybe it would make sense to try make the constructor accessible to support non-public records as well without requiring the user to specify an explicit public constructor. Possibly in a similar way to how ConstructorConstructor.newDefaultConstructor does it (with the ReflectionAccessFilter checks).

Marcono1234 · 2022-09-23T16:56:27Z

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

+
+        private RecordField createRecordField(Field field, Object recordComponent) {
+            TypeToken<?> fieldType = TypeToken.get(RECORD_HELPER.getGenericType(recordComponent));
+            JsonAdapter jsonAdapter = RECORD_HELPER.getAnnotation(recordComponent, JsonAdapter.class);


Similar to https://github.com/google/gson/pull/2197/files#r978712348, I think the annotation has to be obtained from the private field instead of the component.

Ran Google code format on RecordTypeAdapterFactory so that it now conforms to Google style. Added primitive default values for primitive fields, to avoid NPE in the constructor.

staale · 2022-09-25T09:34:22Z

Ideally we would use a multi-release jar file and some build magic, so you wouldn't have to do all that reflection. But that supposes quite a lot of mechanism that we don't have. Meanwhile the reflective approach is way better than what we have, which is nothing.

I think this class is small enough to not impose any costs. Multi release-jars are a bit of a pain IMHO., harder to onboard new developers with multiple source trees, harder to reason about when files are spread out, and one more consideration for anyone consuming the library, including if they update their own build from for example Java 11 to 17.

I have a lot of detailed comments, mostly minor. Some high-level comments:

* Could you run [google-java-format](https://github.com/google/google-java-format) over `RecordTypeAdapterFactory`? That's much easier than me quibbling over the various divergences from Google Java Style. You might want to make the change I requested to use `<ul>` in a javadoc comment first, because otherwise g-j-f will make a mess of that comment.

I have fixed this and your other comments, as well as run Google code format.

* Ideally we'd have some sort of test. Perhaps it could do `assumeTrue(RecordTypeAdapterFactory.SUPPORTS_RECORD_TYPES)` and then use reflection to make an instance of [`UnixDomainPrincipal`](https://docs.oracle.com/en/java/javase/17/docs/api/jdk.net/jdk/net/UnixDomainPrincipal.html), which seems to be the only record type in the Java 17 API. If we can serialize and deserialize that, that's at least basic test coverage. Later we can think about ways to test more things, like `@SerializedName`, but let's at least do the `UnixDomainPrincipal` thing here.

I will look into this. But this would require to run the build on Java 17 unless I am mistaken. There is something called toolchains for Maven that supports different compilers, and could be set up to use Java 17 on tests. However, the plugin does not support download, and this would require additional configuration on each user by adding a toolchain.xml. to their .m2 folder. This is actually a bit easier on Gradle (where I have done something similar), as it can actually download the required jdks from public sources.

staale · 2022-09-25T20:41:47Z

Thanks also from my side!

My initial approach would have been to 'hack' this into ReflectiveTypeAdapterFactory to avoid duplicating much of its logic, but on the other hand that might have been very messy. However, I had not actually tried that yet and I think your approach is fine as well. We just need to pay attention in the future that ReflectiveTypeAdapterFactory and this newly added RecordTypeAdapterFactory don't diverge.

I have created a new pull request with that solution instead: #2201
That approach is different enough that it I feel a new pull request is more appropriate, and the better solution can be made the final solution. I did start that one from scratch, but it is actually less code. This solution was developed outside the Gson codebase, so I created it without reusing the internals in Gson.

I have not looked at your code comments yet, but will get to those later in this pull request.

Marcono1234 · 2022-09-26T09:47:54Z

I have created a new pull request with that solution instead: #2201 That approach is different enough that it I feel a new pull request is more appropriate, and the better solution can be made the final solution. I did start that one from scratch, but it is actually less code. This solution was developed outside the Gson codebase, so I created it without reusing the internals in Gson.

I have not looked at your code comments yet, but will get to those later in this pull request.

Thanks! That pull request looks a bit more promising in my opinion because it avoids duplicating ReflectiveTypeAdapterFactory's logic. Do you want to continue working on both nonetheless (in case you have the time), or primarily on the new pull request now?

If you only want to continue working on the new pull request, it might be good at the end to have a look at the review comments here again nonetheless; maybe some of them also apply to the new pull request.

gbjordal · 2022-09-26T14:11:45Z

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

+        // might differ from the source.
+        Integer fieldIndex = recordFieldIndex.get(fieldName);
+        if (fieldIndex != null) {
+          values[fieldIndex] = recordFields[fieldIndex].readValueForKey(in);


Hi, I was also working on a record type adapter when I came across this PR.
After playing around a bit with it, I think there is a missing null check here:

Suggested change

values[fieldIndex] = recordFields[fieldIndex].readValueForKey(in);

Object value = recordFields[fieldIndex].readValueForKey(in);

if(value != null){

values[fieldIndex] = value

}

Without this, the default values for primitives is overwritten with null.

Possibly the null check should only be applied if the component type is a primitive to allow duplicate non-primitive members to overwrite each others values, to be consistent with ReflectiveTypeAdapterFactory's behavior.

If you only want to continue working on the new pull request, it might be good at the end to have a look at the review comments here again nonetheless; maybe some of them also apply to the new pull request.

I think the new pull request is the way to go, as it is a bit more integrated with the normal reflection flow. I will still check these reviews to see if any apply. Thanks for the input.

staale · 2022-09-26T20:35:57Z

Closing this in favor of #2201 - which is a bit more integrated with the rest of the Gson codebase.

eamonnmcmanus requested changes Sep 22, 2022

View reviewed changes

eamonnmcmanus requested a review from Marcono1234 September 22, 2022 18:34

github-advanced-security bot found potential problems Sep 22, 2022

View reviewed changes

gson/src/main/java/com/google/gson/internal/bind/RecordTypeAdapterFactory.java Outdated Show resolved Hide resolved

Marcono1234 reviewed Sep 22, 2022

View reviewed changes

Marcono1234 reviewed Sep 23, 2022

View reviewed changes

Fixed review comments by @eamonnmcmanus

080fb64

Ran Google code format on RecordTypeAdapterFactory so that it now conforms to Google style. Added primitive default values for primitive fields, to avoid NPE in the constructor.

Marcono1234 mentioned this pull request Sep 26, 2022

Support Java Records when present in JVM. #2201

Merged

gbjordal reviewed Sep 26, 2022

View reviewed changes

staale closed this Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Java Records when present in JVM. #2197

Support Java Records when present in JVM. #2197

staale commented Sep 22, 2022

eamonnmcmanus left a comment

Marcono1234 left a comment

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

eamonnmcmanus Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022 •

edited

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 23, 2022

Marcono1234 Sep 23, 2022 •

edited

eamonnmcmanus Sep 23, 2022

Marcono1234 Sep 23, 2022 •

edited

eamonnmcmanus Sep 23, 2022

Marcono1234 Sep 24, 2022

Marcono1234 Sep 23, 2022

Marcono1234 Sep 23, 2022

Marcono1234 Sep 23, 2022

Marcono1234 Sep 23, 2022 •

edited

staale commented Sep 25, 2022

staale commented Sep 25, 2022

Marcono1234 commented Sep 26, 2022

gbjordal Sep 26, 2022 •

edited

Marcono1234 Sep 26, 2022

staale Sep 26, 2022

staale commented Sep 26, 2022

	* on Java 8, we need to use reflection to handle RecordComponent instances.
	* on Java versions which don't have records, we need to use reflection to handle RecordComponent instances.

	* Create a new RecordHelper to handle reflection for RecordComponent on Java 17.
	* Create a new RecordHelper to handle reflection for RecordComponent.

	* {@link RecordFieldFactory} to create adapters for Java 17 records.
	* {@link RecordFieldFactory} to create adapters for Java 16 records.

	return isRecord != null && Boolean.TRUE.equals(isRecord.invoke(type));
	return Boolean.TRUE.equals(isRecord.invoke(type));

	// If we are on Java 17, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,
	// If we are on Java 16 or newer, we want to include the RecordTypeAdapterFactory before the ReflectiveTypeAdapterFactory,

	factories.add(new RecordTypeAdapterFactory(this.excluder, constructorConstructor, jsonAdapterFactory));
	factories.add(new RecordTypeAdapterFactory(excluder, constructorConstructor, jsonAdapterFactory));

	FilterResult filterResult = ReflectionAccessFilterHelper.getFilterResult(reflectionFilters, raw);
	if (filterResult == FilterResult.BLOCK_ALL) {
	throw new JsonIOException("ReflectionAccessFilter does not permit using reflection for "
	+ raw + ". Register a TypeAdapter for this type or adjust the access filter.");
	}

-          values[fieldIndex] = recordFields[fieldIndex].readValueForKey(in);
+          Object value = recordFields[fieldIndex].readValueForKey(in);
+          if(value != null){
+              values[fieldIndex] = value
+          }

Support Java Records when present in JVM. #2197

Support Java Records when present in JVM. #2197

Conversation

staale commented Sep 22, 2022

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 Sep 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 Sep 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 Sep 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 Sep 23, 2022 • edited

Choose a reason for hiding this comment

staale commented Sep 25, 2022

staale commented Sep 25, 2022

Marcono1234 commented Sep 26, 2022

gbjordal Sep 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staale commented Sep 26, 2022

Marcono1234 Sep 22, 2022 •

edited

Marcono1234 Sep 23, 2022 •

edited

Marcono1234 Sep 23, 2022 •

edited

Marcono1234 Sep 23, 2022 •

edited

gbjordal Sep 26, 2022 •

edited