Fixed #421 (phew!)

release-notes/CREDITS

@@ -143,3 +143,7 @@ Benson Margulies:
 Steve Sanbeg: (sanbeg@github)
   * Contributed #482: Make date parsing error behavior consistent with JDK
    (2.4.1)
+
+Lovro Pandžić (lpandzic@github)
+  * Reported #421: @JsonCreator not used in case of multiple creators with parameter names
+   (2.5.0)

release-notes/VERSION

@@ -1,6 +1,8 @@
 Project: jackson-databind
 Version: 2.5.0 (xx-xxx-2014)
 
+#421: @JsonCreator not used in case of multiple creators with parameter names
+ (reported by Lovro P, lpandzic@github)
 #521: Keep bundle annotations, prevent problems with recursive annotation
    types
  (reported by tea-dragon@github)

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

@@ -363,16 +363,115 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
         return (ValueInstantiator) ClassUtil.createInstance(instClass,
                 config.canOverrideAccessModifiers());
     }
-    
+
     protected void _addDeserializerConstructors
         (DeserializationContext ctxt, BeanDescription beanDesc, VisibilityChecker<?> vchecker,
          AnnotationIntrospector intr, CreatorCollector creators)
         throws JsonMappingException
     {
-        /* First things first: the "default constructor" (zero-arg
-         * constructor; whether implicit or explicit) is NOT included
-         * in list of constructors, so needs to be handled separately.
-         */
+        // First things first: the "default constructor" (zero-arg
+        // constructor; whether implicit or explicit) is NOT included
+        // in list of constructors, so needs to be handled separately.
+        AnnotatedConstructor defaultCtor = beanDesc.findDefaultConstructor();
+        if (defaultCtor != null) {
+            if (!creators.hasDefaultCreator() || intr.hasCreatorAnnotation(defaultCtor)) {
+                creators.setDefaultCreator(defaultCtor);
+            }
+        }
+
+        // And then other constructors
+        for (AnnotatedConstructor ctor : beanDesc.getConstructors()) {
+            boolean isCreator = intr.hasCreatorAnnotation(ctor);
+            int argCount = ctor.getParameterCount();
+            // some single-arg factory methods (String, number) are auto-detected
+            if (argCount == 1) {
+                AnnotatedParameter param = ctor.getParameter(0);
+                // NOTE: only consider EXPLICIT names for auto-detection
+                PropertyName pn = _findExplicitParamName(param, intr);
+                Object injectId = intr.findInjectableValueId(param);
+
+                if ((injectId == null) && (pn == null || pn.isEmpty())) { // not property based
+                    _handleSingleArgumentConstructor(ctxt, beanDesc, vchecker, intr, creators,
+                            ctor, isCreator,
+                            vchecker.isCreatorVisible(ctor), pn);
+                    // otherwise just ignored
+                    continue;
+                }
+                // fall through if there's name
+            }
+
+            // 1 or more args; all params must have name annotations
+            AnnotatedParameter nonAnnotatedParam = null;            
+            CreatorProperty[] properties = new CreatorProperty[argCount];
+            int explicitNameCount = 0;
+            int implicitNameCount = 0;
+            int injectCount = 0;            
+            for (int i = 0; i < argCount; ++i) {
+                AnnotatedParameter param = ctor.getParameter(i);
+                PropertyName name = _findExplicitParamName(param, intr);
+                Object injectId = intr.findInjectableValueId(param);
+                if (name != null && name.hasSimpleName()) {
+                    ++explicitNameCount;
+                    properties[i] = constructCreatorProperty(ctxt, beanDesc, name, i, param, injectId);
+                    continue;
+                }
+                if (injectId != null) {
+                    ++injectCount;
+                    properties[i] = constructCreatorProperty(ctxt, beanDesc, name, i, param, injectId);
+                    continue;
+                }
+                NameTransformer unwrapper = intr.findUnwrappingNameTransformer(param);
+                if (unwrapper != null) {
+                    properties[i] = constructCreatorProperty(ctxt, beanDesc, UNWRAPPED_CREATOR_PARAM_NAME, i, param, null);
+                    ++explicitNameCount;
+                    continue;
+                }
+                // One more thing: implicit names are ok iff ctor has creator annotation
+                if (isCreator) {
+                    name = _findImplicitParamName(param, intr);
+                    if (name != null && !name.isEmpty()) {
+                        ++implicitNameCount;
+                        properties[i] = constructCreatorProperty(ctxt, beanDesc, name, i, param, injectId);
+                        continue;
+                    }
+                }
+                if (nonAnnotatedParam == null) {
+                    nonAnnotatedParam = param;
+                }
+                // 23-Sep-2014, tatu: TODO: probably will need support for using implicit
+                //   names, still, 
+            }
+
+            final int namedCount = explicitNameCount + implicitNameCount;
+            // Ok: if named or injectable, we have more work to do
+            if (isCreator || explicitNameCount > 0 || injectCount > 0) {
+                // simple case; everything covered:
+                if ((namedCount + injectCount) == argCount) {
+                    creators.addPropertyCreator(ctor, properties);
+                } else if ((explicitNameCount == 0) && ((injectCount + 1) == argCount)) {
+                    // [712] secondary: all but one injectable, one un-annotated (un-named)
+                    creators.addDelegatingCreator(ctor, properties);
+                } else { // otherwise, epic fail
+                    throw new IllegalArgumentException("Argument #"+nonAnnotatedParam.getIndex()
+                            +" of constructor "+ctor+" has no property name annotation; must have name when multiple-parameter constructor annotated as Creator");
+                }
+            }
+        }
+    }
+
+    /* 23-Sep-2014, tatu: This is the old definition of method above, from 2.4.x,
+     *   rewritten in 2.5. Left here until 2.6, just in case there's a regression,
+     *   and we need to inspect diffs...
+     */
+/*    
+    protected void XXX_addDeserializerConstructors
+        (DeserializationContext ctxt, BeanDescription beanDesc, VisibilityChecker<?> vchecker,
+         AnnotationIntrospector intr, CreatorCollector creators)
+        throws JsonMappingException
+    {
+        // First things first: the "default constructor" (zero-arg
+        // constructor; whether implicit or explicit) is NOT included
+        // in list of constructors, so needs to be handled separately.
         AnnotatedConstructor defaultCtor = beanDesc.findDefaultConstructor();
         if (defaultCtor != null) {
             if (!creators.hasDefaultCreator() || intr.hasCreatorAnnotation(defaultCtor)) {
@@ -382,6 +481,7 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
 
         PropertyName[] ctorPropNames = null;
         AnnotatedConstructor propertyCtor = null;
+        
         for (BeanPropertyDefinition propDef : beanDesc.findProperties()) {
             if (propDef.getConstructorParameter() != null) {
                 AnnotatedParameter param = propDef.getConstructorParameter();
@@ -390,18 +490,23 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
                     if (propertyCtor == null) {
                         propertyCtor = (AnnotatedConstructor) owner;
                         ctorPropNames = new PropertyName[propertyCtor.getParameterCount()];
+                    } else if (owner != propertyCtor) {
+                        // there is a mismatch of some kind here (problem?)
+                        continue;
                     }
                     ctorPropNames[param.getIndex()] = propDef.getFullName();
                 }
             }
         }
 
         for (AnnotatedConstructor ctor : beanDesc.getConstructors()) {
+
             int argCount = ctor.getParameterCount();
             boolean isCreator = intr.hasCreatorAnnotation(ctor) || (ctor == propertyCtor);
 //            boolean isCreator = intr.hasCreatorAnnotation(ctor);
             
             boolean isVisible =  vchecker.isCreatorVisible(ctor);
+            
             // some single-arg constructors (String, number) are auto-detected
             if (argCount == 1) {
                 PropertyName name = (ctor == propertyCtor) ? ctorPropNames[0] : null;
@@ -416,9 +521,8 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
             // [JACKSON-541] improved handling a bit so:
             // 2 or more args; all params must have name annotations
             // ... or @JacksonInject (or equivalent)
-            /* [JACKSON-711] One more possibility; can have 1 or more injectables, and
-             * exactly one non-annotated parameter: if so, it's still delegating.
-             */
+            // [JACKSON-711] One more possibility; can have 1 or more injectables, and
+            // exactly one non-annotated parameter: if so, it's still delegating.
             AnnotatedParameter nonAnnotatedParam = null;
             int namedCount = 0;
             int injectCount = 0;
@@ -452,8 +556,6 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
                     }
                 }
             }
-
-            // Ok: if named or injectable, we have more work to do
             if (isCreator || namedCount > 0 || injectCount > 0) {
                 // simple case; everything covered:
                 if ((namedCount + injectCount) == argCount) {
@@ -467,6 +569,7 @@ public ValueInstantiator _valueInstantiatorInstance(DeserializationConfig config
             }
         }
     }
+*/
 
     protected boolean _handleSingleArgumentConstructor(DeserializationContext ctxt,
             BeanDescription beanDesc, VisibilityChecker<?> vchecker,
@@ -546,6 +649,7 @@ protected boolean _handleSingleArgumentConstructor(DeserializationContext ctxt,
                 }
                 continue;
             }
+
             // some single-arg factory methods (String, number) are auto-detected
             if (argCount == 1) {
                 AnnotatedParameter param = factory.getParameter(0);
@@ -737,6 +841,15 @@ protected PropertyName _findExplicitParamName(AnnotatedParameter param, Annotati
         }
         return null;
     }
+
+    protected PropertyName _findImplicitParamName(AnnotatedParameter param, AnnotationIntrospector intr)
+    {
+        String str = intr.findImplicitPropertyName(param);
+        if (str != null && !str.isEmpty()) {
+            return new PropertyName(str);
+        }
+        return null;
+    }
 
     protected boolean _hasExplicitParamName(AnnotatedParameter param, AnnotationIntrospector intr)
     {

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

@@ -467,11 +467,15 @@ protected void _addCreatorParam(AnnotatedParameter param)
         if (!expl) {
             if (impl.isEmpty()) {
                 /* Important: if neither implicit nor explicit name, can not make use
-                 * of this creator paramter -- may or may not be a problem, verified
+                 * of this creator parameter -- may or may not be a problem, verified
                  * at a later point.
                  */
                 return;
             }
+            // Also: if this occurs, there MUST be explicit annotation on creator itself
+            if (!_annotationIntrospector.hasCreatorAnnotation(param.getOwner())) {
+                return;
+            }
             pn = new PropertyName(impl);
         }
 

src/test/java/com/fasterxml/jackson/failing/TestCreators421.java → src/test/java/com/fasterxml/jackson/databind/creators/TestCreators421.java

@@ -1,4 +1,4 @@
-package com.fasterxml.jackson.failing;
+package com.fasterxml.jackson.databind.creators;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -13,21 +13,19 @@ static class MultiCtor
     {
         protected String _a, _b;
 
-        @JsonCreator
-        static MultiCtor factory(@JsonProperty("a") String a, @JsonProperty("b") String b) {
-            return new MultiCtor(a, b, Boolean.TRUE);
-        }
-
         private MultiCtor() { }
-
         private MultiCtor(String a, String b, Boolean c) {
             if (c == null) {
                 throw new RuntimeException("Wrong factory!");
             }
             _a = a;
             _b = b;
         }
-
+
+        @JsonCreator
+        static MultiCtor factory(@JsonProperty("a") String a, @JsonProperty("b") String b) {
+            return new MultiCtor(a, b, Boolean.TRUE);
+        }
     }
 
     @SuppressWarnings("serial")

src/test/java/com/fasterxml/jackson/failing/TestTypeWithJsonValue466.java

@@ -1,6 +1,7 @@
 package com.fasterxml.jackson.failing;
 
 import java.io.*;
+import java.math.BigDecimal;
 import java.util.*;
 
 import com.fasterxml.jackson.annotation.*;
@@ -12,17 +13,17 @@ public class TestTypeWithJsonValue466 extends BaseMapTest
     // The following is required for the testDecimalMetadata test case. That case fails.
     @JsonTypeName(value = "decimalValue")
     public static class DecimalValue {
-        private java.math.BigDecimal value;
-        public DecimalValue(){ this.value = java.math.BigDecimal.valueOf( 1234.4321 ); }
+        private BigDecimal value;
+        public DecimalValue() { value = new BigDecimal("111.1"); }
 
         @JsonValue
-        public java.math.BigDecimal getValue(){ return value; }
+        public BigDecimal getValue(){ return value; }
     }
 
     @JsonPropertyOrder({"key","value"})
     public static class DecimalEntry {
-    public DecimalEntry(){}
-        public String getKey(){ return "num"; }
+    public DecimalEntry() {}
+        public String getKey() { return "num"; }
 
         @JsonTypeInfo(use=JsonTypeInfo.Id.NAME, include=JsonTypeInfo.As.EXTERNAL_PROPERTY)
         public DecimalValue getValue(){
@@ -41,10 +42,10 @@ public List<DecimalEntry> getMetadata() {
     @JsonTypeName(value = "doubleValue")
     public static class DoubleValue {
         private Double value;
-        public DoubleValue(){ this.value = 1234.4321; }
+        public DoubleValue() { value = 1234.25; }
 
         @JsonValue
-        public Double getValue(){ return value; }
+        public Double getValue() { return value; }
     }
 
     @JsonPropertyOrder({"key","value"})
@@ -67,14 +68,14 @@ public List<DoubleEntry> getMetadata() {
 
     public void testDoubleMetadata() throws IOException {
         DoubleMetadata doub = new DoubleMetadata();
-        String expected = "{\"metadata\":[{\"key\":\"num\",\"value\":1234.4321,\"@type\":\"doubleValue\"}]}";
+        String expected = "{\"metadata\":[{\"key\":\"num\",\"value\":1234.25,\"@type\":\"doubleValue\"}]}";
         String json = MAPPER.writeValueAsString(doub);
         assertEquals("Serialized json not equivalent", expected, json);
     }
 
     public void testDecimalMetadata() throws IOException{
         DecimalMetadata dec = new DecimalMetadata();
-        String expected = "{\"metadata\":[{\"key\":\"num\",\"value\":1234.4321,\"@type\":\"decimalValue\"}]}";
+        String expected = "{\"metadata\":[{\"key\":\"num\",\"value\":111.1,\"@type\":\"decimalValue\"}]}";
         String json = MAPPER.writeValueAsString(dec);
         assertEquals("Serialized json not equivalent", expected, json);
     }