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!

…tally make test "work".

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

fxshlein · 2024-01-06T22:53:13Z

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

+    static class Outer {
+        @JsonUnwrapped(prefix = "inner-")
+        Inner inner;
+    }
+
+    static class Inner {
+        private final String _property;
+
+        public Inner(@JsonProperty("property") String property) {
+            _property = property;
+        }
+
+        public String getProperty() {
+            return _property;
+        }
+    }


@cowtowncoder While looking at something else, I noticed that you could not do this because the NameTransformer used for unwrapping is not applied to _propertyBasedCreator, only to _beanProperties. This commit seems to fix that: d65dfac. I guess it broadly fits the scope of the PR, but I can also remove it and make a separate one. In theory its more or less unrelated.

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!

fxshlein added 2 commits December 20, 2023 01:59

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

cb39812

remove unused function

d155cec

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

add test for @JsonUnwrapped in records

6dcaadb

cowtowncoder reviewed Dec 20, 2023

View reviewed changes

fxshlein added 2 commits December 20, 2023 02:29

code style fixes

f0d6d46

do round-trip in unwrapped record test

9001cd6

Merge branch '2.17' into fxshlein/2.17

411c6b8

cowtowncoder reviewed Jan 5, 2024

View reviewed changes

Minor change to test to ensure that Field assignment couldn't acciden…

fd13cd9

…tally make test "work".

cowtowncoder reviewed Jan 5, 2024

View reviewed changes

cowtowncoder and others added 2 commits January 4, 2024 19:00

convert tabs -> spaces for test class

0f38ad8

remove unnecessary unwrapped check

7ef8358

fxshlein force-pushed the 2.17 branch 2 times, most recently from 1c6e6b5 to 539272d Compare January 6, 2024 22:45

fix @JsonUnwrapped for fields with property based creator

d65dfac

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

fxshlein commented Jan 6, 2024

View reviewed changes

handle multiple unnamed unwrapped creator properties

1a7ed14

Merge branch '2.17' into fxshlein/2.17

28f9874

cowtowncoder added 3 commits January 13, 2024 19:53

Minor tweaking

a77c868

Merge branch '2.17' into 2.17

226259d

Merge branch '2.17' into 2.17

9f49cfd

wingsofovnia mentioned this pull request Mar 24, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fxshlein commented Dec 20, 2023

fxshlein Dec 20, 2023 •

edited

cowtowncoder Dec 20, 2023

cowtowncoder Jan 5, 2024

cowtowncoder Dec 20, 2023

cowtowncoder Dec 20, 2023

cowtowncoder Dec 20, 2023

fxshlein Dec 20, 2023

cowtowncoder Dec 20, 2023

cowtowncoder Dec 20, 2023

cowtowncoder commented Dec 20, 2023

cowtowncoder Dec 20, 2023

fxshlein Dec 20, 2023

fxshlein commented Dec 20, 2023

cowtowncoder Jan 5, 2024

cowtowncoder Jan 5, 2024

fxshlein Jan 6, 2024 •

edited

cowtowncoder Jan 7, 2024

cowtowncoder Jan 5, 2024

fxshlein Jan 6, 2024

cowtowncoder Jan 5, 2024

fxshlein Jan 6, 2024

fxshlein commented Jan 7, 2024

cowtowncoder commented Jan 13, 2024

wingsofovnia commented Mar 24, 2024

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

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

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

fxshlein commented Jan 7, 2024

cowtowncoder commented Jan 13, 2024

wingsofovnia commented Mar 24, 2024

fxshlein Dec 20, 2023 •

edited

fxshlein Jan 6, 2024 •

edited