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.

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

Add any-setter handling in PropertyValueBuffer post #562 #4720

Merged
merged 6 commits into from
Sep 29, 2024

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Sep 28, 2024

fixes #4508

This sort of adds missing handling in addition to #4508. Where we should also consider situations like #4508, where extensions (like Kotlin module) iterates through parameters one by one, via hasParmeter() and getParameter(), unlike StdValueInstantiator going through all parameters at once via getParameters(SettableBeanProperty[]).

Reproduction with fix

  1. pull : https://github.com/JooHyukKim/jackson-module-kotlin/tree/test-databind-4508
  2. then pull : https://github.com/JooHyukKim/jackson-databind/tree/DEMO-fix-4508 (or directly pull the branch in this PR, just make sure to update the pom.xml in kotlin module accordingly.
  3. build the databind module
  4. run Github562Test

@JooHyukKim JooHyukKim changed the title [DRAFT] Add any-setter handling in PropertyValueBuffer post #562 Add any-setter handling in PropertyValueBuffer post #562 Sep 28, 2024
Comment on lines 141 to 145
if (_anyParamSetter != null) {
if (prop.getCreatorIndex() == _anyParamSetter.getParameterIndex()) {
return true;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if this is the most optimal way of checking, any idea?

Copy link
Member

Choose a reason for hiding this comment

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

Let me rewrite it slightly, considering that expectation is that match (true) is more likely than non-match.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rewrite looks cleaner, thanks!

@@ -81,6 +81,9 @@ Project: jackson-databind
(contributed by @pjfanning)
#4709: Add `JacksonCollectors` with `toArrayNode()` implementation
(contributed by @rikkarth)
#4508: Deserialized JsonAnySetter field in Kotlin data class is null #4508
(reported by @MaximValeev)
Copy link
Member

Choose a reason for hiding this comment

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

2.18.0 is released already - shouldn't this be in the 2.18.1 section?

/**
* Helper method called to create and set any values buffered for "any setter"
*
* @since 2.18
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need since annotations on private methods? In which case, this is wrong, it should be 2.18.1 - be I prefer not to annotate private methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense.

@pjfanning
Copy link
Member

any way to test this in a unit test?

@JooHyukKim
Copy link
Member Author

any way to test this in a unit test?

Will try to come up one soon!

@JooHyukKim
Copy link
Member Author

Added reproduction (with fix) in PR descrption. cc @pjfanning @cowtowncoder

@cowtowncoder cowtowncoder merged commit a79ce09 into FasterXML:2.18 Sep 29, 2024
7 checks passed
@JooHyukKim JooHyukKim deleted the fix-4508 branch September 29, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants