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 RuntimeTypeAdapterFactory #2139

Merged
merged 2 commits into from Jul 21, 2022
Merged

Fix RuntimeTypeAdapterFactory #2139

merged 2 commits into from Jul 21, 2022

Conversation

t-oster
Copy link
Contributor

@t-oster t-oster commented Jun 27, 2022

Trying to use this class as is results in the type-property not being serialized into the JSON, thus it is not present on deserialization.
The fix from #712 (comment) works. No idea why this is not merged yet.

@google-cla
Copy link

google-cla bot commented Jun 27, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jun 27, 2022

Thanks for the proposal! Would it be possible to add a test that fails without this fix and passes with it?

@t-oster
Copy link
Contributor Author

t-oster commented Jun 28, 2022

@eamonnmcmanus like this? Or in a single commit? The thing is in the existing test, the base class was explicitely given, so the bug did not occur. I just removed the explicit typeOfSrc parameter. In a real world example, the object could be a collection element or an attribute of a class being serialized, so there would be no typeOfSrc either. If it is not given, the RuntimeTypeAdapterFactory.create method does not work, because it just compares if the type matches the base-type excaclty, instead of if it is supertype.

@t-oster
Copy link
Contributor Author

t-oster commented Jun 28, 2022

...btw it would be great, if the gson-extras would be released on maven somehow, because currently many people just copy the class to their source-code and won't profit from future updates.

@@ -205,7 +205,7 @@ public RuntimeTypeAdapterFactory<T> registerSubtype(Class<? extends T> type) {

@Override
public <R> TypeAdapter<R> create(Gson gson, TypeToken<R> type) {
if (type.getRawType() != baseType) {
if (null == type || !baseType.isAssignableFrom(type.getRawType())) {
Copy link
Member

@eamonnmcmanus eamonnmcmanus Jun 28, 2022

Choose a reason for hiding this comment

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

Would you mind fixing the Yoda condition? i.e. type == null instead.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jun 28, 2022

Yes, I think this test change is enough. Just one style nit, which I should have mentioned before.

Regarding gson-extras, I think that's an unrelated question? This project is in maintenance mode so realistically we're probably not going to be adding brand new artifacts to Maven Central.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Jun 29, 2022

In a real world example, the object could be a collection element or an attribute of a class being serialized, so there would be no typeOfSrc either.

It does work if the field type or collection element type is the base type. Consider the following class:

static class Test {
  Base base;
  Sub sub;

  List<Base> baseList;
  List<Sub> subList;
}

When creating an instance of Sub and assigning it to base and sub, and a List containing a Sub object and assigning it to baseList and subList, the JSON will currently contain the type for the Base fields, but not for the Sub ones. There hasn't been any official statement about the behavior (and I am not a member of this project), but I could imagine that this behavior might have been intended. When your model class explicitly uses a field of a specific subclass, then including the type might be redundant because deserialization would work without the type.
An issue only occurs when you serialize an object as a different type than you deserialize it, as shown in the example in #712. A similar issue would also occur when there is an intermediate class, e.g. Base > Sub1 > Sub2. Then a field with type Sub1 but runtime value of Sub2 will not have the type member either.

So even if the behavior was originally intended to be like this, it might be good nonetheless to change it, as done in this PR.
However, @t-oster, maybe it would be good to not modify the original test method but instead add a separate one to verify that it still behaves as expected when the class is explicitly specified for toJson. What do you think?

Trying to use this class as is results in the type-property not being serialized into the JSON, thus it is not present on deserialization.
The fix from google#712 (comment) works. No idea why this is not merged yet.
@t-oster
Copy link
Contributor Author

t-oster commented Jun 29, 2022

@eamonnmcmanus I inverted the null check. @Marcono1234 I am not sure if I understand, but I had the error when trying to expliciteley serialize a list of the base-class, so this was definitely a bug IMHO. Could you maybe create an example? If the serialization does not add the type field, the deserialization will fail. And if it adds the field, the deserialization will remove it, so I do not see the redundancy.

@eamonnmcmanus is only the extras on maintenance mode or the whole GSON project? Why? Are the receommended alternatives?

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Jun 29, 2022

so I do not see the redundancy

What I meant was when serializing as Sub and also deserializing as Sub, then the type field would be redundant, and I assumed that might have been the intention for the current behavior. But as you say, when deserializing as Base this will fail because type is missing, so the fix introduced by this PR is needed. Sorry if my comment caused confusion.

Regarding the tests, what I meant was that it should ideally cover both:

  • toJson(subInstance)
  • toJson(subInstance, BaseClass.class)

Your changes modify the tests to only cover toJson(subInstance), but not toJson(subInstance, BaseClass.class) anymore (if I see that correctly).

However, I am not a member of this project so unlike eamonnmcmanus' comments feel free to consider mine only as suggestions.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jul 21, 2022

is only the extras on maintenance mode or the whole GSON project? Why? Are there recommended alternatives?

The whole project. It is fine for people to continue using Gson but for new projects I would probably recommend Moshi.

@eamonnmcmanus eamonnmcmanus merged commit 2eb3758 into google:master Jul 21, 2022
5 checks passed
@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jul 27, 2022

FYI this change caused problems at Google, where we do in fact use the extras project as-is. The problems are similar to the List scenario described by @Marcono1234 . I think the change here should be an optional behaviour of RuntimeTypeAdapterFactory that is triggered by calling a new recognizeSubclasses() method, rather than always happening. I'm planning a PR to that effect. It means that other people who have copies of the class can update their copy without fear of being broken.

eamonnmcmanus added a commit that referenced this pull request Jul 27, 2022
…lly. (#2160)

PR #2139 changed this factory so that if given a certain baseType, it will also
recognize any subtype of that type. That is often the right thing to do, but it
is a change in behaviour, and does in fact break at least one current client of
this code. So instead we introduce a new `recognizeSubclasses()` method that
triggers this behaviour. When the method is not called, we revert to the old
behaviour of only recognizing instances of the exact class `baseType`.
@t-oster
Copy link
Contributor Author

t-oster commented Jul 28, 2022

Ok. Thanks for the heads up. I'm sorry this introduced errors, but with your recognizeSubclasses() method this should be OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants