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.

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

Validate TypeToken.getParameterized arguments #2166

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 5 additions & 3 deletions gson/src/main/java/com/google/gson/internal/$Gson$Types.java
Expand Up @@ -16,6 +16,9 @@

package com.google.gson.internal;

import static com.google.gson.internal.$Gson$Preconditions.checkArgument;
import static com.google.gson.internal.$Gson$Preconditions.checkNotNull;

import java.io.Serializable;
import java.lang.reflect.Array;
import java.lang.reflect.GenericArrayType;
Expand All @@ -32,9 +35,6 @@
import java.util.NoSuchElementException;
import java.util.Properties;

import static com.google.gson.internal.$Gson$Preconditions.checkArgument;
import static com.google.gson.internal.$Gson$Preconditions.checkNotNull;

/**
* Static methods for working with types.
*
Expand Down Expand Up @@ -486,6 +486,7 @@ private static final class ParameterizedTypeImpl implements ParameterizedType, S
private final Type[] typeArguments;

public ParameterizedTypeImpl(Type ownerType, Type rawType, Type... typeArguments) {
checkNotNull(rawType);
// require an owner type if the raw type needs it
if (rawType instanceof Class<?>) {
Class<?> rawTypeAsClass = (Class<?>) rawType;
Expand Down Expand Up @@ -552,6 +553,7 @@ private static final class GenericArrayTypeImpl implements GenericArrayType, Ser
private final Type componentType;

public GenericArrayTypeImpl(Type componentType) {
checkNotNull(componentType);
this.componentType = canonicalize(componentType);
}

Expand Down
40 changes: 39 additions & 1 deletion gson/src/main/java/com/google/gson/reflect/TypeToken.java
Expand Up @@ -16,8 +16,8 @@

package com.google.gson.reflect;

import com.google.gson.internal.$Gson$Types;
import com.google.gson.internal.$Gson$Preconditions;
import com.google.gson.internal.$Gson$Types;
import java.lang.reflect.GenericArrayType;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
Expand Down Expand Up @@ -319,8 +319,46 @@ public static <T> TypeToken<T> get(Class<T> type) {
/**
* Gets type literal for the parameterized type represented by applying {@code typeArguments} to
* {@code rawType}.
*
* @throws IllegalArgumentException
* If {@code rawType} is not of type {@code Class}, or if the type arguments are invalid for
* the raw type
*/
public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments) {
$Gson$Preconditions.checkNotNull(rawType);
$Gson$Preconditions.checkNotNull(typeArguments);

// Perform basic validation here because this is the only public API where users
// can create malformed parameterized types
if (!(rawType instanceof Class)) {
// See also https://bugs.openjdk.org/browse/JDK-8250659
Copy link
Member

Choose a reason for hiding this comment

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

Grumble. I wish we hadn't duplicated this mistake.

throw new IllegalArgumentException("rawType must be of type Class, but was " + rawType);
}
Class<?> rawClass = (Class<?>) rawType;
TypeVariable<?>[] typeVariables = rawClass.getTypeParameters();

int expectedArgsCount = typeVariables.length;
int actualArgsCount = typeArguments.length;
if (actualArgsCount != expectedArgsCount) {
throw new IllegalArgumentException(rawClass.getName() + " requires " + expectedArgsCount +
" type arguments, but got " + actualArgsCount);
}

for (int i = 0; i < expectedArgsCount; i++) {
Type typeArgument = typeArguments[i];
Class<?> rawTypeArgument = $Gson$Types.getRawType(typeArgument);
TypeVariable<?> typeVariable = typeVariables[i];

for (Type bound : typeVariable.getBounds()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about this. Suppose you have interface NumberSet<E extends Number> extends Set<E>. Then Java will allow you to write NumberSet<?>, which effectively means NumberSet<? extends Number>. Would people expect to be able to write TypeToken.getParameterized(NumberSet.class, wildcard) where wildcard is a Type representing ?? I may well be overthinking this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I did not consider that. Though users probably cannot easily obtain a WildcardType, and additionally Gson's current type resolution logic does not seem to handle wildcards that well. It seems to only use their bounds but ignores the bounds of the type variable. So users might run into a ClassCastException.

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);
}
}
}

return new TypeToken<>($Gson$Types.newParameterizedTypeWithOwner(null, rawType, typeArguments));
}

Expand Down
108 changes: 108 additions & 0 deletions gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java
Expand Up @@ -16,6 +16,7 @@

package com.google.gson.reflect;

import java.lang.reflect.GenericArrayType;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -91,6 +92,12 @@ public void testArrayFactory() {
TypeToken<?> expectedListOfStringArray = new TypeToken<List<String>[]>() {};
Type listOfString = new TypeToken<List<String>>() {}.getType();
assertEquals(expectedListOfStringArray, TypeToken.getArray(listOfString));

try {
TypeToken.getArray(null);
fail();
} catch (NullPointerException e) {
}
}

public void testParameterizedFactory() {
Expand All @@ -104,6 +111,97 @@ public void testParameterizedFactory() {
Type listOfString = TypeToken.getParameterized(List.class, String.class).getType();
Type listOfListOfString = TypeToken.getParameterized(List.class, listOfString).getType();
assertEquals(expectedListOfListOfListOfString, TypeToken.getParameterized(List.class, listOfListOfString));

TypeToken<?> expectedWithExactArg = new TypeToken<GenericWithBound<Number>>() {};
assertEquals(expectedWithExactArg, TypeToken.getParameterized(GenericWithBound.class, Number.class));

TypeToken<?> expectedWithSubclassArg = new TypeToken<GenericWithBound<Integer>>() {};
assertEquals(expectedWithSubclassArg, TypeToken.getParameterized(GenericWithBound.class, Integer.class));

TypeToken<?> expectedSatisfyingTwoBounds = new TypeToken<GenericWithMultiBound<ClassSatisfyingBounds>>() {};
assertEquals(expectedSatisfyingTwoBounds, TypeToken.getParameterized(GenericWithMultiBound.class, ClassSatisfyingBounds.class));
}

public void testParameterizedFactory_Invalid() {
try {
TypeToken.getParameterized(null, new Type[0]);
fail();
} catch (NullPointerException e) {
}

GenericArrayType arrayType = (GenericArrayType) TypeToken.getArray(String.class).getType();
try {
TypeToken.getParameterized(arrayType, new Type[0]);
fail();
} catch (IllegalArgumentException e) {
assertEquals("rawType must be of type Class, but was java.lang.String[]", e.getMessage());
}

try {
TypeToken.getParameterized(String.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("java.lang.String requires 0 type arguments, but got 1", e.getMessage());
}

try {
TypeToken.getParameterized(List.class, new Type[0]);
fail();
} catch (IllegalArgumentException e) {
assertEquals("java.util.List requires 1 type arguments, but got 0", e.getMessage());
}

try {
TypeToken.getParameterized(List.class, String.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("java.util.List requires 1 type arguments, but got 2", e.getMessage());
}

try {
TypeToken.getParameterized(GenericWithBound.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Type argument class java.lang.String does not satisfy bounds "
+ "for type variable T declared by " + GenericWithBound.class,
e.getMessage());
}

try {
TypeToken.getParameterized(GenericWithBound.class, Object.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Type argument class java.lang.Object does not satisfy bounds "
+ "for type variable T declared by " + GenericWithBound.class,
e.getMessage());
}

try {
TypeToken.getParameterized(GenericWithMultiBound.class, Number.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Type argument class java.lang.Number does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class,
e.getMessage());
}

try {
TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Type argument interface java.lang.CharSequence does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class,
e.getMessage());
}

try {
TypeToken.getParameterized(GenericWithMultiBound.class, Object.class);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Type argument class java.lang.Object does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class,
e.getMessage());
}
}

private static class CustomTypeToken extends TypeToken<String> {
Expand Down Expand Up @@ -158,3 +256,13 @@ public void testTypeTokenRaw() {
}
}
}

// Have to declare these classes here as top-level classes because otherwise tests for
// TypeToken.getParameterized fail due to owner type mismatch
class GenericWithBound<T extends Number> {
}
class GenericWithMultiBound<T extends Number & CharSequence> {
}
@SuppressWarnings("serial")
abstract class ClassSatisfyingBounds extends Number implements CharSequence {
}