-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@JsonCreator not used in case of multiple creators with parameter names #421
Comments
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 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). |
No, the class in question has a constructor with 3 parameters and a static factory method with 2 arguments and is annotated with |
Ok thanks. For what it is worth, rule is clear: one and only constructor may be annotated with 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. |
This doesn't apply in this case since the constructor isn't annotated with |
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. |
Ok, hope to see this resolved soon. |
I had a look at the test, and can run it and reproduce the failure. Hope to figure out more what exactly is happening. |
Have you had a chance to look at this yet? |
@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). |
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. |
@lpandzic no prob, it looks like updates do not always get through (or somehow are not indicated), I occasionally miss notices as well. |
I've looked at the test and there are 2 problems:
|
Ok. |
Actually, diff in test is that I added 2 constructors, not ctor + factory method. Accidental difference. |
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 |
Ok looks like I can now reproduce the problem as reported (yay!). |
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. |
Ok, great. Hopefully this will solve the issue with the parameter names module. 👍 |
That would be nice, but I am not quite that hopeful. But first things first... need to actually fix this one. |
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. |
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 |
And, with that, this issue is finally fixed in |
Nice, good job! :) |
The current approach of using multiple `@JsonCreator` annotations is wrong, because Jackson supports only on creator function [1]. Replace it with a custom deserializer because upcoming changes to `LicenseFinding` will make it more complex to support old versions of the class. [1] FasterXML/jackson-databind#421 (comment) Signed-off-by: Martin Nonnenmacher <[email protected]>
The current approach of using multiple `@JsonCreator` annotations is wrong, because Jackson supports only on creator function [1]. Replace it with a custom deserializer because upcoming changes to `LicenseFinding` will make it more complex to support old versions of the class. [1] FasterXML/jackson-databind#421 (comment) Signed-off-by: Martin Nonnenmacher <[email protected]>
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 thegivenAnnotationIntrospectorFindsConstructorParameterNames
in the test the test will pass.The text was updated successfully, but these errors were encountered: