google/gson

getDelegateAdapter() does not work properly in TypeAdapterFactory used from JsonAdapter annotation

zman0900 opened this issue · 6 comments

GSON's getDelegateAdapter method does not appear to work properly inside of a TypeAdapterFactory that is used through the @JsonAdapter annotation. This was found using GSON 2.8.0 and Java 7.

For example, given a class like:

public final class Thing {
    @JsonAdapter(NonEmptyMapAdapterFactory.class)
    private final Map<Integer, String> data;

    public Thing() {
        data = new HashMap<>();
    }
}

And a TypeAdapterFactory like:

public class NonEmptyMapAdapterFactory implements TypeAdapterFactory {

    @Override
    public <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> type) {
        if (!Map.class.isAssignableFrom(type.getRawType())) {
            return null;
        }

        //final TypeAdapter<T> delegate = gson.getAdapter(type);
        final TypeAdapter<T> delegate = gson.getDelegateAdapter(this, type);

        return new TypeAdapter<T>() {
            @Override
            public void write(final JsonWriter writer, T value) throws IOException {
                final Map map = (Map) value;
                if (map == null || map.isEmpty()) {
                    writer.nullValue();
                } else {
                    delegate.write(writer, value);
                }
            }

            @SuppressWarnings("unchecked")
            @Override
            public T read(final JsonReader reader) throws IOException {
                final T map = delegate.read(reader);
                if (map == null) {
                    return (T) new HashMap();
                } else {
                    return map;
                }
            }
        };
    }

}

The call to gson.getDelegateAdapter(this, type); returns an adapter of type "ReflectiveTypeAdapterFactory$Adapter". Calling gson.getAdapter(type); instead does return the expected "MapTypeAdapterFactory$Adapter", but this obviously won't work correctly if this adapter factory is instead used without the JsonAdapter annotation.

If the annotation is removed from the field in the Thing class and gson is instead set up like:

new GsonBuilder()
        .registerTypeAdapterFactory(new NonEmptyMapAdapterFactory())
        .create();

Then the custom TypeAdapterFactory shown above works exactly as expected: gson.getDelegateAdapter(this, type); returns a "MapTypeAdapterFactory$Adapter", but calling gson.getAdapter(type); returns the the same class, resulting in a stack overflow in the read and write methods.

The problem seems to happen because of this code at the beginning of getDelegateAdapter():

// Hack. If the skipPast factory isn't registered, assume the factory is being requested via
// our @JsonAdapter annotation.
if (!factories.contains(skipPast)) {
  skipPast = jsonAdapterFactory;
}

In the list of factories, jsonAdapterFactory comes after the entry for the MapTypeAdapterFactory, causing both to be skipped.

Thanks for the detailed bug.
Can you submit a Pull Request that reproduces the problem?
If you can include a fix too, even better. Thanks.

I was wondering if this bug will be solved soon, since im in an similar spot as @zman0900

Kind regards
Thomas

Very interested in a resolution. Thank you.

nivs commented

A workaround to overcome this issue is to use gson.getDelegateAdapter(gson.excluder(), type) instead of gson.getDelegateAdapter(this, type).

When used from a JsonAdapter annotation, getDelegateAdapter() ignores skipPast if it does not specify a registered adapter, and uses the JsonAdapterAnnotationTypeAdapterFactory instead - effectively skipping past all type adapters registered by the user. gson.excluder() is registered just before the user's type adapters, and if used instead of this, they won't be skipped.

That workaround is however quite fragile because you must never use such a type adapter factory as regular factory registered on GsonBuilder or in a @JsonAdapter annotation on a type (in contrast to using it on a field, which this issue is about). Otherwise you will end up in an infinite loop.

Also what is the expected behavior? Since @JsonAdapter on a field overwrites any type adapter factory, I can imagine that users want different behavior of getDelegateAdapter in this case:

  • To start before the excluder, so the field is excluded if necessary
  • To start before the excluder to use the default for Object and JsonElement, but ignore the excluder
  • To start behind the excluder (?)

Edit: Nevermind, seems like excluder has higher precedence than @JsonAdapter, so annotation would not even be considered if excluder excluded field.

Changing this could also break backwards compatibility in case someone relies on the reflection-based strategy being used, in case getDelegateAdapter would be changed and returns a different type adapter then.

Can this be worked?