jackson-module-kotlin icon indicating copy to clipboard operation
jackson-module-kotlin copied to clipboard

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

Open cowtowncoder opened this issue 1 year ago • 5 comments

With 2.18 jackson-databind has a new, rewritten logic for POJO Property detection: work was done mostly under https://github.com/FasterXML/jackson-databind/issues/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. Canonical -- currently only detected for Record types
  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 definition of canonical 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 Canonical, Properties-based creator to use, if any. Method would only be called 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.

cowtowncoder avatar Jun 11 '24 02:06 cowtowncoder

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?

JooHyukKim avatar Jun 11 '24 15:06 JooHyukKim

@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.

cowtowncoder avatar Jun 12 '24 01:06 cowtowncoder

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.

k163377 avatar Jun 14 '24 06:06 k163377

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.

cowtowncoder avatar Jun 14 '24 16:06 cowtowncoder

Now that we all agree, filed https://github.com/FasterXML/jackson-databind/issues/4584 as a start.

JooHyukKim avatar Jun 16 '24 03:06 JooHyukKim

Quick note: https://github.com/FasterXML/jackson-databind/issues/4584 was just implemented in jackson-databind 2.18 branch.

cowtowncoder avatar Jul 11 '24 04:07 cowtowncoder

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

k163377 avatar Jul 17 '24 13:07 k163377

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 avatar Jul 17 '24 18:07 cowtowncoder