Finish BasicDeserializerFactory refactoring (complete #4515) (#4543)

Author: cowtowncoder

release-notes/VERSION-2.x

@@ -24,6 +24,7 @@ Project: jackson-databind
  (contributed by @pjfanning)
 #4483: Remove `final` on method BeanSerializer.serialize()
  (contributed by Matthew L)
+#4515: Rewrite Bean Property Introspection logic in Jackson 2.x
 
 2.17.1 (04-May-2024)
 

src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

@@ -259,6 +259,11 @@ protected boolean _isStdJDKCollection(JavaType type)
             //    of matches), or ambitious? Let's do latter for now.
             if (Collection.class.isAssignableFrom(raw)
                     || Map.class.isAssignableFrom(raw)) {
+                // 28-May-2024, tatu: Complications wrt [databind#4515] / [databind#2795]
+                //    mean that partial introspection NOT for inner classes
+                if (raw.toString().indexOf('$') > 0) {
+                    return false;
+                }
                 return true;
             }
         }

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

@@ -662,6 +662,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
@@ -683,9 +685,30 @@ 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);
             }
         }
 
@@ -703,6 +726,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()) {
@@ -728,6 +765,35 @@ private void _removeDisabledCreators(List<PotentialCreator> ctors)
         }
     }
 
+    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,
@@ -743,48 +809,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;
-                }
-            }
-
-            // 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);
-                }
+                isPropsBased = _isExplicitlyAnnotatedCreatorPropsBased(ctor,
+                        props, ctorDetector);
             }
 
-            if (propsBased) {
+            if (isPropsBased) {
                 // Skipping done if we already got higher-precedence Creator
                 if (!skipPropsBased) {
                     collector.setPropertiesBased(_config, ctor, "explicit");
@@ -795,6 +838,53 @@ 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 based on Field/Getter/Setter
+            String implName = ctor.implicitNameSimple(0);
+            if ((implName != null) && props.containsKey(implName)) {
+                return true;
+            }
+            // Second: injectable also suffices
+            if ((_annotationIntrospector != null)
+                    && _annotationIntrospector.findInjectableValue(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)
     {
@@ -813,6 +903,50 @@ 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
+        if (!_visibilityChecker.isCreatorVisible(ctor.creator())) {
+            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(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
+                if (!ctorDetector.singleArgCreatorDefaultsToProperties()
+                        && !props.containsKey(ctor.implicitNameSimple(0))) {
+                    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/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java

@@ -142,10 +142,28 @@ public boolean hasExplicitNames() {
         return false;
     }
 
+    public boolean hasNameFor(int ix) {
+        return (explicitParamNames[ix] != null)
+                || (implicitParamNames[ix] != null);
+    }
+
+    public boolean hasNameOrInjectForAllParams(MapperConfig<?> config)
+    {
+        final AnnotationIntrospector intr = config.getAnnotationIntrospector();
+        for (int i = 0, end = implicitParamNames.length; i < end; ++i) {
+            if (!hasNameFor(i)) {
+                if (intr == null || intr.findInjectableValue(creator.getParameter(i)) == null) {
+                    return false;
+                }
+            }
+        }
+        return true;
+    }
+
     public PropertyName explicitName(int ix) {
         return explicitParamNames[ix];
     }
-    
+
     public PropertyName implicitName(int ix) {
         return implicitParamNames[ix];
     }

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

@@ -69,10 +69,10 @@ public List<PotentialCreator> getExplicitDelegating() {
     }
 
     public List<PotentialCreator> getImplicitDelegatingFactories() {
-        return implicitDelegatingFactories;
+        return (implicitDelegatingFactories == null) ? Collections.emptyList() : implicitDelegatingFactories;
     }
 
     public List<PotentialCreator> getImplicitDelegatingConstructors() {
-        return implicitDelegatingConstructors;
+        return (implicitDelegatingConstructors == null) ? Collections.emptyList() : implicitDelegatingConstructors;
     }
 }

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

@@ -34,7 +34,7 @@ public RecordWithDelegation(String value) {
             this.value = "del:"+value;
         }
 
-        @JsonValue()
+        @JsonValue
         public String getValue() {
             return "val:"+value;
         }

src/test-jdk17/java/com/fasterxml/jackson/databind/records/RecordCreatorsTest.java

@@ -197,18 +197,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/com/fasterxml/jackson/databind/records/RecordExplicitCreatorsTest.java

@@ -9,6 +9,7 @@
 import com.fasterxml.jackson.databind.*;
 import com.fasterxml.jackson.databind.cfg.*;
 import com.fasterxml.jackson.databind.exc.InvalidDefinitionException;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
 import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
 import com.fasterxml.jackson.databind.introspect.JacksonAnnotationIntrospector;
 import com.fasterxml.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");
         }
+        */
     }
 
     /*