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

[Discussion] Deprecate non-type-safe Gson.fromJson(..., Type) overloads #2328

Open
Marcono1234 opened this issue Feb 26, 2023 · 1 comment
Open

Comments

@Marcono1234
Copy link
Collaborator

Problem solved by the feature

The Gson.fromJson(..., Type) overloads are non-type-safe because the type parameter T used for the return type is not bound by any of the parameters. This makes it easy to accidentally introduce bugs without noticing them immediately and only later at a completely unrelated location experiencing a ClassCastException. For example:

Type type = new TypeToken<List<Integer>>() {}.getType();
List<String> list = new Gson().fromJson("[1, 2]", type);
// ClassCastException: class Integer cannot be cast to class String
System.out.println("First value: " + list.get(0));

The sample code above does not produce any compiler warnings; the issue here is that type (List<Integer>) does not match the expected return type of fromJson (List<String>).

This basically affects all methods which will get the TypeParameterUnusedInFormals Error Prone suppression. This also affects JsonDeserializationContext.deserialize, see #2216.

Feature description

Deprecate all fromJson(..., Type) overloads. The deprecation comment should:

  • explain why the methods are unsafe
  • refer to the overload with TypeToken parameter (introduced by Add Gson.fromJson(..., TypeToken) overloads #1700)
  • mention migration paths
    • passing around TypeToken instead of Type in user code (e.g. as method arguments and field values)
    • if necessary converting Type to TypeToken with TypeToken.get(Type), but then it is the user's responsibility to make sure the code is safe
    • if necessary obtaining Type from TypeToken by calling TypeToken.getType() (this should be relatively cheap since the method is just a plain getter method)

There is however no need to remove the deprecated methods then, they could probably stay there 'indefinitely'.

Risks

This might cause a lot of deprecation noise for users because until recently (see #1700) the overloads with TypeToken parameter did not exist yet, so most code probably still uses fromJson(..., Type). Though on the other hand this might increase awareness that the code of the users is potentially unsafe or even incorrect which they just did not notice yet.

Maybe it should also be investigated (if that is possible) how often users run into this issue compared to other issues, mainly TypeToken capturing type-erased type parameters, see #1219.

Alternatives / workarounds

Do nothing, and risk that users keep running into difficult to debug ClassCastExceptions.

@MaicolAntali
Copy link
Contributor

I agree with you the Gson.fromJson(..., Type) is not very safe. In fact, in this PR #2320, I had to add a @SuppressWarnings("TypeParameterUnusedInFormals") because ErrorProne catches this issue.

ErrorProne Doc: TypeParameterUnusedInFormals

For me, we can deprecate this method. The others .fromJson(...) methods are type-safe and do the same work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants