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.

Sign up for GitHub

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

Jump to bottom

Fix issues 4697 and 2461 (@JsonUnwrapped caching) #4722

Merged
merged 8 commits into from
Oct 3, 2024
Merged

Fix issues 4697 and 2461 (@JsonUnwrapped caching) #4722

merged 8 commits into from
Oct 3, 2024

Conversation

SandeepGaur2016
Copy link
Contributor

@SandeepGaur2016 SandeepGaur2016 commented Sep 29, 2024 •
edited by cowtowncoder
Loading

I recently raised a bug (4697) and have now had some time to investigate the issue. Although this is my first contribution, I noticed that when we obtain a serializer from the case, the name transformation is reset in the _findAndAddDynamic method of UnwrappingBeanPropertyWriter. However, this method does not account for inner or nested properties. I have implemented a fix to reinitialize the serializer in such cases.

It has also fixed looks same issue from raised earlier (#2461)

Thanks,
Sandeep Gaur

@SandeepGaur2016 SandeepGaur2016 mentioned this pull request Sep 29, 2024
Closed
1 task
JooHyukKim
JooHyukKim reviewed Sep 29, 2024
View reviewed changes
Copy link
Member

@JooHyukKim JooHyukKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few notes, thanks!

src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated Show resolved Hide resolved
src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
@@ -459,7 +470,7 @@ Object readResolve() {
_field = null;
}
if (_serializer == null) {
_dynamicSerializers = PropertySerializerMap.emptyForProperties();
_dynamicSerializers = emptyForProperties();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, unncessary change (at least for this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, though it was a clean way to import statically

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer avoiding static imports for method calls fwtw. Maybe that should be added in style guide (or maybe it is mentioned and CONTRIBUTING.md should refer). I know IDEs can show where method comes from but for me (and I realize this is matter of preference) little bit of redundancy can be helpful.

src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
Comment on lines 329 to 334
if(base instanceof UnwrappingBeanPropertyWriter) {
_dynamicSerializers =
emptyForProperties();
} else {
_dynamicSerializers = base._dynamicSerializers;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the format consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I will set the formatter.

src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
Comment on lines 359 to 365
if(base instanceof UnwrappingBeanPropertyWriter) {
_dynamicSerializers =
emptyForProperties();
} else {
_dynamicSerializers = base._dynamicSerializers;
}
_suppressNulls = base._suppressNulls;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting needs to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
@@ -325,7 +326,12 @@ protected BeanPropertyWriter(BeanPropertyWriter base, PropertyName name) {
base._internalSettings);
}
_cfgSerializationType = base._cfgSerializationType;
_dynamicSerializers = base._dynamicSerializers;
if(base instanceof UnwrappingBeanPropertyWriter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency from super class to subclasses doesn't seem to be the right direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, did the same thing in a better way now

JooHyukKim
JooHyukKim reviewed Sep 29, 2024
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
Comment on lines 359 to 363
_dynamicSerializers = PropertySerializerMap.emptyForProperties();
} else {
_dynamicSerializers = base._dynamicSerializers;
}
_suppressNulls = base._suppressNulls;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these constructors called? Does this mean serializer information from 'base' should be lost?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_dynamicSerializers is purely performance optimization, caching to avoid unnecessary full introspection -- should never be needed for correctness. Hence if necessary we can just use empty instance for copy constructors.

cowtowncoder
cowtowncoder reviewed Sep 30, 2024
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
@@ -325,7 +326,11 @@ protected BeanPropertyWriter(BeanPropertyWriter base, PropertyName name) {
base._internalSettings);
}
_cfgSerializationType = base._cfgSerializationType;
_dynamicSerializers = base._dynamicSerializers;
if (base.needToResetSerialization()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure check is needed, should probably simply reset always; I'll add a general note to PR.

cowtowncoder
cowtowncoder reviewed Sep 30, 2024
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/ser/PropertyWriter.java Outdated
@@ -100,6 +100,13 @@ public <A extends Annotation> A findAnnotation(Class<A> acls) {
/**********************************************************
*/

/**
* A way to reset serialization to correctly populate serializer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unfortunately doesn't really describe anything....

@cowtowncoder
Copy link
Member

Ok: first of all, yes, it does sound like (and makes sense, too) that the issue is incorrect caching of resolved serializers: _dynamicSerializers is strictly for performance optimization. As such, maybe we should just be more aggressively clearing it on copy constructors: I'm fine not requiring checker method for trying to figure out.

Alternatively there is an existing method BeanPropertyWriter.isUnwrapping() which could (and for practical reasons I'd argue) and should be called instead of adding a new method.

But it seems to me simplest to just re-initialize -- empty instances are shared stateless singletons so there's no real cost for constructing. And in most cases this makes no difference anyway.

@SandeepGaur2016
Copy link
Contributor Author

SandeepGaur2016 commented Sep 30, 2024 •
edited
Loading

Hi,
I have removed the condition of UnwrappingBeanPropertyWrite, I simply set the _dynamicSerializers as empty list.

Thanks,
Sandeep gaur

@cowtowncoder cowtowncoder added cla-needed PR looks good (although may also require code review), but CLA needed from submitter 2.19 Issues planned at 2.19 or later labels Oct 1, 2024
@cowtowncoder
Copy link
Member

Ok, happy to merge this fix. Just one process thing first: since this is (I think) your first contribution, we'd need CLA from:

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

(this only needs to be done once before the first PR gets merged -- one is good for all future PRs).

It is usually easiest to print, fill & sign, scan/photo, email to cla at fasterxml dot com.
Once I get it, I'll get this merged ASAP.

Thank you again @SandeepGaur2016 for the contribution!

@cowtowncoder cowtowncoder changed the title Fixed issues 4697 and 2461 Fix issues 4697 and 2461 (@JsonUnwrapped caching) Oct 2, 2024
@SandeepGaur2016
Copy link
Contributor Author

I have sent the CLA form.

Thanks,
Sandeep Gaur

@cowtowncoder cowtowncoder added cla-received PR already covered by CLA (optional label) and removed cla-needed PR looks good (although may also require code review), but CLA needed from submitter labels Oct 3, 2024
@cowtowncoder
Copy link
Member

CLA received, can proceed!

@cowtowncoder cowtowncoder merged commit a900b49 into FasterXML:2.19 Oct 3, 2024
8 checks passed
@cowtowncoder cowtowncoder added this to the 2.19.0 milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JooHyukKim JooHyukKim JooHyukKim left review comments

@cowtowncoder cowtowncoder cowtowncoder approved these changes

Assignees
No one assigned
Labels
2.19 Issues planned at 2.19 or later cla-received PR already covered by CLA (optional label)
Projects
None yet
Milestone
2.19.0
Development

Successfully merging this pull request may close these issues.
3 participants
@SandeepGaur2016 @cowtowncoder @JooHyukKim