Skip to content

Allow using @JsonPropertyOrder with any-property (@JsonAnyGetter) #4775

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

Merged
merged 48 commits into from
Feb 9, 2025

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Nov 1, 2024

Description

This PR tries to achieve functionality as per title by making any-getter a BeanPropertyWriter (subtype).
Previously any-getter property has been handled as standalone. Being standalone meaning it need individual handling for all cases, unlike other sub-classes of BeanPropertyWriter naturally gets bunch of property handling applied.

Notes

Same as #4396, but rebased against 2.19 version. This PR resolves... #4388 and part of #2592 AnyGetter with JsonInclude + JsonFilter combo

@JooHyukKim JooHyukKim closed this Nov 1, 2024
@cowtowncoder
Copy link
Member

@JooHyukKim Ok, I am trying to understand the PR and I think what is needed is high-level description of logical change(s) being made. I think I have an idea but since you understand the intent, could you write a description of changes (more on "why" things changed than "what").
I can then better review this PR so we can get it merged.

@JooHyukKim
Copy link
Member Author

@JooHyukKim Ok, I am trying to understand the PR and I think what is needed is high-level description of logical change(s) being made.

Will do! 👍🏼

Comment on lines +48 to +51
// In 2.19, Fails Actual
// : {"value":{"firstProperty":"first","secondProperties":"second","forthProperty":"forth","third_A":"third_A","third_B":"third_B"}}
// Getting better, after #4775 in 2.19, fails Actual
// : {"value":["first","second",{"third_A":"third_A","third_B":"third_B"},"forth"]}
Copy link
Member Author

Choose a reason for hiding this comment

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

@cowtowncoder this what I meant in above comment

@JooHyukKim
Copy link
Member Author

@JooHyukKim Ok, I am trying to understand the PR and I think what is needed is high-level description of logical change(s) being made. I think I have an idea but since you understand the intent, could you write a description of changes (more on "why" things changed than "what"). I can then better review this PR so we can get it merged.

@cowtowncoder updated PR description as suggested 👍🏼. While writing the PR description, I was reminded of what we did with Enums via #3832 way back, felt similar idk 😅

@cowtowncoder
Copy link
Member

Looks good, will merge. May be tricky to merge to master but... hope it's ok.

@cowtowncoder cowtowncoder merged commit 1d30be6 into FasterXML:2.19 Feb 9, 2025
8 checks passed
@JooHyukKim
Copy link
Member Author

Looks good, will merge. May be tricky to merge to master but... hope it's ok.

@cowtowncoder Lemme me know if anything!


final ObjectMapper MAPPER = newJsonMapper();

@JacksonTestFailureExpected
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Move to test dir, out of tofix

Copy link
Member

Choose a reason for hiding this comment

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

Is the test then not failing or... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the test is expected behavior, we may move to tests dir.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #4965 accordingly.

@cowtowncoder
Copy link
Member

Looks good, will merge. May be tricky to merge to master but... hope it's ok.

@cowtowncoder Lemme me know if anything!

Merging went just fine! Yay.

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 9, 2025

@JooHyukKim Ouch. One problem: XML module broke

https://github.com/FasterXML/jackson-dataformat-xml/actions/runs/13222535686/job/36909393854

since it refer to _anyGetterWriter due to sub-classing BeanSerializer etc...

And on short term that also breaks Kotlin module's CI (since it -- unfortunately -- depends on XML module for tests (IMO, shouldn't)).

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.

2 participants