FasterXML / jackson-core

Core part of Jackson that defines Streaming API as well as basic shared abstractions
Apache License 2.0
2.27k stars 799 forks source link

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

Closed Gems closed 3 weeks ago

Gems commented 3 weeks ago

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.

cowtowncoder commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

I did the changes and sent the signed agreement.

cowtowncoder commented 3 weeks ago

@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 commented 3 weeks ago

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