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

Improve TypeToken creation validation #2072

Merged
merged 13 commits into from Apr 19, 2022
+110 −40
Merged

Improve TypeToken creation validation #2072

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f8b33f4
Add comments regarding multiple bounds of wildcard
Marcono1234 Jul 25, 2020
9faa6c3
Remove WildcardType check in getCollectionElementType
Marcono1234 Jul 25, 2020
074ec48
Remove redundant getRawType call from MapTypeAdapterFactory
Marcono1234 Jul 26, 2020
a264348
Make TypeToken members private
Marcono1234 Jul 29, 2020
0e69593
Remove incorrect statement about TypeToken wildcards
Marcono1234 Jul 29, 2020
02b7500
Only allow direct subclasses of TypeToken
Marcono1234 Jul 29, 2020
85159c4
Throw exception when TypeToken captures type variable
Marcono1234 Jul 29, 2020
11c1d05
Make $Gson$Types members private
Marcono1234 Jul 29, 2020
f853dbb
Rename $Gson$Types.getGenericSupertype parameter
Marcono1234 Jul 29, 2020
0530cde
Merge branch 'master' into marcono1234/TypeToken-improvements
Marcono1234 Feb 6, 2022
4436078
Improve tests and handle raw TypeToken supertype better
Marcono1234 Feb 6, 2022
446bf92
Make some $Gson$Types members package-private again to prevent synthe…
Marcono1234 Feb 6, 2022
cba0307
Remove TypeToken check for type variable
Marcono1234 Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 25 additions & 22 deletions gson/src/main/java/com/google/gson/internal/$Gson$Types.java
View file
Open in desktop
Expand Up @@ -154,7 +154,10 @@ public static Class<?> getRawType(Type type) {
return Object.class;

} else if (type instanceof WildcardType) {
return getRawType(((WildcardType) type).getUpperBounds()[0]);
Type[] bounds = ((WildcardType) type).getUpperBounds();
// Currently the JLS only permits one bound for wildcards so using first bound is safe
assert bounds.length == 1;
return getRawType(bounds[0]);

} else {
String className = type == null ? "null" : type.getClass().getName();
Expand All @@ -163,7 +166,7 @@ public static Class<?> getRawType(Type type) {
}
}

static boolean equal(Object a, Object b) {
private static boolean equal(Object a, Object b) {
return a == b || (a != null && a.equals(b));
}

Expand Down Expand Up @@ -225,10 +228,6 @@ public static boolean equals(Type a, Type b) {
}
}

static int hashCodeOrZero(Object o) {
return o != null ? o.hashCode() : 0;
}

public static String typeToString(Type type) {
return type instanceof Class ? ((Class<?>) type).getName() : type.toString();
}
Expand All @@ -238,19 +237,19 @@ public static String typeToString(Type type) {
* IntegerSet}, the result for when supertype is {@code Set.class} is {@code Set<Integer>} and the
* result when the supertype is {@code Collection.class} is {@code Collection<Integer>}.
*/
static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResolve) {
if (toResolve == rawType) {
private static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> supertype) {
if (supertype == rawType) {
return context;
}

// we skip searching through interfaces if unknown is an interface
if (toResolve.isInterface()) {
if (supertype.isInterface()) {
Class<?>[] interfaces = rawType.getInterfaces();
for (int i = 0, length = interfaces.length; i < length; i++) {
if (interfaces[i] == toResolve) {
if (interfaces[i] == supertype) {
return rawType.getGenericInterfaces()[i];
} else if (toResolve.isAssignableFrom(interfaces[i])) {
return getGenericSupertype(rawType.getGenericInterfaces()[i], interfaces[i], toResolve);
} else if (supertype.isAssignableFrom(interfaces[i])) {
return getGenericSupertype(rawType.getGenericInterfaces()[i], interfaces[i], supertype);
}
}
}
Expand All @@ -259,17 +258,17 @@ static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResol
if (!rawType.isInterface()) {
while (rawType != Object.class) {
Class<?> rawSupertype = rawType.getSuperclass();
if (rawSupertype == toResolve) {
if (rawSupertype == supertype) {
return rawType.getGenericSuperclass();
} else if (toResolve.isAssignableFrom(rawSupertype)) {
return getGenericSupertype(rawType.getGenericSuperclass(), rawSupertype, toResolve);
} else if (supertype.isAssignableFrom(rawSupertype)) {
return getGenericSupertype(rawType.getGenericSuperclass(), rawSupertype, supertype);
}
rawType = rawSupertype;
}
}

// we can't resolve this further
return toResolve;
return supertype;
}

/**
Expand All @@ -279,10 +278,13 @@ static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResol
*
* @param supertype a superclass of, or interface implemented by, this.
*/
static Type getSupertype(Type context, Class<?> contextRawType, Class<?> supertype) {
private static Type getSupertype(Type context, Class<?> contextRawType, Class<?> supertype) {
if (context instanceof WildcardType) {
// wildcards are useless for resolving supertypes. As the upper bound has the same raw type, use it instead
context = ((WildcardType)context).getUpperBounds()[0];
Type[] bounds = ((WildcardType)context).getUpperBounds();
// Currently the JLS only permits one bound for wildcards so using first bound is safe
assert bounds.length == 1;
context = bounds[0];
}
checkArgument(supertype.isAssignableFrom(contextRawType));
return resolve(context, contextRawType,
Expand All @@ -306,9 +308,6 @@ public static Type getArrayComponentType(Type array) {
public static Type getCollectionElementType(Type context, Class<?> contextRawType) {
Type collectionType = getSupertype(context, contextRawType, Collection.class);

if (collectionType instanceof WildcardType) {
collectionType = ((WildcardType)collectionType).getUpperBounds()[0];
}
if (collectionType instanceof ParameterizedType) {
return ((ParameterizedType) collectionType).getActualTypeArguments()[0];
}
Expand Down Expand Up @@ -440,7 +439,7 @@ private static Type resolve(Type context, Class<?> contextRawType, Type toResolv
return toResolve;
}

static Type resolveTypeVariable(Type context, Class<?> contextRawType, TypeVariable<?> unknown) {
private static Type resolveTypeVariable(Type context, Class<?> contextRawType, TypeVariable<?> unknown) {
Class<?> declaredByRaw = declaringClassOf(unknown);

// we can't reduce this further
Expand Down Expand Up @@ -522,6 +521,10 @@ public Type getOwnerType() {
&& $Gson$Types.equals(this, (ParameterizedType) other);
}

private static int hashCodeOrZero(Object o) {
return o != null ? o.hashCode() : 0;
}

@Override public int hashCode() {
return Arrays.hashCode(typeArguments)
^ rawType.hashCode()
Expand Down
3 changes: 1 addition & 2 deletions gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java
View file
Open in desktop
Expand Up @@ -120,8 +120,7 @@ public MapTypeAdapterFactory(ConstructorConstructor constructorConstructor,
return null;
}

Class<?> rawTypeOfSrc = $Gson$Types.getRawType(type);
Type[] keyAndValueTypes = $Gson$Types.getMapKeyAndValueTypes(type, rawTypeOfSrc);
Type[] keyAndValueTypes = $Gson$Types.getMapKeyAndValueTypes(type, rawType);
TypeAdapter<?> keyAdapter = getKeyAdapter(gson, keyAndValueTypes[0]);
TypeAdapter<?> valueAdapter = gson.getAdapter(TypeToken.get(keyAndValueTypes[1]));
ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);
Expand Down
69 changes: 54 additions & 15 deletions gson/src/main/java/com/google/gson/reflect/TypeToken.java
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.lang.reflect.TypeVariable;
import java.lang.reflect.WildcardType;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -37,17 +38,14 @@
* <p>
* {@code TypeToken<List<String>> list = new TypeToken<List<String>>() {};}
*
* <p>This syntax cannot be used to create type literals that have wildcard
* parameters, such as {@code Class<?>} or {@code List<? extends CharSequence>}.
*
* @author Bob Lee
* @author Sven Mawson
* @author Jesse Wilson
*/
public class TypeToken<T> {
final Class<? super T> rawType;
final Type type;
final int hashCode;
private final Class<? super T> rawType;
private final Type type;
private final int hashCode;

/**
* Constructs a new type literal. Derives represented class from type
Expand All @@ -56,10 +54,17 @@ public class TypeToken<T> {
* <p>Clients create an empty anonymous subclass. Doing so embeds the type
* parameter in the anonymous class's type hierarchy so we can reconstitute it
* at runtime despite erasure.
*
* <p>Because {@code TypeToken} is mainly intended for usage with Gson
* (and not other libraries) using a type variable as part of the type
* argument for {@code TypeToken} is not allowed. Due to type erasure the
* runtime type of a type variable is not available to Gson and therefore
* it cannot provide the functionality the user might expect, which would
* give a false sense of type-safety.
*/
@SuppressWarnings("unchecked")
protected TypeToken() {
this.type = getSuperclassTypeParameter(getClass());
this.type = getTypeTokenTypeArgument();
this.rawType = (Class<? super T>) $Gson$Types.getRawType(type);
this.hashCode = type.hashCode();
}
Expand All @@ -68,23 +73,57 @@ protected TypeToken() {
* Unsafe. Constructs a type literal manually.
*/
@SuppressWarnings("unchecked")
TypeToken(Type type) {
private TypeToken(Type type) {
this.type = $Gson$Types.canonicalize($Gson$Preconditions.checkNotNull(type));
this.rawType = (Class<? super T>) $Gson$Types.getRawType(this.type);
this.hashCode = this.type.hashCode();
}

/**
* Returns the type from super class's type parameter in {@link $Gson$Types#canonicalize
* Verifies that {@code this} is an instance of a direct subclass of TypeToken and
* returns the type argument for {@code T} in {@link $Gson$Types#canonicalize
* canonical form}.
*/
static Type getSuperclassTypeParameter(Class<?> subclass) {
Type superclass = subclass.getGenericSuperclass();
if (superclass instanceof Class) {
throw new RuntimeException("Missing type parameter.");
private Type getTypeTokenTypeArgument() {
Type superclass = getClass().getGenericSuperclass();
if (superclass instanceof ParameterizedType) {
ParameterizedType parameterized = (ParameterizedType) superclass;
if (parameterized.getRawType() == TypeToken.class) {
Type typeArgument = $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
verifyNoTypeVariable(typeArgument);
return typeArgument;
}
}
// Check for raw TypeToken as superclass
else if (superclass == TypeToken.class) {
throw new IllegalStateException("TypeToken must be created with a type argument: new TypeToken<...>() {}; "
+ "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
}

// User created subclass of subclass of TypeToken
throw new IllegalStateException("Must only create direct subclasses of TypeToken");
}

private static void verifyNoTypeVariable(Type type) {
if (type instanceof TypeVariable) {
TypeVariable<?> typeVariable = (TypeVariable<?>) type;
throw new IllegalArgumentException("TypeToken type argument must not contain a type variable; captured type variable "
Copy link
Member

Choose a reason for hiding this comment

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

If we were designing the code now, this would be a good check to make. In the deserialization case allowing a type variable allows a silent unchecked conversion. But there's a lot of existing client code which might be working perfectly well with these arguably-illegitimate captured type variables. To get an idea, I ran this change against all of Google's internal tests. There were two test failures due to this new exception. One of them looked like this, in outline:

public abstract class AbstractAction<D extends Dto, T extends Service<D>> implements Action {
  ...
  private final Type resultListType = new TypeToken<List<D>>() {}.getType();
  ...
  protected PlainTextResponse resultToJson(D result) {
    return new PlainTextResponse(GSON.toJson(result, getDtoClass()));
  }

  protected PlainTextResponse resultToJson(List<D> result) {
    return new PlainTextResponse(GSON.toJson(result, resultListType));
  }

  public Class<D> getDtoClass() {
    throw new UnsupportedOperationException("not implemented yet");
  }
}

Then subclasses implement getDtoClass() appropriately.

I think this code would work just as well if the declaration were this:

  private final Type resultListType = new TypeToken<List<? extends Dto>>() {}.getType();

However I also think the code is correct as written. It wouldn't be correct if it were trying to deserialize a List<D>, but here apparently the code only serializes.

So I think this change breaks some correct code, and I don't feel we can justify that.

(The other test failure was deserializing, but it also apparently worked despite being unsound.)

The rest of the PR looks like a good set of improvements, if you want to just remove the verifyNoTypeVariable part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even serialization can be broken due to this. For your case let's imagine there is a Dto subclass which has a custom type adapter. Then serializing it with resultToJson(D) works fine, but resultToJson(List<D>) erroneously uses the adapter for Dto instead of D.

The current situation is really unfortunate... I think it would be really useful to have the type variable check. For this PR I can omit it, but I am not sure about completely giving up on that check.

Luckily if the type variable has no bounds, then ObjectTypeAdapter uses the runtime type adapter, so at least in these cases it is probably not causing such big issues (unless a user explicitly wanted to serialize as supertype T instead of using the runtime type, if they have separate adapters).

As side note: I would recommend changing your AbstractAction code to construct the resultListType using Gson's TypeToken.getParameterized(Type, Type...) (if possible), that would fix all issues with it. Sorry if that is a bit presumptuous 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eamonnmcmanus, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

My assumption is that if Google's code includes two unrelated tests that fail with this check, other people's code might too. I can fix the Google ones but not the other ones. I'm just not feeling that this is a worthwhile check to make now, even though (as I said) I completely agree that it would have made sense if it had always been present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this stricter validation be considered for a future version again? As pointed out above, most likely all cases where a TypeToken captures a type variable are not safe, potentially in very subtle ways.

Just recently two Stack Overflow questions were created where the author stumbled exactly over this issue:

+ typeVariable.getName() + " declared by " + typeVariable.getGenericDeclaration());
} else if (type instanceof GenericArrayType) {
verifyNoTypeVariable(((GenericArrayType) type).getGenericComponentType());
} else if (type instanceof ParameterizedType) {
for (Type typeArgument : ((ParameterizedType) type).getActualTypeArguments()) {
verifyNoTypeVariable(typeArgument);
}
} else if (type instanceof WildcardType) {
WildcardType wildcardType = (WildcardType) type;
for (Type bound : wildcardType.getLowerBounds()) {
verifyNoTypeVariable(bound);
}
for (Type bound : wildcardType.getUpperBounds()) {
verifyNoTypeVariable(bound);
}
}
ParameterizedType parameterized = (ParameterizedType) superclass;
return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
}

/**
Expand Down