-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Potential fix for #5184 (Record ignoral) #5419
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
base: 2.x
Are you sure you want to change the base?
Conversation
|
This PR seems to be meant #5184 ? 4 and 8 are flipped :) @cowtowncoder |
|
Thank you @JooHyukKim -- you are right. FIxed. :) |
| // @JsonIgnore that would conflict with record component properties. | ||
| // For example, a record with component "value" should not have its property ignored | ||
| // just because there's a non-accessor method "getValue()" marked with @JsonIgnore. | ||
| if (ignore && isRecordType() && !_forSerialization && !nameExplicit) { |
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 might be ok but...
| // just because there's a non-accessor method "getValue()" marked with @JsonIgnore. | ||
| if (ignore && isRecordType() && !_forSerialization && !nameExplicit) { | ||
| // Only skip if this is NOT the actual record accessor method | ||
| if (_accessorNaming instanceof DefaultAccessorNamingStrategy.RecordNaming) { |
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 gets into too much special casing; need to have better knowledge of "true" Record properties.
Possible fix for #5184: not really loving all special-casing but might help figure out cleaner fix.