Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ConstructorConstructor creating mismatching Collection and Map instances #2068

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix ConstructorConstructor creating mismatching Collection and Map instances #2068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5b83a03
Fix ConstructorConstructor creating mismatching Collection instances
Marcono1234 Jan 29, 2022
d5d7cdd
Support EnumMap deserialization
Marcono1234 Jan 29, 2022
c6aa3b1
Use LinkedTreeMap for maps with String supertype as key
Marcono1234 May 17, 2022
2b6fa06
Merge branch 'master' into marcono1234/Collection-default-instance-cr…
Marcono1234 May 17, 2022
6a63611
Fix test method name typo
Marcono1234 May 17, 2022
9952bda
Merge 'main' & some small changes
Marcono1234 Sep 5, 2024
1f496d7
Fix `ProtoTypeAdapter` being unable to deserialize certain repeated f…
Marcono1234 Sep 6, 2024
f8ca948
Adjust tests
Marcono1234 Sep 6, 2024
e70a754
Don't use LinkedTreeMap for String supertypes as key & small test fix
Marcono1234 Sep 7, 2024
86b13f2
Fix typo in exception message
Marcono1234 Sep 12, 2024
ae94ae8
Merge remote-tracking branch 'remotes/origin/main' into marcono1234/C…
Marcono1234 Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions gson/src/main/java/com/google/gson/Gson.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,8 +1106,7 @@ public JsonReader newJsonReader(Reader reader) {
* @see #fromJson(String, TypeToken)
*/
public <T> T fromJson(String json, Class<T> classOfT) throws JsonSyntaxException {
T object = fromJson(json, TypeToken.get(classOfT));
return Primitives.wrap(classOfT).cast(object);
return fromJson(json, TypeToken.get(classOfT));
}

/**
Expand Down Expand Up @@ -1198,8 +1197,7 @@ public <T> T fromJson(String json, TypeToken<T> typeOfT) throws JsonSyntaxExcept
*/
public <T> T fromJson(Reader json, Class<T> classOfT)
throws JsonSyntaxException, JsonIOException {
T object = fromJson(json, TypeToken.get(classOfT));
return Primitives.wrap(classOfT).cast(object);
return fromJson(json, TypeToken.get(classOfT));
}

/**
Expand Down Expand Up @@ -1360,7 +1358,19 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
JsonToken unused = reader.peek();
isEmpty = false;
TypeAdapter<T> typeAdapter = getAdapter(typeOfT);
return typeAdapter.read(reader);
T object = typeAdapter.read(reader);
Class<?> expectedTypeWrapped = Primitives.wrap(typeOfT.getRawType());
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems all other fromJson methods delegate to this one, so I have added the type check (which was previously Primitives.wrap(classOfT).cast(object) in the other fromJson methods) here instead.
However, this method did not actually have this check before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this change against all of Google's internal tests and found a lot of failures with this exception, or with an exception about not being able to instantiate the abstract class com.google.common.collect.ImmutableList. In both cases I think the code in question should be fixed, because it is relying on accidental behaviour. A pattern I saw quite often was this:

@AutoValue
public abstract Foo {
  public abstract ImmutableList<Bar> bars();

  public static TypeAdapter<Foo> typeAdapter(Gson gson) {
    return new AutoValue_Foo.GsonTypeAdapter(gson);
  }

  @AutoValue.Builder
  public interface Builder {
    public Builder setBars(List<Bar> bars);
    public Foo build();
  }
}

This is using auto-value-gson to serialize and deserialize AutoValue classes. It works even if no TypeAdapterFactory has been registered for ImmutableList, because Gson deserializes a plain List. Auto-value-gson constructs the Foo instance using its builder, and the deserialized List works with the setBars method. That method calls ImmutableList.copyOf(List) so we do get the right ImmutableList type. But the additional checks in this PR don't let us get that far.

As I say, code like that only works accidentally. My plan is to add a GuavaCollectionsTypeAdapterFactory to extras and update the internal code to use that. I already found several custom ImmutableListTypeAdapterFactory and ImmutableMapTypeAdapterFactory classes which could be switched to use this.

Some of the other failures were just plain using the wrong types, which again happened to work through erasure and the like.

So in short, I think this change is OK, but it may require some client code to be updated. Most of the updates should be straightforward and will lead to client code that is more correct.

I think it will be a while before we can complete this because of all the adjustments that will need to be made.

if (object != null && !expectedTypeWrapped.isInstance(object)) {
throw new ClassCastException(
"Type adapter '"
Comment on lines +1364 to +1365
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not completely sure if this custom ClassCastException is really worth it, maybe a Class#cast call would suffice, as done before.

This check will only help when directly using fromJson for an incorrect adapter, but if the adapter is used indirectly (for example for a list value) it would still cause other less descriptive exceptions later.

+ typeAdapter
+ "' returned wrong type; requested "
+ typeOfT.getRawType()
+ " but got instance of "
+ object.getClass()
+ "\nVerify that the adapter was registered for the correct type.");
}
return object;
} catch (EOFException e) {
/*
* For compatibility with JSON 1.5 and earlier, we return null for empty
Expand Down Expand Up @@ -1405,8 +1415,7 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
* @see #fromJson(JsonElement, TypeToken)
*/
public <T> T fromJson(JsonElement json, Class<T> classOfT) throws JsonSyntaxException {
T object = fromJson(json, TypeToken.get(classOfT));
return Primitives.wrap(classOfT).cast(object);
return fromJson(json, TypeToken.get(classOfT));
}

/**
Expand Down
198 changes: 124 additions & 74 deletions gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,9 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.Set;
import java.util.SortedMap;
import java.util.SortedSet;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentNavigableMap;
import java.util.concurrent.ConcurrentSkipListMap;

/** Returns a function that can construct an instance of a requested type. */
Expand Down Expand Up @@ -321,7 +315,6 @@ public T construct() {
}

/** Constructors for common interface types like Map and List and their subtypes. */
@SuppressWarnings("unchecked") // use runtime checks to guarantee that 'T' is what it is
private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
final Type type, Class<? super T> rawType) {

Expand All @@ -334,78 +327,135 @@ private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
*/

if (Collection.class.isAssignableFrom(rawType)) {
if (SortedSet.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new TreeSet<>();
}
};
} else if (Set.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new LinkedHashSet<>();
}
};
} else if (Queue.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new ArrayDeque<>();
}
};
} else {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new ArrayList<>();
}
};
}
@SuppressWarnings("unchecked")
ObjectConstructor<T> constructor = (ObjectConstructor<T>) newCollectionConstructor(rawType);
return constructor;
}

if (Map.class.isAssignableFrom(rawType)) {
if (ConcurrentNavigableMap.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new ConcurrentSkipListMap<>();
}
};
} else if (ConcurrentMap.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new ConcurrentHashMap<>();
}
};
} else if (SortedMap.class.isAssignableFrom(rawType)) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new TreeMap<>();
}
};
} else if (type instanceof ParameterizedType
&& !String.class.isAssignableFrom(
TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType())) {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new LinkedHashMap<>();
}
};
} else {
return new ObjectConstructor<T>() {
@Override
public T construct() {
return (T) new LinkedTreeMap<>();
}
};
}
@SuppressWarnings("unchecked")
ObjectConstructor<T> constructor = (ObjectConstructor<T>) newMapConstructor(type, rawType);
return constructor;
}

// Unsupported type; try other means of creating constructor
return null;
Comment on lines +341 to +342
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #419 (comment) the concern was mentioned that the fallback will be Unsafe which might leave the Collection or Map implementation in a broken state because fields have not been initialized. And the built-in Collection and Map adapters might then encounter confusing exceptions when trying to add elements.

Not sure if we should throw an exception then in case of Collection or Map? Or we could maybe add an allowUnsafe parameter here (which is considered in addition to GsonBuilder#disableJdkUnsafe), which the built-in Collection and Map adapters can set to false because calling add and put will likely fail afterwards.

}

// Suppress Error Prone false positive: Pattern is reported for overridden methods, see
// https://github.com/google/error-prone/issues/4563
@SuppressWarnings("NonApiType")
private static ObjectConstructor<? extends Collection<? extends Object>> newCollectionConstructor(
Class<?> rawType) {

// First try List implementation
if (rawType.isAssignableFrom(ArrayList.class)) {
return new ObjectConstructor<ArrayList<Object>>() {
@Override
public ArrayList<Object> construct() {
return new ArrayList<>();
}
};
}
// Then try Set implementation
else if (rawType.isAssignableFrom(LinkedHashSet.class)) {
return new ObjectConstructor<LinkedHashSet<Object>>() {
@Override
public LinkedHashSet<Object> construct() {
return new LinkedHashSet<>();
}
};
}
// Then try SortedSet / NavigableSet implementation
else if (rawType.isAssignableFrom(TreeSet.class)) {
return new ObjectConstructor<TreeSet<Object>>() {
@Override
public TreeSet<Object> construct() {
return new TreeSet<>();
}
};
}
// Then try Queue implementation
else if (rawType.isAssignableFrom(ArrayDeque.class)) {
return new ObjectConstructor<ArrayDeque<Object>>() {
@Override
public ArrayDeque<Object> construct() {
return new ArrayDeque<>();
}
};
}

// Was unable to create matching Collection constructor
return null;
}

private static boolean hasStringKeyType(Type mapType) {
// If mapType is not parameterized, assume it might have String as key type
if (!(mapType instanceof ParameterizedType)) {
return true;
}

Type[] typeArguments = ((ParameterizedType) mapType).getActualTypeArguments();
if (typeArguments.length == 0) {
return false;
}
return $Gson$Types.getRawType(typeArguments[0]) == String.class;
}

// Suppress Error Prone false positive: Pattern is reported for overridden methods, see
// https://github.com/google/error-prone/issues/4563
@SuppressWarnings("NonApiType")
private static ObjectConstructor<? extends Map<? extends Object, Object>> newMapConstructor(
final Type type, Class<?> rawType) {
// First try Map implementation
/*
* Legacy special casing for Map<String, ...> to avoid DoS from colliding String hashCode
* values for older JDKs; use own LinkedTreeMap<String, Object> instead
*/
if (rawType.isAssignableFrom(LinkedTreeMap.class) && hasStringKeyType(type)) {
return new ObjectConstructor<LinkedTreeMap<Object, Object>>() {
@Override
public LinkedTreeMap<Object, Object> construct() {
return new LinkedTreeMap<>();
}
};
} else if (rawType.isAssignableFrom(LinkedHashMap.class)) {
return new ObjectConstructor<LinkedHashMap<Object, Object>>() {
@Override
public LinkedHashMap<Object, Object> construct() {
return new LinkedHashMap<>();
}
};
}
// Then try SortedMap / NavigableMap implementation
else if (rawType.isAssignableFrom(TreeMap.class)) {
return new ObjectConstructor<TreeMap<Object, Object>>() {
@Override
public TreeMap<Object, Object> construct() {
return new TreeMap<>();
}
};
}
// Then try ConcurrentMap implementation
else if (rawType.isAssignableFrom(ConcurrentHashMap.class)) {
return new ObjectConstructor<ConcurrentHashMap<Object, Object>>() {
@Override
public ConcurrentHashMap<Object, Object> construct() {
return new ConcurrentHashMap<>();
}
};
}
// Then try ConcurrentNavigableMap implementation
else if (rawType.isAssignableFrom(ConcurrentSkipListMap.class)) {
return new ObjectConstructor<ConcurrentSkipListMap<Object, Object>>() {
@Override
public ConcurrentSkipListMap<Object, Object> construct() {
return new ConcurrentSkipListMap<>();
}
};
}

// Was unable to create matching Map constructor
return null;
}

Expand Down
55 changes: 55 additions & 0 deletions gson/src/test/java/com/google/gson/GsonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,61 @@ public Object read(JsonReader in) {
}
}

@Test
public void testFromJson_WrongResultType() {
class IntegerAdapter extends TypeAdapter<Integer> {
@Override
public Integer read(JsonReader in) throws IOException {
in.skipValue();
return 3;
}

@Override
public void write(JsonWriter out, Integer value) {
throw new AssertionError("not needed for test");
}

@Override
public String toString() {
return "custom-adapter";
}
}

Gson gson = new GsonBuilder().registerTypeAdapter(Boolean.class, new IntegerAdapter()).create();
// Use `Class<?>` here to avoid that the JVM itself creates the ClassCastException (though the
// check below for the custom message would detect that as well)
Class<?> deserializedClass = Boolean.class;
var exception =
assertThrows(ClassCastException.class, () -> gson.fromJson("true", deserializedClass));
assertThat(exception)
.hasMessageThat()
.isEqualTo(
"Type adapter 'custom-adapter' returned wrong type; requested class java.lang.Boolean"
+ " but got instance of class java.lang.Integer\n"
+ "Verify that the adapter was registered for the correct type.");

// Returning boxed primitive should be allowed (e.g. returning `Integer` for `int`)
Gson gson2 = new GsonBuilder().registerTypeAdapter(int.class, new IntegerAdapter()).create();
assertThat(gson2.fromJson("0", int.class)).isEqualTo(3);

class NullAdapter extends TypeAdapter<Object> {
@Override
public Object read(JsonReader in) throws IOException {
in.skipValue();
return null;
}

@Override
public void write(JsonWriter out, Object value) {
throw new AssertionError("not needed for test");
}
}

// Returning `null` should be allowed
Gson gson3 = new GsonBuilder().registerTypeAdapter(Boolean.class, new NullAdapter()).create();
assertThat(gson3.fromJson("true", Boolean.class)).isNull();
}

@Test
public void testGetAdapter_Null() {
Gson gson = new Gson();
Expand Down
Loading