google / gson

A Java serialization/deserialization library to convert Java Objects into JSON and back
Apache License 2.0
23.23k stars 4.27k forks source link

Fix $Gson$Types equals method for TypeVariable when its generic declaration is not a Class #2599

Closed d-william closed 7 months ago

d-william commented 7 months ago

Purpose

When $Gson$Types equals method check two TypeVariable, it can return false while Objects#equals(Object, Object) returns true.

Description

public class Main {

    private static class MyClass {

        public <T> T method() { return null; }

    }

    public static void main(String[] args) throws NoSuchMethodException {
        Method m1 = MyClass.class.getMethod("method");
        Method m2 = MyClass.class.getMethod("method");

        Type rt1 = m1.getGenericReturnType();
        Type rt2 = m2.getGenericReturnType();

        TypeToken<?> t1 = TypeToken.get(rt1);
        TypeToken<?> t2 = TypeToken.get(rt2);

        System.out.println(Objects.equals(rt1, rt2)); // true
        System.out.println(Objects.equals(t1, t2));  // false
    }

}

The root cause is here. For Class generic declaration, reference equality is enough because there is one reference of each class. But for Methodgeneric declaration, it is not, Class#getMethod(String, Class<?>...) returns a new instance of the Method per call.

Checklist

google-cla[bot] commented 7 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Marcono1234 commented 7 months ago

Thanks for this pull request, and for fixing this bug!

Could you please add a unit test similar to what you have shown in your example code (probably either to GsonTypesTest or TypeTokenTest)? I assume you can add this dummy class with generic method as local class within the test method, so that it does not have to be directly inside the enclosing test class.

Also, out of curiosity, how did you notice this bug? Creating TypeTokens which capture type variables is discouraged and will be disallowed in future Gson versions (see #2376). But obtaining it in the way shown in your example will still work (because it provides no false sense of type safety).

d-william commented 7 months ago

I should be able to add the tests you request in a few days :)

To answer your question, I am implementing JSON objects representing method signature with the type information of a potentially declaring generic class.

class SimpleClass {

    public void method1(String str) {...} 

} 

Will give :

{
    "name" : "method1", 
    "parameters" : [
        {
            "name" : "str",
            "type" : "java.lang.String"
        } 
    ] 
} 
class GenericClass<T> {

    public void method2(T value) {...} 

} 

GenericClass<Number> will give :

{
    "name" : "method2", 
    "parameters" : [
        {
            "name" : "value",
            "type" : "java.lang.Number"
        } 
    ]  
} 
class OtherClass {

    public <T> void method3(T param) {...} 

} 

Will give :

{
    "name" : "method3", 
    "parameters" : [
        {
            "name" : "param",
            "type" : { "name" : "T", "declaring" : "package.OtherClass#method3..." }
        } 
    ] 
} 

And with POJO of this JSON objects, I am led to check equality between them. I encountered this problem when trying with two objects from scenario 3, because internally, parameter types use TypeToken.

Marcono1234 commented 7 months ago

Thanks for the explanation. I guess Gson's TypeToken can be used for that use case, but it is mainly designed to be used with the Gson API. Maybe it would be better to use a more general "type token" class, such as Guava's com.google.common.reflect.TypeToken which also offers more functionality. What do you think?

Nonetheless, the bug here with the Gson TypeToken should still be fixed.

d-william commented 7 months ago

Test added :)

Marcono1234 commented 7 months ago

Could you please address the build failures though? It probably suffices to place @SuppressWarnings({"unused", "TypeParameterUnusedInFormals"}) (or {"UnusedMethod", "UnusedVariable", "TypeParameterUnusedInFormals}") on TypeVariableTest respectively its constructor and method.