Add Gson.fromJson(..., TypeToken) overloads

[Marcono1234]

Previously only Gson.fromJson(..., Type) existed which is however not type-safe since the generic type parameter T used for the return type is not bound.
Example:

Type type = new TypeToken<List<String>>(){}.getType();
// Compiles fine, but causes issue at runtime
List<Integer> list = gson.fromJson(..., type);

Since these methods are often used in the form

gson.fromJson(..., new TypeToken<...>(){}.getType())

this commit now adds overloads which accept a TypeToken and are therefore
more type-safe:

TypeToken<List<String>> type = new TypeToken<List<String>>(){};
// This won't compile (as expected)
List<Integer> list = gson.fromJson(..., type);

With Java >= 9 another advantage is that you can use the 'diamond syntax' for anonymous classes (see JDK-8062373). If Gson had fromJson overloads with TypeToken this would allow quite concise usage:

Map<String, List<String>> map = gson.fromJson(..., new TypeToken<>(){});

Additional changes:

• Adds fromJson(JsonReader, Class<T>) for symmetry with other methods
  Edit: Removed again because it breaks source compatibility, see comment
• Fixed some javadoc grammar mistakes
• Added javadoc @see tags; they currently have this order:
  1. Overload which serializes to writer / string; respectively deserializes from reader / string
  2. Overload which supports generic types
• Consistently write "JSON" in uppercase
• Explicitly state in javadoc that fromJson methods other than fromJson(JsonReader, ...) fully consume JSON (see JsonReader not strict about JSON document consumption #835)
• More precise placement of @SuppressWarnings("unchecked")

It appears TypeToken overloads were considered in the past but were then rejected, see #248 (comment). However, I have not found the discussion explaining why they were rejected.

Open questions:

• Maybe it would even be reasonable to deprecate the fromJson methods taking a Type (but keeping them indefinitely). While this might be disruptive for users initially, switching to the TypeToken variant will make their code safer. If they have, for whatever reason, only a Type available but no TypeToken, then they can construct one with TypeToken.get(Type), but it is their task then to verify that the casts are safe.

[eamonnmcmanus]

This makes sense to me, and I don't know why the idea was summarily dismissed in 2010. The main drawback I see is that new TypeToken<List<String>>() {} is an object allocation and an additional anonymous subclass, but you already have that if you're currently getting a Type from the same expression with .getType() on the end.

I'm running the change against all of Google's internal Gson client code just in case the new overload introduces some compilation problem, though I can't really see what that would be.

[eamonnmcmanus]

You could fix the typo "your" here while you are at it.

[eamonnmcmanus]

Since the text says "you must use", I think it should detail all the options, so both Type and TypeToken overloads. There may be cases where it is still preferable to use the Type one. TypeToken could come first, though, and the example show just that.

[Marcono1234]

I have reworded the whole paragraph a bit because it was also incorrect regarding serialization. I hope the new wording is fine; though please let me know if I should adjust something.

[eamonnmcmanus]

I think there should probably be at least a couple of new test methods covering the new overloads.

[Marcono1234]

The main drawback I see is that new TypeToken<List<String>>() {} is an object allocation and an additional anonymous subclass

It appears there is unfortunately no other solution to this until the Java language gets proper compiler support for this (similar to Kotlin's reified type parameters?). Guava with its TypeToken and Jackson with its TypeReference seem to take the same approach.

However, you can safely store the TypeToken in a static final field to avoid object allocations. (But I assume the memory consumed by these allocations is negligible anyways.)

I think there should probably be at least a couple of new test methods covering the new overloads.

Fair point; I have added them to ParameterizedTypesTest, I hope the way I implemented them is fine for you.

Also, should there be toJson overloads with TypeToken parameter? Personally I am not sure if the small advantages (if any?) this provides would justify the new methods. For fromJson this is a different story because it provides type safety there.

[eamonnmcmanus]

New test code looks good! Just one more thing...

[eamonnmcmanus]

Perhaps surprisingly, this is not a source-compatible change. It seems completely logical to add this overload to the other fromJson(JsonReader...) ones, but there's an issue. When I ran this PR against all of Google's builds, I encountered this:

  private static <ResponseT extends ApiMessage> ApiMessageHttpResponseParser<ResponseT> create(
      final ResponseT responseInstance) {
    final Gson baseGson = new GsonBuilder().create();
    TypeAdapter responseTypeAdapter;
    Gson responseMarshaller = null;

    if (responseInstance != null) {
      responseTypeAdapter =
          new TypeAdapter<ResponseT>() {
            @Override
            public void write(JsonWriter out, ResponseT value) {
              baseGson.toJson(value, responseInstance.getClass(), out);
            }

            @Override
            public ResponseT read(JsonReader in) {
              return baseGson.fromJson(in, responseInstance.getClass());
            }
          };

This is not type-safe. The type of responseInstance.getClass() is Class<? extends ApiMessage>, and ? extends ApiMessage is not assignable to ResponseT. It compiles without this PR because there is no Class<T> overload so it gets the type-unsafe Type overload.

If the code here instead had return baseGson.fromJson(in, new TypeToken<ResponseT>() {}.getType()) then it would compile, with or without this PR, and then with this PR the .getType() can be removed. It also looks as if this class no longer exists in the current version of gax-java.

I found a second problem with code that was calling this method with Map.class. The unchecked conversion from raw Map to Map<String, ?> drew a warning, and the code was being compiled with -Werror. The same kind of fix applies, of course.

On the whole, I think we should drop this overload. It's probably better to retain the minor inconsistency of not having it than to expose users to new problems.

[Marcono1234]

Thanks for running it against all Google builds.

If the code here instead had return baseGson.fromJson(in, new TypeToken<ResponseT>() {}.getType()) then it would compile

But it would not be type safe either, in fact it would even be less type safe than the original code respectively not work properly at all because it would parse the JSON data as the erasure of the type variable (ApiMessage). This is why it would have been good to fix #1219, and why I had this originally in #2072 😕. The previous code would not have been type safe though in case ResponseT was a parameterized type.

I found a second problem with code that was calling this method with Map.class. The unchecked conversion from raw Map to Map<String, ?> drew a warning

For what it is worth this would not have been safe either. Even though one might expect that a raw Map can only be deserialized to a Map<String, ...> due to JSON only permitting strings as member names, Gson actually supports non string values as seen with #1247.

So yes, this will break source compatibility, but it might get users to write safer code, or at least become aware that their code is not safe and then manually suppress the compiler warnings. Therefore, personally I think it might be acceptable to break source compatibility here; but maybe there are cases where the existing code is actually safe and this would create too much trouble...

If you really want, I can remove it though.

(This also goes into the same direction why I mentioned in the description of this pull request to consider deprecating the fromJson(..., Type) overloads. That would be rather extreme, but maybe it could be considered at some point in the future.)

[eamonnmcmanus]

Well, the title of this PR is Add Gson.fromJson(..., TypeToken) overloads, so perhaps we can keep it focused on that. If you want to make a separate PR that adds fromJson(JsonReader, Class<T>) then we can debate the source-incompatibility problem there.

The situation is a bit different from #2072, in that already-compiled code will continue to work. It's only new compilations that might fail, and probably only when the code being compiled is doing type-unsafe things. So I think there's a stronger case to be made for the change. Nevertheless, I think it should be separate, if only so I can prepare Google's code-base by fixing the problems I mentioned.

Regarding the type-safety of the first problem, I think the underlying issue is that the method doesn't have enough information to behave correctly. It would need to have something like a Class<ResponseT> parameter, and then of course that could be correctly passed to the existing Type overload, or to the new Class<T> overload. I agree that the new Class<T> overload makes it possible to discover this problem, while the new TypeToken<T> overload just gives us a different way to sweep the unsoundness under the carpet. I further agree that ideally we would simply fix #1219 to disallow that unsoundness. I haven't changed my mind since #2072, though: I still feel that it's extremely important that people can update their Gson dependency without breaking things that currently work. I'll add that this would be yet another case where a compatibility-level concept might allow us to fix the errors of the past.

[Marcono1234]

You are right, I have removed the method again and hope the changes are fine like that.

Sorry for being a bit relentlessly. It is just pretty frustrating and annoying knowing that there are bugs / flaws with Gson and seeing that they cannot be fixed or wasting work on trying to fix them. This is of course not your fault, and I think it is quite valuable that you have a different view of maintaining a library as popular as Gson.

[eamonnmcmanus]

I forgot to answer the question about toJson overloads. I agree that they are probably not worthwhile.

[eamonnmcmanus]

Thanks for your patience, and for all the hard work you've put into this PR!