FasterXML / jackson-module-kotlin

Module that adds support for serialization/deserialization of Kotlin (http://kotlinlang.org) classes and data classes.
Apache License 2.0
1.11k stars 175 forks source link

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

Closed cowtowncoder closed 1 week ago

cowtowncoder commented 1 month ago

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. 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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

cowtowncoder commented 1 week ago

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

k163377 commented 1 week ago

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

cowtowncoder commented 1 week ago

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.