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

Allow @JsonAnySetter on Creators #4558

Merged
merged 28 commits into from
Jun 9, 2024
Merged

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Jun 1, 2024

resolves #562

This version is more adherent to existing call flow and simply extends existing implementations, such as....

  • Introduction of SettableAnyProperty subclasses JsonNodeParameterAnyProperty and MapParameterAnyProperty
  • Introduction of SettableAnyProperty methods getParameterIndex() createParameterObject() to streamline deserialization by PropertyValueBuffer

Also suggestions from PR #4366 have been followed

  • Added support for JsonNode and ObjectNode
  • Added tests for record also

@JooHyukKim JooHyukKim changed the title Allow @JsonAnySetter on Creator constructors Allow @JsonAnySetter on Creators Jun 1, 2024
@cowtowncoder
Copy link
Member

Excellent progress! Hoping to review this in detail (had a quick look, much improved from what I already saw -- and not just wrt addressing things I mentioned but overall)

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 2, 2024

FWTW, this is part of one of two listed "major design/implementation issues" page:

https://github.com/FasterXML/jackson-future-ideas/wiki/Major-Design---Implementation-Issues

so not "just" one of top-voted Mostly Wanted features but also considered a significant design/implementation concern.

if (name == null) {
boolean isAnySetter = Boolean.TRUE.equals(ctxt.getAnnotationIntrospector().hasAnySetter(param));
if (isAnySetter) {
if (anySetterFound) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we lose information on any-setter, and only call introspector to find multiples here... but then need to re-introspect later on?

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 remembered now, this code serves two purposes.

  • One is to detect and handle multiple any-setter (which, as you said, could be moved to later one below)
  • The other one, which I do need help with is... not any-setter not having any property name here. So any-setter creator parameter without any name specified (via @JsonProperty) throws error.

Copy link
Member Author

Choose a reason for hiding this comment

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

LMK if you have better idea with the second problem I am facing, @cowtowncoder

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Yes. For that purpose, we could only do a lookup if name not found. Let me think about this a bit, good point, something is needed here.

@cowtowncoder cowtowncoder merged commit 65a00fd into FasterXML:2.18 Jun 9, 2024
8 checks passed
@cowtowncoder cowtowncoder added most-wanted Tag to indicate that there is heavy user +1'ing action 2.18 labels Jun 9, 2024
@cowtowncoder cowtowncoder added this to the 2.18.0 milestone Jun 9, 2024
@JooHyukKim
Copy link
Member Author

Phew, that was alot of work 😆 Hopefully, it won't be too hard merging into master.
Thank you for your help! @cowtowncoder

@JooHyukKim JooHyukKim deleted the 562-version2 branch June 9, 2024 02:38
@cowtowncoder
Copy link
Member

@JooHyukKim thank you for actually implementing it & going through all tiny things I wanted changed :)

Surprisingly merge to master was quite straight-forward this time around.

And with this, we are getting somewhat close to 2.18.0 minimum feature set, as far as I care.
I do want this performance optimization: FasterXML/jackson-core#1284 for streaming but otherwise not many must-haves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.18 most-wanted Tag to indicate that there is heavy user +1'ing action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow @JsonAnySetter to flow through Creators
2 participants