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 Creator constructors #4366

Closed
wants to merge 72 commits into from

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Feb 3, 2024

resolves #562

@JooHyukKim JooHyukKim marked this pull request as draft February 3, 2024 14:39

// [databind#562]: Respect @JsonAnySetter in @JsonCreator
// check if we have anySetter Param
AnnotationIntrospector ai = _context.getAnnotationIntrospector();
Copy link
Member

Choose a reason for hiding this comment

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

Alas, this is not acceptable from performance perspective: all annotation introspection must occur when constructing deserializers and related handlers. It should be possible to change handling to keep track of information about existence of "any setter", instead of trying to introspect for every deserialization call.

@cowtowncoder
Copy link
Member

Impressive. I am bit hesitant to consider this as one more thing before work on Property Introspection rewrite, but no harm in figuring out how this could work. Added some notes.

@JooHyukKim
Copy link
Member Author

Impressive. I am bit hesitant to consider this as one more thing before work on Property Introspection rewrite,

I concur. And also in the issue #562, this feature is to get included in 3.x.

So I think we can safely consider this feature only after Property Introspection rewrite.

but no harm in figuring out how this could work. Added some notes.

Thankssss! I will keep them updated

@JooHyukKim JooHyukKim changed the title [DRAFT] for #562 feature Allow @JsonAnySetter on Creator constructors Feb 9, 2024
@JooHyukKim JooHyukKim marked this pull request as ready for review May 31, 2024 12:32
@JooHyukKim
Copy link
Member Author

JooHyukKim commented May 31, 2024

WDYT, @cowtowncoder ?
We can visit #4396 after this.

@cowtowncoder
Copy link
Member

WDYT, @cowtowncoder ? We can visit #4396 after this.

Ok. I am still struggling with this; reading again. It is improving, but I am still worried about added complexity. But at the same time this IS a VERY highly voted feature (that is, #562 is)

So it'd be great to be able to somehow simplify handling. Not sure how to, tho, so I need to keep on re-reading code.

Aside from that, some missing things:

  1. No tests yet for Record case? (should work fine I assume, but just noting)
  2. Only supports Map-backed any-setter, but not JsonNode-backed one (unlike Field-based one) -- adding support is probably not hard but does add to code, special casing

@JooHyukKim JooHyukKim marked this pull request as draft June 1, 2024 00:29
@JooHyukKim
Copy link
Member Author

JooHyukKim commented Jun 1, 2024

So it'd be great to be able to somehow simplify handling. Not sure how to, tho, so I need to keep on re-reading code.

I totally agree. Current implementation is both complex and scattered around places.
I sort of focused on making work, not so much around maintainability.

I will probably start isolating the any-setter logics first, maybe to a class, then will read through your comments and add suggested missing parts.

thnx @cowtowncoder

PS: Reviews applied first.

@JooHyukKim
Copy link
Member Author

Temporarily closing this in favor of #4558 where I added record and JsonNode tests
/cc @cowtowncoder

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.

Allow @JsonAnySetter to flow through Creators
2 participants