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 "canonical" Constructor for Scala (case) classes #677

Open
cowtowncoder opened this issue Jun 10, 2024 · 4 comments
Labels

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 10, 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. 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)

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.

I am not sure if similar functionality should be available to allow implicit Delegating Creator: possibly?
If so, it would be given 2 sets of potential candidates, as well as previously chosen Properties-based creator, if any. But I am not yet sure that is needed.

Would the first part make sense, @pjfanning, from Scala module perspective?

@cowtowncoder
Copy link
Member Author

Note on properties- vs delegation-based Creators -- instead of 2 calls, I was also wondering about single call, that could return one (or zero) of each. This because we can accept one properties-based and delegating creators, used as alternatives (for JSON Object, properties-based, for other JSON values, delegating).
This might be easier to work with by Scala module, but interface would be clumsier.

@pjfanning
Copy link
Member

@cowtowncoder This module already has a very complicated AnnotationIntrospector that works. I would prefer not to rewrite it. If you add extra methods to the super class that I need to override and add to this module's AnnotationIntrospector to avoid compilation issues, I will add something but it will probably be the minimal needed to get things working again.

This module traditionally has not had many tests that test what happens when you start adding loads of Jackson annotations to Scala classes. We don't get a large amount of issues complaining about this - so I can only assume that users of this module usually rely on the vanilla behaviour that you get if you don't add Jackson annotations.

If we get users complaining, I can handle their complaints on a case by case basis. I fear that making big changes is as likely to break things for some existing users than to fix anything - I can't think of any outstanding issues that might be fixed by the new jackson-databind support.

For the record, Java Records are very similar in intent to Scala Case Classes. This module has long been built around supporting Case Classes.

@cowtowncoder
Copy link
Member Author

@pjfanning If it ain't broke don't fix it -- I opened this issue to probe if this could help or not. If not, there is no strict need to do anything with new functionality.
Even if I add a new extension point, it will absolutely not be required to be used by Scala module.
There's higher likelihood it could prove useful with Kotlin data classes which I think are similar to Scala Case classes.

Having said that, I was hoping that much improved handling for explicit core annotations could be nice benefit: Scala module would require less code to provide basic abilities for Jackson annotations.

@cowtowncoder cowtowncoder changed the title Work with jackson-databind to better support default of "canonical" Constructor for Scala classes See if there is benefit from integrating with jackson-databind better wrt "canonical" Constructor for Scala (case) classes Jun 11, 2024
@cowtowncoder
Copy link
Member Author

We can leave this open for a bit, and if nothing comes out of it, close.

@cowtowncoder cowtowncoder changed the title See if there is benefit from integrating with jackson-databind better wrt "canonical" Constructor for Scala (case) classes See if there is benefit from integrating with jackson-databind better wrt detecting "canonical" Constructor for Scala (case) classes Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants