FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Deserialization of a certain kinds of parametrized properties fail to resolve `?` into expected bounds, resulting in `LinkedHashMap` #4118

Open mariuszpala opened 1 year ago

mariuszpala commented 1 year ago

Search before asking

Describe the bug

Recently our code editor advices as to change the class definition, the deserialization stopped working correctly.

We have such a class:

public class TestAttribute<T extends TestAttribute<?>> {

    protected List<T> attributes;
}

When we create a parent object with a list of such properties 2-level deep, on 2rd level the deserialization doesn't work and it deserializes into LinkedHashMap instead of TestAttribute. See the screenshot below.

image

But when I remove the "<?>" from the "extends", then the IDE complains about it, but the deserialization works just fine

image image

Version Information

2.15.2, JDK17

Reproduction

  1. Brief code sample/snippet: include here in preformatted/code section

    public class TestAttribute<T extends TestAttribute<?> {
    
    protected List<T> attributes;
    
    public TestAttribute() {
    }
    public TestAttribute(List<T> attributes) {
        this.attributes = attributes;
    }
    
    public List<T> getAttributes() {
        return attributes;
    }
    
    public void setAttributes(List<T> attributes) {
        this.attributes = attributes;
    }
    }
public class TestObject {

    private List<TestAttribute<?>> attributes = new ArrayList<>();

    public TestObject() {
    }

    public TestObject(List<TestAttribute<?>> attributes) {
        this.attributes = attributes;
    }

    public List<TestAttribute<?>> getAttributes() {
        return attributes;
    }

    public void setAttributes(List<TestAttribute<?>> attributes) {
        this.attributes = attributes;
    }
}
ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
        objectMapper.configure(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY, true);
        objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
        objectMapper.configure(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL, true);
        objectMapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
        objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
        objectMapper.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);
        objectMapper.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS);

        TestAttribute a = new TestAttribute(null);
        TestAttribute b = new TestAttribute(List.of(a));
        TestAttribute c = new TestAttribute(List.of(b));

        TestObject test = new TestObject(List.of(c));
        String serialized = objectMapper.writeValueAsString(test);
        System.out.println(serialized);

        TestObject deserialized = objectMapper.readValue(serialized, TestObject.class);
        System.out.println(deserialized.getAttributes().get(0).getAttributes().get(0).getClass().getName());
  1. Textual explanation: include here When the code above is executed, the last line fails
java.lang.ClassCastException: class java.util.LinkedHashMap cannot be cast to class com.me.samples.TestAttribute (java.util.LinkedHashMap is in module java.base of loader 'bootstrap'; com.me.samples.TestAttribute is in unnamed module of loader 'app')
    at com.me.samples.PlainTest.main(PlainTest.java:38)

Expected behavior

Deserialization should work just fine and deserialize a list of TestAttribute objects correctly but instead the LinkedHashMap is deserialized instead unless I remove <?> from all my code which keeps the IDE complaining abut the missing raw parameter.

Additional context

No response

cowtowncoder commented 1 year ago

While I appreciate textual explanation of problems, would it be possible to have an isolated test case showing exact problem case and nothing else? (that is, remove as much of existing code from above as possible while remaining the case of exception being thrown)

And no printing of values with System.out.println() -- assertions only, please.

mariuszpala commented 1 year ago

Just try this code:

TestAttribute<?> testAttribute = deserialized.getAttributes().get(0).getAttributes().get(0);

it throws the error I mentioned above:

java.lang.ClassCastException: class java.util.LinkedHashMap cannot be cast to class com.me.samples.TestAttribute (java.util.LinkedHashMap is in module java.base of loader 'bootstrap'; com.me.samples.TestAttribute is in unnamed module of loader 'app')
    at com.me.samples.PlainTest.main(PlainTest.java:38)
yihtserns commented 1 year ago

This is the class chain:

TestObject(attributes=[TestAttribute(attributes=[?])]

When deserializing this JSON using the class chain above:

{                                 // TestObject
    "attributes": [               // attributes=[TestAttribute]
        {                         // TestAttribute
            "attributes": [       // attributes=[?]
                {                 // ? <---- Without the type info, Jackson can only convert this into a Map
                    "attributes": [{}]
                 }
            ]
        }
    ]
}

If you remove <?> from TestObject.attributes::List<TestAttribute<?>> to become TestObject.attributes::List<TestAttribute>, the class chain becomes:

TestObject(attributes=[TestAttribute(attributes=[T extends TestAttribute(attributes=[?])])])

...so when deserializing:

{                                 // TestObject
    "attributes": [               // attributes=[TestAttribute]
        {                         // TestAttribute
            "attributes": [       // attributes=[T extends TestAttribute]
                {                 // T extends TestAttribute <---- Jackson knows to convert this part to TestAttribute
                    "attributes": [{}]
                 }
            ]
        }
    ]
}
yawkat commented 1 year ago

@yihtserns while the type for the inner object is '?', it is still known statically that it is bounded by TestAttribute<?>, so jackson should have enough type info to read it as such.

yihtserns commented 1 year ago

@yawkat I don't understand, can you describe what you mean in detail? TestAttribute<?> is declared in two places, so I don't know what you're referring to.

It'll be good if you also illustrate the class/field chain that you have in mind (to demonstrate what type info that is available at runtime that can be obtained using Java reflection API).

yawkat commented 1 year ago

// ? <---- Without the type info, Jackson can only convert this into a Map

This does not have to be the case. Jackson can infer safely that here, the lowest bound of the wildcard is TestAttribute<?>, not Object. That seems to be the central issue that leads to this bug.

yihtserns commented 1 year ago

@yawkat

This does not have to be the case. Jackson can infer safely that here, the lowest bound of the wildcard is TestAttribute<?>, not Object...

public class TestObject {
                                   ⬇ this part declares that TestAttribute.attributes is a List<?>
    private List<TestAttribute<?>> attributes ...
        ...
}

The above results in class/field chain: TestObject(attributes=[TestAttribute(attributes=[?])].


Again, please stop writing just TestAttribute<?> because there are 2 places where that is declared - I don't know which one you're referring to. If you don't describe clearly, I wouldn't know know:

public class TestAttribute<...TestAttribute<?>> { // if you're talking about this...
       ...
}

public class TestObject {

    private List<TestAttribute<?>> attributes...  // ...or if you're talking about this.
        ...
}
cowtowncoder commented 1 year ago

Just try this code:

TestAttribute<?> testAttribute = deserialized.getAttributes().get(0).getAttributes().get(0);

it throws the error I mentioned above:

java.lang.ClassCastException: class java.util.LinkedHashMap cannot be cast to class com.me.samples.TestAttribute (java.util.LinkedHashMap is in module java.base of loader 'bootstrap'; com.me.samples.TestAttribute is in unnamed module of loader 'app')
  at com.me.samples.PlainTest.main(PlainTest.java:38)

Sigh. This AND full class definition AND mapper configuration AND actual read call (which matters wrt type information passed).

I was hoping for a minimal unit test, not an incomplete one-liner.

But I can work with this I suppose.

cowtowncoder commented 1 year ago

Ok. I think I can see what is going on.

First of all, yes, it should be possible to figure out typing in this case. And it took some thinking to know why type resolution doesn't work.

And the reason is, I think, that TypeFactory is asked to resolve type List<TestAttribute<?>> in TestObject, wherein resolution of ? would require finding bounds from within TestAttribute, and not only rely on local information (where we do not have bound).

So the fix is probably needed for TypeFactory; assuming it is passed enough information to handle this case.

yawkat commented 1 year ago

i made a pr with a fix: #4122

yihtserns commented 1 year ago

Table of current behaviours when deserializing this JSON: {"attributes":[{"attributes":[{"attributes":[{}]}]}]}

Root class deserialized.toString()
Scenario 1
public class TestObject {
    private List<TestAttribute> attributes;
    ...
}
TestObject{attributes=[TestAttribute{attributes=[TestAttribute{attributes=[{}]}]}]}

NOTE: Seems like when List<TestAttribute> is not "sub-parameterized", it would use the type parameter info directly from:
class TestAttribute<T extends TestAttribute>
...to construct the nested entries.
Scenario 2
public class TestObject {
    private List<TestAttribute<TestAttribute>> attributes;
    ...
}
TestObject{attributes=[TestAttribute{attributes=[TestAttribute{attributes=[{}]}]}]}

NOTE: Seems like when List<TestAttribute<TestAttribute>> is "sub-parameterized", it would use that "sub-parameter" type to construct the nested entries.
Scenario 3
public class TestObject {
    private List<TestAttribute<SubClassOfTestAttribute>> attributes;
    ...
}
TestObject{attributes=[TestAttribute{attributes=[SubClassOfTestAttribute{attributes=[TestAttribute{attributes=null}]}]}]}

NOTE: Similar to the one up there, it uses the "sub-parameter" type SubClassOfTestAttribute to construct the nested entries.
Scenario 4
public class TestObject {
    private List<TestAttribute<?>> attributes;
    ...
}
TestObject{attributes=[TestAttribute{attributes=[{attributes=[{}]}]}]}

NOTE: Similar to the two up there, it uses the "sub-parameter" type <?> to construct the nested entries - in this case as Map.
Scenario 5
public class TestObject {
    private List<TestAttribute<? extends TestAttribute>> attributes;
    ...
}
TestObject{attributes=[TestAttribute{attributes=[TestAttribute{attributes=[TestAttribute{attributes=null}]}]}]}

NOTE: While similar to the two up there, i.e. uses the "sub-parameter" type <? extends TestAttribute> to construct the nested entries of type TestAttribute, ⚠️ why is there an additional nested depth???.

So, this issue is about requesting behaviour change for the Scenario 4: if wildcard type is used, look for and use the type parameter info from the class itself, mimicking the behaviour from Scenario 1.


On wildcard as parameter type for a bounded type parameter

I was curious on why would Java compiler allows a bounded type parameter e.g. <T extends Something> to be parameterizable by <?> which means "anything" i.e. can be something that does NOT extends Something.

Apparently that can be useful: https://en.wikipedia.org/wiki/Wildcard_(Java)#Wildcard_as_parameter_type. Or maybe someone is trying to sell a Java compiler limitation/bug as a feature. πŸ€”

Anyway, that's just a trivia, unrelated to the reason why this ticket was created in the first place: he just wanted to stop the code editor warnings and using wildcard type was the only practical choice (hey @SuppressWarnings("rawtypes") would've worked too πŸ˜‹).

yihtserns commented 1 year ago

BTW, title is currently inaccurate: should be "wildcard parameterized" not "raw parameterized" (which actually means not parameterized).

JooHyukKim commented 1 year ago

According to JLS Chapter 4,

The wildcard ? extends Object is equivalent to the unbounded wildcard ?.

which I think means that unless user explicitly specifies a TypeReference or target class for both deserialization and serialization, the most Jackson can do is not throw error (what @mariuszpala reported in the first place) and return "any" subtype of Object that Jackson maintainers "think" reasonable.

JooHyukKim commented 1 year ago

The original report says...

Deserialization should work just fine and deserialize a list of TestAttribute objects correctly

that deser should work just fine, but I don't think we should assume what the words "fine"/"correctly" mean, especially when the problem is about types but the report doesn't provide FULL class definition like @cowtowncoder said above like...

Sigh. This AND full class definition AND mapper configuration AND actual read call (which matters wrt type information passed)

... this.

yawkat commented 1 year ago

@JooHyukKim yes the JLS states that ? is equivalent to ? extends Object, that is the problem (getUpperBounds returns Object). But that does not mean Object is actually the upper bound of all types possible with the wildcard. With knowledge of the type variable declaration, we can in this case determine a more specific but still correct upper bound. All possible "instantiations" of the wildcard have to match the wildcard upper bound AND the bound defined on the type variable, so if the former is Object, we can still safely use the latter.

On Thu, Sep 21, 2023, 00:36 Kim, Joo Hyuk @.***> wrote:

The original report says...

Deserialization should work just fine and deserialize a list of TestAttribute objects correctly

that deser should work just fine, but I don't think we should assume what the words "fine"/"correctly" mean, especially when the problem is about types but the report doesn't provide FULL class definition like @cowtowncoder https://github.com/cowtowncoder said above like...

Sigh. This AND full class definition AND mapper configuration AND actual read call (which matters wrt type information passed)

... this.

β€” Reply to this email directly, view it on GitHub https://github.com/FasterXML/jackson-databind/issues/4118#issuecomment-1728513686, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASIZINGFWKP4U2JM446NT3X3NVX5ANCNFSM6AAAAAA4426GTQ . You are receiving this because you were mentioned.Message ID: @.***>

JooHyukKim commented 1 year ago

we can still safely use the latter.

true, πŸ‘πŸΌπŸ‘πŸΌ

cowtowncoder commented 1 year ago

As per #4122 this might now work -- would be great to get an extracted unit test to verify (tests for #4122 reproduce underlying issue, fix).

JooHyukKim commented 1 year ago

4128 should have the unit test now πŸ‘πŸΌπŸ‘πŸΌ

cowtowncoder commented 1 year ago

Fixed via #4122, as shown by unit tests added; closing.

cowtowncoder commented 11 months ago

Unfortunately need to revert the fix for 2.16 timeframe -- hoping to re-tackle for 2.17.

poolsnowhui commented 9 months ago

env:

  1. spring boot 3.2.0
  2. jdk17 My application have this error, too. I have the RequestBody of Map, this Map is ' Map<Long, Set> idToViolationResultMap'. Wherein, ViolationResult is the model. This Map is the Map<Long, Set> in the debug mode. image2023-12-19_14-33-30
yihtserns commented 9 months ago

@poolsnowhui that looks like a Spring WebMVC problem, not Jackson's.