Fixing records deserialization issues with read-access when json renamed to same property name

[iifawzi]

Hello, this is an attempt to resolve deserialization issues when @JsonProperty.value has the same name as the property.

After debugging, I realized that this's the root cause of #5049. Given for records no constructors' parameters, this condition always passes for both attributes because for read-access only, ctorParam is always null due to removeNonVisible logic.
Changing this to ensure we don't get into the replacement if it is null seemed enough to fix the duplicate issue and to also fix the ignorance of the access for records #4628

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

Line 1815 in 25e1135

if (cprop.getConstructorParameter() == ctorParam) {

However, as this seems to be fixing all issues related to records, it's still not fixing the issue with JsonIgnore mentioned #4628. The root cause is this:

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

Lines 1436 to 1449 in 25e1135

	if (_replaceCreatorProperty(_creatorProperties, prop)) {
	// [databind#2001]: New name of property was ignored previously? Remove from ignored
	// 01-May-2018, tatu: I have a feeling this will need to be revisited at some point,
	// to avoid removing some types of removals, possibly. But will do for now.
	// 16-May-2020, tatu: ... and so the day came, [databind#2118] failed
	// when explicit rename added to ignorals (for READ_ONLY) was suddenly
	// removed from ignoral list. So, added a guard statement above so that
	// ignoral is ONLY removed if there was matching creator property.
	//
	// Chances are this is not the last tweak we need but... that bridge then etc
	if (_ignoredPropertyNames != null) {
	_ignoredPropertyNames.remove(name);
	}

given we're renaming to same property name, it's then removed from the ignorable list, I'm still unsure of how this should be fixed, so will try to tackle it in a separate PR, in this PR I marked the test as expected to fail so we're aware of it.

[iifawzi]

CLA signed earlier today for this repoisotry

[cowtowncoder]

2 quick questions:

1. I think you are saying this does fix Duplicate creator property "b" (index 0 vs 1) on simple java record #5049, but not @JsonIgnore and @JsonProperty.access=READ_ONLY on Record property ignored for deserialization #4628 ?
2. While there's some theoretical risk, I feel risk is low and it'd be good to actually target 2.18 branch (for 2.18.4) with this (or separate) PR? (note: I always merge forward so only one PR needed, but needs to target oldest minor version branch)

On (2), actually, I think I could try cherry-picking after merging this against 2.19 so that's probably fine.

[iifawzi]

2 quick questions:

1. I think you are saying this does fix Duplicate creator property "b" (index 0 vs 1) on simple java record #5049, but not @JsonIgnore & @JsonProperty.access=READ_ONLY on Record property ignored for deserialization, if @JsonProperty.value is the same as the property name #4628 ?
2. While there's some theoretical risk, I feel risk is low and it'd be good to actually target 2.18 branch (for 2.18.4) with this (or separate) PR? (note: I always merge forward so only one PR needed, but needs to target oldest minor version branch)

On (2), actually, I think I could try cherry-picking after merging this against 2.19 so that's probably fine.

• Yes, it fully resolve Duplicate creator property "b" (index 0 vs 1) on simple java record #5049 and the first part of @JsonIgnore and @JsonProperty.access=READ_ONLY on Record property ignored for deserialization #4628 @JsonProperty.access=READ_ONLY, but not with the usage of @JsonIgnore which's second part of @JsonIgnore and @JsonProperty.access=READ_ONLY on Record property ignored for deserialization #4628
• I just checked cherry-picking against 2.18 and there's no conflicts but sorting the test file out, so I think we can move into that direction, but if you wish, I can create a separate PR

[cowtowncoder]

@iifawzi Ok if it's simple enough, separate PR against 2.18 would be ideal. If not, I can figure out cherry-picking.

This is excellent progress -- thank you very much for working on this!

[iifawzi]

@iifawzi Ok if it's simple enough, separate PR against 2.18 would be ideal. If not, I can figure out cherry-picking.

This is excellent progress -- thank you very much for working on this!

Thank you @cowtowncoder!
You can find #5073 targeting 2.18.

[cowtowncoder]

Superceded by #5073, closing.