@JsonCreator not used in case of multiple creators with parameter names

[lpandzic]

Currently deserialiazing a class that has multiple creators with available parameter names doesn't prioritize the creator annotated with @JsonCreator.

In the new java 8 ParameterNamesAnnotationIntrospector parameter names are resolved for every creator in a class that has been compiled with -parameters option. JacksonAnnotationIntrospector on the other hand, can resolve parameter names only by using @JsonProperty.
When there are multiple creators with parameter names available it seems that the creator with longest parameter list is the preffered one. In the case of JacksonAnnotationIntrospector this is not a problem since it wouldn't make sense for a client to annotate parameters with @JsonProperty on more than one creator.

I've made a test that isolates this issue in a fork. The mocked introspector mimics the behavior of the ParameterNamesAnnotationIntrospector in the new module and if you ignore the givenAnnotationIntrospectorFindsConstructorParameterNames in the test the test will pass.

[cowtowncoder]

Just to be sure: do you mean single-argument constructors/factory methods? Handling of multi-argument creators should strictly only use single one, annotated with @JsonCreator, and ignore the rest. Unless... maybe this was changed later on, to consider "all arguments annotated" to also include? This sounds familiar, so I assume this is the case there.

If so, one possibility would be to try to distinguish between the case of explicit annotation from implicit names (by Java 8, or paranamer, or Scala introspector).

[lpandzic]

No, the class in question has a constructor with 3 parameters and a static factory method with 2 arguments and is annotated with @JsonCreator. Please take a look at the linked test.

[cowtowncoder]

Ok thanks. For what it is worth, rule is clear: one and only constructor may be annotated with @JsonCreator, and same is true for static methods.

If one of each is specified, however, I don't remember which one is preferred; and I am not sure what would be the best heuristics. But it does not rely on number of arguments and instead of always favors one type (constructor or static factory) over the other.

I hope to have time to dig in the test later on.

[lpandzic]

If one of each is specified, however, I don't remember which one is preferred; and I am not sure what would be the best heuristics. But it does not rely on number of arguments and instead of always favors one type (constructor or static factory) over the other.

This doesn't apply in this case since the constructor isn't annotated with @JsonCreator.

[cowtowncoder]

Ok. So in case of multiple factory methods, I would have expected an exception, as per existing intent. Jackson always resolves to a single creator statically, and is not based on trying to detect from data which one is applicable. This is not to say that more advanced selection would not be useful or desirable, just that it is not the way functionality exists at this point.

[lpandzic]

Ok, hope to see this resolved soon.
Thank you for your time.

[cowtowncoder]

I had a look at the test, and can run it and reproduce the failure. Hope to figure out more what exactly is happening.

[lpandzic]

Have you had a chance to look at this yet?

[cowtowncoder]

@lpandzic I don't actually know how to reproduce the problem at this point: I added a test under "TestCreators3", but it actually passes at this point (for master, pre-2.4.2).
Do you think you could modify that test class to show remaining issue? Forked project has more complete test, but I have trouble finding out what is the minimal way to reproduce the problem.

[lpandzic]

Hey, sorry for not responding sooner. I've missed this reply somehow, I'll take a look at it as soon as I have some time.

[cowtowncoder]

@lpandzic no prob, it looks like updates do not always get through (or somehow are not indicated), I occasionally miss notices as well.

[lpandzic]

I've looked at the test and there are 2 problems:

1. there is no introspector that resolves parameter names of both constructor and static factory method
2. both constructor and static factory method should have same parameters (number and types).

[cowtowncoder]

Ok.

[cowtowncoder]

Actually, diff in test is that I added 2 constructors, not ctor + factory method. Accidental difference.

[cowtowncoder]

Ok. Modifying test to have same set up does not yet trigger the problem, but this is because I provide names for creator properties explicitly. So I will need to add AnnotationIntrospector to try to tease out the problem.

[cowtowncoder]

Ok looks like I can now reproduce the problem as reported (yay!).

[cowtowncoder]

Starting look into the root cause. Looks like a significant refactoring may be needed here, so I'd rather try to fix this for 2.5, to try to minimize regression for patch versions. Master branch was just changed to 2.5.0-SNAPSHOT so this should not slow things, but I'll mention this just in case.

[lpandzic]

Ok, great. Hopefully this will solve the issue with the parameter names module. 👍

[cowtowncoder]

That would be nice, but I am not quite that hopeful. But first things first... need to actually fix this one.

[apatrida]

Yes, I'm seeing odd cases (will try to narrow it down) with Kotlin module where the absence of JsonCreator doesn't affect Jackson trying to use a constructor (maybe because by default Kotlin only has one unless it can also create a default constructor in rare cases) regardless of JsonCreator annotation being present. So I probably should document for the Kotlin plugin that JsonCreator is not necessary UNLESS you have done the work so that Kotlin also created a default parameterless constructor.

[cowtowncoder]

More and more I look into this problem, more I realize that introspection of constructors and factory methods should be completely rewritten... in this case, parameters for factory method (correctly found), and constructor (incorrectly) get mixed up, resulting in constructors parameters being considered explicitly named (since explicit-ness is part of property, not accessor, when BasicDeserializerFactory considers things).

[cowtowncoder]

And, with that, this issue is finally fixed in master, for 2.5.0. I'll deploy SNAPSHOT version now.

[lpandzic]

Nice, good job! :)