Support Java Records when present in JVM.

[staale]

Fixes #1794

Added support in the ReflectionHelper to detect if a class is a record on the JVM (via reflection), and if so, we will create a special RecordAdapter to deserialize records, using the canoncial constructor.

The ReflectionTypeAdapterFactory had to be refactored a bit to support this. The Adapter class inside the factory is now abstract, with concrete implementations for normal field reflection and for Records. The common code is in the Adapter, with each implementation deserializing values into an intermediary object.

For the FieldReflectionAdapter, the intermediary is actually the final result, and field access is used to write to fields as before. For the RecordAdapter the intermediary is the Object[] to pass to the Record constructor.

[Marcono1234]

Thanks!

I really like the intermediary approach you took here to store the canonical constructor arguments.

[staale]

I really like the intermediary approach you took here to store the canonical constructor arguments.

Thanks, it is in part inspired by how Collectors work (when applying them, not implementing). I have now also fixed so that I use the Record accessor method to read out values. However, I am a bit uncertain about the approach I have now, because it does not take into consideration that records can be non-public. In those cases the constructor and accessor must be made visible I think. Would be good to have an additional source tree for tests that only applied to Java 17.

[Marcono1234]

Unfortunately GitHub has the bad habit of hiding unresolved comments, but there are a few more review comments:

In case you are not aware of it, you can also show all unresolved conversations on the "Files" tab, see documentation.

[staale]

Unfortunately GitHub has the bad habit of hiding unresolved comments, but there are a few more review comments:

I thought those where hidden because they were resolved. I will work through that stack next then.

[Marcono1234]

In those cases the constructor and accessor must be made visible I think.

Accessor methods will be fine I think because they are guaranteed to be public (see Language Specification). But as mentioned earlier the canonical constructor will probably be an issue. Though you can address that later if you want.

Also no hurry, take your time.

[eamonnmcmanus]

In #2203 I'm adding a skeletal test for records which I'm hoping you will be able to fill out to get some good test coverage here. I did the necessary work to ensure that the new test is only compiled in the JDK 17 build.

[staale]

I looked at https://www.baeldung.com/maven-multi-release-jars to create a multi-release jar, and have a separate Java16 source folder. Unfortunately this seems to conflict with bnd https://github.com/bndtools/bnd/tree/master/maven/bnd-maven-plugin

[ERROR] Failed to execute goal biz.aQute.bnd:bnd-maven-plugin:6.3.1:bnd-process (default) on project gson:
Classes found in the wrong directory: {META-INF/versions/16/com/google/gson/internal/reflect/RecordHelper.class=com.google.gson.internal.reflect.RecordHelper} -> [Help 1]

If we get this to work, we can avoid reflection. However, we will end up with 2 files to maintain wrt. reflection, probably better to move all the record related methods into a separate RecordHelper, or JVM16Util (and JVM9Util for access reflection related to Java 9)

[Marcono1234]

A few more smaller suggestions, but I think this is looking really good! Thanks a lot for your work so far.

I looked at https://www.baeldung.com/maven-multi-release-jars to create a multi-release jar

I think for now this reflection-based approach is fine. Even though in the past for other occasions I suggested using multi-release JARs as well, it also has it flaws. The main problem is when users create a JAR with dependencies (also called "fat JAR" or "uber JAR" sometimes) because most build tool plugins (e.g. Maven Shade Plugin or Gradle Shadow plugin) do not set Multi-Release: true in the MANIFEST.MF, and most users are not aware of this or forget it. So Gson would manually have to check whether it is running with JDK > X or whether certain features (such as records here) are supported and verify that the multi-release classes were properly loaded.

Also, I think there is no need to force-push to this GitHub pull request; @eamonnmcmanus will most likely squash the commits anyway on merge. Please let us know though if you don't want this and want the individual commits to be preserved.

[staale]

In #2203 I'm adding a skeletal test for records which I'm hoping you will be able to fill out to get some good test coverage here. I did the necessary work to ensure that the new test is only compiled in the JDK 17 build.

Thanks, I see that it is merged, so I can rebase on top of master to also have these tests actually run.

[Marcono1234]

so I can rebase on top of master

As mentioned before, unless you want the Git history of this PR to be preserved you can also just merge in master into this branch. Otherwise when you rebase and force-push GitHub will probably be unable to match the open review comments to the rebased commits, and it makes comparing the changes slightly more cumbersome.

[staale]

Fixed review comments.

[Marcono1234]

Thanks for your changes! But please avoid force-pushing this branch (as mentioned before). The commits of this pull request will be squashed anyways, (maybe) unless you explicitly say you don't want this. But even in that case, please squash and force-push once all comments are resolved at the end.

With force-pushing (especially when modifying the previous commits) I cannot easily review the incremental changes you did but I would have to review all of your changes all over again... (or I have to compare the differences between the force-pushes)
It also renders GitHub's review comments ineffective because they link to commits which don't exist anymore. So I would have to search the comments here on the conversation tab to find out their context and then manually try to figure out if they still apply.

I think these changes are fine now and further review rounds would just cause this to drag on. For any smaller remaining things I can try to create a follow-up pull request.

[Marcono1234]

@eamonnmcmanus could you please have a look?

[eamonnmcmanus]

Great! I'm just going to run this through all of Google's tests. I don't really expect that to reveal any problems but I've been surprised before.

[staale]

Thanks for your changes! But please avoid force-pushing this branch (as mentioned before).

Sorry, will not do so again. I am used to Gerrit workflow at work, where we try to keep a clean history. I did one force push to fix the committer of the first commit, as that was using my work email, and not my private email. If this is squash merged, that's great.

[eamonnmcmanus]

Thanks again! This will be very helpful for users. There are a few minor things but it's probably easier for me to address them in a followup.

[Marcono1234]

Thanks again @staale for your work on this! Sorry that my last comment was a bit harsh and that my review comments were a bit excessive. Your changes integrated really well with the existing reflective adapter. Hopefully users will enjoy the full Record class support!

[GeorgeCodes17]

I am a bit confused why using this code is giving "**java.lang.AssertionError: java.lang.IllegalAccessException: Can not set final java.lang.String field com.Schoolio.objects.TimetableObject.end to java.lang.String**". Can anyone help, what is wrong with my implementation?

[Marcono1234]

@GeorgeCodes17, which Gson version are you using? Support for Record classes was added in Gson 2.10; the exception message you showed looks like the one reported for older Gson versions which did not properly support Record classes.

[GeorgeCodes17]

@GeorgeCodes17, which Gson version are you using? Support for Record classes was added in Gson 2.10; the exception message you showed looks like the one reported for older Gson versions which did not properly support Record classes.

Gson 2.8.5 version