Fix RuntimeTypeAdapterFactory

[t-oster]

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]

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]

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

[t-oster]

@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]

...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.

[eamonnmcmanus]

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

[eamonnmcmanus]

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]

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?

[t-oster]

@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]

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]

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]

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.

[t-oster]

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