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.

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

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:

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 February 16, 2022 00:17
@eamonnmcmanus
Copy link
Member

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
@Marcono1234
Copy link
Collaborator Author

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

@Marcono1234 Marcono1234 deleted the marcono1234/TypeToken-improvements branch April 19, 2022 16:29
sgammon added a commit to elide-dev/elide that referenced this pull request Jan 15, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.code.gson:gson-parent](https://togithub.com/google/gson) |
`2.9.0` -> `2.10.1` |
[![age](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/compatibility-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson-parent/2.10.1/confidence-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/)
|
| [com.google.code.gson:gson](https://togithub.com/google/gson) |
`2.9.0` -> `2.10.1` |
[![age](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/compatibility-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10.1/confidence-slim/2.9.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>google/gson</summary>

###
[`v2.10`](https://togithub.com/google/gson/blob/HEAD/CHANGELOG.md#Version-210)

- Support for serializing and deserializing Java records, on Java ≥ 16.
([google/gson#2201)
- Add `JsonArray.asList` and `JsonObject.asMap` view methods
([google/gson#2225)
- Fix `TypeAdapterRuntimeTypeWrapper` not detecting reflective
`TreeTypeAdapter` and `FutureTypeAdapter`
([google/gson#1787)
- Improve `JsonReader.skipValue()`
([google/gson#2062)
- Perform numeric conversion for primitive numeric type adapters
([google/gson#2158)
- Add `Gson.fromJson(..., TypeToken)` overloads
([google/gson#1700)
- Fix changes to `GsonBuilder` affecting existing `Gson` instances
([google/gson#1815)
- Make `JsonElement` conversion methods more consistent and fix javadoc
([google/gson#2178)
- Throw `UnsupportedOperationException` when `JsonWriter.jsonValue` is
not supported
([google/gson#1651)
- Disallow `JsonObject` `Entry.setValue(null)`
([google/gson#2167)
- Fix `TypeAdapter.toJson` throwing AssertionError for custom
IOException
([google/gson#2172)
- Convert null to JsonNull for `JsonArray.set`
([google/gson#2170)
- Fixed nullSafe usage.
([google/gson#1555)
- Validate `TypeToken.getParameterized` arguments
([google/gson#2166)
- Fix [#&#8203;1702](https://togithub.com/google/gson/issues/1702):
Gson.toJson creates CharSequence which does not implement toString
([google/gson#1703)
- Prefer existing adapter for concurrent `Gson.getAdapter` calls
([google/gson#2153)
- Improve `ArrayTypeAdapter` for `Object[]`
([google/gson#1716)
- Improve `AppendableWriter` performance
([google/gson#1706)

###
[`v2.9.1`](https://togithub.com/google/gson/blob/HEAD/CHANGELOG.md#Version-291)

- Make `Object` and `JsonElement` deserialization iterative rather than

recursi[google/gson#1912)
- Added parsing support for enum that has overridden toString() method
([google/gson#1950)
- Removed support for building Gson with Gradle
([google/gson#2081)
- Removed obsolete `codegen` hierarchy
([google/gson#2099)
- Add support for reflection access filter
([google/gson#1905)
- Improve `TypeToken` creation validation
([google/gson#2072)
- Add explicit support for `float` in `JsonWriter`
([google/gson#2130,
[google/gson#2132)
- Fail when parsing invalid local date
([google/gson#2134)

Also many small improvements to javadoc.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/elide-dev/v3).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDIuMCIsInVwZGF0ZWRJblZlciI6IjM0LjEwMi4wIn0=-->
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