google / gson

A Java serialization/deserialization library to convert Java Objects into JSON and back
Apache License 2.0
23.27k stars 4.27k forks source link

Change in parsing between versions 2.7 and 2.8.5 #1731

Closed Aladdill closed 2 years ago

Aladdill commented 4 years ago

Hi, My company used till recently version 2.1, while working on a certain feature, we found, that one of bugs in 2.1 prevent from us creating the feature, so we upgraded to version 2.8.6. after upgrade feature worked well, but one of our POJOs was parsed differently. After some investigation we found, that parsing still works fine in version 2.7. Attached a test that will pass on 2.7 and fail on 2.8.6. Thanks.

JsonParsingTest.zip

lyubomyr-shaydariv commented 4 years ago

Despite it looks like Gson 2.8.2 seems to have changed the way it interprets numbers, you should not serialize/deserialize classes you don't control. See these for more information: #1573, #1613, #1660.

What you should do is:

private static final Gson gson = new GsonBuilder()
.registerTypeAdapterFactory(new TypeAdapterFactory() {
    @Override
    @Nullable
    public <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> typeToken) {
        if ( !Range.class.isAssignableFrom(typeToken.getRawType()) ) {
            return null;
        }
        final Type typeArgument = ((ParameterizedType) typeToken.getType()).getActualTypeArguments()[0];
        @SuppressWarnings("unchecked")
        final TypeAdapter<Comparable<?>> elementTypeAdapter = (TypeAdapter<Comparable<?>>) gson.getDelegateAdapter(this, TypeToken.get(typeArgument));
        final TypeAdapter<Range<? extends Comparable<?>>> rangeTypeAdapter = new TypeAdapter<Range<? extends Comparable<?>>>() {
            @Override
            public void write(final JsonWriter jsonWriter, final Range<? extends Comparable<?>> range)
                    throws IOException {
                // TODO add from/to support?
                jsonWriter.beginObject();
                jsonWriter.name("v1");
                elementTypeAdapter.write(jsonWriter, range.v1);
                jsonWriter.name("v2");
                elementTypeAdapter.write(jsonWriter, range.v2);
                jsonWriter.endObject();
            }

            @Override
            public Range<? extends Comparable<?>> read(final JsonReader jsonReader)
                    throws IOException {
                // TODO add from/to support?
                jsonReader.beginObject();
                @Nullable
                Comparable<?> v1 = null;
                @Nullable
                Comparable<?> v2 = null;
                while ( jsonReader.hasNext() ) {
                    switch ( jsonReader.nextName() ) {
                    case "v1":
                        v1 = elementTypeAdapter.read(jsonReader);
                        break;
                    case "v2":
                        v2 = elementTypeAdapter.read(jsonReader);
                        break;
                    default:
                        // ignore the rest
                        jsonReader.skipValue();
                        break;
                    }
                }
                if ( v1 == null || v2 == null ) {
                    throw new IllegalArgumentException("Insufficient range: " + v1 + ", " + v2);
                }
                @SuppressWarnings({ "unchecked", "rawtypes" })
                final Range<? extends Comparable<?>> range = new Range(v1, v2);
                return range;
            }
        }
                .nullSafe();
        @SuppressWarnings("unchecked")
        final TypeAdapter<T> castRangeTypeAdapter = (TypeAdapter<T>) rangeTypeAdapter;
        return castRangeTypeAdapter;
    }
})
.create();

The type adapter will pass the following test:

final JsonElement el = JsonParser.parseString("{\"from\":1,\"to\":50,\"v1\":1,\"v2\":50}");
System.out.println("el: " + el);
final Range<Integer> range = gson.fromJson(el, rangeOfIntegerType);
System.out.println("Range: " + range);
Assertions.assertEquals(new Range<>(1, 50), range);
System.out.println(gson.toJson(new Range<>(1, 50), rangeOfIntegerType));
Aladdill commented 4 years ago

Thanks, I thought of creating type adapter.

sindilevich commented 4 years ago

Great insight into the internal workings of Gson, @lyubomyr-shaydariv , thank you!

However, what is the reason for a TypeAdapter for org.jooq.lambda.tuple.Range<T>, which is a simple structure of two T v1, T v2 fields? This class neither more complex than any other POJO I control, nor it requires special handling for (de-)serialization. A simple bi-directional conversions of an instance of new Range<Integer>(1, 50) <=> {v1: 1, v2: 50}, that's all. And it seemed working properly up until Gson 2.8.2, as you've mentioned.

Requiring a TypeAdapter for each third-party library class that only requires a straightforward bi-directional conversion is an overkill, and will introduce tons of boilerplate code into projects.

lyubomyr-shaydariv commented 4 years ago

@sindilevich

However, what is the reason for a TypeAdapter for org.jooq.lambda.tuple.Range, which is a simple structure of two T v1, T v2 fields? This class neither more complex than any other POJO I control, nor it requires special handling for (de-)serialization.

Well, I wouldn't call that class a POJO at all... Gson relies on non-transient/non-excluded instance fields regardless their visibility in respective plain objects. What would happen if the library vendor decides adding new (especially private) fields or removing existing (especially private) ones? Your JSON-serialized would get broken just because you don't control the structure of the classes. For another example, the from and to properties from the JSON above might make me think that they might appear somewhere in the third-party classes, but at least I don't see these two in org.jooq:jool:0.9.12. I just find such classes too fragile to use in such a scenario.

And it seemed working properly up until Gson 2.8.2, as you've mentioned.

I don't know what really caused the change, but for some reason Gson does not seem to be able to infer the second type parameter:

This issue looks like a tricky found regression happened, I'm assuming, somewhere between 2.8.1 and 2.8.2.

Requiring a TypeAdapter for each third-party library class that only requires a straightforward bi-directional conversion is an overkill, and will introduce tons of boilerplate code into projects.

You don't need type adapters (that might be implemented reusable anyway) if you have plain DTOs that serve the dumb (de)serialization purposes (no direct third-party coupling but controlling the entire binding structure, no inheritance, no complicated implementation interface model, etc):

@Test
public void testDedicatedDto()
        throws IOException {
    final Gson gson = new GsonBuilder()
            // no adapter
            .create();
    final Type rangeDtoOfIntegerType = new TypeToken<RangeDto<Integer>>() {}.getType();
    try ( final JsonReader jsonReader = new JsonReader(new StringReader(JSON)) ) {
        final Range<Integer> range = gson.<RangeDto<Integer>>fromJson(jsonReader, rangeDtoOfIntegerType)
                .toRange();
        Assertions.assertEquals(new Range<>(1, 50), range);
    }
}

private static final class RangeDto<T extends Comparable<T>> {

    final T v1;
    final T v2;

    private RangeDto(final T v1, final T v2) {
        this.v1 = v1;
        this.v2 = v2;
    }

    Range<T> toRange() {
        return new Range<>(v1, v2);
    }

}

The class above declared exactly two fields to serialize and deserialize, so you'd not care if the Range class changes its internal structure. #1573 and #1613 are great examples what would happen when handling uncontrolled types.

sindilevich commented 4 years ago

@lyubomyr-shaydariv, thank you for your detailed answer!

I would like to clarify a couple of points raising from your answer, however.

Your JSON-serialized would get broken just because you don't control the structure of the classes.

I fully agree with this statement, yet please acknowledge that a TypeAdapter/DTO would not help out in this scenario either. Whether a third-party class adds new fields, removes or changes the existing ones, you'd need to update your TypeAdapter/DTO. There simply isn't a bulletproof solution here. Moreover, if you need that third-party in your domain, and reach as far as introducing your own POJO for (de-)serialization, you would just as busy updating the POJO and the bi-directional conversion process, as if you were using the third-party's class straight away for (de-)serialization.

That's why I see no reason introducing the JSON <-> RangeDto<T> <-> Range<T> conversion chain in place for JSON <-> Range<T>.

This issue looks like a tricky found regression happened, I'm assuming, somewhere between 2.8.1 and 2.8.2.

May I assume the Gson maintenance team will consider this issue as a bug, therefore investigate the issue and possibly fix it in further versions of the library?

Thank you for your time!

lyubomyr-shaydariv commented 4 years ago

Whether a third-party class adds new fields, removes or changes the existing ones, you'd need to update your TypeAdapter/DTO. There simply isn't a bulletproof solution here. Moreover, if you need that third-party in your domain, and reach as far as introducing your own POJO for (de-)serialization, you would just as busy updating the POJO and the bi-directional conversion process, as if you were using the third-party's class straight away for (de-)serialization.

Yes, there is not a bulletproof solution, but I believe it's a small price for not having issues that might be expensive in the future. Especially, if there's a regression in newer versions of Gson. :)

May I assume the Gson maintenance team will consider this issue as a bug, therefore investigate the issue and possibly fix it in further versions of the library?

Sure, why not? It really looks like a regression/bug in how Gson resolves types and type parameters. I just don't know how long it would take to get it fixed/reviewed/merged/released if anyone picks it and gets it fixed.

Marcono1234 commented 4 years ago

I fully agree with this statement, yet please acknowledge that a TypeAdapter/DTO would not help out in this scenario either. Whether a third-party class adds new fields, removes or changes the existing ones, you'd need to update your TypeAdapter/DTO.

@sindilevich, the TypeAdapter @lyubomyr-shaydariv proposed above only works with the public API Range exposes. So unless the public API changes (which is very unlikely and would then break more than serialization) it will continue to work, even if the internal implementation, such as the private fields, changes in any way. The strings "v1" and "v2" only refer to the names of the JSON properties. However, since the TypeAdapter handles writing and reading of it, it can choose any name for them. It could for example also call them "from" and "to" (as hinted by the comment in the example code). Or it could even (de-)serialize the Range as JSON array, e.g.: new Range(1, 3) -Gson-> [1, 3].

Marcono1234 commented 4 years ago

It appears #1391 fixes the issue described here.

sindilevich commented 4 years ago

@Marcono1234, thank you so much for pointing to the https://github.com/google/gson/pull/1391 fix! Will monitor that to see it really resolves the issue with org.jooq.lambda.tuple.Range<T> as well. Any idea when the fix is merged/released?

Marcono1234 commented 4 years ago

Sorry, I am not a maintainer of this project, so I don't know when this will be merged.

Marcono1234 commented 2 years ago

It looks like at least the originally provided test cases succeeds in Gson 2.9.1 with org.jooq:jool:0.9.14 as dependency for the Range class. Please create a new issue in case you still experience a similar problem with the latest Gson versions.