Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729) #2731

lyubomyr-shaydariv · 2024-08-30T09:15:52Z

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729)

Purpose / Description

Resolves #2729.

Checklist

New code follows the Google Java Style Guide
This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
If necessary, new public API validates arguments, for example rejects null
New public API has Javadoc
- Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
If necessary, new unit tests have been added
- Assertions in unit tests use Truth, see existing tests
- No JUnit 3 features are used (such as extending class TestCase)
- If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
mvn clean verify javadoc:jar passes without errors

Closes #2729.

Marcono1234

Thanks a lot!

I have added a few comments, but feel free to consider them only suggestions. Hopefully they are useful.

Also, don't worry too much about the Git history of this PR branch; the commits will be squashed on merge anyway.

Marcono1234 · 2024-09-01T17:05:36Z

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

-        } else {
-          TypeAdapter.this.write(out, value);
-        }
+    if (!NullSafeTypeAdapter.class.isInstance(this)) {


Minor, but why not !(this instanceof NullSafeTypeAdapter)?

@Marcono1234

Yeah, I had instanceof in mind, but not sure how it all works as of today. The rule org.apache.maven.enforcer.rules.version.RequireJavaVersion fails with

Detected JDK version 21 (JAVA_HOME=/opt/.bin/jdk-21) is not in the allowed range [11,18)

If the type check is changed to !(this instanceof NullSafeTypeAdapter), then:

for JDK 11, it fails with /src/main/java/com/google/gson/TypeAdapter.java:[292,27] illegal generic type for instanceof;

for JDK 17, it fails with /src/main/java/com/google/gson/TypeAdapter.java:[292,16] reifiable types in instanceof are not supported in -source 7.

The Class.instanceOf check works for all. However, according to the source code, it can work with TypeAdapter<?> at least in GsonBuilder.

I seem to have some lack of the Java language knowledge. Please suggest.

Ah sorry, it seems !(this instanceof NullSafeTypeAdapter) is not possible because NullSafeTypeAdapter is a non-static inner class (and the enclosing class TypeAdapter<T> is generic?).

The compiler error message is really not helpful, but in Eclipse IDE the error message included the suggested fix, qualify with the enclosing type: TypeAdapter.NullSafeTypeAdapter. Can you please try this instead?

Suggested change

if (!NullSafeTypeAdapter.class.isInstance(this)) {

if (!(this instanceof TypeAdapter.NullSafeTypeAdapter)) {

I've never encountered this error before preferring static inner classes to capture as few as possible, and now I see there are still "good-to-knows". But no doubt, isAssignable looked really weird and unnatural. Thank you!

Marcono1234 · 2024-09-01T17:08:32Z

gson/src/test/java/com/google/gson/TypeAdapterTest.java

+  }
+
+  private static final TypeAdapter<String> assertionErrorAdapter =
+      new TypeAdapter<String>() {


Minor: String can probably be omitted here since it can be inferred

Suggested change

new TypeAdapter<String>() {

new TypeAdapter<>() {

I merely moved the `TypeAdapter` instance from the method to the field leaving the type parameter as it was previously in method. My rationale is that I should follow the convention and do not ~~touch~~ use other language level for code I don't own and that can be a subject of refactoring in the future.
~~If you're really fine with changing from TypeAdapter<String> to TypeAdapter<>, I'll change <String> to <>.~~

Minor: String can probably be omitted here since it can be inferred

My bad: I seem to have missed the line change I made accidentally. Also I removed the assertionErrorAdapter TypeAdapter type from <String> to <> -- I saw the diamond operator has been in use at least in this test class.

Marcono1234 · 2024-09-01T17:10:27Z

gson/src/test/java/com/google/gson/TypeAdapterTest.java

-      new TypeAdapter<>() {
+      new TypeAdapter<String>() {


I think this is not needed since we compile the test code with Java 11.

Though IDEs sometimes have problems detecting this, and erroneously assume Java 7 is used for test compilation as well.

Marcono1234 · 2024-09-01T17:12:15Z

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

+    return this;
+  }
+
+  private final class NullSafeTypeAdapter extends TypeAdapter<T> {


As suggested in #2729 (comment), do you want to take the chance and implement toString as well. I think at least some other adapters implement it, and it would then make it easier to see which adapter is wrapped by this one.

@Marcono1234
Oh, I seem to have misterstood your toString() suggestion earlier leaving it out of scope (I mistakenly assumed it leads to a stack overflow error, but it doesn't).

~~Could you please suggest the NullSafeTypeAdapter.toString() pattern you'd like to see?~~

As suggested in #2729 (comment), do you want to take the chance and implement toString as well. I think at least some other adapters implement it, and it would then make it easier to see which adapter is wrapped by this one.

I applied the NullSafeTypeAdapter[%s] pattern you suggested in #2729 (comment).

Marcono1234 · 2024-09-01T17:16:01Z

And could you please edit the description of the pull request to say "Closes #2729"? Otherwise GitHub does not understand this, and will keep the issue open after merge, unless they are linked manually.

lyubomyr-shaydariv · 2024-09-01T18:04:03Z

And could you please edit the description of the pull request to say "Closes #2729"? Otherwise GitHub does not understand this, and will keep the issue open after merge, unless they are linked manually.

Do you mean to add Closes #2729 literally, so that GitHub detects the ticked #-id following the Closes ? I was not aware of this behavior, and thought that ... type adapters (#2729) would be sufficient.

Maybe I'm misusing git but git rev-list --perl-regexp --count --grep='\bClose:?\b' fix-2729 returns no commmits. (My local fix-2729 branch is currently pointing to 91246be in my fork repo.)

Marcono1234 · 2024-09-02T22:34:16Z

Do you mean to add Closes #2729 literally, so that GitHub detects the ticked #-id following the Closes ?

Yes, just edit the description of your pull request text and add that anywhere. See for example #2704 which used "Resolves #...".

GitHub recognizes certain keywords and in that case will automatically close the linked issue.
It will also show this information then on the right sidebar of

the issue
the pull request

You can also link issue and pull request manually¹, but it is easier if the creator of the pull request directly does this themselves through the keywords. Otherwise it is easy to forget manually linking the issue and pull request.

That works as well if you write it in the commit message (and the commit is then merged), but personally I avoid this (unless the contribution guide of a project requests it) since for each commit this already shows in the GitHub UI for the issue a "added a commit to ... that referenced this issue" message, which can be rather distracting. So I prefer to only add it to the pull request description.

I was not aware of this behavior, and thought that ... type adapters (#2729) would be sufficient.

It seems that just creates a regular "mention" without having any other effect. See for example #2729 which you referenced:

(No worries though about the multiple commit mentions there.)

If you are a member of the project (?). But I am not a direct member of the Gson project, so I cannot do it here. See also the GitHub documentation. ↩

lyubomyr-shaydariv · 2024-09-03T02:09:36Z

Do you mean to add Closes #2729 literally, so that GitHub detects the ticked #-id following the Closes ?

Yes, just edit the description of your pull request text and add that anywhere.

The funny thing is that I thought you meant the git commit message, not the PR description, and this is why I referenced the git-rev-list command output that produced empty result. And now I also see why it is not a good idea for GitHub, kind of shame on GitHub from this perspective. Thank you for the comprehensive explanation!

from returning nested null-safe type adapters (#2729)

Marcono1234

@eamonnmcmanus, what do you think about this pull request?

eamonnmcmanus

Thanks for doing this!

lyubomyr-shaydariv force-pushed the fix-2729 branch 2 times, most recently from 48627de to 91246be Compare August 31, 2024 10:57

Marcono1234 reviewed Sep 1, 2024

View reviewed changes

lyubomyr-shaydariv force-pushed the fix-2729 branch from 91246be to 011c870 Compare September 2, 2024 19:26

eamonnmcmanus added the enhancement label Sep 2, 2024

eamonnmcmanus linked an issue Sep 2, 2024 that may be closed by this pull request

Multiple invocation of the TypeAdapter.nullSafe method causes the given type adapter to be wrapped recursively. #2729

Closed

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe()

503a1d5

from returning nested null-safe type adapters (#2729)

lyubomyr-shaydariv force-pushed the fix-2729 branch from 011c870 to 503a1d5 Compare September 3, 2024 02:11

Marcono1234 approved these changes Sep 3, 2024

View reviewed changes

eamonnmcmanus approved these changes Sep 3, 2024

View reviewed changes

eamonnmcmanus merged commit 7c55d8b into google:main Sep 3, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729) #2731

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729) #2731

lyubomyr-shaydariv commented Aug 30, 2024 •

edited

Loading

Marcono1234 left a comment

Marcono1234 Sep 1, 2024

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

Marcono1234 Sep 2, 2024

lyubomyr-shaydariv Sep 3, 2024

Marcono1234 Sep 1, 2024

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 2, 2024

Marcono1234 Sep 1, 2024 •

edited

Loading

Marcono1234 Sep 1, 2024

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 2, 2024 •

edited

Loading

Marcono1234 commented Sep 1, 2024

lyubomyr-shaydariv commented Sep 1, 2024 •

edited

Loading

Marcono1234 commented Sep 2, 2024 •

edited

Loading

lyubomyr-shaydariv commented Sep 3, 2024

Marcono1234 left a comment •

edited

Loading

eamonnmcmanus left a comment

	if (!NullSafeTypeAdapter.class.isInstance(this)) {
	if (!(this instanceof TypeAdapter.NullSafeTypeAdapter)) {

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729) #2731

Add NullSafeTypeAdapter to prevent TypeAdapter.nullSafe() from returning nested null-safe type adapters (#2729) #2731

Conversation

lyubomyr-shaydariv commented Aug 30, 2024 • edited Loading

Purpose / Description

Checklist

Marcono1234 left a comment

Choose a reason for hiding this comment

Marcono1234 Sep 1, 2024

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Marcono1234 Sep 2, 2024

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 3, 2024

Choose a reason for hiding this comment

Marcono1234 Sep 1, 2024

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 2, 2024

Choose a reason for hiding this comment

Marcono1234 Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Marcono1234 Sep 1, 2024

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

lyubomyr-shaydariv Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Marcono1234 commented Sep 1, 2024

lyubomyr-shaydariv commented Sep 1, 2024 • edited Loading

Marcono1234 commented Sep 2, 2024 • edited Loading

Footnotes

lyubomyr-shaydariv commented Sep 3, 2024

Marcono1234 left a comment • edited Loading

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

lyubomyr-shaydariv commented Aug 30, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

Marcono1234 Sep 1, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 1, 2024 •

edited

Loading

lyubomyr-shaydariv Sep 2, 2024 •

edited

Loading

lyubomyr-shaydariv commented Sep 1, 2024 •

edited

Loading

Marcono1234 commented Sep 2, 2024 •

edited

Loading

Marcono1234 left a comment •

edited

Loading