Fix Gson.getDelegateAdapter not working properly for JsonAdapter (#…

…2435)

* Fix `Gson.getDelegateAdapter` not working properly for `JsonAdapter`

* Address review feedback and add comments regarding thread-safety

* Revert InstanceCreator instance validation

* Disallow `null` as `skipPast`

* Avoid `equals` usage in `getDelegateAdapter` & minor other changes

Previously `getDelegateAdapter` called `factories.contains(skipPast)`,
but unlike the other comparisons which check for reference equality,
that would have used the `equals` method.
This could lead to spurious "GSON cannot serialize ..." exceptions
if two factory instances compared equal, but the one provided as
`skipPast` had not been registered yet.

gson/src/main/java/com/google/gson/Gson.java

@@ -16,6 +16,7 @@
 
 package com.google.gson;
 
+import com.google.gson.annotations.JsonAdapter;
 import com.google.gson.internal.ConstructorConstructor;
 import com.google.gson.internal.Excluder;
 import com.google.gson.internal.GsonBuildConfig;
@@ -604,42 +605,50 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
    * adapter that does a little bit of work but then delegates further processing to the Gson
    * default type adapter. Here is an example:
    * <p>Let's say we want to write a type adapter that counts the number of objects being read
-   *  from or written to JSON. We can achieve this by writing a type adapter factory that uses
-   *  the <code>getDelegateAdapter</code> method:
-   *  <pre> {@code
-   *  class StatsTypeAdapterFactory implements TypeAdapterFactory {
-   *    public int numReads = 0;
-   *    public int numWrites = 0;
-   *    public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
-   *      final TypeAdapter<T> delegate = gson.getDelegateAdapter(this, type);
-   *      return new TypeAdapter<T>() {
-   *        public void write(JsonWriter out, T value) throws IOException {
-   *          ++numWrites;
-   *          delegate.write(out, value);
-   *        }
-   *        public T read(JsonReader in) throws IOException {
-   *          ++numReads;
-   *          return delegate.read(in);
-   *        }
-   *      };
-   *    }
-   *  }
-   *  } </pre>
-   *  This factory can now be used like this:
-   *  <pre> {@code
-   *  StatsTypeAdapterFactory stats = new StatsTypeAdapterFactory();
-   *  Gson gson = new GsonBuilder().registerTypeAdapterFactory(stats).create();
-   *  // Call gson.toJson() and fromJson methods on objects
-   *  System.out.println("Num JSON reads" + stats.numReads);
-   *  System.out.println("Num JSON writes" + stats.numWrites);
-   *  }</pre>
-   *  Note that this call will skip all factories registered before {@code skipPast}. In case of
-   *  multiple TypeAdapterFactories registered it is up to the caller of this function to insure
-   *  that the order of registration does not prevent this method from reaching a factory they
-   *  would expect to reply from this call.
-   *  Note that since you can not override type adapter factories for String and Java primitive
-   *  types, our stats factory will not count the number of String or primitives that will be
-   *  read or written.
+   * from or written to JSON. We can achieve this by writing a type adapter factory that uses
+   * the <code>getDelegateAdapter</code> method:
+   * <pre>{@code
+   * class StatsTypeAdapterFactory implements TypeAdapterFactory {
+   *   public int numReads = 0;
+   *   public int numWrites = 0;
+   *   public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
+   *     final TypeAdapter<T> delegate = gson.getDelegateAdapter(this, type);
+   *     return new TypeAdapter<T>() {
+   *       public void write(JsonWriter out, T value) throws IOException {
+   *         ++numWrites;
+   *         delegate.write(out, value);
+   *       }
+   *       public T read(JsonReader in) throws IOException {
+   *         ++numReads;
+   *         return delegate.read(in);
+   *       }
+   *     };
+   *   }
+   * }
+   * }</pre>
+   * This factory can now be used like this:
+   * <pre>{@code
+   * StatsTypeAdapterFactory stats = new StatsTypeAdapterFactory();
+   * Gson gson = new GsonBuilder().registerTypeAdapterFactory(stats).create();
+   * // Call gson.toJson() and fromJson methods on objects
+   * System.out.println("Num JSON reads: " + stats.numReads);
+   * System.out.println("Num JSON writes: " + stats.numWrites);
+   * }</pre>
+   * Note that this call will skip all factories registered before {@code skipPast}. In case of
+   * multiple TypeAdapterFactories registered it is up to the caller of this function to insure
+   * that the order of registration does not prevent this method from reaching a factory they
+   * would expect to reply from this call.
+   * Note that since you can not override the type adapter factories for some types, see
+   * {@link GsonBuilder#registerTypeAdapter(Type, Object)}, our stats factory will not count
+   * the number of instances of those types that will be read or written.
+   *
+   * <p>If {@code skipPast} is a factory which has neither been registered on the {@link GsonBuilder}
+   * nor specified with the {@link JsonAdapter @JsonAdapter} annotation on a class, then this
+   * method behaves as if {@link #getAdapter(TypeToken)} had been called. This also means that
+   * for fields with {@code @JsonAdapter} annotation this method behaves normally like {@code getAdapter}
+   * (except for corner cases where a custom {@link InstanceCreator} is used to create an
+   * instance of the factory).
+   *
    * @param skipPast The type adapter factory that needs to be skipped while searching for
    *   a matching type adapter. In most cases, you should just pass <i>this</i> (the type adapter
    *   factory from where {@code getDelegateAdapter} method is being invoked).
@@ -648,9 +657,10 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
    * @since 2.2
    */
   public <T> TypeAdapter<T> getDelegateAdapter(TypeAdapterFactory skipPast, TypeToken<T> type) {
-    // Hack. If the skipPast factory isn't registered, assume the factory is being requested via
-    // our @JsonAdapter annotation.
-    if (!factories.contains(skipPast)) {
+    Objects.requireNonNull(skipPast, "skipPast must not be null");
+    Objects.requireNonNull(type, "type must not be null");
+
+    if (jsonAdapterFactory.isClassJsonAdapterFactory(type, skipPast)) {
       skipPast = jsonAdapterFactory;
     }
 
@@ -668,7 +678,13 @@ public <T> TypeAdapter<T> getDelegateAdapter(TypeAdapterFactory skipPast, TypeTo
         return candidate;
       }
     }
-    throw new IllegalArgumentException("GSON cannot serialize " + type);
+
+    if (skipPastFound) {
+      throw new IllegalArgumentException("GSON cannot serialize " + type);
+    } else {
+      // Probably a factory from @JsonAdapter on a field
+      return getAdapter(type);
+    }
   }
 
   /**

gson/src/main/java/com/google/gson/InstanceCreator.java

@@ -63,7 +63,7 @@
  * </pre>
  *
  * <p>Note that it does not matter what the fields of the created instance contain since Gson will
- * overwrite them with the deserialized values specified in Json. You should also ensure that a
+ * overwrite them with the deserialized values specified in JSON. You should also ensure that a
  * <i>new</i> object is returned, not a common object since its fields will be overwritten.
  * The developer will need to register {@code IdInstanceCreator} with Gson as follows:</p>
  *
@@ -81,7 +81,7 @@ public interface InstanceCreator<T> {
   /**
    * Gson invokes this call-back method during deserialization to create an instance of the
    * specified type. The fields of the returned instance are overwritten with the data present
-   * in the Json. Since the prior contents of the object are destroyed and overwritten, do not
+   * in the JSON. Since the prior contents of the object are destroyed and overwritten, do not
    * return an instance that is useful elsewhere. In particular, do not return a common instance,
    * always use {@code new} to create a new instance.
    *

gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java

@@ -24,6 +24,9 @@
 import com.google.gson.annotations.JsonAdapter;
 import com.google.gson.internal.ConstructorConstructor;
 import com.google.gson.reflect.TypeToken;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 /**
  * Given a type T, looks for the annotation {@link JsonAdapter} and uses an instance of the
@@ -32,35 +35,85 @@
  * @since 2.3
  */
 public final class JsonAdapterAnnotationTypeAdapterFactory implements TypeAdapterFactory {
+  private static class DummyTypeAdapterFactory implements TypeAdapterFactory {
+    @Override public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
+      throw new AssertionError("Factory should not be used");
+    }
+  }
+
+  /**
+   * Factory used for {@link TreeTypeAdapter}s created for {@code @JsonAdapter}
+   * on a class.
+   */
+  private static final TypeAdapterFactory TREE_TYPE_CLASS_DUMMY_FACTORY = new DummyTypeAdapterFactory();
+
+  /**
+   * Factory used for {@link TreeTypeAdapter}s created for {@code @JsonAdapter}
+   * on a field.
+   */
+  private static final TypeAdapterFactory TREE_TYPE_FIELD_DUMMY_FACTORY = new DummyTypeAdapterFactory();
+
   private final ConstructorConstructor constructorConstructor;
 
+  /**
+   * For a class, if it is annotated with {@code @JsonAdapter} and refers to a {@link TypeAdapterFactory},
+   * stores the factory instance in case it has been requested already.
+   * Has to be a {@link ConcurrentMap} because {@link Gson} guarantees to be thread-safe.
+   */
+  // Note: In case these strong reference to TypeAdapterFactory instances are considered
+  // a memory leak in the future, could consider switching to WeakReference<TypeAdapterFactory>
+  private final ConcurrentMap<Class<?>, TypeAdapterFactory> adapterFactoryMap;
+
   public JsonAdapterAnnotationTypeAdapterFactory(ConstructorConstructor constructorConstructor) {
     this.constructorConstructor = constructorConstructor;
+    this.adapterFactoryMap = new ConcurrentHashMap<>();
+  }
+
+  // Separate helper method to make sure callers retrieve annotation in a consistent way
+  private JsonAdapter getAnnotation(Class<?> rawType) {
+    return rawType.getAnnotation(JsonAdapter.class);
   }
 
   @SuppressWarnings("unchecked") // this is not safe; requires that user has specified correct adapter class for @JsonAdapter
   @Override
   public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> targetType) {
     Class<? super T> rawType = targetType.getRawType();
-    JsonAdapter annotation = rawType.getAnnotation(JsonAdapter.class);
+    JsonAdapter annotation = getAnnotation(rawType);
     if (annotation == null) {
       return null;
     }
-    return (TypeAdapter<T>) getTypeAdapter(constructorConstructor, gson, targetType, annotation);
+    return (TypeAdapter<T>) getTypeAdapter(constructorConstructor, gson, targetType, annotation, true);
   }
 
-  TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gson gson,
-      TypeToken<?> type, JsonAdapter annotation) {
+  // Separate helper method to make sure callers create adapter in a consistent way
+  private static Object createAdapter(ConstructorConstructor constructorConstructor, Class<?> adapterClass) {
     // TODO: The exception messages created by ConstructorConstructor are currently written in the context of
     // deserialization and for example suggest usage of TypeAdapter, which would not work for @JsonAdapter usage
-    Object instance = constructorConstructor.get(TypeToken.get(annotation.value())).construct();
+    return constructorConstructor.get(TypeToken.get(adapterClass)).construct();
+  }
+
+  private TypeAdapterFactory putFactoryAndGetCurrent(Class<?> rawType, TypeAdapterFactory factory) {
+    // Uses putIfAbsent in case multiple threads concurrently create factory
+    TypeAdapterFactory existingFactory = adapterFactoryMap.putIfAbsent(rawType, factory);
+    return existingFactory != null ? existingFactory : factory;
+  }
+
+  TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gson gson,
+      TypeToken<?> type, JsonAdapter annotation, boolean isClassAnnotation) {
+    Object instance = createAdapter(constructorConstructor, annotation.value());
 
     TypeAdapter<?> typeAdapter;
     boolean nullSafe = annotation.nullSafe();
     if (instance instanceof TypeAdapter) {
       typeAdapter = (TypeAdapter<?>) instance;
     } else if (instance instanceof TypeAdapterFactory) {
-      typeAdapter = ((TypeAdapterFactory) instance).create(gson, type);
+      TypeAdapterFactory factory = (TypeAdapterFactory) instance;
+
+      if (isClassAnnotation) {
+        factory = putFactoryAndGetCurrent(type.getRawType(), factory);
+      }
+
+      typeAdapter = factory.create(gson, type);
     } else if (instance instanceof JsonSerializer || instance instanceof JsonDeserializer) {
       JsonSerializer<?> serializer = instance instanceof JsonSerializer
           ? (JsonSerializer<?>) instance
@@ -69,8 +122,16 @@ TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gso
           ? (JsonDeserializer<?>) instance
           : null;
 
+      // Uses dummy factory instances because TreeTypeAdapter needs a 'skipPast' factory for `Gson.getDelegateAdapter`
+      // call and has to differentiate there whether TreeTypeAdapter was created for @JsonAdapter on class or field
+      TypeAdapterFactory skipPast;
+      if (isClassAnnotation) {
+        skipPast = TREE_TYPE_CLASS_DUMMY_FACTORY;
+      } else {
+        skipPast = TREE_TYPE_FIELD_DUMMY_FACTORY;
+      }
       @SuppressWarnings({ "unchecked", "rawtypes" })
-      TypeAdapter<?> tempAdapter = new TreeTypeAdapter(serializer, deserializer, gson, type, null, nullSafe);
+      TypeAdapter<?> tempAdapter = new TreeTypeAdapter(serializer, deserializer, gson, type, skipPast, nullSafe);
       typeAdapter = tempAdapter;
 
       nullSafe = false;
@@ -87,4 +148,45 @@ TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gso
 
     return typeAdapter;
   }
+
+  /**
+   * Returns whether {@code factory} is a type adapter factory created for {@code @JsonAdapter}
+   * placed on {@code type}.
+   */
+  public boolean isClassJsonAdapterFactory(TypeToken<?> type, TypeAdapterFactory factory) {
+    Objects.requireNonNull(type);
+    Objects.requireNonNull(factory);
+
+    if (factory == TREE_TYPE_CLASS_DUMMY_FACTORY) {
+      return true;
+    }
+
+    // Using raw type to match behavior of `create(Gson, TypeToken<T>)` above
+    Class<?> rawType = type.getRawType();
+
+    TypeAdapterFactory existingFactory = adapterFactoryMap.get(rawType);
+    if (existingFactory != null) {
+      // Checks for reference equality, like it is done by `Gson.getDelegateAdapter`
+      return existingFactory == factory;
+    }
+
+    // If no factory has been created for the type yet check manually for a @JsonAdapter annotation
+    // which specifies a TypeAdapterFactory
+    // Otherwise behavior would not be consistent, depending on whether or not adapter had been requested
+    // before call to `isClassJsonAdapterFactory` was made
+    JsonAdapter annotation = getAnnotation(rawType);
+    if (annotation == null) {
+      return false;
+    }
+
+    Class<?> adapterClass = annotation.value();
+    if (!TypeAdapterFactory.class.isAssignableFrom(adapterClass)) {
+      return false;
+    }
+
+    Object adapter = createAdapter(constructorConstructor, adapterClass);
+    TypeAdapterFactory newFactory = (TypeAdapterFactory) adapter;
+
+    return putFactoryAndGetCurrent(rawType, newFactory) == factory;
+  }
 }

gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java

@@ -156,7 +156,7 @@ private BoundField createBoundField(
     if (annotation != null) {
       // This is not safe; requires that user has specified correct adapter class for @JsonAdapter
       mapped = jsonAdapterFactory.getTypeAdapter(
-          constructorConstructor, context, fieldType, annotation);
+          constructorConstructor, context, fieldType, annotation, false);
     }
     final boolean jsonAdapterPresent = mapped != null;
     if (mapped == null) mapped = context.getAdapter(fieldType);

gson/src/main/java/com/google/gson/internal/bind/TreeTypeAdapter.java

@@ -43,11 +43,18 @@ public final class TreeTypeAdapter<T> extends SerializationDelegatingTypeAdapter
   private final JsonDeserializer<T> deserializer;
   final Gson gson;
   private final TypeToken<T> typeToken;
-  private final TypeAdapterFactory skipPast;
+  /**
+   * Only intended as {@code skipPast} for {@link Gson#getDelegateAdapter(TypeAdapterFactory, TypeToken)},
+   * must not be used in any other way.
+   */
+  private final TypeAdapterFactory skipPastForGetDelegateAdapter;
   private final GsonContextImpl context = new GsonContextImpl();
   private final boolean nullSafe;
 
-  /** The delegate is lazily created because it may not be needed, and creating it may fail. */
+  /**
+   * The delegate is lazily created because it may not be needed, and creating it may fail.
+   * Field has to be {@code volatile} because {@link Gson} guarantees to be thread-safe.
+   */
   private volatile TypeAdapter<T> delegate;
 
   public TreeTypeAdapter(JsonSerializer<T> serializer, JsonDeserializer<T> deserializer,
@@ -56,7 +63,7 @@ public TreeTypeAdapter(JsonSerializer<T> serializer, JsonDeserializer<T> deseria
     this.deserializer = deserializer;
     this.gson = gson;
     this.typeToken = typeToken;
-    this.skipPast = skipPast;
+    this.skipPastForGetDelegateAdapter = skipPast;
     this.nullSafe = nullSafe;
   }
 
@@ -94,7 +101,7 @@ private TypeAdapter<T> delegate() {
     TypeAdapter<T> d = delegate;
     return d != null
         ? d
-        : (delegate = gson.getDelegateAdapter(skipPast, typeToken));
+        : (delegate = gson.getDelegateAdapter(skipPastForGetDelegateAdapter, typeToken));
   }
 
   /**