Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1028: Gson.getDelegateAdapter not working correctly for @JsonAdapter field #1690

Closed

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented May 7, 2020

Previously TypeAdapterFactory classes specified using the @JsonAdapter annotation for fields were only be able to get type adapters created by the enum and reflection-based factories when calling Gson.getDelegateAdapter(...).
This pull request changes it so they are able to get type adapters created by all factories registered on the Gson instance.

The behavior for @JsonAdapter on types (in contrast to fields) remains unchanged.

Note that this is an backwards incompatible change in case someone relies on Gson.getDelegateAdapter(...) returning a reflection-based type adapter. However, this might be a rare corner case and it appears keeping this behavior while also fixing #1028 is not possible.

Also note that this solution breaks if the created TypeAdapterFactory leaks a reference to itself, e.g. by storing a reference to itself in a static field or adding it to a static collection, and then later calling getDelegateAdapter. Or if TypeAdapterFactory.create(...) calls getDelegateAdapter from a separate thread. Though both cases are likely rare corner cases.

Previously TypeAdapterFactory classes specified using the @JsonAdapter
annotation for fields were only be able to get type adapters created by the
enum and reflection-based factories when calling Gson.getDelegateAdapter(...).
This commit changes it so they are able to get type adapters created by all
factories registered on the Gson instance.

The behavior for @JsonAdapter on types (in contrast to fields) remains
unchanged.
for (TypeAdapterFactory factory : factories) {
if (!skipPastFound) {
if (factory == skipPast) {
if (skipPast != null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because jsonAdapterFactory.isFieldAdapterFactory would throw a NullPointerException otherwise.

This slightly changes the behavior in that previously null was treated as jsonAdapterFactory, which was however not documented and also likely not intended.

@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Sep 24, 2021

Maybe instead of tracking adapters which have been created for fields annotated with @JsonAdapter; instead track adapters which have been created for classes annotated with @JsonAdapter and decide based on that which adapters to skip in getDelegateAdapter.

This would then allow custom reflection based adapters to support @JsonAdapter and proper usage of getDelegateAdapter as well, which would not be possible with the changes proposed here.

Though both that implementation and this implementation here would behave incorrectly when a custom InstanceCreator has been registered which returns a singleton factory instance, which is also registered as regular factory. (The current Gson implementation would not handle that correctly either.) But that is probably negligible because if @JsonAdapter on the class was called in the first place, then that factory singleton returned null before (since @JsonAdapter on class is called after custom factories), so it would not make sense that this singleton factory is specified for the @JsonAdapter (because it would return null again).
(That logic does not apply for @JsonAdapter on fields, but maybe there it is acceptable that getDelegateAdapter skips past the singleton factory, even though the call happened from a different context, that is from @JsonAdapter; and this is how Gson would behave currently anyways.)

@Marcono1234
Copy link
Collaborator Author

Has been superseded by #2435

@Marcono1234 Marcono1234 deleted the JsonAdapter-getDelegateAdapter branch August 23, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants