Experimenting with #1467: Support for @JsonUnwrapped in @JsonCreators

[fxshlein]

I was more or less just messing around with an approach to solve this for now without the big refactor mentioned in the issue. Perhaps this is already fine? The test I added is green at least, but I have no idea what I might have missed. After all, if an "object with three properties" test covered a considerable amount of jackson's feature set, I wouldn't be using it... 😆

[cowtowncoder]

Whoa! Impressive work. I share your suspicion wrt test coverage (lack thereof), but it is what it is.

I'd need to read this through with more thought and focus to see if there's anything specifically that wouldn't work (or be against design). In the meantime I added some notes on code style issues (which are obviously not super important but might as well mention now).

Also, assuming we get through this to merge it, one thing needed (if not already done) is CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

which we need before merging the first contribution. It's usually easiest to print, fill & sign, scan/photo, email to cla at fasterxml dot com. Only needed once, good for all future contributions.

[fxshlein]

@cowtowncoder I sent the signed CLA to cla@.... 🙂

Just to verify, here it says to email the CLA to info@..., are both fine?

[fxshlein]

I noticed a minor issue with the placeholder name: If you use two @JsonUnwrapped parameters in a creator without explicitly giving it a name @JsonProperty, they will be marked as duplicates, as they both get the placeholder:

Caused by: java.lang.IllegalArgumentException: Duplicate creator property "@JsonUnwrapped" (index 2 vs 3) for type ...

I added a test case here: https://github.com/FasterXML/jackson-databind/pull/4271/files#diff-1b40e79714a359efac38026f7dfe9dbf45f4def1877d805437ba48955ae9478eR68-R96

From what I can see, there are three solutions to this:

1. Just let it happen, maybe add a special exception message that tells users to simply distinguish the properties using @JsonProperty. On one hand this should fit right in, as the user probably has to have @JsonProperty on all parameters anyways, but on the other hand it would be kind of weird since the name is never actually used.
2. Change the placeholder to include the index ("unnamed @JsonUnwrapped property at N"). This is the one I implemented now, which makes the test case work. This is nice since the name is more unique, and the names also double as human-readable identifiers in error messages.
3. Do a refactor to allow unwrapped properties to bypass all the places where properties are placed in a map, using their name as a key. I started this here: https://github.com/fxshlein/jackson-databind/commit/3bd10a06548260445cfcde0643f885de5537a09d, but it's already a pretty massive undertaking without even touching everything.

[cowtowncoder]

To me (2), adding index for bogus name, sounds reasonable at this point. +1.

[wingsofovnia]

While looking for a stop-gap solution for this in GitHub, I found this (usage) rather straightforward solution for @JsonUnwrapped support in Kotlin data classes. Perhaps this might be helpful for you or others who seek a temp solution.

Looking forward to having this merged. Thank you for the contribution!

[eranl]

@cowtowncoder, what's the status of this PR?

[JooHyukKim]

@eranl The submitter stop working 6months ago. Unless target issue is highly voted, it would be reasonable to say that there is small chance of merging unless @fxshlein or someone else picks it back up

[cowtowncoder]

Quick note: this would need to be re-based on 2.18 which may be tricky due to major POJO property introspection rewrite (or might be easy). This won't be merged in 2.17 due to high risk of regression for patch. If rebased against 2.18, merging could be considered; definitely the problem to solve is worth tackling.

[cowtowncoder]

Re-based, but now there are conflicts to resolve... not sure I have time to tackle those right now.

[cowtowncoder]

Ok. Had a look and... yeah, no. These merge conflicts are actually beyond my repairing. I think I broke this pr, unfortunately; it would need to be re-created starting from 2.18 branch. The problem is that lots of code was removed from src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java and replaced with additions in src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java.

Unfortunately those 2 classes were heavily modified by this PR.

But if anyone has time and interest to reconstruct this PR based off 2.18, we could consider it (or, in near future when 2.18 released, against 2.19).

[fxshlein]

@eranl The submitter stop working 6months ago. Unless target issue is highly voted, it would be reasonable to say that there is small chance of merging unless @fxshlein or someone else picks it back up

I'm sick at the moment, but maybe I can give it a shot later this week. I was just waiting for the PR to be merged tho, as far as I was concerned nothing was left to do, right? As far as the last comment, I had already implemented the second solution, I was just presenting alternatives. Maybe that caused a misunderstanding, sorry!

[cowtowncoder]

@fxshlein Unfortunately as per my comment, PR's changes heavily conflict with the POJO property discovery rewrite in 2.18. And being essentially a new feature (although not technically API change), I wouldn't want to sneak it in a patch release of 2.17 -- it needs to go in a .0 version of a minor version. Right now that'd be 2.18.0 via 2.18 branch.

I wasn't able to quite see how to resolve conflicts of this PR, but of course if you can figure it out, we can use this PR. Alternatively it may be simpler (if more work) to start with 2.18 and re-create changes -- logic of POJO property discovery hasn't changed (minus bug fixes), just implementation.