Adjust Record adapter and extend test coverage #2224

Marcono1234 · 2022-10-14T20:41:45Z

Changes:

Make record constructor and accessor accessible (unless reflection access filter prevents it)
Disallow null for primitive component, see Small adjustments to the new record code. #2219 (comment)
Skip static fields when deserializing records
Would not work without special casing and I am not sure if serializing or deserializing static fields was ever officially documented as supported (just happens to work by using GsonBuilder.excludeFieldsWithModifiers). For serialization there might be legit use cases (e.g. to encode a version number), but deserialization it rather questionable.
Disallow @SerializedName placed only on accessor method
Implemented this because we have the accessor method available for this check anyway, and because for records the accessor method is actually called on serialization so users might expect @SerializedName to work on them.
Adjust ReflectionHelper.makeAccessible to create description of object only when exception is thrown instead of eagerly
Prefix names of test classes ReflectiveTypeAdapterFactoryTest and ReflectionHelperTest with Java17
This makes sure that we notice when the checked JDK class UnixDomainPrincipal is missing for some reason even though the test runs on Java >= 17. Otherwise these tests might just be silently skipped without us noticing.
Arguably these tests could also be rewritten to use custom record classes now that the tests are only compiled on Java 17 or newer, but on the other hand it might also be valuable that they test against a record class which is not part of Gson's tests.
... for further changes see messages of subsequent commits

Marcono1234 · 2022-10-14T20:48:47Z

gson/src/test/java/com/google/gson/internal/bind/Java17ReflectiveTypeAdapterFactoryTest.java

  }

  @Test
  public void testCustomAdapterForRecords() {
    Gson gson = new Gson();
    TypeAdapter<?> recordAdapter = gson.getAdapter(unixDomainPrincipalClass);
-    TypeAdapter<?> defaultReflectionAdapter = gson.getAdapter(UserPrincipal.class);
+    TypeAdapter<?> defaultReflectionAdapter = gson.getAdapter(DummyClass.class);


Have changed this because UserPrincipal is an interface without any fields, so assuming that the normal reflection based adapter is used for it might be a bit brittle.

eamonnmcmanus

Thanks for this!

This is my first pass. I think I will need to spend more time looking at this, though.

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

gson/src/main/java/com/google/gson/internal/reflect/ReflectionHelper.java

eamonnmcmanus · 2022-10-16T01:29:24Z

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

-          accessor = ReflectionHelper.getAccessor(raw, field);
+        if (isRecord) {
+          // If there is a static field on a record, there will not be an accessor. Instead we will use the default
+          // field logic for dealing with statics. For deserialization the field is ignored.


Just to clarify: by default, static fields in records will be neither serialized nor deserialized, right? Only if that weird excludeFieldsWithModifiers method is called? Because I definitely don't think static fields should be serialized.

Yes that is correct, see default excluded modifiers here:

gson/gson/src/main/java/com/google/gson/internal/Excluder.java

Line 55 in c2458bf

private int modifiers = Modifier.TRANSIENT | Modifier.STATIC;

I have mainly added this to handle it in a reasonable way instead of failing with some obscure exception.

For static fields maybe there are legit (opt-in) use cases, e.g. to automatically encode a version number:

record Response( ... ) { public static final int VERSION = 1; }

Though I am not sure if someone is actually using this, and it is certainly not obvious (and probably not even officially supported) to use excludeFieldsWithModifiers for this.

If you want I can adjust it to also always exclude static fields on serialization, and we can then wait for someone to request this feature.

Yes, I think by default static fields should not be serialized. I suppose they should be if someone calls excludeFieldsWithModifiers(0), though even then I don't really understand why people would want that. I'm not really convinced by the VERSION use case. We would serialize that field, and then what would we do on deserialization? Do we check that the version matches? Do we try to overwrite the VERSION field? If we don't, isn't that inconsistent? I think if you really want a version then it should either be an instance field or you should use a custom TypeAdapter.

(I looked at the history, and apparently excludeFieldsWithModifiers was already present in the first version on GitHub, dated 2008. So who knows why it was added. For what it's worth, Google's code has only a handful of calls to this method, and exactly one that is including STATIC, perhaps accidentally.)

I'm not really convinced by the VERSION use case. We would serialize that field, and then what would we do on deserialization?

It would probably be most useful for services which only serialize data, e.g. a REST API which generates the response. At least that is the most reasonable situation I can come up with at the moment.

It appears the excludeFieldsWithModifiers trick is known, see this StackOverflow answer.

eamonnmcmanus · 2022-10-18T16:48:27Z

gson/src/test/java/com/google/gson/functional/Java17RecordTest.java

+  public void testStaticFieldSerialization() {
+    Gson gson = new GsonBuilder()
+        // Include static fields
+        .excludeFieldsWithModifiers(0)


Could you also test that the field is not serialized without this?

It looks like there was already a test for this: com.google.gson.functional.UncategorizedTest.testStaticFieldsAreNotSerialized()
And additionally multiple tests implicitly relied on static fields being ignored by default. However, I have now extended these new test methods to also test the default Gson behavior.

Also, I have noticed that Field.set per documentation does not support static final fields. So to avoid Gson claiming "Unexpected IllegalAccessException occurred...", I have added custom handling for that case to provide a better exception message.

gson/src/test/java/com/google/gson/functional/Java17RecordTest.java

eamonnmcmanus

(It was unclear to me whether there is in fact test coverage for static fields not being serialized by default.)

Previously it would report "Unexpected IllegalAccessException occurred..." due to the uncaught IllegalAccessException.

Such an exception is not 'unexpected' (which was claimed by the previous exception handling) because user code could throw it.

Marcono1234 · 2022-10-19T23:30:02Z

Sorry for these additional commits. I did not consider these cases initially and only noticed now that they are not covered by any tests. I hope the additional commits I pushed are fine.

Marcono1234 · 2022-10-19T23:34:07Z

gson/src/test/java/com/google/gson/functional/Java17RecordTest.java

+  /** Tests behavior when the canonical constructor throws an exception */
+  @Test
+  public void testThrowingConstructor() {
+    record LocalRecord(String s) {


It appears the Eclipse compiler produces malformed byte code for this record class ~~(have not investigated that further yet)~~. Just as forewarning in case you are experiencing a java.lang.VerifyError when running this test in Eclipse.

Edit: Have created eclipse-jdt/eclipse.jdt.core#487 for that.

eamonnmcmanus

Looks good to me. Let me know if you think it is ready to be merged.

Marcono1234 · 2022-10-22T12:07:53Z

It should be ready to merge.

In case you plan to create a release soon (per Semantic Versioning that would be 2.10.0 I assume), it would be good if you could take a look at #2212. If those changes are too experimental / hacky in your opinion, then at least during the release process the @since Javadoc tags for the new methods would have to be added manually.

Adjust Record adapter and extend test coverage

0e407fb

Marcono1234 commented Oct 14, 2022

View reviewed changes

eamonnmcmanus requested changes Oct 16, 2022

View reviewed changes

Marcono1234 added 2 commits October 16, 2022 17:00

Address review feedback

7267193

Make constructor string more concise

e1f696c

eamonnmcmanus reviewed Oct 18, 2022

View reviewed changes

Add tests for Gson default behavior for static fields

55bbdca

Marcono1234 marked this pull request as draft October 18, 2022 23:14

Marcono1234 added 2 commits October 19, 2022 01:14

Improve exception for deserializing static final field

f499528

Previously it would report "Unexpected IllegalAccessException occurred..." due to the uncaught IllegalAccessException.

Improve handling of exception thrown by accessor

e28ad83

Such an exception is not 'unexpected' (which was claimed by the previous exception handling) because user code could throw it.

Marcono1234 force-pushed the marcono1234/record-follow-up branch from d72a9d1 to e28ad83 Compare October 18, 2022 23:23

Marcono1234 marked this pull request as ready for review October 18, 2022 23:24

Improve constructor invocation exception handling and add tests

2f444b2

Marcono1234 commented Oct 19, 2022

View reviewed changes

eamonnmcmanus approved these changes Oct 20, 2022

View reviewed changes

eamonnmcmanus merged commit 66d9621 into google:master Oct 22, 2022

Marcono1234 deleted the marcono1234/record-follow-up branch October 22, 2022 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust Record adapter and extend test coverage #2224

Adjust Record adapter and extend test coverage #2224

Marcono1234 commented Oct 14, 2022 •

edited

Marcono1234 Oct 14, 2022

eamonnmcmanus left a comment

eamonnmcmanus Oct 16, 2022

Marcono1234 Oct 16, 2022 •

edited

eamonnmcmanus Oct 16, 2022

Marcono1234 Oct 16, 2022

eamonnmcmanus Oct 18, 2022

Marcono1234 Oct 18, 2022

eamonnmcmanus left a comment

Marcono1234 commented Oct 19, 2022

Marcono1234 Oct 19, 2022 •

edited

eamonnmcmanus left a comment

Marcono1234 commented Oct 22, 2022

Adjust Record adapter and extend test coverage #2224

Adjust Record adapter and extend test coverage #2224

Conversation

Marcono1234 commented Oct 14, 2022 • edited

Marcono1234 Oct 14, 2022

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

eamonnmcmanus Oct 16, 2022

Choose a reason for hiding this comment

Marcono1234 Oct 16, 2022 • edited

Choose a reason for hiding this comment

eamonnmcmanus Oct 16, 2022

Choose a reason for hiding this comment

Marcono1234 Oct 16, 2022

Choose a reason for hiding this comment

eamonnmcmanus Oct 18, 2022

Choose a reason for hiding this comment

Marcono1234 Oct 18, 2022

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 commented Oct 19, 2022

Marcono1234 Oct 19, 2022 • edited

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 commented Oct 22, 2022

Marcono1234 commented Oct 14, 2022 •

edited

Marcono1234 Oct 16, 2022 •

edited

Marcono1234 Oct 19, 2022 •

edited