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

Make JsonGenerator::writeTypePrefix method to not make a WRAPPER_ARRAY when typeIdDef.id == null #1356

Merged
merged 4 commits into from
Nov 9, 2024

Conversation

Gems
Copy link
Contributor

@Gems Gems commented Nov 5, 2024

This PR addresses GitHub issue #4772.

Summary

In certain cases where @JsonTypeInfo is used with JsonTypeInfo.Id.DEDUCTION, typeDef.id may resolve to null. For example, in the referenced issue, this occurs with a class based on the Collection interface, resulting in typeDef.id being null.

Change Details

This update modifies the sonGenerator::writeTypePrefix method so that it doesn’t generate a wrapping array with a "null" key for classes with a null typeDef.id. The current behavior, which includes an unnecessary wrapping array labeled by "null", can be misleading. By skipping this wrapper, we aim to make the serialization output more intuitive and aligned with user expectations.

Rationale

The previous behavior of adding a wrapping array with a "null" key was confusing and did not provide meaningful value to users. Removing it simplifies the output and makes the serialization format clearer, especially in cases involving classes based on generic interfaces like Collection.

Impact

This change will make the output format more predictable and consistent, especially for users employing @JsonTypeInfo with JsonTypeInfo.Id.DEDUCTION. Existing tests and validation tools should ensure that this change does not disrupt compatibility with existing deserialization setups.

@Gems Gems changed the title fix: Make JsonGenerator::writeTypePrefix method to not make a WRAPPER_ARRAY when typeIdDef.id == null fix: Make JsonGenerator::writeTypePrefix method to not make a WRAPPER_ARRAY when typeIdDef.id == null Nov 5, 2024
@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Nov 5, 2024
@cowtowncoder
Copy link
Member

One process thing: beyond code review, before merging I'd need CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(unless already done earlier; one CLA per contributor is fine for all Jackson contributions).
The usual way is to print, fill & sign, scan/photo, email to cla at fasterxml dot com.

@cowtowncoder
Copy link
Member

Ah ok: so we don't want type id like:

["null", [ 1, 2, "foo" ]]

which I totally agree with.

However, actual null value seems more reasonable.

Alternatively another maybe even better way would be to consider null typeId to mean "no type wrapping" (neither ARRAY nor OBJECT). But the change as shown doesn't, I think, do that.

One problem with changes to Type Id handling is that it is feature jackson-databind only uses, but nothing in jackson-core. So unit test suite does not cover it well (although there is src/test/java/com/fasterxml/jackson/core/write/WriteTypeIdTest.java).

Oh: which reminds me: we'd definitely need a test added to src/test/java/com/fasterxml/jackson/core/write/WriteTypeIdTest.java to verify expected behavior.

@Gems
Copy link
Contributor Author

Gems commented Nov 7, 2024

I have to say that I don't have sufficient visibility into the design idea of the writeTypePrefix and thus it's a bit more stubbing in the dark rather than fully conscious decision-making having full picture in mind.

Though at least it's thought-provoking, and this was my goal.

I support the idea to "consider null typeId to mean "no type wrapping" (neither ARRAY nor OBJECT)". And you are right, the provided change doesn't cover it fully.

I'll do the drill related to CLA and improve the change in this PR some time soon.

Thanks for your feedback as usual.

@Gems
Copy link
Contributor Author

Gems commented Nov 7, 2024

I did the changes and sent the signed agreement.

@cowtowncoder cowtowncoder added cla-received PR already covered by CLA (optional label) and removed cla-needed PR looks good (although may also require code review), but CLA needed from submitter labels Nov 8, 2024
@cowtowncoder
Copy link
Member

@Gems looks good -- did just a quick first pass, need to take some more time, but I think this should work. Got CLA so we are good with that too.

Will try local build to hopefully ensure that format modules (esp. YAML, XML) are ok with this change (should be but it's potentially brittle area)

@cowtowncoder cowtowncoder changed the title fix: Make JsonGenerator::writeTypePrefix method to not make a WRAPPER_ARRAY when typeIdDef.id == null Make JsonGenerator::writeTypePrefix method to not make a WRAPPER_ARRAY when typeIdDef.id == null Nov 8, 2024
@cowtowncoder
Copy link
Member

Ok, some of my logic changes proved wrong wrt short-circuiting handling: cause issue with YAML. Will revert.

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

LGTM -- made some stylistic changes, but essentially included as-is.

@cowtowncoder cowtowncoder merged commit 1e919b3 into FasterXML:2.19 Nov 9, 2024
8 checks passed
cowtowncoder pushed a commit to FasterXML/jackson-databind that referenced this pull request Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received PR already covered by CLA (optional label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants