Fix #3125

Author: cowtowncoder

release-notes/VERSION-2.x

@@ -30,6 +30,9 @@ Project: jackson-databind
 #3117: Use more limiting default visibility settings for JDK types (java.*, javax.*)
 #3122: Deep merge for `JsonNode` using `ObjectReader.readTree()`
  (reported by Eric S)
+#3125: IllegalArgumentException: Conflicting setter definitions for property
+  with more than 2 setters
+ (reported by mistyzyq@github)
 #3130: Serializing java.lang.Thread fails on JDK 11 and above (should suppress
   serialization of ClassLoader)
 - Fix to avoid problem with `BigDecimalNode`, scale of `Integer.MIN_VALUE` (see

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

@@ -1,6 +1,7 @@
 package com.fasterxml.jackson.databind.introspect;
 
 import java.util.*;
+import java.util.stream.Collectors;
 
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -483,6 +484,59 @@ public AnnotatedMethod getSetter()
         return curr.value;
     }
 
+    /**
+     * Helper method called in cases where we have encountered two setter methods
+     * that have same precedence and cannot be resolved. This does not yet necessarily
+     * mean a failure since it is possible something with a higher precedence could
+     * still be found; handling is just separated into separate method for convenience.
+     *
+     * @param curr
+     * @param next
+     *
+     * @return Chosen setter method, if any
+     *
+     * @throws IllegalArgumentException If conflict could not be resolved
+     *
+     * @since 2.13
+     */
+    protected AnnotatedMethod _selectSetterFromMultiple(Linked<AnnotatedMethod> curr,
+            Linked<AnnotatedMethod> next)
+    {
+        // First: store reference to the initial possible conflict
+        List<AnnotatedMethod> conflicts = new ArrayList<>();
+        conflicts.add(curr.value);
+        conflicts.add(next.value);
+
+        next = next.next;
+        for (; next != null; next = next.next) {
+            AnnotatedMethod selected = _selectSetter(curr.value, next.value);
+            if (selected == curr.value) {
+                // No change, next was lower-precedence
+                continue;
+            }
+            if (selected == next.value) {
+                // Hooray! Found a higher-priority one; clear conflict list
+                conflicts.clear();
+                curr = next;
+                continue;
+            }
+            // Tie means one more non-resolved, add
+            conflicts.add(next.value);
+        }
+
+        // It is possible we resolved it; if so:
+        if (conflicts.isEmpty()) {
+            _setters = curr.withoutNext();
+            return curr.value;
+        }
+        // Otherwise
+        String desc = conflicts.stream().map(AnnotatedMethod::getFullName)
+                .collect(Collectors.joining(" vs "));
+        throw new IllegalArgumentException(String.format(
+                "Conflicting setter definitions for property \"%s\": %s",
+                getName(), desc));
+    }
+
     // @since 2.13
     protected AnnotatedMethod _selectSetter(AnnotatedMethod currM, AnnotatedMethod nextM)
     {
@@ -521,15 +575,6 @@ protected AnnotatedMethod _selectSetter(AnnotatedMethod currM, AnnotatedMethod n
                 : _annotationIntrospector.resolveSetterConflict(_config, currM, nextM);
     }
 
-    // @since 2.13
-    protected AnnotatedMethod _selectSetterFromMultiple(Linked<AnnotatedMethod> curr,
-            Linked<AnnotatedMethod> next)
-    {
-        throw new IllegalArgumentException(String.format(
-                "Conflicting setter definitions for property \"%s\": %s vs %s",
-                getName(), curr.value.getFullName(), next.value.getFullName()));
-    }
-
     @Override
     public AnnotatedField getField()
     {

src/test/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollectorTest.java

@@ -231,14 +231,6 @@ public DuplicateGetterCreatorBean(@JsonProperty("bloop") @A boolean bloop) {}
         public boolean getBloop() { return true; }
     }
 
-    // [databind#3125]: As per existing (2.7+) logic we SHOULD tie-break
-    // in favor of `String` but code up until 2.12 short-circuited early fail
-    static class DupSetter3125Bean {
-        public void setValue(Integer value) { }
-        public void setValue(Boolean value) { }
-        public void setValue(String value) { }
-    }
-
     /*
     /**********************************************************
     /* Unit tests
@@ -500,22 +492,7 @@ public void testDuplicateGettersCreator() throws Exception
         assertNotNull(prop._getters.next);
         assertTrue(prop._getters.next.value.hasAnnotation(A.class));
     }
-/*
-    // [databind#3125]
-    public void testDuplicateSetters() throws Exception
-    {
-        POJOPropertiesCollector coll = collector(MAPPER, DupSetter3125Bean.class,
-                false);
-        final List<BeanPropertyDefinition> props = coll.getProperties();
-        assertEquals(1, props.size());
-        POJOPropertyBuilder prop = (POJOPropertyBuilder) props.get(0);
-        assertEquals("value", prop.getName());
-        // but this failed
-        AnnotatedMethod m = prop.getSetter();
-        assertNotNull(m);
-        assertEquals(String.class, m.getRawParameterType(0));
-    }
-*/
+
     private void _verifyProperty(BeanDescription beanDesc,
     		boolean verifyDesc, boolean verifyIndex, String expDefaultValue)
     {
@@ -551,10 +528,11 @@ private void _verifyProperty(BeanDescription beanDesc,
             }
         }
     }
+
     /*
-    /**********************************************************
+    /**********************************************************************
     /* Helper methods
-    /**********************************************************
+    /**********************************************************************
      */
 
     protected POJOPropertiesCollector collector(ObjectMapper m0,

src/test/java/com/fasterxml/jackson/databind/introspect/SetterConflictTest.java

@@ -1,7 +1,11 @@
 package com.fasterxml.jackson.databind.introspect;
 
+import java.util.List;
+
 import com.fasterxml.jackson.annotation.JsonSetter;
+
 import com.fasterxml.jackson.databind.*;
+import com.fasterxml.jackson.databind.exc.InvalidDefinitionException;
 
 // mostly for [databind#1033]
 public class SetterConflictTest extends BaseMapTest
@@ -30,10 +34,26 @@ public void setBloop(Object bloop) {
         }
     }
 
+    // [databind#3125]: As per existing (2.7+) logic we SHOULD tie-break
+    // in favor of `String` but code up until 2.12 short-circuited early fail
+    static class DupSetter3125Bean {
+        String str;
+        
+        public void setValue(Integer value) { throw new RuntimeException("Integer: wrong!"); }
+        public void setValue(Boolean value) { throw new RuntimeException("Boolean: wrong!"); }
+        public void setValue(String value) { str = value; }
+    }
+
+    static class DupSetter3125BeanFail {
+        public void setValue(Integer value) { throw new RuntimeException("Integer: wrong!"); }
+        public void setValue(Boolean value) { throw new RuntimeException("Boolean: wrong!"); }
+        public void setValue(List<String> value) { throw new RuntimeException("List: wrong!"); }
+    }
+
     /*
-    /**********************************************************
+    /**********************************************************************
     /* Test methods
-    /**********************************************************
+    /**********************************************************************
      */
 
     private final ObjectMapper MAPPER = newJsonMapper();
@@ -56,4 +76,55 @@ public void testConflictingSetters() throws Exception
                 DuplicateSetterBean2979.class);
         assertEquals(Boolean.TRUE, result.value);
     }
+
+    // [databind#3125]
+    public void testDuplicateSetterResolutionOk() throws Exception
+    {
+        POJOPropertiesCollector coll = collector(MAPPER, DupSetter3125Bean.class,
+                false);
+        final List<BeanPropertyDefinition> props = coll.getProperties();
+        assertEquals(1, props.size());
+        POJOPropertyBuilder prop = (POJOPropertyBuilder) props.get(0);
+        assertEquals("value", prop.getName());
+        // but this failed
+        AnnotatedMethod m = prop.getSetter();
+        assertNotNull(m);
+        assertEquals(String.class, m.getRawParameterType(0));
+
+        // and then actual usage too
+        DupSetter3125Bean value = MAPPER.readValue(a2q("{'value':'foo'}"),
+                DupSetter3125Bean.class);
+        assertEquals("foo", value.str);
+    }
+
+    // [databind#3125]: caught case
+    public void testDuplicateSetterResolutionFail() throws Exception
+    {
+        try {
+            MAPPER.readValue(a2q("{'value':'foo'}"),
+                    DupSetter3125BeanFail.class);
+            fail("Should not pass");
+        } catch (InvalidDefinitionException e) {
+            verifyException(e, "Conflicting setter definitions for property \"value\"");
+        }
+    }
+
+    /*
+    /**********************************************************************
+    /* Helper methods
+    /**********************************************************************
+     */
+
+    protected POJOPropertiesCollector collector(ObjectMapper m0,
+            Class<?> cls, boolean forSerialization)
+    {
+        BasicClassIntrospector bci = new BasicClassIntrospector();
+        // no real difference between serialization, deserialization, at least here
+        if (forSerialization) {
+            return bci.collectProperties(m0.getSerializationConfig(),
+                    m0.constructType(cls), null, true);
+        }
+        return bci.collectProperties(m0.getDeserializationConfig(),
+                m0.constructType(cls), null, false);
+    }
 }