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

See if there is benefit from integrating with jackson-databind better wrt detecting "default" Constructor for Kotlin (data) classes #805

Closed
cowtowncoder opened this issue Jun 11, 2024 · 8 comments · Fixed by #818
Milestone

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 11, 2024

With 2.18 jackson-databind has a new, rewritten logic for POJO Property detection: work was done mostly under FasterXML/jackson-databind#4515.

Logic, at high-level, selects at most one "Properties-based" Creator (constructor / static factory method), in following order:

  1. Explicitly annotated (either @JsonCreator on ctor/factory, or at least one parameter of ctor/factory annotated with @JsonProperty)
  2. Default/preferred/primary -- currently only detected for Record types (canonical constructor)
  3. Implicit Constructor: if all parameters of a constructor have implicit names, is visible (by default: public) and there is only one such choice (not even 0-args "default" constructor)

Oh these steps, (2) is basically new category added, to better support "no-annotations" use case but with reliable detection and mode selection.

This works for many cases. But JVM languages like Scala (and Kotlin) typically have their own definitions of a default constructor, so it'd be good to allow pluggable handling.
(in plain Java we only have Records with the concept of specific canonical (primary) constructor, over alternatives that are also visible)

My initial idea would be to add one (ideally) new method in AnnotationIntrospector, which would take 2 sets of candidates -- Constructors and Factory Methods (PotentialCreator) -- that might qualify; and result would be zero or one of passed-in choices to indicate Default/preferred, Properties-based creator to use, if any.
Method would only be called (or its return value used) if no explicit choice were found.

Would this make sense from Kotlin module perspective @k163377 @JooHyukKim ?

My intent is specifically to allow code in module to get simpler, get better out-of-the-box support for core Jackson annotations, and not to complicate things.

EDIT: now databind issue #4584 has been implemented, adding AnnotationIntrospector.findDefaultCreator(...) which would be the mechanism.

@JooHyukKim
Copy link
Member

Makes sense. So we are making (2) pluggable from modules.

The approach itself sounds good and all, so I guess the problem lies in whether the introduction of new method could smoothly fit in to the existing functionality, with minimal regression.

So the question might be... do we have enough test cases here in Kotlin module?

@cowtowncoder
Copy link
Member Author

@JooHyukKim Exactly, great summary. I am not sure on test coverage, to be honest.
Similarly although I know some parts of Kotlin module have heavily customized handling over jackson-databind defaults, not everything is. So that will probably determine how easy changes are.

@k163377
Copy link
Contributor

k163377 commented Jun 14, 2024

Yes.

Currently, there is a risk that the registration order of kotlin-module disables the explicit JsonCreator detection process by other modules.
This feature would also be useful in terms of simplifying the process.

@cowtowncoder
Copy link
Member Author

Right: handling of explicit Creators is improved, and pluggable (yet to be added) Canonical Creator detection (if and only if no explicit ones found) could help both reduce code needed on Module side and make handling work more uniformly.

I think there are still some other aspects databind would need to expose, wrt handling of missing values. But it'd be great if there was a way to convert/upgrade things incrementally.

@JooHyukKim
Copy link
Member

Now that we all agree, filed FasterXML/jackson-databind#4584 as a start.

@cowtowncoder cowtowncoder changed the title See if there is benefit from integrating with jackson-databind better wrt detecting "canonical" Constructor for Kotlin (data) classes See if there is benefit from integrating with jackson-databind better wrt detecting "default" Constructor for Kotlin (data) classes Jul 11, 2024
@cowtowncoder
Copy link
Member Author

Quick note: FasterXML/jackson-databind#4584 was just implemented in jackson-databind 2.18 branch.

k163377 added a commit to k163377/jackson-module-kotlin that referenced this issue Jul 14, 2024
Changed to implement findDefaultCreator.
Fixes FasterXML#805.
@k163377
Copy link
Contributor

k163377 commented Jul 17, 2024

@cowtowncoder
Merged changes.
This should improve initialization performance and memory consumption by reducing the amount of reflection.
Thank you for adding this feature!

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Jul 17, 2024

Excellent! It is also possible that handling of standard Jackson annotations for Creators might be improved as well (ability to configure constructor parameters etc, even when not using explicit @JsonCreator, as an example).

And just to add a link: looks like #818 was the PR that resolved this issue.

@cowtowncoder cowtowncoder added this to the 2.18.0 milestone Jul 17, 2024
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 a pull request may close this issue.

3 participants