Skip to content

Deserialization error with custom AnnotationIntrospector in Jackson 2.8.8 #1756

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

Closed
danielnorberg opened this issue Aug 29, 2017 · 11 comments
Closed

Comments

@danielnorberg
Copy link
Contributor

In Jackson 2.8.8 I am getting a deserialization error when using a custom AnnotationIntrospector together with @JsonIgnore and @JsonDeserialize annotations.

An exception occured while executing the Java class. Can not construct instance of Repro$FoobarImpl: no suitable constructor found, can not deserialize from Object value (missing default constructor or creator, or perhaps need to add/enable type information?)
at [Source: {"bar":"bar", "foo":"foo"}; line: 1, column: 2]
  • Jackson 2.8.6 and below works fine.
  • Omitting either @JsonIgnore or @JsonDeserialize annotations makes deserialization work.
  • Using a module to register custom deserializer instead of @JsonDeserialize also works.

Here is a full repro, as minimal as I could make it:

https://github.com/danielnorberg/jackson-28-annotation-introspection-breakage-repro

I will try to bisect my way to exactly what jackson commit is causing this issue.

Btw, thanks for Jackson, it's truly an awesome library and I'm always happy using it =)

@cowtowncoder
Copy link
Member

Thank you for reporting the issue. I hope to look into that soon.
One thing perhaps worth checking is if 2.8.10 makes any difference (probably not).

@danielnorberg
Copy link
Contributor Author

I took a stab at bisecting jackson-databind and it seems like 997261885 might be the culprit. That commit is a fix for '@JsonIgnore' annotation not working with creator properties, serialization #1317 which seems related to my current issue.

Neither 2.8.10 nor 2.9.0 remediates the issue.

Thanks for taking a look!

@cowtowncoder
Copy link
Member

Ok interesting. Took a while to understand where everything comes from (at first thought it's missing @JsonCreator, but custom AnnotationIntrospector does provide it). But after going through it, yes, it looks like that should work but does not.
2.9.0 gives slightly better error message which suggests that for some reason constructor is not accepted as creator.

@danielnorberg
Copy link
Contributor Author

Yeah, to give some more context, this is how the auto-matter jackson module works.

Tried to debug this a bit. BasicDeserializerFactory#_addDeserializerConstructors() ends up calling BasicDeserializerFactory#_checkImplicitlyNamedConstructors, which I guess is expected. Then the presence of @JsonDeserialize on Foobar#bar() makes JacksonAnnotationIntrospector#findNameForDeserialization() return PropertyName.USE_DEFAULT which then makes BasicDeserializerFactory#_findParamName() not call AnnotationIntrospector#findImplicitPropertyName(). My custom annotation introspector does not implement findNameForDeserialization(), not clear to me if that should be necessary or if only findImplicitPropertyName() should be enough.

Have yet to look into why the presence of @JsonIgnore on Foobar#foo() makes a difference for the property bar.

@danielnorberg
Copy link
Contributor Author

Seems like without @JsonIgnore on Foobar#foo(), the _checkImplicitlyNamedConstructors() code path is not taken. Instead creators.addPropertyCreator() is called because propDefs contains the foo property and implicitWithCreatorCount == argCount.

danielnorberg pushed a commit to danielnorberg/helios that referenced this issue Sep 6, 2017
People are having issues with jackson 2.8.8 as required by helios-test
due to this issue: FasterXML/jackson-databind#1756
@cowtowncoder
Copy link
Member

So, findImplicitPropertyName() establishes auto-detectable name for Creator parameters (like constructor arguments), but without indicating that the name is explicitly forced via annotations.
An analogy would be that a method like public int getValue(); has implicit name of "value", but no explicitly annotated name. This is important for some things like name strategies: implicit names are passed for renaming but explicit ones not.
Whether you want to override both methods depends on expected semantics: return explicit name (findNameForDeserialization()) would be equivalent to having @JsonProperty annotation.

Most likely difference in handling, between implicit and explicit names, is that only explicit names indicate that a constructor should be used as Creator method even without @JsonCreator annotation: implicit names do not indicate that (but if there is @JsonCreator names exist).

@cowtowncoder
Copy link
Member

Ok. Interesting. I suspect this may have to do with @JsonIgnore pruning one of properties (since there is no explicit in matching constructor parameter). Reason for @JsonDeserialize having such effect is more difficult to understand, and that especially looks like a bug.

As a work-around I would suggest implementing findNameForDeserialization() since doing that seems to work around the issue: this is because it is then equivalent of adding @JsonProperty.
So doing this should not hurt and does allow by-passing issue on short term.

I hope to figure out more about what is going on, but that may take a while.

cowtowncoder added a commit that referenced this issue Sep 19, 2017
@danielnorberg
Copy link
Contributor Author

@cowtowncoder Right, I think one reason that I didn't implement findNameForDeserialization() is that this produces Unrecognized field errors on deserialization if there are explicit @JsonProperty("renamed_bar") annotations on the field methods in the "value type" interface declaration.

My understanding of how this works in jackson < 2.8.8 is that my findImplicitPropertyName returns the same name (e.g. "bar") for both the method @JsonProperty("renamed_bar") String bar(); in the "value type" interface declaration and the constructor parameter @Field("bar") String bar in the concrete implementation. Jackson then respects the @JsonProperty annotation on the method and figures out that e.g. the field {"renamed_bar":..} in the json input should be fed to the bar constructor parameter.

This change to my repro code illustrates the issue: danielnorberg/jackson-28-annotation-introspection-breakage-repro@9b6d11d

@cowtowncoder
Copy link
Member

Unfortunately test I now have does not fail, but I can't be 100% sure that is due to fix or test itself not reproducing the problem.
However, changes I have made should help since a possible root cause is that annotation merging did not work when implicit names (from Java 8, jackson-module-parameter-names, or equivalent) differed (as opposed to case with no parameter names where no implicit name available), so it is possible 2.9.2 has working handling for the original issue.
I don't think I will try backporting the change into 2.8 since risk/benefit ratio for that branch is bit different (that is, need to be extremely careful wrt possible regression).

This area will also be rewritten for Jackson 3.0 (and there I am confident problem can be resolved for good).

@danielnorberg
Copy link
Contributor Author

danielnorberg commented Sep 29, 2017

Yeah, with 2.9.2-SNAPSHOT I cannot repro any more either \o/

mvn -Djackson.version=2.9.2-SNAPSHOT clean compile exec:java -Dexec.mainClass=Repro
...
[INFO] --- exec-maven-plugin:1.6.0:java (default-cli) @ jackson-28-annotation-introspection-breakage-repro ---
FoobarImpl{foo='foo', bar='bar'}
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
...

🍾 🎉

@cowtowncoder
Copy link
Member

@danielnorberg Most excellent. I hope fix I made (based on findings with early 3.0 work) helps with other tricky problems, some of which I thought would not be easy to fix.

danielnorberg pushed a commit to danielnorberg/helios that referenced this issue Oct 14, 2017
Jackson 2.9.2 has a fix for an issue that affects automatter users,
of which several are also helios-testing users.

FasterXML/jackson-databind#1756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants