"Conflicting setter definitions for property" exception for `Map` subtype during deserialization

[fschmager]

I've been deserializing Spring's HttpHeaders without issue until I upgraded to latests Spring 2.3.0 which upgraded Jackson from 2.10.3 to 2.11.

After the upgrade to Jackson 2.11 I get this exception when trying to deserialize an HttpHeaders object:

com.fasterxml.jackson.databind.JsonMappingException: Conflicting setter definitions for property "date": org.springframework.http.HttpHeaders#setDate(1 params) vs org.springframework.http.HttpHeaders#setDate(1 params)

(same can happen for lastModified or any other overloaded setter)

There are 3 setDate methods on HttpHeaders.

It's throw as part of POJOPropertyBuilder#getSetter()

I've traced it back to this change in POJOPropertiesCollector as part of #2555

I couldn't find any way to disable indexing. #2555 is hinting that there isn't one:

It may become necessary to allow NOT doing this, too (via MapperFeature), but let's start by assumption that index should be used.

I'm wondering if the indexing change broke backward compatibility.

Version exhibiting behavior: 2.11
Last working version (that I tried): 2.10.4

[cowtowncoder]

@fschmager First of all, thank you for reporting the issue.

Without looking deeply into the problem, I do not see immediate cause in the specific change, nor would there be reason to change it.
However, it is possible that this change or some other could have either fixed a check that should already have been done (i.e. use case was invalid), or, conversely, blocked legit usage.

My first reaction is that since all 3 setDate() method have unrelated (wrt Java type hierarchy) types, this is a conflict, and surfacing it makes sense. If so, there was something in conflict resolution that was missing a check (most likely) or, possibly, some logic that was selecting one of setters over others.

If this was during serialization, I would accept that it would be irrelevant (no setters used), but the way Jackson handles deserialization there is a requirement that one and exactly one mutator (field, setter, constructor parameter) is used.
In some cases multiple candidates can be pruned to just one, for example:

1. If only one annotated with inclusion (like @JsonProperty) use that; or if all but one excluded (with @JsonIgnore)
2. If types are related, more specific one used (like setValue(Number n) vs setValue(Long l), latter would be used)

but other than that, there is no logic to try to guess which setter to select.

So. I am surprised this did not fail with 2.10.x; and am not sure how else this could be handled.

[fschmager]

@cowtowncoder I'm with you. Your explanations make total sense. Jackson just can't know which mutator to use.

I've just created a sample project trying to round-trip a few objects with multiple overloaded setters, HttpHeaders among them. None work. All complain about the same thing: Conflicting setter definitions. No difference between 2.10.x and 2.11.

I feel like I'm already at stage 6 of debugging: How did that ever work?

I'm going back to my project where "it used to work" (or me thinking it did) to debug a bit more with 2.10.x to see if HttpHeaders was actually being deserialized and which setter was being selected.

[cowtowncoder]

@fschmager thank you. Regressions like this are always regrettable; let me know what you find.

For what it is worth, the idea of allowing pluggable conflict resolvers has been floating around for a while, but I've always postponed it with "should be part of that bigger rewrite of introspection I will for [some future version]".

[fschmager]

Now that was fun, but I think I figured "it" out.

How did this ever work

HttpHeaders implements MulitMap where MultiValueMap<K, V> extends Map<K, List<V>>.

Jackson determines that the JavaType of HttpHeaders is:

[map type; class org.springframework.http.HttpHeaders, [simple type, class java.lang.String] -> [collection type; class java.util.List, contains [simple type, class java.lang.String]]]

which means when an object mapper reads the value it uses a MapDeserializer calling put. The object also having overloaded convenience setters is irrelevant.

Discussion

The behavior of ObjectMapper#canDeserialize(JavaType type, AtomicReference<Throwable> cause) has changed between 2.10.4 and 2.11.0 due to the "indexing" change explained in the initial comment.

Despite HttpHeaders being a "map", ObjectMapper#canDeserialize is now saying it can't deserialize it anymore. This is where I believe a regression slipped in. Yes, HttpHeaders has multiple overloaded single arg setters, but more importantly it is a "map type" which mean it can in fact be deserialized.

I'm not sure how this should be fixed, but POJOPropertiesCollector._anyIndex is either too naive or shouldn't be used in this case because the object is of "map type" instead of being a "pojo.

How does Spring use ObjectMapper

Spring checks if an object is deserializable before deserializing it in HttpMessageConverterExtractor which calls AbstractJackson2HttpMessageConverter#canRead which calls objectMapper.canDeserialize(javaType, causeRef).

With 2.10.4 objectMapper.canDeserialize returned true allowing obectMapper.readValue to do the right thing (deserializing a map), but with 2.11.0 this breaks.

Example code

I've verified the changed behavior by running the following with Jackson 2.10.4 and 2.11.0. The differences are highlighted as comment inline:

import static org.junit.jupiter.api.Assertions.assertEquals;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.type.TypeFactory;
import java.lang.reflect.Type;
import java.time.Instant;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.http.HttpHeaders;


public class OverloadedSettersTest {

    private static final Instant NOW = Instant.now();
    private ObjectMapper objectMapper;
    private HttpHeaders serializedObject;

    @BeforeEach
    public void setup() {
        this.objectMapper = new ObjectMapper();
        serializedObject = new HttpHeaders();
        serializedObject.setDate(NOW); // one for the overloaded setters
    }

    @Test
    public void shouldRoundTripObject() throws JsonProcessingException {
        String json = objectMapper.writeValueAsString(serializedObject);
        System.out.println(json); // {"Date":["Mon, 15 Jun 2020 13:34:46 GMT"]}
        // [map type; class org.springframework.http.HttpHeaders, [simple type, class java.lang.String] ->
        // [collection type; class java.util.List, contains [simple type, class java.lang.String]]]
        JavaType httpHeadersJavaType = toJavaType(HttpHeaders.class);

        checkDeserializability(httpHeadersJavaType);

        HttpHeaders deserializedObject = objectMapper.readValue(json, httpHeadersJavaType);
        assertEquals(serializedObject, deserializedObject);
    }

    private void checkDeserializability(JavaType javaType) {
        AtomicReference<Throwable> causeRef = new AtomicReference<>();
        if (!this.objectMapper.canDeserialize(javaType, causeRef)) {
            // succeeds for 2.10.4

            // throws for 2.11.0
            // java.lang.RuntimeException: com.fasterxml.jackson.databind.JsonMappingException: Conflicting setter
            //definitions for property "expires": org.springframework.http.HttpHeaders#setExpires(1 params) vs org
            //.springframework.http.HttpHeaders#setExpires(1 params)
            throw new RuntimeException(causeRef.get());
        }
    }

    private JavaType toJavaType(Type type) {
        TypeFactory typeFactory = this.objectMapper.getTypeFactory();
        return typeFactory.constructType(type);
    }
}

[cowtowncoder]

Ohhhh. Very interesting -- thank you for digging this up. It does sound like regression, but obviously I'll need to do some more digging too.
Method canDeserialize() (and matching canSerialize()) are bit odd, and I actually wish I had not added them -- there is not much upside in trying to figure it out, nor are results all that reliable with many failures only occurring if you actually try to (de)serialize something.

That said, it should still work consistently across versions, using as similar heuristics as possible.

I'll have to figure out how to reproduce this without adding spring dependency. I can probably add test dep temporarily, then try to strip type definition down to minimal core.

As to indexing part: I don't think is is naive, but rather it sounds like problem is a side-effect of introspection that is done for information that turns out not to be needed -- "getter" is simply requested to find aggregated annotation definitions (to find index), but asking for getter then triggers pruning/validation. This calculation is not needed for Maps, but depending on how eagerly/lazily it is done, it may be deferred just enough not to be needed.
This is bit of a recurring issue, and although here it is clear failure should not occur, there are other cases where choice between eager/lazy failure is not even clear -- there is long-running debate on early failures being good (if something is broken it is much better to learn early) vs sometimes late failure avoids fail altogether (theoretical use that never occurs). This is sort of along latter trajectory.

[cowtowncoder]

Interesting note: if I first deserialize, THEN ask canDeserialize(), things... work. Fun fun.
But not in other order.

Actually. This seems to fail sometimes, pass at other times... so is not even stable. Even more fun.

[cowtowncoder]

Looks like I can reproduce this with a simplified version; oddly it is bit sensitive on naming of setters (possibly depends on certain ordering of conflicting methods?).

[cowtowncoder]

Some more background info: attempts to find properties are not entirely irrelevant wrt Maps (or Collections) since it is possible to have @JsonCreator that takes properties... and this is what code is looking for actually. And to figure out true configuration of properties, both mutators (setters) and accessors (getters) are considered, although with different precedence (serialization gives getters higher precedence and vice versa).

So figuring out how to make it all work is bit delicate...

[cowtowncoder]

Ok, one possibly simple approach would be to simply not sort properties if deserializing: order should only matter with serialization. Adding a check in POJOPropertiesCollector seems to prevent the issue. This might be the way to go -- but I'll have to think about this carefully as my spider sense is tingling that this is kind of small change that sometimes can have unexpected side effects.

[cowtowncoder]

Ah. The one case I did miss -- but that unit tests did not! -- is the POJO-as-array case.
When deserializing POJOs from as-array, ordering is 100% required unfortunately, and this is probably why sorting is forced if there is any indicator of order (and for which "any-indexed" is then called, which triggers the issue).

Back to Ye Old Drawing Board.

[cowtowncoder]

Still trying to figure out what to do here: problem is tricky.

But I did find one work-around as the problem is triggered by creation of ValueInstantiator: by using annotation @JsonValueInstantiator it is possible to assign custom one to prevent the need for default one (alas, registered one via Module interface does NOT work because default one is to be passed there :-( :-( :-( ).
So, with something like

    static class MyInstantiator extends ValueInstantiator.Base {
        public MyInstantiator() {
            super(HttpHeaders.class);
        }

        @Override
        public boolean canCreateUsingDefault() { return true; }
        @Override
        public Object createUsingDefault(DeserializationContext ctxt) {
            return new HttpHeaders();
        }
    }

and registered it using mix-in annotations

@JsonValueInstantor(MyInstantiator.class)
static class MixIn { }

mapper.addMixIn(HttpHeaders.class, MixIn.class);

will prevent this failure.