-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Experimenting with #1467: Support for @JsonUnwrapped in @JsonCreators #4271
base: 2.19
Are you sure you want to change the base?
Conversation
public boolean isUnwrapped(SettableBeanProperty property) { | ||
return this._unwrappedPropertyNames.contains(property.getName()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be sufficient to check for which properties are unwrapped? Not sure how different things affect the result of getName()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory names should be PropertyName
instances (matters with XML, mostly), but internally a few things are still just String
s so I suspect this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in name :)
@@ -1,101 +0,0 @@ | |||
package com.fasterxml.jackson.databind.struct; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this test deleted? Was it just renamed or ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the test that verified that error messages were printed when using @JsonUnwrapped
in a creator, which hopefully shouldn't be needed anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Yes, that makes sense. :)
/** | ||
* Tests to verify [databind#1467]. | ||
*/ | ||
public class TestUnwrappedWithJsonCreator extends BaseMapTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newer naming convention leads to UnwrappedWith[Json]CreatorTest
(we haven't renamed all or even most tests but trying to move in that direction).
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 |
public void testUnwrappedWithRecord() throws Exception | ||
{ | ||
String json = "{\"unrelated\": \"unrelatedValue\", \"property1\": \"value1\", \"property2\": \"value2\"}"; | ||
RecordWithJsonUnwrapped outer = MAPPER.readValue(json, RecordWithJsonUnwrapped.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to a round-trip. Should I also change the tests in UnwrappedWithCreatorTest
? Since there are already similar tests for serializing regular classes.
@cowtowncoder I sent the signed CLA to Just to verify, here it says to email the CLA to |
@@ -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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
... 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I thought it might just contain @JsonUnwrapped
:)
protected final List<SettableBeanProperty> _properties; | ||
protected final Set<String> _unwrappedPropertyNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and if isUnwrapped()
is not actually needed, can drop this Set
and code that maintains it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the set and the check!
public class UnwrappedWithCreatorTest extends BaseMapTest | ||
{ | ||
static class ExplicitWithoutName { | ||
private final String _unrelated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
539272d
to
d65dfac
Compare
.../java/com/fasterxml/jackson/databind/struct/UnwrappedPropertyBasedCreatorWithPrefixTest.java
Show resolved
Hide resolved
I noticed a minor issue with the placeholder name: If you use two
I added a test case here: From what I can see, there are three solutions to this:
|
To me (2), adding index for bogus name, sounds reasonable at this point. +1. |
While looking for a stop-gap solution for this in GitHub, I found this (usage) rather straightforward solution for Looking forward to having this merged. Thank you for the contribution! |
@cowtowncoder, what's the status of this PR? |
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. |
Re-based, but now there are conflicts to resolve... not sure I have time to tackle those right now. |
Ok. Had a look and... yeah, no. These merge conflicts are actually beyond my repairing. 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). |
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! |
@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 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. |
27ce184
to
22cb7ee
Compare
Co-authored-by: Tatu Saloranta <[email protected]>
Hi @cowtowncoder! Sorry for the long wait, completely forgot about this PR if I'm honest. I reimplemented everything on top of |
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 🙂 |
@@ -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}. | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add @since 2.18
? likewise with any other new public methods.
@@ -713,25 +713,6 @@ public String toString() | |||
/********************************************************** | |||
*/ | |||
|
|||
protected SettableBeanProperty _rename(SettableBeanProperty prop, NameTransformer xf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks backward compatibility - add back the old method and deprecate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, readded it!
} | ||
|
||
protected UnwrappedPropertyHandler(List<SettableBeanProperty> props) { | ||
protected UnwrappedPropertyHandler(List<SettableBeanProperty> creatorProps, List<SettableBeanProperty> props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to keep the existing method and deprecate it
_properties = props; | ||
} | ||
|
||
public void addCreatorProperty(SettableBeanProperty property) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to have javadoc including its @since 2.18
return newProps; | ||
} | ||
|
||
public PropertyValueBuffer processUnwrappedCreatorProperties( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javadoc with @since 2.18
/** | ||
* We need a placeholder for creator properties that don't have a name, | ||
* but are marked with `@JsonWrapped` annotation. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@since 2.18
Quick note: due to timing, this won't make it in 2.18, too risky. |
haha, I already figured when I saw the 2.18 RC a few days ago. Changed it to 2.19! |
@cowtowncoder I changed the base branch to 2.19, seems to work without conflicts. I guess this could be merged now that the 2.19 branch exists, right? |
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... 😆