Fix #3143

cowtowncoder · cowtowncoder · commit 43ab2884d461 · 2021-06-15T12:53:49.000-07:00
diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x
@@ -1299,6 +1299,9 @@ Halil İbrahim Şener (hisener@github)
   * Reported #2962: Auto-detection of constructor-based creator method skipped if there is
    an annotated factory-based creator method (regression from 2.11)
    (2.12.1)
+  * Reported #3143: String-based `Map` key deserializer is not deterministic when there is no
+   single arg constructor
+   (2.13.0)
 
 Faron Dutton (fdutton@github)
   * Contributed fix for #2990: Breaking API change in `BasicClassIntrospector` (2.12.0)
diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x
@@ -42,6 +42,9 @@ Project: jackson-databind
  (reported by mistyzyq@github)
 #3130: Serializing java.lang.Thread fails on JDK 11 and above (should suppress
   serialization of ClassLoader)
+#3143: String-based `Map` key deserializer is not deterministic when there is no
+  single arg constructor
+ (reported by Halil İbrahim Ş)
 #3154: Add ArrayNode#set(int index, primitive_type value)
  (contributed by Tarekk Mohamed A)
 #3174: DOM `Node` serialization omits the default namespace declaration
diff --git a/src/main/java/com/fasterxml/jackson/databind/BeanDescription.java b/src/main/java/com/fasterxml/jackson/databind/BeanDescription.java
@@ -4,6 +4,7 @@
 import java.lang.reflect.Method;
 import java.util.*;
 
+import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonFormat;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
@@ -137,8 +138,24 @@ public boolean isNonStaticInnerClass() {
     /**********************************************************
      */
 
+    /**
+     * Helper method that will return all non-default constructors (that is,
+     * constructors that take one or more arguments) this class has.
+     */
     public abstract List<AnnotatedConstructor> getConstructors();
 
+    /**
+     * Method similar to {@link #getConstructors()} except will also introspect
+     * {@code JsonCreator.Mode} and filter out ones marked as not applicable and
+     * include mode (or lack thereof) for remaining constructors.
+     *<p>
+     * Note that no other filtering (regarding visibility or other annotations)
+     * is performed
+     *
+     * @since 2.13
+     */
+    public abstract List<AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode>> getConstructorsWithMode();
+
     /**
      * Helper method that will check all static methods of the bean class
      * that seem like factory methods eligible to be used as Creators.
@@ -159,6 +176,14 @@ public boolean isNonStaticInnerClass() {
      */
     public abstract List<AnnotatedMethod> getFactoryMethods();
 
+    /**
+     * Method similar to {@link #getFactoryMethods()} but will return {@code JsonCreator.Mode}
+     * metadata along with qualifying factory method candidates.
+     *
+     * @since 2.13
+     */
+    public abstract List<AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode>> getFactoryMethodsWithMode();
+
     /**
      * Method that will locate the no-arg constructor for this class,
      * if it has one, and that constructor has not been marked as
diff --git a/src/main/java/com/fasterxml/jackson/databind/DeserializationContext.java b/src/main/java/com/fasterxml/jackson/databind/DeserializationContext.java
@@ -663,15 +663,23 @@ public final JsonDeserializer<Object> findRootValueDeserializer(JavaType type)
     public final KeyDeserializer findKeyDeserializer(JavaType keyType,
             BeanProperty prop) throws JsonMappingException
     {
-        KeyDeserializer kd = _cache.findKeyDeserializer(this,
-                _factory, keyType);
+        KeyDeserializer kd;
+        // 15-Jun-2021, tatu: Needed wrt [databind#3143]
+        try {
+            kd = _cache.findKeyDeserializer(this, _factory, keyType);
+        } catch (IllegalArgumentException iae) {
+            // We better only expose checked exceptions, since those
+            // are what caller is expected to handle
+            reportBadDefinition(keyType, ClassUtil.exceptionMessage(iae));
+            kd = null;
+        }
         // Second: contextualize?
         if (kd instanceof ContextualKeyDeserializer) {
             kd = ((ContextualKeyDeserializer) kd).createContextual(this, prop);
         }
         return kd;
     }
-    
+
     /*
     /**********************************************************
     /* Public API, ObjectId handling
diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializers.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializers.java
@@ -2,9 +2,14 @@
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
+import java.util.List;
 
+import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.databind.*;
 import com.fasterxml.jackson.databind.deser.KeyDeserializers;
+import com.fasterxml.jackson.databind.introspect.AnnotatedAndMetadata;
+import com.fasterxml.jackson.databind.introspect.AnnotatedConstructor;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
 import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
 import com.fasterxml.jackson.databind.util.ClassUtil;
 import com.fasterxml.jackson.databind.util.EnumResolver;
@@ -47,36 +52,123 @@ public static KeyDeserializer constructDelegatingKeyDeserializer(Deserialization
     
     public static KeyDeserializer findStringBasedKeyDeserializer(DeserializationConfig config,
             JavaType type)
-    {
+        throws JsonMappingException
+   {
+        // 15-Jun-2021, tatu: As per [databind#3143], full introspection needs to consider
+        //   as set of possibilities. Basically, precedence is:
+        //
+        //   1. Explicitly annotated 1-String-arg constructor, if one exists
+        //   2. Explicitly annotated Factory method: just one allowed (exception if multiple)
+        //   3. Implicit 1-String-arg constructor (no visibility checks for backwards
+        //      compatibility reasons; should probably be checked in future, 3.0?)
+        //   4. Implicit Factory method with name of "valueOf()" (primary) or
+        //      "fromString()" (secondary). Likewise, no visibility check as of yet.
+
         // We don't need full deserialization information, just need to know creators.
-        BeanDescription beanDesc = config.introspect(type);
+        final BeanDescription beanDesc = config.introspectForCreation(type);
         // Ok, so: can we find T(String) constructor?
-        Constructor<?> ctor = beanDesc.findSingleArgConstructor(String.class);
-        if (ctor != null) {
-            if (config.canOverrideAccessModifiers()) {
-                ClassUtil.checkAndFixAccess(ctor, config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
-            }
-            return new StdKeyDeserializer.StringCtorKeyDeserializer(ctor);
+        final AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode> ctorInfo = _findStringConstructor(beanDesc);
+        // Explicit?
+        if ((ctorInfo != null) && (ctorInfo.metadata != null)) {
+            return _constructCreatorKeyDeserializer(config, ctorInfo.annotated);
         }
         // or if not, "static T valueOf(String)" (or equivalent marked
         // with @JsonCreator annotation?)
-        Method m = beanDesc.findFactoryMethod(String.class);
-        if (m != null){
+        final List<AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode>> factoryCandidates
+            = beanDesc.getFactoryMethodsWithMode();
+
+        // But must now filter out invalid candidates, both by signature (must take 1 and
+        // only 1 arg; that arg must be of type `String`) and by annotations (we only
+        // accept "delegating" style, so remove PROPERTIES)
+        factoryCandidates.removeIf(m ->
+            (m.annotated.getParameterCount() != 1)
+                || (m.annotated.getRawParameterType(0) != String.class)
+                || (m.metadata == JsonCreator.Mode.PROPERTIES)
+                );
+
+        // Any explicit?
+        final AnnotatedMethod explicitFactory = _findExplicitStringFactoryMethod(factoryCandidates);
+        if (explicitFactory != null) {
+            return _constructCreatorKeyDeserializer(config, explicitFactory);
+        }
+        // If we had implicit Constructor, that'd work now
+        if (ctorInfo != null) {
+            return _constructCreatorKeyDeserializer(config, ctorInfo.annotated);
+        }
+        // And finally, if any implicit factory methods, acceptable now
+        // nope, no such luck...
+        if (!factoryCandidates.isEmpty()) {
+            // 15-Jun-2021, tatu: Ideally we would provide stabler ordering, but for now
+            //   let's simply pick the first one
+            return _constructCreatorKeyDeserializer(config, factoryCandidates.get(0).annotated);
+        }
+        return null;
+    }
+
+    private static KeyDeserializer _constructCreatorKeyDeserializer(DeserializationConfig config,
+            AnnotatedMember creator)
+    {
+        if (creator instanceof AnnotatedConstructor) {
+            Constructor<?> rawCtor = ((AnnotatedConstructor) creator).getAnnotated();
             if (config.canOverrideAccessModifiers()) {
-                ClassUtil.checkAndFixAccess(m, config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
+                ClassUtil.checkAndFixAccess(rawCtor, config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
+            }
+            return new StdKeyDeserializer.StringCtorKeyDeserializer(rawCtor);
+        }
+        Method m = ((AnnotatedMethod) creator).getAnnotated();
+        if (config.canOverrideAccessModifiers()) {
+            ClassUtil.checkAndFixAccess(m, config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS));
+        }
+        return new StdKeyDeserializer.StringFactoryKeyDeserializer(m);
+    }
+
+    // 13-Jun-2021, tatu: For now just look for constructor that takes one `String`
+    //      argument (could look for CharSequence) and hence can have just one, no dups
+    private static AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode> _findStringConstructor(BeanDescription beanDesc)
+    {
+        for (AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode> entry
+                : beanDesc.getConstructorsWithMode())
+        {
+            // BeanDescription method does NOT filter out various types so check
+            // it takes single argument.
+            final AnnotatedConstructor ctor = entry.annotated;
+            if ((ctor.getParameterCount() == 1)
+                    && (String.class == ctor.getRawParameterType(0))) {
+                return entry;
             }
-            return new StdKeyDeserializer.StringFactoryKeyDeserializer(m);
         }
-        // nope, no such luck...
         return null;
     }
-    
+
+    private static AnnotatedMethod _findExplicitStringFactoryMethod(
+            List<AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode>> candidates)
+        throws JsonMappingException
+    {
+        AnnotatedMethod match = null;
+        for (AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode> entry : candidates) {
+            // Note: caller has filtered out invalid candidates; all we need to check are dups
+            if (entry.metadata != null) {
+                if (match == null) {
+                    match = entry.annotated;
+                } else {
+                    // 15-Jun-2021, tatu: Not optimal type or information, but has to do for now
+                    //    since we do not get DeserializationContext
+                    Class<?> rawKeyType = entry.annotated.getDeclaringClass();
+                    throw new IllegalArgumentException(
+"Multiple suitable annotated Creator factory methods to be used as the Key deserializer for type "
+                            +ClassUtil.nameOf(rawKeyType));
+                }
+            }
+        }
+        return match;
+    }
+
     /*
     /**********************************************************
     /* KeyDeserializers implementation
     /**********************************************************
      */
-    
+
     @Override
     public KeyDeserializer findKeyDeserializer(JavaType type,
             DeserializationConfig config, BeanDescription beanDesc) throws JsonMappingException
diff --git a/src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedAndMetadata.java b/src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedAndMetadata.java
@@ -0,0 +1,22 @@
+package com.fasterxml.jackson.databind.introspect;
+
+/**
+ * Silly little "Pair" class needed for 2-element tuples (without
+ * adding dependency to one of 3rd party packages that has one).
+ *
+ * @since 2.13
+ */
+public class AnnotatedAndMetadata<A extends Annotated, M extends Object>
+{
+    public final A annotated;
+    public final M metadata;
+
+    public AnnotatedAndMetadata(A ann, M md) {
+        annotated = ann;
+        metadata = md;
+    }
+
+    public static <A extends Annotated, M> AnnotatedAndMetadata<A, M> of(A ann, M md) {
+        return new AnnotatedAndMetadata<>(ann, md);
+    }
+}
diff --git a/src/main/java/com/fasterxml/jackson/databind/introspect/BasicBeanDescription.java b/src/main/java/com/fasterxml/jackson/databind/introspect/BasicBeanDescription.java
@@ -346,6 +346,23 @@ public List<AnnotatedConstructor> getConstructors() {
         return _classInfo.getConstructors();
     }
 
+    @Override
+    public List<AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode>> getConstructorsWithMode() {
+        List<AnnotatedConstructor> allCtors = _classInfo.getConstructors();
+        if (allCtors.isEmpty()) {
+            return Collections.emptyList();
+        }
+        List<AnnotatedAndMetadata<AnnotatedConstructor, JsonCreator.Mode>> result = new ArrayList<>();
+        for (AnnotatedConstructor ctor : allCtors) {
+            JsonCreator.Mode mode = _annotationIntrospector.findCreatorAnnotation(_config, ctor);
+            if (mode == JsonCreator.Mode.DISABLED) {
+                continue;
+            }
+            result.add(AnnotatedAndMetadata.of(ctor, mode));
+        }
+        return result;
+    }
+
     @Override
     public Object instantiateBean(boolean fixAccess) {
         AnnotatedConstructor ac = _classInfo.getDefaultConstructor();
@@ -571,6 +588,30 @@ public List<AnnotatedMethod> getFactoryMethods()
         return result;
     }
 
+    @Override // since 2.13
+    public List<AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode>> getFactoryMethodsWithMode()
+    {
+        List<AnnotatedMethod> candidates = _classInfo.getFactoryMethods();
+        if (candidates.isEmpty()) {
+            return Collections.emptyList();
+        }
+        List<AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode>> result = null;
+        for (AnnotatedMethod am : candidates) {
+            AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode> match
+                = findFactoryMethodMetadata(am);
+            if (match != null) {
+                if (result == null) {
+                    result = new ArrayList<>();
+                }
+                result.add(match);
+            }
+        }
+        if (result == null) {
+            return Collections.emptyList();
+        }
+        return result;
+    }
+
     @Override
     @Deprecated // since 2.13
     public Constructor<?> findSingleArgConstructor(Class<?>... argTypes)
@@ -646,6 +687,45 @@ protected boolean isFactoryMethod(AnnotatedMethod am)
         return false;
     }
 
+    // @since 2.13
+    protected AnnotatedAndMetadata<AnnotatedMethod, JsonCreator.Mode> findFactoryMethodMetadata(AnnotatedMethod am)
+    {
+        // First: return type must be compatible with the introspected class
+        // (i.e. allowed to be sub-class, although usually is the same class)
+        Class<?> rt = am.getRawReturnType();
+        if (!getBeanClass().isAssignableFrom(rt)) {
+            return null;
+        }
+        // Also: must be a recognized factory method, meaning:
+        // (a) marked with @JsonCreator annotation, or
+        // (b) 1-argument "valueOf" (at this point, need not be public), or
+        // (c) 1-argument "fromString()" AND takes {@code String} as the argument
+        JsonCreator.Mode mode = _annotationIntrospector.findCreatorAnnotation(_config, am);
+        if (mode != null) {
+            if (mode == JsonCreator.Mode.DISABLED) {
+                return null;
+            }
+            return AnnotatedAndMetadata.of(am, mode);
+        }
+        final String name = am.getName();
+        // 24-Oct-2016, tatu: As per [databind#1429] must ensure takes exactly one arg
+        if ("valueOf".equals(name)) {
+            if (am.getParameterCount() == 1) {
+                return AnnotatedAndMetadata.of(am, mode);
+            }
+        }
+        // [databind#208] Also accept "fromString()", if takes String or CharSequence
+        if ("fromString".equals(name)) {
+            if (am.getParameterCount() == 1) {
+                Class<?> cls = am.getRawParameterType(0);
+                if (cls == String.class || CharSequence.class.isAssignableFrom(cls)) {
+                    return AnnotatedAndMetadata.of(am, mode);
+                }
+            }
+        }
+        return null;
+    }
+
     /**
      * @deprecated since 2.8
      */
diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/jdk/MapKeyDeserialization3143Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/jdk/MapKeyDeserialization3143Test.java
diff --git a/src/test/java/com/fasterxml/jackson/failing/KeyDeserializer3143Test.java b/src/test/java/com/fasterxml/jackson/failing/KeyDeserializer3143Test.java
