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

@@ -631,16 +631,16 @@ public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
    * 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);
+   * 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.
+   * 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
@@ -660,11 +660,8 @@ public <T> TypeAdapter<T> getDelegateAdapter(TypeAdapterFactory skipPast, TypeTo
     Objects.requireNonNull(skipPast, "skipPast must not be null");
     Objects.requireNonNull(type, "type must not be null");
 
-    if (jsonAdapterFactory.isClassJsonAdapterFactory(type.getRawType(), skipPast)) {
+    if (jsonAdapterFactory.isClassJsonAdapterFactory(type, skipPast)) {
       skipPast = jsonAdapterFactory;
-    } else if (!factories.contains(skipPast)) {
-      // Probably a factory from @JsonAdapter on a field
-      return getAdapter(type);
     }
 
     boolean skipPastFound = false;
@@ -681,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/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java

@@ -54,9 +54,10 @@ private static class DummyTypeAdapterFactory implements TypeAdapterFactory {
   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 {@code TypeAdapterFactory}
-   * stores the factory instance, in case it has been requested already.
+   * 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
@@ -121,16 +122,12 @@ 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) {
-        // Use dummy `skipPast` value and put it into factory map; otherwise TreeTypeAdapter's call to
-        // `Gson.getDelegateAdapter` would cause infinite recursion because it would keep returning the
-        // adapter specified by @JsonAdapter
         skipPast = TREE_TYPE_CLASS_DUMMY_FACTORY;
-        adapterFactoryMap.put(type.getRawType(), skipPast);
       } else {
-        // Use dummy `skipPast` value, but don't put it into factory map; this way `Gson.getDelegateAdapter`
-        // will return regular adapter for field type without actually skipping past any factory
         skipPast = TREE_TYPE_FIELD_DUMMY_FACTORY;
       }
       @SuppressWarnings({ "unchecked", "rawtypes" })
@@ -154,12 +151,19 @@ TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gso
 
   /**
    * Returns whether {@code factory} is a type adapter factory created for {@code @JsonAdapter}
-   * placed on {@code rawType}.
+   * placed on {@code type}.
    */
-  public boolean isClassJsonAdapterFactory(Class<?> rawType, TypeAdapterFactory factory) {
-    Objects.requireNonNull(rawType);
+  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`

gson/src/test/java/com/google/gson/GsonTest.java

@@ -310,6 +310,18 @@ class DummyFactory implements TypeAdapterFactory {
       public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
         return (TypeAdapter<T>) adapter;
       }
+
+      // Override equals to verify that reference equality check is performed by Gson,
+      // and this method is ignored
+      @Override
+      public boolean equals(Object obj) {
+        return obj instanceof DummyFactory && ((DummyFactory) obj).adapter.equals(adapter);
+      }
+
+      @Override
+      public int hashCode() {
+        return adapter.hashCode();
+      }
     }
 
     DummyAdapter adapter1 = new DummyAdapter(1);
@@ -334,6 +346,14 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
     assertThat(gson.getDelegateAdapter(factory2, type)).isEqualTo(adapter1);
     // Default Gson adapter should be returned
     assertThat(gson.getDelegateAdapter(factory1, type)).isNotInstanceOf(DummyAdapter.class);
+
+    DummyFactory factory1Eq = new DummyFactory(adapter1);
+    // Verify that test setup is correct
+    assertThat(factory1.equals(factory1Eq)).isTrue();
+    // Should only consider reference equality and ignore that custom `equals` method considers
+    // factories to be equal, therefore returning `adapter2` which came from `factory2` instead
+    // of skipping past `factory1`
+    assertThat(gson.getDelegateAdapter(factory1Eq, type)).isEqualTo(adapter2);
   }
 
   @Test

gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnClassesTest.java

@@ -283,17 +283,21 @@ private static final class D {
    */
   @Test
   public void testDelegatingAdapterFactory() {
-    WithDelegatingFactory deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class);
+    @SuppressWarnings("unchecked")
+    WithDelegatingFactory<String> deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class);
     assertThat(deserialized.f).isEqualTo("de");
 
-    WithDelegatingFactory serialized = new WithDelegatingFactory("se");
+    deserialized = new Gson().fromJson("{\"custom\":{\"f\":\"de\"}}", new TypeToken<WithDelegatingFactory<String>>() {});
+    assertThat(deserialized.f).isEqualTo("de");
+
+    WithDelegatingFactory<String> serialized = new WithDelegatingFactory<>("se");
     assertThat(new Gson().toJson(serialized)).isEqualTo("{\"custom\":{\"f\":\"se\"}}");
   }
   @JsonAdapter(WithDelegatingFactory.Factory.class)
-  private static class WithDelegatingFactory {
-    String f;
+  private static class WithDelegatingFactory<T> {
+    T f;
 
-    WithDelegatingFactory(String f) {
+    WithDelegatingFactory(T f) {
       this.f = f;
     }
 
@@ -396,10 +400,10 @@ public void testDelegating_SameFactoryClass() {
         .create();
 
     // Should use both factories, and therefore have `{"custom": ... }` twice
-    WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"custom\":{\"f\":\"de\"}}}", WithDelegatingFactory.class);
+    WithDelegatingFactory<?> deserialized = gson.fromJson("{\"custom\":{\"custom\":{\"f\":\"de\"}}}", WithDelegatingFactory.class);
     assertThat(deserialized.f).isEqualTo("de");
 
-    WithDelegatingFactory serialized = new WithDelegatingFactory("se");
+    WithDelegatingFactory<String> serialized = new WithDelegatingFactory<>("se");
     assertThat(gson.toJson(serialized)).isEqualTo("{\"custom\":{\"custom\":{\"f\":\"se\"}}}");
   }
 
@@ -425,10 +429,10 @@ public void testDelegating_SameFactoryInstance() {
     // or not, it can only work based on the `skipPast` factory, so if the same factory instance is used
     // the one registered with `GsonBuilder.registerTypeAdapterFactory` actually skips past the @JsonAdapter
     // one, so the JSON string is `{"custom": ...}` instead of `{"custom":{"custom":...}}`
-    WithDelegatingFactory deserialized = gson.fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class);
+    WithDelegatingFactory<?> deserialized = gson.fromJson("{\"custom\":{\"f\":\"de\"}}", WithDelegatingFactory.class);
     assertThat(deserialized.f).isEqualTo("de");
 
-    WithDelegatingFactory serialized = new WithDelegatingFactory("se");
+    WithDelegatingFactory<String> serialized = new WithDelegatingFactory<>("se");
     assertThat(gson.toJson(serialized)).isEqualTo("{\"custom\":{\"f\":\"se\"}}");
   }
 

gson/src/test/java/com/google/gson/functional/JsonAdapterAnnotationOnFieldsTest.java

@@ -477,17 +477,21 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
    */
   @Test
   public void testDelegatingAdapterFactory() {
-    WithDelegatingFactory deserialized = new Gson().fromJson("{\"f\":\"test\"}", WithDelegatingFactory.class);
+    @SuppressWarnings("unchecked")
+    WithDelegatingFactory<String> deserialized = new Gson().fromJson("{\"f\":\"test\"}", WithDelegatingFactory.class);
+    assertThat(deserialized.f).isEqualTo("test-custom");
+
+    deserialized = new Gson().fromJson("{\"f\":\"test\"}", new TypeToken<WithDelegatingFactory<String>>() {});
     assertThat(deserialized.f).isEqualTo("test-custom");
 
-    WithDelegatingFactory serialized = new WithDelegatingFactory();
+    WithDelegatingFactory<String> serialized = new WithDelegatingFactory<>();
     serialized.f = "value";
     assertThat(new Gson().toJson(serialized)).isEqualTo("{\"f\":\"value-custom\"}");
   }
-  private static class WithDelegatingFactory {
+  private static class WithDelegatingFactory<T> {
     @SuppressWarnings("SameNameButDifferent") // suppress Error Prone warning; should be clear that `Factory` refers to nested class
     @JsonAdapter(Factory.class)
-    String f;
+    T f;
 
     static class Factory implements TypeAdapterFactory {
       @SuppressWarnings("unchecked")