From b1c399fd6296f2e61de37109847c957bab42d736 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 19 Apr 2022 17:20:58 +0200 Subject: [PATCH] Improve `TypeToken` creation validation (#2072) * Add comments regarding multiple bounds of wildcard * Remove WildcardType check in getCollectionElementType The returned Type is never a wildcard due to the changes made to getSupertype by commit b1fb9ca9a1bea5440bc6a5b506ccf67236b08243. * Remove redundant getRawType call from MapTypeAdapterFactory getRawType(TypeToken.getType()) is the same as calling TypeToken.getRawType(). * Make TypeToken members private * Remove incorrect statement about TypeToken wildcards It is possible to use wildcards as part of the type argument, e.g.: `new TypeToken>() {}` * Only allow direct subclasses of TypeToken Previously subclasses of subclasses (...) of TypeToken were allowed which can behave incorrectly when retrieving the type argument, e.g.: class SubTypeToken extends TypeToken {} new SubTypeToken() {}.getType() This returned `String` despite the class extending TypeToken. * Throw exception when TypeToken captures type variable Due to type erasure the runtime type argument for a type variable is not available. Therefore there is no point in capturing a type variable and it might even give a false sense of type-safety. * Make $Gson$Types members private * Rename $Gson$Types.getGenericSupertype parameter Rename the method parameter to match the documentation of the method and to be similar to getSupertype(...). * Improve tests and handle raw TypeToken supertype better * Make some $Gson$Types members package-private again to prevent synthetic accessors * Remove TypeToken check for type variable As mentioned in review comments, there are cases during serialization where usage of the type variable is not so problematic (but still not ideal). --- .../com/google/gson/internal/$Gson$Types.java | 47 ++++++++------- .../internal/bind/MapTypeAdapterFactory.java | 3 +- .../com/google/gson/reflect/TypeToken.java | 41 ++++++++----- .../google/gson/reflect/TypeTokenTest.java | 59 ++++++++++++++++++- 4 files changed, 110 insertions(+), 40 deletions(-) diff --git a/gson/src/main/java/com/google/gson/internal/$Gson$Types.java b/gson/src/main/java/com/google/gson/internal/$Gson$Types.java index ee9cc72d..9891154a 100644 --- a/gson/src/main/java/com/google/gson/internal/$Gson$Types.java +++ b/gson/src/main/java/com/google/gson/internal/$Gson$Types.java @@ -154,7 +154,10 @@ public final class $Gson$Types { 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(); @@ -163,7 +166,7 @@ public final class $Gson$Types { } } - static boolean equal(Object a, Object b) { + private static boolean equal(Object a, Object b) { return a == b || (a != null && a.equals(b)); } @@ -225,10 +228,6 @@ public final class $Gson$Types { } } - 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(); } @@ -238,19 +237,19 @@ public final class $Gson$Types { * IntegerSet}, the result for when supertype is {@code Set.class} is {@code Set} and the * result when the supertype is {@code Collection.class} is {@code Collection}. */ - 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); } } } @@ -259,17 +258,17 @@ public final class $Gson$Types { 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; } /** @@ -279,10 +278,13 @@ public final class $Gson$Types { * * @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, @@ -306,9 +308,6 @@ public final class $Gson$Types { 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]; } @@ -440,7 +439,7 @@ public final class $Gson$Types { 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 @@ -522,6 +521,10 @@ public final class $Gson$Types { && $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() diff --git a/gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java b/gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java index 65e5789a..f7c5a554 100644 --- a/gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java +++ b/gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java @@ -120,8 +120,7 @@ public final class MapTypeAdapterFactory implements TypeAdapterFactory { 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 constructor = constructorConstructor.get(typeToken); diff --git a/gson/src/main/java/com/google/gson/reflect/TypeToken.java b/gson/src/main/java/com/google/gson/reflect/TypeToken.java index e122f407..b12d201f 100644 --- a/gson/src/main/java/com/google/gson/reflect/TypeToken.java +++ b/gson/src/main/java/com/google/gson/reflect/TypeToken.java @@ -37,17 +37,20 @@ import java.util.Map; *

* {@code TypeToken> list = new TypeToken>() {};} * - *

This syntax cannot be used to create type literals that have wildcard - * parameters, such as {@code Class} or {@code List}. + *

Capturing a type variable as type argument of a {@code TypeToken} should + * be avoided. Due to type erasure the runtime type of a type variable is not + * available to Gson and therefore it cannot provide the functionality one + * might expect, which gives a false sense of type-safety at compilation time + * and can lead to an unexpected {@code ClassCastException} at runtime. * * @author Bob Lee * @author Sven Mawson * @author Jesse Wilson */ public class TypeToken { - final Class rawType; - final Type type; - final int hashCode; + private final Class rawType; + private final Type type; + private final int hashCode; /** * Constructs a new type literal. Derives represented class from type @@ -59,7 +62,7 @@ public class TypeToken { */ @SuppressWarnings("unchecked") protected TypeToken() { - this.type = getSuperclassTypeParameter(getClass()); + this.type = getTypeTokenTypeArgument(); this.rawType = (Class) $Gson$Types.getRawType(type); this.hashCode = type.hashCode(); } @@ -68,23 +71,33 @@ public class 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) $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) { + return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]); + } } - ParameterizedType parameterized = (ParameterizedType) superclass; - return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]); + // 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"); } /** diff --git a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java index 40572716..1cdd7361 100644 --- a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java +++ b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java @@ -27,9 +27,8 @@ import junit.framework.TestCase; /** * @author Jesse Wilson */ -@SuppressWarnings({"deprecation"}) public final class TypeTokenTest extends TestCase { - + // These fields are accessed using reflection by the tests below List listOfInteger = null; List listOfNumber = null; List listOfString = null; @@ -37,6 +36,7 @@ public final class TypeTokenTest extends TestCase { List> listOfSetOfString = null; List> listOfSetOfUnknown = null; + @SuppressWarnings({"deprecation"}) public void testIsAssignableFromRawTypes() { assertTrue(TypeToken.get(Object.class).isAssignableFrom(String.class)); assertFalse(TypeToken.get(String.class).isAssignableFrom(Object.class)); @@ -44,6 +44,7 @@ public final class TypeTokenTest extends TestCase { assertFalse(TypeToken.get(ArrayList.class).isAssignableFrom(RandomAccess.class)); } + @SuppressWarnings({"deprecation"}) public void testIsAssignableFromWithTypeParameters() throws Exception { Type a = getClass().getDeclaredField("listOfInteger").getGenericType(); Type b = getClass().getDeclaredField("listOfNumber").getGenericType(); @@ -56,6 +57,7 @@ public final class TypeTokenTest extends TestCase { assertFalse(TypeToken.get(b).isAssignableFrom(a)); } + @SuppressWarnings({"deprecation"}) public void testIsAssignableFromWithBasicWildcards() throws Exception { Type a = getClass().getDeclaredField("listOfString").getGenericType(); Type b = getClass().getDeclaredField("listOfUnknown").getGenericType(); @@ -69,6 +71,7 @@ public final class TypeTokenTest extends TestCase { // assertTrue(TypeToken.get(b).isAssignableFrom(a)); } + @SuppressWarnings({"deprecation"}) public void testIsAssignableFromWithNestedWildcards() throws Exception { Type a = getClass().getDeclaredField("listOfSetOfString").getGenericType(); Type b = getClass().getDeclaredField("listOfSetOfUnknown").getGenericType(); @@ -102,4 +105,56 @@ public final class TypeTokenTest extends TestCase { Type listOfListOfString = TypeToken.getParameterized(List.class, listOfString).getType(); assertEquals(expectedListOfListOfListOfString, TypeToken.getParameterized(List.class, listOfListOfString)); } + + private static class CustomTypeToken extends TypeToken { + } + + public void testTypeTokenNonAnonymousSubclass() { + TypeToken typeToken = new CustomTypeToken(); + assertEquals(String.class, typeToken.getRawType()); + assertEquals(String.class, typeToken.getType()); + } + + /** + * User must only create direct subclasses of TypeToken, but not subclasses + * of subclasses (...) of TypeToken. + */ + public void testTypeTokenSubSubClass() { + class SubTypeToken extends TypeToken {} + class SubSubTypeToken1 extends SubTypeToken {} + class SubSubTypeToken2 extends SubTypeToken {} + + try { + new SubTypeToken() {}; + fail(); + } catch (IllegalStateException expected) { + assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage()); + } + + try { + new SubSubTypeToken1(); + fail(); + } catch (IllegalStateException expected) { + assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage()); + } + + try { + new SubSubTypeToken2(); + fail(); + } catch (IllegalStateException expected) { + assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage()); + } + } + + @SuppressWarnings("rawtypes") + public void testTypeTokenRaw() { + try { + new TypeToken() {}; + fail(); + } catch (IllegalStateException expected) { + assertEquals("TypeToken must be created with a type argument: new TypeToken<...>() {}; " + + "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.", + expected.getMessage()); + } + } }