Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

yihtserns · 2024-07-21T13:24:03Z

Proposal for #4626.

Turns out Record fields (or rather the annotations on them) are needed for both serialization (#3628, #3895, #3906 (comment)) & deserialization (#4626).

So we need to either keep them or change the way/order we harvest annotation info in POJOPropertiesCollector (for someone who knows every little about the codebase, I only dared to propose the former).

@JsonIgnore

…property when its accessor method is annotated with @JsonIgnore.

@JsonIgnore

…property when a Record's component has @JsonIgnore + the corresponding accessor method is overridden without @JsonIgnore.

…f ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying.

yihtserns · 2024-07-21T17:20:58Z

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

-        //    since there should not be setters.
-        final boolean inferMutators = !isRecordType()
-                && _config.isEnabled(MapperFeature.INFER_PROPERTY_MUTATORS);
+        final boolean inferMutators = _config.isEnabled(MapperFeature.INFER_PROPERTY_MUTATORS);


I didn't really understand this change, but I reverted it simply because it was part of #3737 even though it has no effect on #4626.

But I just found out that reverting this fixed #4630.

@JsonIgnoreProperties

…operty when a Record's component has @JsonIncludeProperties/@JsonIgnoreProperties + the corresponding accessor method is overridden.

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

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

cowtowncoder · 2024-07-21T21:36:59Z

@yihtserns Overall I think that makes sense, given it addresses problems you demonstrated.
Just want to change the checking of if an AnnotatedField is part of Record a bit, to do it at caller, not within AnnotatedField itself (since caller has all the information about declaring type).

yihtserns · 2024-07-21T22:28:56Z

Although there's no test, I just found out that this change broke #562 implementation for Records...

Could've closed #3439, but now it's blocked again... 😢

cowtowncoder · 2024-07-21T22:31:50Z

@yihtserns Ugh. Could you file a separate new issue (referencing #562 and this PR)?
Too bad we have many tests for POJOs wrt 562, but not any Record one.

Btw, big THANK YOU for finding all these issues as well as obviously fixing.

cowtowncoder · 2024-07-21T22:42:06Z

@yihtserns Not only that, but looks like jackson-module-kotlin (2.18 and master) tests started failing :-(

https://github.com/FasterXML/jackson-module-kotlin/actions/runs/10031852459

(on plus side, cascading rebuilds expose these failures sooner)

/cc @k163377

yihtserns · 2024-07-21T23:30:42Z

@cowtowncoder Seems like this is a bad PR, can you please revert it and I'll see if I can rework it?

Sorry for the trouble.

cowtowncoder · 2024-07-21T23:58:26Z

@yihtserns was about to suggest the same. Will do.

One thing that'd be good would be to take unit tests from here, add under .../failing.

cowtowncoder · 2024-07-22T00:11:14Z

@yihtserns Hmmmh. This is odd... Kotlin failures remain after revert. So it seems some other commit broke it, not this PR. But I don't see previous cascading build failures so I am bit confused as to what might have caused breakage.
Perhaps this could be re-applied, if #562 problem could be resolved.

cc @k163377 Looks like there are 3 test failures for jackson-module-kotlin; unclear what changed to cause that.

yihtserns · 2024-07-22T00:41:34Z

Created PR #4633 to add test cases for Records+JsonAnySetter including all the workarounds shared in #3439.

yihtserns · 2024-07-22T01:44:23Z

@cowtowncoder I suspect the Kotlin failure is caused by Kotlin refusing to play nice with FasterXML/jackson-annotations#258 (it generated a dodgy looking .class).

I downgraded jackson-annotations to 2.17.2 locally and the tests are passing again.

Add test cases to verify that Records' deserialization will ignore a …

f1b1078

…property when its accessor method is annotated with @JsonIgnore.

yihtserns force-pushed the records-desered-jsonignore branch from f2b2141 to ea3135a Compare July 21, 2024 13:31

yihtserns added 2 commits July 21, 2024 21:33

Add test cases to demonstrate that deserialization is not ignoring a …

2096c42

…property when a Record's component has @JsonIgnore + the corresponding accessor method is overridden without @JsonIgnore.

Ignore Records' immutable fields in BeanDeserializerFactory instead o…

2b4cf9f

…f ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying.

yihtserns force-pushed the records-desered-jsonignore branch from ea3135a to 2b4cf9f Compare July 21, 2024 13:33

yihtserns commented Jul 21, 2024

View reviewed changes

Add test cases to demonstrate that serialization is not ignoring a pr…

2ce22e3

…operty when a Record's component has @JsonIncludeProperties/@JsonIgnoreProperties + the corresponding accessor method is overridden.

yihtserns mentioned this pull request Jul 21, 2024

@JsonIncludeProperties, @JsonIgnoreProperties ignored when serializing Records, if there is getter override #4630

Closed

1 task

cowtowncoder reviewed Jul 21, 2024

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedField.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Jul 21, 2024

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java Show resolved Hide resolved

cowtowncoder reviewed Jul 21, 2024

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java Outdated Show resolved Hide resolved

cowtowncoder added 3 commits July 21, 2024 14:59

Merge branch '2.18' into records-desered-jsonignore

7a46880

Change the way (immutable) Record field is dropped

8eb2f55

Update release notes

ae1083d

cowtowncoder merged commit f26f9e3 into FasterXML:2.18 Jul 21, 2024
7 checks passed

cowtowncoder mentioned this pull request Jul 21, 2024

Revert "Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying." #4631

Merged

This was referenced Jul 22, 2024

Add Records+JsonAnySetter workarounds shared in #3439, as test cases. #4633

Closed

@JsonAnySetter not working when annotated on both constructor parameter & field #4634

Closed

Try to fix #4626, #4630 (start with failing tests) #4635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

yihtserns commented Jul 21, 2024 •

edited

Loading

yihtserns Jul 21, 2024 •

edited

Loading

cowtowncoder Jul 21, 2024

cowtowncoder commented Jul 21, 2024

yihtserns commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 21, 2024 •

edited

Loading

yihtserns commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 21, 2024

cowtowncoder commented Jul 22, 2024 •

edited

Loading

yihtserns commented Jul 22, 2024

yihtserns commented Jul 22, 2024 •

edited

Loading

Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

Conversation

yihtserns commented Jul 21, 2024 • edited Loading

yihtserns Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

cowtowncoder Jul 21, 2024

Choose a reason for hiding this comment

cowtowncoder commented Jul 21, 2024

yihtserns commented Jul 21, 2024 • edited Loading

cowtowncoder commented Jul 21, 2024 • edited Loading

cowtowncoder commented Jul 21, 2024 • edited Loading

yihtserns commented Jul 21, 2024 • edited Loading

cowtowncoder commented Jul 21, 2024

cowtowncoder commented Jul 22, 2024 • edited Loading

yihtserns commented Jul 22, 2024

yihtserns commented Jul 22, 2024 • edited Loading

yihtserns commented Jul 21, 2024 •

edited

Loading

yihtserns Jul 21, 2024 •

edited

Loading

yihtserns commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 21, 2024 •

edited

Loading

yihtserns commented Jul 21, 2024 •

edited

Loading

cowtowncoder commented Jul 22, 2024 •

edited

Loading

yihtserns commented Jul 22, 2024 •

edited

Loading