google / gson

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

When field is marked to be excluded from deserialization, it is still deserialized. #2348

Open jan-pesta opened 1 year ago

jan-pesta commented 1 year ago

Gson version 2.10

Java 11

Used tools Eclipse + Maven

Description

I have added Exclusion strategy for deserialization of one field, and that strategy is ignored and Exception: declares multiple JSON fields named field is thrown.

Expected behavior

I would expect that when strategy return true in method shouldSkipField, that the field will be ignored

Actual behavior

Exception: declares multiple JSON fields named field is thrown because field is not ignored

Reproduction steps

Create class and subclass with fields which share serialized name.

import com.google.gson.*;
import com.google.gson.annotations.SerializedName;

public class GSonTest
{
    public static void main(String[] args)
    {
        GsonBuilder builder = new GsonBuilder();
        builder.addDeserializationExclusionStrategy(new BaseDataExclusionStrategy());
        SpecificClass specificClass = builder.create().fromJson("{\"data\":\"Test String\"}", SpecificClass.class);

    }

    public static class BaseClass
    {
        @SerializedName("data")
        private Object data = null;
    }

    public static class SpecificClass extends BaseClass
    {
        @SerializedName("data")
        private String concreteData = null;
    }

    private static class BaseDataExclusionStrategy implements ExclusionStrategy 
    {
        @Override
        public boolean shouldSkipClass(Class<?> arg0)
        {
            return false;
        }

        @Override
        public boolean shouldSkipField(FieldAttributes field)
        {
            return "data".equals(field.getName()) && Object.class.equals(field.getDeclaredClass());
        }

    }
}

Exception stack trace

Exception in thread "main" java.lang.IllegalArgumentException: class GSonTest$SpecificClass declares multiple JSON fields named data
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:217)
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:112)
    at com.google.gson.Gson.getAdapter(Gson.java:531)
    at com.google.gson.Gson.fromJson(Gson.java:1057)
    at com.google.gson.Gson.fromJson(Gson.java:1016)
    at com.google.gson.Gson.fromJson(Gson.java:959)
    at com.google.gson.Gson.fromJson(Gson.java:927)
    at GSonTest.main(GSonTest.java:16)

Code Snippet I think is reason for it: https://github.com/google/gson/blob/26229d33d81f0d60f508411eb3c541f3e87322ac/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java#L260

        boolean serialize = includeField(field, true);
        boolean deserialize = includeField(field, false);
        if (!serialize && !deserialize) {
          continue;
        }

I think it should be here logical OR instead of AND

Marcono1234 commented 1 year ago

Can you please provide a complete minimal, reproducible example? Especially the code of your exclusion strategy, how you set up the GsonBuilder and how you use the created Gson instance would be important.

Also, the Gson code you referenced looks correct to me. It allows you to define an exclusion strategy

Using a logical OR in the code above would only allow excluding fields for serialization and deserialization at the same time.

jan-pesta commented 1 year ago

Hi @Marcono1234, I have just updated the original post with simple code to reproduce problem.

Yes, I agree that logical OR will not be solution, but the check should be extended, if deserialization is in process, and field should be excluded for it, it should continue with next filed. In this case as the filed does not have exclusion for serialization, it is still deserialized.

I have debuged that part of code, and I have following variables set: serialize = true, deserialize=false, and condition is not valid to skip a field.

Marcono1234 commented 1 year ago

Should your program later also serialize data, or will it only ever deserialize data? Because the exception still highlights a potential problem: During serialization both fields would be written with the same name. While the JSON specification allows duplicate member names, it discourages such JSON data, and other JSON libraries might reject the data or might not be able to properly handle it.

There might also be some misunderstanding here. As seen in the stack trace of the exception, it occurs while creating the TypeAdapter, which is used for both serialization and deserialization and then cached. So the issue here is not Gson behaves incorrect specifically only for deserialization (calling fromJson).

I am (personally) not sure if it is worth adjusting Gson to handle this situation better (or differently). Even if you don't perform serialization, the easiest solution would be to just register the exclusion strategy also for serialization, e.g.:

BaseDataExclusionStrategy exclusionStrategy = new BaseDataExclusionStrategy();
builder.addDeserializationExclusionStrategy(exclusionStrategy);
builder.addSerializationExclusionStrategy(exclusionStrategy);

Or

builder.setExclusionStrategies(new BaseDataExclusionStrategy());

With that change your example code does not throw an exception anymore.

jan-pesta commented 1 year ago

I know about it, and I used that as workaround. I am not planning to serialize such data, I have such structure generated from yaml from thirdparty as response from backend.

I was thinking that it is confusing that exclusion strategy has to be paired always, otherwise it is not working. Also if you have non duplicated field and only one strategy is defined, the field will be processed (not sure here about serialization, but looks like common code) e.q. you do not want to serialize password or any other security field.

Marcono1234 commented 1 year ago

The problem from the ReflectiveTypeAdapterFactory perspective is that it cannot know that you only want to perform deserialization; it has to account for both serialization and deserialization. The only solution would be to defer the duplicate field check until the adapter is actually used. However, that might not always be desirable because it prevents failing fast.

the field will be processed (not sure here about serialization, but looks like common code) e.q. you do not want to serialize password or any other security field

No, for all other aspects the separation between serialization and deserialization works as expected. The part of the ReflectiveTypeAdapterFactory code you referenced earlier uses the serialize and deserialize values further below to act accordingly during serialization and deserialization.

So personally I would consider the solutions mentioned above, registering the exclusion strategy also for serialization, the proper solution here and not only a workaround. It does not even have any disadvantages in your case since you are not performing serialization anyways (but Gson cannot know that).