Merge #4515 from 2.18 to master (#4547)

Author: cowtowncoder

src/main/java/tools/jackson/databind/deser/BasicDeserializerFactory.java

@@ -609,6 +609,8 @@ protected void _addCreators(Map<String, POJOPropertyBuilder> props)
         // Next: remove creators marked as explicitly disabled
         _removeDisabledCreators(constructors);
         _removeDisabledCreators(factories);
+        // And then remove non-annotated static methods that do not look like factories
+        _removeNonFactoryStaticMethods(factories);
 
         // and use annotations to find explicitly chosen Creators
         if (_useAnnotations) { // can't have explicit ones without Annotation introspection
@@ -630,13 +632,37 @@ protected void _addCreators(Map<String, POJOPropertyBuilder> props)
         // (JDK 17 Record/Scala/Kotlin)
         if (!creators.hasPropertiesBased()) {
             // for Records:
-            if ((canonical != null) && constructors.contains(canonical)) {
-                constructors.remove(canonical);
-                creators.setPropertiesBased(_config, canonical, "canonical");
+            if (canonical != null) {
+                // ... but only process if still included as a candidate
+                if (constructors.remove(canonical)) {
+                    // But wait! Could be delegating
+                    if (_isDelegatingConstructor(canonical)) {
+                        creators.addExplicitDelegating(canonical);
+                    } else {
+                        creators.setPropertiesBased(_config, canonical, "canonical");
+                    }
+                }
+            }
+        }
+
+        // One more thing: if neither explicit (constructor or factory) nor
+        // canonical (constructor?), consider implicit Constructor with
+        // all named.
+        final ConstructorDetector ctorDetector = _config.getConstructorDetector();
+        if (!creators.hasPropertiesBasedOrDelegating()
+                && !ctorDetector.requireCtorAnnotation()) {
+            // But only if no default constructor available OR if we are configured
+            // to prefer properties-based Creators
+            if ((_classDef.getDefaultConstructor() == null)
+                    || ctorDetector.singleArgCreatorDefaultsToProperties()) {
+                _addImplicitConstructor(creators, constructors, props);
             }
         }
 
         // Anything else left, add as possible implicit Creators
+        // ... but first, trim non-visible
+        _removeNonVisibleCreators(constructors);
+        _removeNonVisibleCreators(factories);
         creators.setImplicitDelegating(constructors, factories);
 
         // And finally add logical properties for the One Properties-based
@@ -650,6 +676,20 @@ protected void _addCreators(Map<String, POJOPropertyBuilder> props)
         }
     }
 
+    // Method to determine if given non-explictly-annotated constructor
+    // looks like delegating one
+    private boolean _isDelegatingConstructor(PotentialCreator ctor)
+    {
+        // Only consider single-arg case, for now
+        if (ctor.paramCount() == 1) {
+            // Main thing: @JsonValue makes it delegating:
+            if ((_jsonValueAccessors != null) && !_jsonValueAccessors.isEmpty()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private List<PotentialCreator> _collectCreators(List<? extends AnnotatedWithParams> ctors)
     {
         if (ctors.isEmpty()) {
@@ -675,6 +715,49 @@ private void _removeDisabledCreators(List<PotentialCreator> ctors)
         }
     }
 
+    private void _removeNonVisibleCreators(List<PotentialCreator> ctors)
+    {
+        Iterator<PotentialCreator> it = ctors.iterator();
+        while (it.hasNext()) {
+            PotentialCreator ctor = it.next();
+            boolean visible = (ctor.paramCount() == 1)
+                    ? _visibilityChecker.isScalarConstructorVisible(ctor.creator())
+                    : _visibilityChecker.isCreatorVisible(ctor.creator());
+            if (!visible) {
+                it.remove();
+            }
+        }
+    }
+
+    private void _removeNonFactoryStaticMethods(List<PotentialCreator> ctors)
+    {
+        final Class<?> rawType = _type.getRawClass();
+        Iterator<PotentialCreator> it = ctors.iterator();
+        while (it.hasNext()) {
+            // explicit mode? Retain (for now)
+            PotentialCreator ctor = it.next();
+            if (ctor.creatorMode() != null) {
+                continue;
+            }
+            // Copied from `BasicBeanDescription.isFactoryMethod()`
+            AnnotatedWithParams factory = ctor.creator();
+            if (rawType.isAssignableFrom(factory.getRawType())
+                    && ctor.paramCount() == 1) {
+                String name = factory.getName();
+
+                if ("valueOf".equals(name)) {
+                    continue;
+                } else if ("fromString".equals(name)) {
+                    Class<?> cls = factory.getRawParameterType(0);
+                    if (cls == String.class || CharSequence.class.isAssignableFrom(cls)) {
+                        continue;
+                    }
+                }
+            }
+            it.remove();
+        }
+    }
+
     private void _addExplicitlyAnnotatedCreators(PotentialCreators collector,
             List<PotentialCreator> ctors,
             Map<String, POJOPropertyBuilder> props,
@@ -690,48 +773,25 @@ private void _addExplicitlyAnnotatedCreators(PotentialCreators collector,
             if (ctor.creatorMode() == null) {
                 continue;
             }
+
             it.remove();
 
-            Boolean propsBased = null;
-            
+            boolean isPropsBased;
+
             switch (ctor.creatorMode()) {
             case DELEGATING:
-                propsBased = false;
+                isPropsBased = false;
                 break;
             case PROPERTIES:
-                propsBased = true;
+                isPropsBased = true;
                 break;
             case DEFAULT:
             default:
-                // First things first: if not single-arg Creator, must be Properties-based
-                // !!! Or does it? What if there's @JacksonInject etc?
-                if (ctor.paramCount() != 1) {
-                    propsBased = true;
-                }
+                isPropsBased = _isExplicitlyAnnotatedCreatorPropsBased(ctor,
+                        props, ctorDetector);
             }
 
-            // Must be 1-arg case:
-            if (propsBased == null) {
-                // Is ambiguity/heuristics allowed?
-                if (ctorDetector.requireCtorAnnotation()) {
-                    throw new IllegalArgumentException(String.format(
-                            "Ambiguous 1-argument Creator; `ConstructorDetector` requires specifying `mode`: %s",
-                            ctor));
-                }
-    
-                // First things first: if explicit names found, is Properties-based
-                ctor.introspectParamNames(_config);
-                propsBased = ctor.hasExplicitNames()
-                        || ctorDetector.singleArgCreatorDefaultsToProperties();
-                // One more possibility: implicit name that maps to implied
-                // property based on Field/Getter/Setter
-                if (!propsBased) {
-                    String implName = ctor.implicitNameSimple(0);
-                    propsBased = (implName != null) && props.containsKey(implName);
-                }
-            }
-
-            if (propsBased) {
+            if (isPropsBased) {
                 // Skipping done if we already got higher-precedence Creator
                 if (!skipPropsBased) {
                     collector.setPropertiesBased(_config, ctor, "explicit");
@@ -742,6 +802,56 @@ private void _addExplicitlyAnnotatedCreators(PotentialCreators collector,
         }
     }
 
+    private boolean _isExplicitlyAnnotatedCreatorPropsBased(PotentialCreator ctor,
+            Map<String, POJOPropertyBuilder> props, ConstructorDetector ctorDetector)
+    {
+        if (ctor.paramCount() == 1) {
+            // Is ambiguity/heuristics allowed?
+            switch (ctorDetector.singleArgMode()) {
+            case DELEGATING:
+                return false;
+            case PROPERTIES:
+                return true;
+            case REQUIRE_MODE:
+                throw new IllegalArgumentException(String.format(
+"Single-argument constructor (%s) is annotated but no 'mode' defined; `ConstructorDetector`"
++ "configured with `SingleArgConstructor.REQUIRE_MODE`",
+ctor.creator()));
+            case HEURISTIC:
+            default:
+            }
+        }
+
+        // First: if explicit names found, is Properties-based
+        ctor.introspectParamNames(_config);
+        if (ctor.hasExplicitNames()) {
+            return true;
+        }
+        // Second: [databind#3180] @JsonValue indicates delegating
+        if ((_jsonValueAccessors != null) && !_jsonValueAccessors.isEmpty()) {
+            return false;
+        }
+        if (ctor.paramCount() == 1) {
+            // One more possibility: implicit name that maps to implied
+            // property with at least one visible accessor
+            String implName = ctor.implicitNameSimple(0);
+            if (implName != null) {
+                POJOPropertyBuilder prop = props.get(implName);
+                if ((prop != null) && prop.anyVisible() && !prop.anyIgnorals()) {
+                    return true;
+                }
+            }
+            // Second: injectable also suffices
+            if ((_annotationIntrospector != null)
+                    && _annotationIntrospector.findInjectableValue(_config, ctor.param(0)) != null) {
+                return true;
+            }
+            return false;
+        }
+        // Trickiest case: rely on existence of implicit names and/or injectables
+        return ctor.hasNameOrInjectForAllParams(_config);
+    }
+
     private void _addCreatorsWithAnnotatedNames(PotentialCreators collector,
             List<PotentialCreator> ctors)
     {
@@ -760,6 +870,55 @@ private void _addCreatorsWithAnnotatedNames(PotentialCreators collector,
         }
     }
 
+    private boolean _addImplicitConstructor(PotentialCreators collector,
+            List<PotentialCreator> ctors, Map<String, POJOPropertyBuilder> props)
+    {
+        // Must have one and only one candidate
+        if (ctors.size() != 1) {
+            return false;
+        }
+        final PotentialCreator ctor = ctors.get(0);
+        // which needs to be visible
+        final boolean visible = (ctor.paramCount() == 1)
+                ? _visibilityChecker.isScalarConstructorVisible(ctor.creator())
+                : _visibilityChecker.isCreatorVisible(ctor.creator());
+        if (!visible) {
+            return false;
+        }
+        ctor.introspectParamNames(_config);
+
+        // As usual, 1-param case is distinct
+        if (ctor.paramCount() != 1) {
+            if (!ctor.hasNameOrInjectForAllParams(_config)) {
+                return false;
+            }
+        } else {
+            // First things first: if only param has Injectable, must be Props-based
+            if ((_annotationIntrospector != null)
+                    && _annotationIntrospector.findInjectableValue(_config, ctor.param(0)) != null) {
+                // props-based, continue
+            } else {
+                // may have explicit preference
+                final ConstructorDetector ctorDetector = _config.getConstructorDetector();
+                if (ctorDetector.singleArgCreatorDefaultsToDelegating()) {
+                    return false;
+                }
+                // if not, prefer Properties-based if explicit preference OR
+                // property with same name with at least one visible accessor
+                if (!ctorDetector.singleArgCreatorDefaultsToProperties()) {
+                    POJOPropertyBuilder prop = props.get(ctor.implicitNameSimple(0));
+                    if ((prop == null) || !prop.anyVisible() || prop.anyIgnorals()) {
+                        return false;
+                    }
+                }
+            }
+        }
+
+        ctors.remove(0);
+        collector.setPropertiesBased(_config, ctor, "implicit");
+        return true;
+    }
+
     private void _addCreatorParams(Map<String, POJOPropertyBuilder> props,
             PotentialCreator ctor, List<POJOPropertyBuilder> creatorProps)
     {

src/main/java/tools/jackson/databind/introspect/POJOPropertiesCollector.java

@@ -199,18 +199,15 @@ public void testDeserializeUsingCanonicalConstructor_WhenJsonCreatorConstructorE
         }
     }
 
+    // 23-May-2024, tatu: Logic changed as part of [databind#4515]: explicit properties-based
+    //   Creator does NOT block implicit delegating Creators. So formerly (pre-2.18) failing
+    //   case is now expected to pass.
     @Test
     public void testDeserializeUsingImplicitFactoryMethod_WhenJsonCreatorConstructorExists_WillFail() throws Exception {
-        try {
-            MAPPER.readValue("123", RecordWithJsonPropertyWithJsonCreator.class);
-
-            fail("should not pass");
-        } catch (MismatchedInputException e) {
-            verifyException(e, "Cannot construct instance");
-            verifyException(e, "RecordWithJsonPropertyWithJsonCreator");
-            verifyException(e, "although at least one Creator exists");
-            verifyException(e, "no int/Int-argument constructor/factory method");
-        }
+        RecordWithJsonPropertyWithJsonCreator value = MAPPER.readValue("123",
+                RecordWithJsonPropertyWithJsonCreator.class);
+        assertEquals(123, value.id());
+        assertEquals("JsonCreatorConstructor", value.name());
     }
 
     /*

src/test-jdk17/java/tools/jackson/databind/records/RecordExplicitCreatorsTest.java

@@ -9,6 +9,7 @@
 import tools.jackson.databind.*;
 import tools.jackson.databind.cfg.*;
 import tools.jackson.databind.exc.InvalidDefinitionException;
+import tools.jackson.databind.exc.UnrecognizedPropertyException;
 import tools.jackson.databind.introspect.AnnotatedMember;
 import tools.jackson.databind.introspect.JacksonAnnotationIntrospector;
 import tools.jackson.databind.json.JsonMapper;
@@ -147,13 +148,20 @@ public void testMultiArgAsProperties() throws Exception
     @Test
     public void test1ArgDefaultsToPropsMultipleCtors() throws Exception
     {
+        // 23-May-2024, tatu: Will fail differently with [databind#4515]; default
+        //   constructor available, implicit ones ignored
         try {
             MAPPER_PROPS.readValue(a2q("{'value' : 137 }"),
                 SingleArg2CtorsNotAnnotated.class);
             fail("Should not pass");
+        } catch (UnrecognizedPropertyException e) {
+            verifyException(e, "\"value\"");
+        }
+            /*
         } catch (InvalidDefinitionException e) {
             verifyException(e, "Conflicting property-based creators");
         }
+        */
     }
 
     /*

src/test/java/tools/jackson/databind/deser/creators/ConstructorDetector1498Test.java

@@ -183,7 +183,12 @@ public void testSingleStringArgWithImplicitName() throws Exception
         final ObjectMapper mapper = jsonMapperBuilder()
                 .annotationIntrospector(new MyParamIntrospector("value"))
                 .build();
-        StringyBean bean = mapper.readValue(q("foobar"), StringyBean.class);
+        // 23-May-2024, tatu: [databind#4515] Clarifies handling to make
+        //   1-param Constructor with implicit name auto-discoverable
+        //   This is compatibility change so hopefully won't bite us but...
+        //   it seems like the right thing to do.
+//        StringyBean bean = mapper.readValue(q("foobar"), StringyBean.class);
+        StringyBean bean = mapper.readValue(a2q("{'value':'foobar'}"), StringyBean.class);
         assertEquals("foobar", bean.getValue());
     }
 

src/test/java/tools/jackson/databind/deser/creators/SingleArgCreatorTest.java

@@ -4,7 +4,6 @@
 import java.math.BigInteger;
 import java.util.*;
 
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import com.fasterxml.jackson.annotation.*;
@@ -61,14 +60,14 @@ protected BooleanConstructorBean2(boolean b) {
     }
 
     static class DoubleConstructorBean {
-        Double d; // cup?
+        Double d;
         @JsonCreator protected DoubleConstructorBean(Double d) {
             this.d = d;
         }
     }
 
     static class FactoryBean {
-        double d; // teehee
+        double d;
 
         private FactoryBean(double value, boolean dummy) { d = value; }
 
@@ -413,7 +412,6 @@ public void testStringFactoryAlt() throws Exception
     //    handling seems inconsistent wrt Constructor/Factory precedence,
     //    will tackle at a later point -- this is the last JDK8 fail
     @Test
-    @Disabled
     public void testConstructorAndFactoryCreator() throws Exception
     {
         CreatorBeanWithBoth bean = MAPPER.readValue