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

fxshlein · 2023-12-20T01:07:44Z

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

fxshlein · 2023-12-20T01:13:19Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

+ public boolean isUnwrapped(SettableBeanProperty property) {
+ return this._unwrappedPropertyNames.contains(property.getName());
+ }


Would this be sufficient to check for which properties are unwrapped? Not sure how different things affect the result of getName()

In theory names should be PropertyName instances (matters with XML, mostly), but internally a few things are still just Strings so I suspect this is fine.

As per my comment higher up, I am not sure this method is actually needed: at least tests included seem to pass just fine without it being called (as well as all other existing tests).

cowtowncoder · 2023-12-20T01:13:54Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

 import com.fasterxml.jackson.databind.deser.SettableBeanProperty;
 import com.fasterxml.jackson.databind.util.NameTransformer;
 import com.fasterxml.jackson.databind.util.TokenBuffer;

+import java.io.IOException;


Could you move these back to where they were? (I assume IDE just reordered them automatically).

(we finally have Coding Style Guide -> https://github.com/FasterXML/jackson/blob/master/contribution/jackson-coding-style-guide.md

that explains intended ordering)

cowtowncoder · 2023-12-20T01:15:43Z

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java

@@ -716,6 +717,14 @@ private void _addCreatorParam(Map<String, POJOPropertyBuilder> props,
 PropertyName pn = _annotationIntrospector.findNameForDeserialization(param);
 boolean expl = (pn != null && !pn.isEmpty());
 if (!expl) {
+ boolean unrwapping = _annotationIntrospector.findUnwrappingNameTransformer(param) != null;


typo in name :)

cowtowncoder · 2023-12-20T01:16:45Z

src/test/java/com/fasterxml/jackson/databind/struct/UnwrappedCreatorParam265Test.java

@@ -1,101 +0,0 @@
-package com.fasterxml.jackson.databind.struct;


Why was this test deleted? Was it just renamed or ... ?

This was the test that verified that error messages were printed when using @JsonUnwrapped in a creator, which hopefully shouldn't be needed anymore :)

Ah. Yes, that makes sense. :)

cowtowncoder · 2023-12-20T01:17:37Z

src/test/java/com/fasterxml/jackson/databind/struct/TestUnwrappedWithJsonCreator.java

+/**
+ * Tests to verify [databind#1467].
+ */
+public class TestUnwrappedWithJsonCreator extends BaseMapTest


Newer naming convention leads to UnwrappedWith[Json]CreatorTest (we haven't renamed all or even most tests but trying to move in that direction).

cowtowncoder · 2023-12-20T01:22:05Z

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.

cowtowncoder · 2023-12-20T01:24:23Z

src/test-jdk17/java/com/fasterxml/jackson/databind/records/RecordWithJsonUnwrappedTest.java

+ public void testUnwrappedWithRecord() throws Exception
+ {
+ String json = "{\"unrelated\": \"unrelatedValue\", \"property1\": \"value1\", \"property2\": \"value2\"}";
+ RecordWithJsonUnwrapped outer = MAPPER.readValue(json, RecordWithJsonUnwrapped.class);


Works, but might make sense to first serialize from instance, then read back (round-trip). To make both directions work (I realize that this PR is just for deserialization but still)

Changed it to a round-trip. Should I also change the tests in UnwrappedWithCreatorTest? Since there are already similar tests for serializing regular classes.

fxshlein · 2023-12-20T02:06:02Z

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

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

cowtowncoder · 2024-01-05T02:32:34Z

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializer.java

@@ -855,7 +855,7 @@ protected Object deserializeUsingPropertyBasedWithUnwrapped(JsonParser p, Deseri
 if (buffer.readIdProperty(propName) && creatorProp == null) {
 continue;
 }
- if (creatorProp != null) {
+ if (creatorProp != null && !_unwrappedPropertyHandler.isUnwrapped(creatorProp)) {


I'll have to think about this; ideally such lookup shouldn't be needed (rather, creatorProp would have knowledge of whether it's created via unwrapping).

Oddly enough, removing this call makes no difference; all tests pass the same (including Record test).

So is it actually needed? Either we should have a test that fails without it, or it should just be removed, I think.

Yeah, it seems like this makes no sense anymore, I probably implemented it before adapting the rest of the code. The issue this would have solved is if you have a structure like this:

record Outer(@JsonUnwrapped Inner propertyName) {} record Inner(String propertyName, String otherPropertyName) {}

There is a property called propertyName inside and outside the unwrapped structure, so when deserializing and encountering the property called "propertyName", I initially thought it would find that property. But as far as this code is concerned, the outer property is called "@JsonUnwrapped" (because this is the fake name it's given), so it should not clash either way. Because of this fake name, the _unwrappedPropertyNames set used for isUnwrapped actually only contains "@JsonUnwrapped"... 😄

Right, I thought it might just contain @JsonUnwrapped :)

cowtowncoder · 2024-01-05T02:53:20Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

 protected final List<SettableBeanProperty> _properties;
+ protected final Set<String> _unwrappedPropertyNames;


... and if isUnwrapped() is not actually needed, can drop this Set and code that maintains it.

Removed the set and the check!

cowtowncoder · 2024-01-05T02:58:17Z

src/test/java/com/fasterxml/jackson/databind/struct/UnwrappedWithCreatorTest.java

+public class UnwrappedWithCreatorTest extends BaseMapTest
+{
+ static class ExplicitWithoutName {
+ private final String _unrelated;


@fxshlein I changed internal field names to make 100% sure test could not accidentally work due to field name matching constructor property name (since that would actually work despite final field declaration, in absence of Creator parameters).

.../java/com/fasterxml/jackson/databind/struct/UnwrappedPropertyBasedCreatorWithPrefixTest.java

fxshlein · 2024-01-07T02:37:37Z

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:

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.
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.
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: fxshlein@3bd10a0, but it's already a pretty massive undertaking without even touching everything.

cowtowncoder · 2024-01-13T00:44:54Z

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

wingsofovnia · 2024-03-24T08:59:04Z

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 · 2024-07-16T23:38:48Z

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

JooHyukKim · 2024-07-16T23:49:52Z

@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 · 2024-07-17T02:09:48Z

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 · 2024-07-17T02:12:34Z

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

cowtowncoder · 2024-07-17T04:07:46Z

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 · 2024-07-17T06:40:29Z

@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 · 2024-07-17T15:32:01Z

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

Co-authored-by: Tatu Saloranta <tatu.saloranta@iki.fi>

fxshlein · 2024-09-01T09:57:28Z

Hi @cowtowncoder! Sorry for the long wait, completely forgot about this PR if I'm honest.

I reimplemented everything on top of 1.18. I can't rename the branch in the PR, so it's still called 1.17, but it's the latest branch. I'll also deploy the new updated branch from my fork to our internal maven repo and run our tests using that. Those use lots of @JsonUnwrapped annotations in previously unsupported places.

fxshlein · 2024-09-01T21:31:31Z

Quick update, I tested the updated branch with our internal code (which was based on the old branch) now, and it still works perfectly. I think this should be ready to merge now 🙂

pjfanning · 2024-09-01T21:47:42Z

src/main/java/com/fasterxml/jackson/databind/deser/SettableBeanProperty.java

@@ -584,6 +585,25 @@ public final Object deserializeWith(JsonParser p, DeserializationContext ctxt,
 return value;
 }

+ /**
+ * Returns a copy of this property, unwrapped using the given {@link NameTransformer}.
+ */


Can you add @since 2.18? likewise with any other new public methods.

pjfanning · 2024-09-01T21:48:22Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/BeanPropertyMap.java

@@ -713,25 +713,6 @@ public String toString()
 /**********************************************************
 */

- protected SettableBeanProperty _rename(SettableBeanProperty prop, NameTransformer xf)


this breaks backward compatibility - add back the old method and deprecate it

the method can call the new unwrap method, but we need to keep the existing method (its implementation can change - as long as it the change retains the existing behaviour)

good point, readded it!

pjfanning · 2024-09-01T21:50:27Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

 }

- protected UnwrappedPropertyHandler(List<SettableBeanProperty> props) {
+ protected UnwrappedPropertyHandler(List<SettableBeanProperty> creatorProps, List<SettableBeanProperty> props) {


you need to keep the existing method and deprecate it

pjfanning · 2024-09-01T21:51:04Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

 _properties = props;
 }

+ public void addCreatorProperty(SettableBeanProperty property) {


needs to have javadoc including its @since 2.18

pjfanning · 2024-09-01T21:51:31Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

+ return newProps;
+ }
+
+ public PropertyValueBuffer processUnwrappedCreatorProperties(


javadoc with @since 2.18

pjfanning · 2024-09-01T21:51:49Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/UnwrappedPropertyHandler.java

+ /**
+ * We need a placeholder for creator properties that don't have a name,
+ * but are marked with `@JsonWrapped` annotation.
+ */


@since 2.18

cowtowncoder · 2024-09-02T20:53:40Z

Quick note: due to timing, this won't make it in 2.18, too risky.

fxshlein · 2024-09-03T18:22:01Z

haha, I already figured when I saw the 2.18 RC a few days ago. Changed it to 2.19!

fxshlein mentioned this pull request Dec 20, 2023

Support @JsonUnwrapped with @JsonCreator (if possible) #1467

Open

fxshlein commented Dec 20, 2023

View reviewed changes

cowtowncoder reviewed Dec 20, 2023

View reviewed changes

cowtowncoder reviewed Jan 5, 2024

View reviewed changes

fxshlein force-pushed the 2.17 branch 3 times, most recently from 539272d to d65dfac Compare January 6, 2024 22:47

fxshlein commented Jan 6, 2024

View reviewed changes

.../java/com/fasterxml/jackson/databind/struct/UnwrappedPropertyBasedCreatorWithPrefixTest.java Show resolved Hide resolved

wingsofovnia mentioned this pull request Mar 24, 2024

Jackson property annotations are not working with data classes FasterXML/jackson-module-kotlin#56

Closed

cowtowncoder changed the base branch from 2.17 to 2.18 July 17, 2024 02:10

fxshlein force-pushed the 2.17 branch 2 times, most recently from 27ce184 to 22cb7ee Compare September 1, 2024 09:43

Implement FasterXML#1467: Support for @JsonCreator and @JsonUnwrapped

ffbcbce

Co-authored-by: Tatu Saloranta <tatu.saloranta@iki.fi>

fxshlein force-pushed the 2.17 branch from 22cb7ee to ffbcbce Compare September 1, 2024 09:44

pjfanning reviewed Sep 1, 2024

View reviewed changes

fxshlein added 2 commits September 2, 2024 12:45

add deprecations and versions to comments

ff377aa

add missing since comment to property

d01aee2

fxshlein force-pushed the 2.17 branch from 2c1d096 to d01aee2 Compare September 2, 2024 10:56

update since comments to 2.19

1ed23f2

		@@ -1,101 +0,0 @@
		package com.fasterxml.jackson.databind.struct;

		protected final List<SettableBeanProperty> _properties;
		protected final Set<String> _unwrappedPropertyNames;

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

Are you sure you want to change the base?

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

Conversation

fxshlein commented Dec 20, 2023

fxshlein Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxshlein commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxshlein Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxshlein commented Jan 7, 2024

cowtowncoder commented Jan 13, 2024

wingsofovnia commented Mar 24, 2024

eranl commented Jul 16, 2024

JooHyukKim commented Jul 16, 2024

cowtowncoder commented Jul 17, 2024 • edited Loading

cowtowncoder commented Jul 17, 2024

cowtowncoder commented Jul 17, 2024

fxshlein commented Jul 17, 2024

cowtowncoder commented Jul 17, 2024

fxshlein commented Sep 1, 2024

fxshlein commented Sep 1, 2024

pjfanning Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

pjfanning Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Sep 2, 2024

fxshlein commented Sep 3, 2024

fxshlein Dec 20, 2023 •

edited

Loading

fxshlein Jan 6, 2024 •

edited

Loading

cowtowncoder commented Jul 17, 2024 •

edited

Loading

pjfanning Sep 1, 2024 •

edited

Loading

pjfanning Sep 1, 2024 •

edited

Loading