Skip to content
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

default mapper cannot handle basic immutable class #3898

Merged
merged 12 commits into from
Jun 10, 2024

Conversation

pjfanning
Copy link
Member

The test in this PR fails with:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.fasterxml.jackson.failing.TestSetterlessProperty$ImmutableId` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{
  "id" : 13
}"; line: 2, column: 3]

	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1739)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1364)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1424)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3772)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3740)
	at com.fasterxml.jackson.failing.TestSetterlessProperty.testSetterlessProperty(TestSetterlessProperty.java:34)
        ....

As someone who codes mainly in Scala, this is how I prefer to create Java Beans - with immutable values - thus, no setters.

I know we have Java Records but this style is useful for older Java runtimes.

Is there any chance that this could be made to work with a default mapper with no annotations or customisation of the mapper?

@cowtowncoder
Copy link
Member

There are 2 related problems as to why this can't quite be made to work at this point.

The main problem is that the names of Constructor parameters are only available with Parameter names module (or by Kotlin/Scala module providing implicit names similarly).
This can be mocked in tests however; this is done for some existing unit tests in jackson-databind (search for "findImplicitPropertyName()" override for AnnotationIntrospector).

Second challenge is figuring out choice between Properties- and Delegating- Creator; this because 1-argument case is fundamentally ambiguous.
In this case it could probably be heuristically determined from existence of matching getter.

Anyway: adding implicit parameter name access, I think this test might actually pass already?

@pjfanning
Copy link
Member Author

I tried various changes to the ImmutableId class. The prop names were not a problem. None of these helped:

  • adding a custom annotation introspector
  • adding @JsonProperty annotations

The main reason for the ParamNamesModule is Java 6 and 7 - Java 8+ has better support for finding parameter names.

In the end, the only thing that helped was adding an extra no-param constructor. It is not ideal to have it such that users need to modify their classes to get deserialization to work.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 8, 2023

Use of @JsonProperty for constructor parameter(s) (just one of them if parameter names module exists; otherwise all) really does work. If not that's a bug. That's not "no annotations" but avoids use of @JsonCreator.
But note that they are specifically required for constructor parameters to make Constructor "explicitly annotated"; adding them in fields/getters/setters won't help.

But generally we could solve this if Property Discovery was rewritten -- my long-term nr 1 target. There's #3719 related to that; explaining the issue.
Specifically discovery of implicit Creators occurs too late in the process: if it happened earlier, we could auto-detect more cases as well (since the process actually would start with Creator discovery instead of other accessors).

Downside is that I do not thing incremental smaller changes allow doing what is ideal; we have probably ran out of incremental fixes and new ones are likely counter-productive (adding hacks to handle individual problem cases, leading to more complex and error-prone combinations).

As to timeline, it's been "next minor version" since maybe 2.12. But lately vuln/cve focus has taken lots of time. Maybe with 2.16 things are different. :)

EDIT: (10-Jun-2024)

Above-mentioned property discovery rewrite was done via #4515 and is now in 2.18 branch.
Hence further progress possible.

@cowtowncoder cowtowncoder changed the base branch from 2.16 to 2.18 June 10, 2024 01:27
@cowtowncoder
Copy link
Member

@pjfanning After rebasing to 2.18, merging changes to latest, updating wrt JUnit 5 test changes, all 5 tests now actually pass (probably due to #4515)! (when running locally, failing tests would be skipped on CI)

But I am not sure what to do with the tests however, as I think they probably overlap with other tests under com.fasterxml.jackson.databind.deser.creators.

WDYT? Some of these could be merged with existing creators test(s), or I can close the PR. But if some cases were not tested (possible, although I have moved 5 formerly failing test to regular ones), would of course be nice to have better coverage.

@pjfanning
Copy link
Member Author

@cowtowncoder I moved the test to the 'ser' package. I think it is useful to add this test class as a regression scenario.

@cowtowncoder
Copy link
Member

Ok. I'll probably move it to under "creators" (since that's where @JsonCreator-related tests go, figure out better name for class) but keep them.
Thanks!

@cowtowncoder cowtowncoder merged commit 13f59e0 into FasterXML:2.18 Jun 10, 2024
8 checks passed
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

Successfully merging this pull request may close these issues.

2 participants