Refactoring to start solving #3125

Author: cowtowncoder

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

@@ -524,6 +524,20 @@ public Class<?>[] findViews(Annotated a)
         return (ann == null) ? null : ann.value();
     }
 
+    /**
+     * Specific implementation that will use following tie-breaker on
+     * given setter parameter types:
+     *<ol>
+     * <li>If either one is primitive type then either return {@code null}
+     *   (both primitives) or one that is primitive (when only primitive)
+     *  </li>
+     * <li>If only one is of type {@code String}, return that setter
+     *  </li>
+     * <li>Otherwise return {@code null}
+     *  </li>
+     * </ol>
+     * Returning {@code null} will indicate that resolution could not be done.
+     */
     @Override // since 2.7
     public AnnotatedMethod resolveSetterConflict(MapperConfig<?> config,
             AnnotatedMethod setter1, AnnotatedMethod setter2)
@@ -537,10 +551,12 @@ public AnnotatedMethod resolveSetterConflict(MapperConfig<?> config,
             if (!cls2.isPrimitive()) {
                 return setter1;
             }
+            // 10-May-2021, tatu: if both primitives cannot decide
+            return null;
         } else if (cls2.isPrimitive()) {
             return setter2;
         }
-        
+
         if (cls1 == String.class) {
             if (cls2 != String.class) {
                 return setter1;
@@ -558,7 +574,7 @@ public PropertyName findRenameByField(MapperConfig<?> config,
         // Nothing to report, only used by modules. But define just as documentation
         return null;
     }
-    
+
     /*
     /**********************************************************
     /* Annotations for Polymorphic Type handling

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

@@ -466,58 +466,70 @@ public AnnotatedMethod getSetter()
         }
         // 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;
-                }
+            AnnotatedMethod selected = _selectSetter(curr.value, next.value);
+            if (selected == curr.value) {
                 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;
-                }
+            if (selected == next.value) {
+                curr = next;
+                continue;
             }
-            throw new IllegalArgumentException(String.format(
- "Conflicting setter definitions for property \"%s\": %s vs %s",
- getName(), curr.value.getFullName(), next.value.getFullName()));
+            // 10-May-2021, tatu: unbreakable tie, for now; offline handling
+            return _selectSetterFromMultiple(curr, next);
         }
+
         // One more thing; to avoid having to do it again...
         _setters = curr.withoutNext();
         return curr.value;
     }
 
+    // @since 2.13
+    protected AnnotatedMethod _selectSetter(AnnotatedMethod currM, AnnotatedMethod nextM)
+    {
+        // Allow masking, i.e. do not fail if one is in super-class from the other
+        final Class<?> currClass = currM.getDeclaringClass();
+        final Class<?> nextClass = nextM.getDeclaringClass();
+        if (currClass != nextClass) {
+            if (currClass.isAssignableFrom(nextClass)) { // next is more specific
+                return nextM;
+            }
+            if (nextClass.isAssignableFrom(currClass)) { // current more specific
+                return currM;
+            }
+        }
+
+        /* 30-May-2014, tatu: Two levels of precedence:
+         * 
+         * 1. Regular setters ("setX(...)")
+         * 2. Implicit, possible setters ("x(...)")
+         */
+        // 25-Apr-2021, tatu: This is probably wrong, should not rely on
+        //    hard-coded "set" prefix here.
+        int priNext = _setterPriority(nextM);
+        int priCurr = _setterPriority(currM);
+
+        if (priNext != priCurr) {
+            // Smaller value, higher; so, if next has higher precedence:
+            if (priNext < priCurr) {
+                return nextM;
+            }
+            // otherwise current one has, proceed
+            return currM;
+        }
+        // 11-Dec-2015, tatu: As per [databind#1033] allow pluggable conflict resolution
+        return (_annotationIntrospector == null) ? null
+                : _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,6 +231,14 @@ 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
@@ -492,7 +500,22 @@ 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)
     {