From 8451c1fa63f5198daf38498869836e85c85bdd08 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 11 Oct 2022 01:10:48 +0200 Subject: [PATCH] Fix TypeAdapterRuntimeTypeWrapper not detecting reflective TreeTypeAdapter and FutureTypeAdapter (#1787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix TypeAdapterRuntimeTypeWrapper not detecting reflective TreeTypeAdapter Previously on serialization TypeAdapterRuntimeTypeWrapper preferred a TreeTypeAdapter without `serializer` which falls back to the reflective adapter. This behavior was incorrect because it caused the reflective adapter for a Base class to be used for serialization (indirectly as TreeTypeAdapter delegate) instead of using the reflective adapter for a Subclass extending Base. * Address review feedback * Convert TypeAdapterRuntimeTypeWrapperTest to JUnit 4 test * Prefer wrapped reflective adapter for serialization of subclass * Detect reflective adapter used as delegate for Gson.FutureTypeAdapter * Tiny style tweak. Co-authored-by: Éamonn McManus --- gson/src/main/java/com/google/gson/Gson.java | 22 +- .../SerializationDelegatingTypeAdapter.java | 14 ++ .../gson/internal/bind/TreeTypeAdapter.java | 13 +- .../bind/TypeAdapterRuntimeTypeWrapper.java | 28 ++- .../TypeAdapterRuntimeTypeWrapperTest.java | 193 ++++++++++++++++++ 5 files changed, 256 insertions(+), 14 deletions(-) create mode 100644 gson/src/main/java/com/google/gson/internal/bind/SerializationDelegatingTypeAdapter.java create mode 100644 gson/src/test/java/com/google/gson/functional/TypeAdapterRuntimeTypeWrapperTest.java diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index bb3e2c77..22071a17 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -32,6 +32,7 @@ import com.google.gson.internal.bind.MapTypeAdapterFactory; import com.google.gson.internal.bind.NumberTypeAdapter; import com.google.gson.internal.bind.ObjectTypeAdapter; import com.google.gson.internal.bind.ReflectiveTypeAdapterFactory; +import com.google.gson.internal.bind.SerializationDelegatingTypeAdapter; import com.google.gson.internal.bind.TypeAdapters; import com.google.gson.internal.sql.SqlTypesSupport; import com.google.gson.reflect.TypeToken; @@ -1315,7 +1316,7 @@ public final class Gson { return fromJson(new JsonTreeReader(json), typeOfT); } - static class FutureTypeAdapter extends TypeAdapter { + static class FutureTypeAdapter extends SerializationDelegatingTypeAdapter { private TypeAdapter delegate; public void setDelegate(TypeAdapter typeAdapter) { @@ -1325,18 +1326,23 @@ public final class Gson { delegate = typeAdapter; } - @Override public T read(JsonReader in) throws IOException { + private TypeAdapter delegate() { if (delegate == null) { - throw new IllegalStateException(); + throw new IllegalStateException("Delegate has not been set yet"); } - return delegate.read(in); + return delegate; + } + + @Override public TypeAdapter getSerializationDelegate() { + return delegate(); + } + + @Override public T read(JsonReader in) throws IOException { + return delegate().read(in); } @Override public void write(JsonWriter out, T value) throws IOException { - if (delegate == null) { - throw new IllegalStateException(); - } - delegate.write(out, value); + delegate().write(out, value); } } diff --git a/gson/src/main/java/com/google/gson/internal/bind/SerializationDelegatingTypeAdapter.java b/gson/src/main/java/com/google/gson/internal/bind/SerializationDelegatingTypeAdapter.java new file mode 100644 index 00000000..dad4ff11 --- /dev/null +++ b/gson/src/main/java/com/google/gson/internal/bind/SerializationDelegatingTypeAdapter.java @@ -0,0 +1,14 @@ +package com.google.gson.internal.bind; + +import com.google.gson.TypeAdapter; + +/** + * Type adapter which might delegate serialization to another adapter. + */ +public abstract class SerializationDelegatingTypeAdapter extends TypeAdapter { + /** + * Returns the adapter used for serialization, might be {@code this} or another adapter. + * That other adapter might itself also be a {@code SerializationDelegatingTypeAdapter}. + */ + public abstract TypeAdapter getSerializationDelegate(); +} diff --git a/gson/src/main/java/com/google/gson/internal/bind/TreeTypeAdapter.java b/gson/src/main/java/com/google/gson/internal/bind/TreeTypeAdapter.java index b7e92495..560234c0 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/TreeTypeAdapter.java +++ b/gson/src/main/java/com/google/gson/internal/bind/TreeTypeAdapter.java @@ -38,7 +38,7 @@ import java.lang.reflect.Type; * tree adapter may be serialization-only or deserialization-only, this class * has a facility to lookup a delegate type adapter on demand. */ -public final class TreeTypeAdapter extends TypeAdapter { +public final class TreeTypeAdapter extends SerializationDelegatingTypeAdapter { private final JsonSerializer serializer; private final JsonDeserializer deserializer; final Gson gson; @@ -97,6 +97,15 @@ public final class TreeTypeAdapter extends TypeAdapter { : (delegate = gson.getDelegateAdapter(skipPast, typeToken)); } + /** + * Returns the type adapter which is used for serialization. Returns {@code this} + * if this {@code TreeTypeAdapter} has a {@link #serializer}; otherwise returns + * the delegate. + */ + @Override public TypeAdapter getSerializationDelegate() { + return serializer != null ? this : delegate(); + } + /** * Returns a new factory that will match each type against {@code exactType}. */ @@ -169,5 +178,5 @@ public final class TreeTypeAdapter extends TypeAdapter { @Override public R deserialize(JsonElement json, Type typeOfT) throws JsonParseException { return (R) gson.fromJson(json, typeOfT); } - }; + } } diff --git a/gson/src/main/java/com/google/gson/internal/bind/TypeAdapterRuntimeTypeWrapper.java b/gson/src/main/java/com/google/gson/internal/bind/TypeAdapterRuntimeTypeWrapper.java index 6a690919..75a991ea 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/TypeAdapterRuntimeTypeWrapper.java +++ b/gson/src/main/java/com/google/gson/internal/bind/TypeAdapterRuntimeTypeWrapper.java @@ -53,10 +53,12 @@ final class TypeAdapterRuntimeTypeWrapper extends TypeAdapter { if (runtimeType != type) { @SuppressWarnings("unchecked") TypeAdapter runtimeTypeAdapter = (TypeAdapter) context.getAdapter(TypeToken.get(runtimeType)); + // For backward compatibility only check ReflectiveTypeAdapterFactory.Adapter here but not any other + // wrapping adapters, see https://github.com/google/gson/pull/1787#issuecomment-1222175189 if (!(runtimeTypeAdapter instanceof ReflectiveTypeAdapterFactory.Adapter)) { // The user registered a type adapter for the runtime type, so we will use that chosen = runtimeTypeAdapter; - } else if (!(delegate instanceof ReflectiveTypeAdapterFactory.Adapter)) { + } else if (!isReflective(delegate)) { // The user registered a type adapter for Base class, so we prefer it over the // reflective type adapter for the runtime type chosen = delegate; @@ -68,12 +70,30 @@ final class TypeAdapterRuntimeTypeWrapper extends TypeAdapter { chosen.write(out, value); } + /** + * Returns whether the type adapter uses reflection. + * + * @param typeAdapter the type adapter to check. + */ + private static boolean isReflective(TypeAdapter typeAdapter) { + // Run this in loop in case multiple delegating adapters are nested + while (typeAdapter instanceof SerializationDelegatingTypeAdapter) { + TypeAdapter delegate = ((SerializationDelegatingTypeAdapter) typeAdapter).getSerializationDelegate(); + // Break if adapter does not delegate serialization + if (delegate == typeAdapter) { + break; + } + typeAdapter = delegate; + } + + return typeAdapter instanceof ReflectiveTypeAdapterFactory.Adapter; + } + /** * Finds a compatible runtime type if it is more specific */ - private Type getRuntimeTypeIfMoreSpecific(Type type, Object value) { - if (value != null - && (type == Object.class || type instanceof TypeVariable || type instanceof Class)) { + private static Type getRuntimeTypeIfMoreSpecific(Type type, Object value) { + if (value != null && (type instanceof Class || type instanceof TypeVariable)) { type = value.getClass(); } return type; diff --git a/gson/src/test/java/com/google/gson/functional/TypeAdapterRuntimeTypeWrapperTest.java b/gson/src/test/java/com/google/gson/functional/TypeAdapterRuntimeTypeWrapperTest.java new file mode 100644 index 00000000..73a01012 --- /dev/null +++ b/gson/src/test/java/com/google/gson/functional/TypeAdapterRuntimeTypeWrapperTest.java @@ -0,0 +1,193 @@ +package com.google.gson.functional; + +import static org.junit.Assert.assertEquals; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonDeserializationContext; +import com.google.gson.JsonDeserializer; +import com.google.gson.JsonElement; +import com.google.gson.JsonPrimitive; +import com.google.gson.JsonSerializationContext; +import com.google.gson.JsonSerializer; +import com.google.gson.TypeAdapter; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonWriter; +import java.io.IOException; +import java.lang.reflect.Type; +import org.junit.Test; + +public class TypeAdapterRuntimeTypeWrapperTest { + private static class Base { + } + private static class Subclass extends Base { + @SuppressWarnings("unused") + String f = "test"; + } + private static class Container { + @SuppressWarnings("unused") + Base b = new Subclass(); + } + private static class Deserializer implements JsonDeserializer { + @Override + public Base deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) { + throw new AssertionError("not needed for this test"); + } + } + + /** + * When custom {@link JsonSerializer} is registered for Base should + * prefer that over reflective adapter for Subclass for serialization. + */ + @Test + public void testJsonSerializer() { + Gson gson = new GsonBuilder() + .registerTypeAdapter(Base.class, new JsonSerializer() { + @Override + public JsonElement serialize(Base src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive("serializer"); + } + }) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":\"serializer\"}", json); + } + + /** + * When only {@link JsonDeserializer} is registered for Base, then on + * serialization should prefer reflective adapter for Subclass since + * Base would use reflective adapter as delegate. + */ + @Test + public void testJsonDeserializer_ReflectiveSerializerDelegate() { + Gson gson = new GsonBuilder() + .registerTypeAdapter(Base.class, new Deserializer()) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":{\"f\":\"test\"}}", json); + } + + /** + * When {@link JsonDeserializer} with custom adapter as delegate is + * registered for Base, then on serialization should prefer custom adapter + * delegate for Base over reflective adapter for Subclass. + */ + @Test + public void testJsonDeserializer_CustomSerializerDelegate() { + Gson gson = new GsonBuilder() + // Register custom delegate + .registerTypeAdapter(Base.class, new TypeAdapter() { + @Override + public Base read(JsonReader in) throws IOException { + throw new UnsupportedOperationException(); + } + @Override + public void write(JsonWriter out, Base value) throws IOException { + out.value("custom delegate"); + } + }) + .registerTypeAdapter(Base.class, new Deserializer()) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":\"custom delegate\"}", json); + } + + /** + * When two (or more) {@link JsonDeserializer}s are registered for Base + * which eventually fall back to reflective adapter as delegate, then on + * serialization should prefer reflective adapter for Subclass. + */ + @Test + public void testJsonDeserializer_ReflectiveTreeSerializerDelegate() { + Gson gson = new GsonBuilder() + // Register delegate which itself falls back to reflective serialization + .registerTypeAdapter(Base.class, new Deserializer()) + .registerTypeAdapter(Base.class, new Deserializer()) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":{\"f\":\"test\"}}", json); + } + + /** + * When {@link JsonDeserializer} with {@link JsonSerializer} as delegate + * is registered for Base, then on serialization should prefer + * {@code JsonSerializer} over reflective adapter for Subclass. + */ + @Test + public void testJsonDeserializer_JsonSerializerDelegate() { + Gson gson = new GsonBuilder() + // Register JsonSerializer as delegate + .registerTypeAdapter(Base.class, new JsonSerializer() { + @Override + public JsonElement serialize(Base src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive("custom delegate"); + } + }) + .registerTypeAdapter(Base.class, new Deserializer()) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":\"custom delegate\"}", json); + } + + /** + * When a {@link JsonDeserializer} is registered for Subclass, and a custom + * {@link JsonSerializer} is registered for Base, then Gson should prefer + * the reflective adapter for Subclass for backward compatibility (see + * https://github.com/google/gson/pull/1787#issuecomment-1222175189) even + * though normally TypeAdapterRuntimeTypeWrapper should prefer the custom + * serializer for Base. + */ + @Test + public void testJsonDeserializer_SubclassBackwardCompatibility() { + Gson gson = new GsonBuilder() + .registerTypeAdapter(Subclass.class, new JsonDeserializer() { + @Override + public Subclass deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) { + throw new AssertionError("not needed for this test"); + } + }) + .registerTypeAdapter(Base.class, new JsonSerializer() { + @Override + public JsonElement serialize(Base src, Type typeOfSrc, JsonSerializationContext context) { + return new JsonPrimitive("base"); + } + }) + .create(); + + String json = gson.toJson(new Container()); + assertEquals("{\"b\":{\"f\":\"test\"}}", json); + } + + private static class CyclicBase { + @SuppressWarnings("unused") + CyclicBase f; + } + + private static class CyclicSub extends CyclicBase { + @SuppressWarnings("unused") + int i; + + public CyclicSub(int i) { + this.i = i; + } + } + + /** + * Tests behavior when the type of a field refers to a type whose adapter is + * currently in the process of being created. For these cases {@link Gson} + * uses a future adapter for the type. That adapter later uses the actual + * adapter as delegate. + */ + @Test + public void testGsonFutureAdapter() { + CyclicBase b = new CyclicBase(); + b.f = new CyclicSub(2); + String json = new Gson().toJson(b); + assertEquals("{\"f\":{\"i\":2}}", json); + } +}