From a589ef20087b4b0f1ec3048d3ceaef1eedccd09d Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 24 Jun 2023 00:24:23 +0200 Subject: [PATCH] Improve creation of `ParameterizedType` (#2375) - Reject non-generic raw types for TypeToken.getParameterized - Fix ParameterizedTypeImpl erroneously requiring owner type for types without owner --- .../com/google/gson/internal/$Gson$Types.java | 33 ++-- .../com/google/gson/reflect/TypeToken.java | 22 ++- .../google/gson/internal/GsonTypesTest.java | 32 +++- .../google/gson/reflect/TypeTokenTest.java | 164 ++++++++---------- 4 files changed, 132 insertions(+), 119 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 4a925aa49b..470e67dfd9 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 @@ -33,8 +33,8 @@ import java.util.HashMap; import java.util.Map; import java.util.NoSuchElementException; -import java.util.Properties; import java.util.Objects; +import java.util.Properties; /** * Static methods for working with types. @@ -138,9 +138,8 @@ public static Class getRawType(Type type) { } else if (type instanceof ParameterizedType) { ParameterizedType parameterizedType = (ParameterizedType) type; - // I'm not exactly sure why getRawType() returns Type instead of Class. - // Neal isn't either but suspects some pathological case related - // to nested classes exists. + // getRawType() returns Type instead of Class; that seems to be an API mistake, + // see https://bugs.openjdk.org/browse/JDK-8250659 Type rawType = parameterizedType.getRawType(); checkArgument(rawType instanceof Class); return (Class) rawType; @@ -481,19 +480,33 @@ static void checkNotPrimitive(Type type) { checkArgument(!(type instanceof Class) || !((Class) type).isPrimitive()); } + /** + * Whether an {@linkplain ParameterizedType#getOwnerType() owner type} must be specified when + * constructing a {@link ParameterizedType} for {@code rawType}. + * + *

Note that this method might not require an owner type for all cases where Java reflection + * would create parameterized types with owner type. + */ + public static boolean requiresOwnerType(Type rawType) { + if (rawType instanceof Class) { + Class rawTypeAsClass = (Class) rawType; + return !Modifier.isStatic(rawTypeAsClass.getModifiers()) + && rawTypeAsClass.getDeclaringClass() != null; + } + return false; + } + private static final class ParameterizedTypeImpl implements ParameterizedType, Serializable { private final Type ownerType; private final Type rawType; private final Type[] typeArguments; public ParameterizedTypeImpl(Type ownerType, Type rawType, Type... typeArguments) { + // TODO: Should this enforce that rawType is a Class? See JDK implementation of + // the ParameterizedType interface and https://bugs.openjdk.org/browse/JDK-8250659 requireNonNull(rawType); - // require an owner type if the raw type needs it - if (rawType instanceof Class) { - Class rawTypeAsClass = (Class) rawType; - boolean isStaticOrTopLevelClass = Modifier.isStatic(rawTypeAsClass.getModifiers()) - || rawTypeAsClass.getEnclosingClass() == null; - checkArgument(ownerType != null || isStaticOrTopLevelClass); + if (ownerType == null && requiresOwnerType(rawType)) { + throw new IllegalArgumentException("Must specify owner type for " + rawType); } this.ownerType = ownerType == null ? null : canonicalize(ownerType); 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 f4a9c0d943..4a695666c8 100644 --- a/gson/src/main/java/com/google/gson/reflect/TypeToken.java +++ b/gson/src/main/java/com/google/gson/reflect/TypeToken.java @@ -338,8 +338,8 @@ public static TypeToken get(Class type) { * and care must be taken to pass in the correct number of type arguments. * * @throws IllegalArgumentException - * If {@code rawType} is not of type {@code Class}, or if the type arguments are invalid for - * the raw type + * If {@code rawType} is not of type {@code Class}, if it is not a generic type, or if the + * type arguments are invalid for the raw type */ public static TypeToken getParameterized(Type rawType, Type... typeArguments) { Objects.requireNonNull(rawType); @@ -354,6 +354,18 @@ public static TypeToken getParameterized(Type rawType, Type... typeArguments) Class rawClass = (Class) rawType; TypeVariable[] typeVariables = rawClass.getTypeParameters(); + // Note: Does not check if owner type of rawType is generic because this factory method + // does not support specifying owner type + if (typeVariables.length == 0) { + throw new IllegalArgumentException(rawClass.getName() + " is not a generic type"); + } + + // Check for this here to avoid misleading exception thrown by ParameterizedTypeImpl + if ($Gson$Types.requiresOwnerType(rawType)) { + throw new IllegalArgumentException("Raw type " + rawClass.getName() + " is not supported because" + + " it requires specifying an owner type"); + } + int expectedArgsCount = typeVariables.length; int actualArgsCount = typeArguments.length; if (actualArgsCount != expectedArgsCount) { @@ -362,7 +374,7 @@ public static TypeToken getParameterized(Type rawType, Type... typeArguments) } for (int i = 0; i < expectedArgsCount; i++) { - Type typeArgument = typeArguments[i]; + Type typeArgument = Objects.requireNonNull(typeArguments[i], "Type argument must not be null"); Class rawTypeArgument = $Gson$Types.getRawType(typeArgument); TypeVariable typeVariable = typeVariables[i]; @@ -370,8 +382,8 @@ public static TypeToken getParameterized(Type rawType, Type... typeArguments) Class rawBound = $Gson$Types.getRawType(bound); if (!rawBound.isAssignableFrom(rawTypeArgument)) { - throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds " - + "for type variable " + typeVariable + " declared by " + rawType); + throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds" + + " for type variable " + typeVariable + " declared by " + rawType); } } } diff --git a/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java b/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java index af824975c5..0c5adde541 100644 --- a/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java +++ b/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java @@ -17,7 +17,7 @@ package com.google.gson.internal; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertThrows; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; @@ -29,20 +29,33 @@ public final class GsonTypesTest { @Test public void testNewParameterizedTypeWithoutOwner() throws Exception { // List. List is a top-level class - Type type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class); - assertThat(getFirstTypeArgument(type)).isEqualTo(A.class); + ParameterizedType type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class); + assertThat(type.getOwnerType()).isNull(); + assertThat(type.getRawType()).isEqualTo(List.class); + assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class); // A. A is a static inner class. type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, B.class); assertThat(getFirstTypeArgument(type)).isEqualTo(B.class); + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + // NonStaticInner is not allowed without owner + () -> $Gson$Types.newParameterizedTypeWithOwner(null, NonStaticInner.class, A.class)); + assertThat(e).hasMessageThat().isEqualTo("Must specify owner type for " + NonStaticInner.class); + + type = $Gson$Types.newParameterizedTypeWithOwner(GsonTypesTest.class, NonStaticInner.class, A.class); + assertThat(type.getOwnerType()).isEqualTo(GsonTypesTest.class); + assertThat(type.getRawType()).isEqualTo(NonStaticInner.class); + assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class); + final class D { } - try { - // D is not allowed since D is not a static inner class - $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class); - fail(); - } catch (IllegalArgumentException expected) {} + + // D is allowed since D has no owner type + type = $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class); + assertThat(type.getOwnerType()).isNull(); + assertThat(type.getRawType()).isEqualTo(D.class); + assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class); // A is allowed. type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, D.class); @@ -63,6 +76,9 @@ private static final class B { } private static final class C { } + @SuppressWarnings({"ClassCanBeStatic", "UnusedTypeParameter"}) + private final class NonStaticInner { + } /** * Given a parameterized type A<B,C>, returns B. If the specified type is not 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 f727a87a6e..d38f05b1ec 100644 --- a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java +++ b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java @@ -17,9 +17,10 @@ package com.google.gson.reflect; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertThrows; import java.lang.reflect.GenericArrayType; +import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.ArrayList; import java.util.List; @@ -103,11 +104,13 @@ public void testArrayFactory() { Type listOfString = new TypeToken>() {}.getType(); assertThat(TypeToken.getArray(listOfString)).isEqualTo(expectedListOfStringArray); - try { - TypeToken.getArray(null); - fail(); - } catch (NullPointerException e) { - } + TypeToken expectedIntArray = new TypeToken() {}; + assertThat(TypeToken.getArray(int.class)).isEqualTo(expectedIntArray); + + assertThrows(NullPointerException.class, () -> TypeToken.getArray(null)); + } + + static class NestedGeneric { } @Test @@ -131,84 +134,70 @@ public void testParameterizedFactory() { TypeToken expectedSatisfyingTwoBounds = new TypeToken>() {}; assertThat(TypeToken.getParameterized(GenericWithMultiBound.class, ClassSatisfyingBounds.class)).isEqualTo(expectedSatisfyingTwoBounds); + + TypeToken nestedTypeToken = TypeToken.getParameterized(NestedGeneric.class, Integer.class); + ParameterizedType nestedParameterizedType = (ParameterizedType) nestedTypeToken.getType(); + // TODO: This seems to differ from how Java reflection behaves; when using TypeToken>, + // then NestedGeneric does have an owner type + assertThat(nestedParameterizedType.getOwnerType()).isNull(); + assertThat(nestedParameterizedType.getRawType()).isEqualTo(NestedGeneric.class); + assertThat(nestedParameterizedType.getActualTypeArguments()).asList().containsExactly(Integer.class); + + class LocalGenericClass {} + TypeToken expectedLocalType = new TypeToken>() {}; + assertThat(TypeToken.getParameterized(LocalGenericClass.class, Integer.class)).isEqualTo(expectedLocalType); } @Test public void testParameterizedFactory_Invalid() { - try { - TypeToken.getParameterized(null, new Type[0]); - fail(); - } catch (NullPointerException e) { - } + assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(null, new Type[0])); + assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(List.class, new Type[] { null })); GenericArrayType arrayType = (GenericArrayType) TypeToken.getArray(String.class).getType(); - try { - TypeToken.getParameterized(arrayType, new Type[0]); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]"); - } + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(arrayType, new Type[0])); + assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]"); - try { - TypeToken.getParameterized(String.class, String.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("java.lang.String requires 0 type arguments, but got 1"); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(String.class, Number.class)); + assertThat(e).hasMessageThat().isEqualTo("java.lang.String is not a generic type"); - try { - TypeToken.getParameterized(List.class, new Type[0]); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0"); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, new Type[0])); + assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0"); - try { - TypeToken.getParameterized(List.class, String.class, String.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2"); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, String.class, String.class)); + assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2"); - try { - TypeToken.getParameterized(GenericWithBound.class, String.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds " - + "for type variable T declared by " + GenericWithBound.class); - } + // Primitive types must not be used as type argument + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, int.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument int does not satisfy bounds" + + " for type variable E declared by " + List.class); - try { - TypeToken.getParameterized(GenericWithBound.class, Object.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds " - + "for type variable T declared by " + GenericWithBound.class); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, String.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds" + + " for type variable T declared by " + GenericWithBound.class); - try { - TypeToken.getParameterized(GenericWithMultiBound.class, Number.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds " - + "for type variable T declared by " + GenericWithMultiBound.class); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, Object.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds" + + " for type variable T declared by " + GenericWithBound.class); - try { - TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds " - + "for type variable T declared by " + GenericWithMultiBound.class); - } + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Number.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds" + + " for type variable T declared by " + GenericWithMultiBound.class); + + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds" + + " for type variable T declared by " + GenericWithMultiBound.class); + + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Object.class)); + assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds" + + " for type variable T declared by " + GenericWithMultiBound.class); - try { - TypeToken.getParameterized(GenericWithMultiBound.class, Object.class); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds " - + "for type variable T declared by " + GenericWithMultiBound.class); + class Outer { + class NonStaticInner {} } + + e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(Outer.NonStaticInner.class, Object.class)); + assertThat(e).hasMessageThat().isEqualTo("Raw type " + Outer.NonStaticInner.class.getName() + + " is not supported because it requires specifying an owner type"); } private static class CustomTypeToken extends TypeToken { @@ -231,40 +220,23 @@ class SubTypeToken extends TypeToken {} class SubSubTypeToken1 extends SubTypeToken {} class SubSubTypeToken2 extends SubTypeToken {} - try { - new SubTypeToken() {}; - fail(); - } catch (IllegalStateException expected) { - assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); - } + IllegalStateException e = assertThrows(IllegalStateException.class, () -> new SubTypeToken() {}); + assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); - try { - new SubSubTypeToken1(); - fail(); - } catch (IllegalStateException expected) { - assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); - } + e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken1()); + assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); - try { - new SubSubTypeToken2(); - fail(); - } catch (IllegalStateException expected) { - assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); - } + e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken2()); + assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken"); } @SuppressWarnings("rawtypes") @Test public void testTypeTokenRaw() { - try { - new TypeToken() {}; - fail(); - } catch (IllegalStateException expected) { - assertThat(expected).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};" - + " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved." - + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw" - ); - } + IllegalStateException e = assertThrows(IllegalStateException.class, () -> new TypeToken() {}); + assertThat(e).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};" + + " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved." + + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw"); } }