IllegalArgumentException: Conflicting setter definitions for property with more than 2 setters

[mistyzyq]

Hi,
I am using jackson-databind:2.11.1, for some special reason, I must provide multiple compatible setters, I got failed with the following case:

public class Data {
    // the value is the index of EnumValue
    private Integer value;

    public enum EnumValue {
        EV1, EV2, EV3
    }

    public String getValue() {
        return EnumValue.values()[value].name();
    }

    public void setValue(Integer value) {
        this.value = value;
    }
    public void setValue(EnumValue value) {
        this.value = value.ordinal();
    }
    public void setValue(String value) {
        this.value = EnumValue.valueOf(value).ordinal();
    }

    @Override
    public String toString() {
        return "Data [value=" + value + "]";
    }

    public static void main(String[] args) throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();

        Data data = new Data();
        data.setValue(EnumValue.EV3);

        String str = mapper.writeValueAsString(data);
        System.out.println(str);

        Data a2 = mapper.readValue(str, Data.class);
        System.out.print(a2);
    }
}

Try a few more times, sometimes it works fine, and sometimes it throws IllegalArgumentException (Conflicting setter definitions for property).

I have read this part of the source code, the course is probably in the POJOPropertyBuilder.getSetter() method. It seems prefer to the setter with String parameter, and can choose the right one (the one with String parameter) when there are 2 or less setters. In my case, there are 3, If the setter with String parameter is not first two, the exception will be thrown. but it seems that, the order of setters are indeterminate.

I've found a similar issue here #2741, but it was a different case.

[cowtowncoder]

Yes, you have multiple setValue() methods and Jackson cannot distinguish one to use from declarations: there must be just one. You will need to use @JsonProperty to indicate one you want.

As to why one seems to be used over another one, that seems suboptimal. But you are using an old version of jackson-databind so it might be worth checking with 2.11.4 first (in fact, before submitting an issue one always should at least try the latest patch of given minor version!); and if possible, 2.12.3.
Some checks have been added to try to better catch ambiguous cases.

[mistyzyq]

Thanks for your quick reply.

Of course, I can use @JsonProperty and @JsonIgnore to fix this problem, and I did it, so that our projects could publish on time.

I'm sorry, I the version I mentioned is not the latest one. I said I have read this part of the source code (on the latest master branch), but no changes are found. In fact, I tried both 2.11.4 and 2.12.3 just now, problems remain.

My confusion is, if jackson does not consider to support multiple setters, some exception should always be thrown. then, what does the following code doing?

jackson-databind/src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertyBuilder.java

Line 456 in a0ef91c

public AnnotatedMethod getSetter()

    @Override
    public AnnotatedMethod getSetter()
    {
        // Easy with zero or one setters...
        Linked<AnnotatedMethod> curr = _setters;
        if (curr == null) {
            return null;
        }
        Linked<AnnotatedMethod> next = curr.next;
        if (next == null) {
            return curr.value;
        }
        // But if multiple, verify that they do not conflict...
        for (; next != null; next = next.next) {
            // Allow masking, i.e. do not fail if one is in super-class from the other
            Class<?> currClass = curr.value.getDeclaringClass();
            Class<?> nextClass = next.value.getDeclaringClass();
            if (currClass != nextClass) {
                if (currClass.isAssignableFrom(nextClass)) { // next is more specific
                    curr = next;
                    continue;
                }
                if (nextClass.isAssignableFrom(currClass)) { // current more specific
                    continue;
                }
            }
            AnnotatedMethod nextM = next.value;
            AnnotatedMethod currM = curr.value;

            /* 30-May-2014, tatu: Two levels of precedence:
             * 
             * 1. Regular setters ("setX(...)")
             * 2. Implicit, possible setters ("x(...)")
             */
            int priNext = _setterPriority(nextM);
            int priCurr = _setterPriority(currM);

            if (priNext != priCurr) {
                if (priNext < priCurr) {
                    curr = next;
                }
                continue;
            }
            // 11-Dec-2015, tatu: As per [databind#1033] allow pluggable conflict resolution
            if (_annotationIntrospector != null) {
                AnnotatedMethod pref = _annotationIntrospector.resolveSetterConflict(_config,
                        currM, nextM);
                
                // note: should be one of nextM/currM; but no need to check
                if (pref == currM) {
                    continue;
                }
                if (pref == nextM) {
                    curr = next;
                    continue;
                }
            }
            throw new IllegalArgumentException(String.format(
 "Conflicting setter definitions for property \"%s\": %s vs %s",
 getName(), curr.value.getFullName(), next.value.getFullName()));
        }
        // One more thing; to avoid having to do it again...
        _setters = curr.withoutNext();
        return curr.value;
    }

If multiple setters are supported, the above code works fine, only if at least one setter with String type or assignable is found at the first two, but the order of _setters seems are unpredictable. This makes it looks like the Schrodinger's deserializing. Fortunately, we found out about this problem before we released it.

[cowtowncoder]

I agree: if there are 2 conflicting setters, problem should be reported -- no Heisenbugs allowed. I was just hoping that this was one of the cases that was patched against; I forget which issue changes were for (perhaps #426?) but there were changes to tighten detection fairly recently.

I can't spot the problem immediately but it sounds like there is one. String should only really be preferred in a limited number of cases of supertypes like also having setter for CharSequence (case of having java.lang.Object would be tricky one, not even sure if one should be preferable). Other than that, annotation or visibility could be tie-breaker but nothing here is applicable.

So this sounds like a bug, will leave open; hoping to have time to look into this. Fix should likely go in 2.13 since I have learned that quite often bugs like this are features someone somewhere relies on... (less surprises in patches, fixes need to go in minor releases)

[mistyzyq]

I don't think they are the same issue, I do not want to ignore this property.

I am not sure if I understand the real intention of the getSetter(). In my opinion, there may be a simple bug: the exception is thrown quite early before all setters were checked, and could be very easy to fix. Checking next.next == null before throwing the exception may be a way.

            // Throw exception after all setters are checked but failed.
            if (next.next == null) {
                throw new IllegalArgumentException(String.format(
                        "Conflicting setter definitions for property \"%s\": %s vs %s",
                        getName(), curr.value.getFullName(), next.value.getFullName()));
            }

However, this is just an immature idea of mine.

[cowtowncoder]

The code is there to do process as follows:

1. Collect all potential setters, getters, fields
2. Apply all information from visibility, naming, annotations, to find out how getters, setters, fields combine into a set of logical properties
3. Ensure that at most one accessor (thing to get value for serialization), at most one mutator (for deserializer) -- this to make sure there is no ambiguity
4. Expose the final set of properties to build POJO (de)serializers from

So getSetter() triggers final check that there is at most one mutator available. Some logic is used to try to see if in some cases we can resolve seeming conflict, but in general it is enough for just one pair-wise comparison to fail: it should not be necessary to see if there might be even more conflicts, one is enough.

In this case what I don't understand is why and how ordering of setters would prevent conflict indication: to me this seems like a clear case of conflict.

[mistyzyq]

Thanks for your explanation. I still don't understand, why two setters (with String or assignable type) works if at most one mutator is accepted. Is it considered for super class?

Back to the getSetter() question. In this case, there were three setters, with type of Integer, EnumValue, and String, the first two (Integer and EnumValue) are not for deserializer, and are not acceptable as valid mutator by jackson.

Let's try the following case, In the brackets is the type of _setters with ordering. (Please also refer to the code of getSetter() above):

1. [`Integer`, `String`] -> Success
2. [`EnumValue`, `String`] -> Success
3. [`Integer`, `EnumValue`] -> Fail
4. [`Integer`, `String`, `EnumValue`] -> Success
5. [`String`, `Integer`, `EnumValue`] -> Success
6. [`Integer`, `EnumValue`, `String`] -> Fail
7. [`EnumValue`, `Integer`, `String`] -> Fail

These cases should be able to explain the problem. All goes well till these lines:

            if (_annotationIntrospector != null) {
                AnnotatedMethod pref = _annotationIntrospector.resolveSetterConflict(_config,
                        currM, nextM);
                
                // note: should be one of nextM/currM; but no need to check
                if (pref == currM) {
                    continue;
                }
                if (pref == nextM) {
                    curr = next;
                    continue;
                }
            }
            throw new IllegalArgumentException(String.format(
 "Conflicting setter definitions for property \"%s\": %s vs %s",
 getName(), curr.value.getFullName(), next.value.getFullName()));

The resolveSetterConflict() method seems prefer to primitive type and String type:

1. If any one of the first two satisfy the condition, pref will be equals to currM or nextM, the loop can continue, and finally the exact one (with String type) will be found.
2. Otherwise, pref is null, then IllegalArgumentException is thrown. The setter with String type has not been checked till now.

[cowtowncoder]

On setter: key questions is this: is there at most one obvious choice to use or not?
There are couple of rules that may decide between 2 alternatives: String vs CharSequence is one such heuristics. Sub-class vs super-class is another (one defined in sub-class is used). Otherwise, an exception SHOULD be reported. Looks like this does not work here.

But as to 3 choices, 2 not acceptable -- not acceptable based on what? Why wouldn't they be?

    public void setValue(Integer value) {
        this.value = value;
    }
    public void setValue(EnumValue value) {
        this.value = value.ordinal();
    }
    public void setValue(String value) {
        this.value = EnumValue.valueOf(value).ordinal();
    }

Keep in mind that resolution has no knowledge of kind of input there might be: so it would not be making any choices like "can not use Integer since I get JSON String" -- Jackson decides on setter to use without input to consider.

But looking at resolveSetterConflict(); I think it is wrong -- it should NOT be simply preferring any primitive over any non-primitive. It SHOULD consider pairs: int over Integer, char over Character. That is, this is the intent as far as I remember. I should fix that issue.

As to why String is simply preferred over other cases, that is... interesting. I do not remember why this should be the case. But it would explain why no conflict is reported here.

[cowtowncoder]

Ok. So reading through resolveSetterConflict(), you are right, and my understanding wrt resolution was incorrect. It is also true that there is a problem with resolution as currently presented.

Actual precedence is, as you stated:

1. Any primitive (oddly enough, nothing about wrappers)
2. Any String
3. Others

This is at point where other rules would have been applied (annotation trumps non-annotated; sub-class preferred over super-class).

Changes were made in 2.7.0, under #1044, so logic is at this point expected default for Jackson 2.x.

In that sense, it seems that String setter should be selected.

But the remaining big problem is the fact that pair-wise detection does not work without doing full set of calls, because a pair of instances with "other" type might be selected first.

I will have to think about this a bit.

[cowtowncoder]

I think I know at high level how this should work -- as suggested, kind of, existence of 2 equal-precedence setters does NOT in itself yet signal a problem; rather, in such case, processing needs to continue to possibly find something at higher level (in this case String valued setter). This is still doable with existing resolveSetterConflict(), fortunately.
But it won't work simply by continuing processing as-is; a bit more work is needed.

[mistyzyq]

Yes! I think I have provided enough information for the problem.

These cases are generally rare. In fact, I originally thought that Jackson could automatically decide which setter to use according to the type of the input data to be deserialized. This is due to the compatibility problem of the two versions. The serialized output of the old version is String (name of enum), but the new version output is Integer (ordinal of enum) (This may not be a good design, but sometimes it has to be). From the rules of java method overloading, there should be no conflict. If Jackson can better support this feature, it will be even more perfect.

Of course, Jackson can make its own rules and do strict inspections. This case is a performance that is inconsistent with the expectations of the rules.

By the way, to this problem, currently, my final solution is to add @JsonIgnore annotations to all three setters, and then add a new setter with a parameter type of Object and add @JsonProperty annotations to it. I do the corresponding processing inside according to the data type myself.

[cowtowncoder]

Yes I finally understand the problem and once I have time will solve it; the problem is just doing it cleanly. :)

As to working around the issue: no need to @JsonIgnore if you add one @JsonProperty (or @JsonSetter if you prefer) on the setter you want. That will settle the situation fine; implicit (non-annotated) candidates are not used if there is explicit "winner".