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

Improve TypeToken creation validation #2072

Merged

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Feb 6, 2022

Resolves Relates to #1219 (by preventing to capture type variables using TypeToken anonymous subclass)
Related to #1741 (comment).
Includes the changes of #1753, without addressing any of the TODO comments in the description there.

Changes:

  • Minor refactoring, removing redundant code and reducing internal method visibility
  • Enforce only direct subclasses of TypeToken
    Previously Gson allowed subclasses of subclasses (...) of TypeToken. This could lead to incorrect behavior because the type argument lookup simply retrieved the type argument without resolving it, so the type argument might have been completely unrelated to TypeToken's type variable.
  • Prevent TypeToken from capturing type variables (Don't silently ignore missing type information from TypeTokens. #1219)
    As mentioned in that issue, this is pretty error-prone and has lead to multiple quite some confusion already.

No hurry to integrate this, but it might be nice to have these changes eventually. Any feedback is appreciated.

Marcono1234 added 12 commits Jul 26, 2020
The returned Type is never a wildcard due to the changes made to getSupertype
by commit b1fb9ca.
getRawType(TypeToken.getType()) is the same as calling TypeToken.getRawType().
It is possible to use wildcards as part of the type argument, e.g.:
`new TypeToken<List<? extends CharSequence>>() {}`
Previously subclasses of subclasses (...) of TypeToken were allowed which
can behave incorrectly when retrieving the type argument, e.g.:

  class SubTypeToken<T> extends TypeToken<Integer> {}
  new SubTypeToken<String>() {}.getType()

This returned `String` despite the class extending TypeToken<Integer>.
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.
Rename the method parameter to match the documentation of the method and
to be similar to getSupertype(...).
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

@eamonnmcmanus eamonnmcmanus Feb 14, 2022

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

@Marcono1234 Marcono1234 Feb 15, 2022

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

@Marcono1234 Marcono1234 Apr 16, 2022

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

@eamonnmcmanus eamonnmcmanus May 19, 2022

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.

As mentioned in review comments, there are cases during serialization where
usage of the type variable is not so problematic (but still not ideal).
@Marcono1234 Marcono1234 force-pushed the marcono1234/TypeToken-improvements branch from b9436ed to cba0307 Compare Feb 16, 2022
@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Apr 19, 2022

I ran this against all of Google's internal tests and found no problems. Thanks!

@eamonnmcmanus eamonnmcmanus merged commit b1c399f into google:master Apr 19, 2022
3 checks passed
@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Apr 19, 2022

Thanks for merging! What do you think about the above #2072 (comment)?

@Marcono1234 Marcono1234 deleted the marcono1234/TypeToken-improvements branch Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants