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

Add "require type id for subtypes" property for JsonTypeInfo.Value in 3.0 #226

Closed
cowtowncoder opened this issue May 16, 2023 · 8 comments
Closed
Labels
3.x Issues related to Jackson 3.x major version

Comments

@cowtowncoder
Copy link
Member

(follow-up to #223)

With #223, a new property was added in @JsonTypeInfo, via 2.16 branch. That works fine.

But 3.0 (master) has added Value type for JsonTypeInfo (something that we might even want to backport in 2.x since 3.0 is still not out); it will need to handle this new property as well.
I can do that later on if necessary, or if @JooHyukKim you have time maybe you could do it?
(addition of the new property: backporting in 2.16 is totally optional)

@cowtowncoder cowtowncoder added the 3.x Issues related to Jackson 3.x major version label May 16, 2023
@JooHyukKim
Copy link
Member

@cowtowncoder sure👍🏻 I do have time for it. If there is additional related (class/file/link) anything I can start looking into, would you let me know?

@cowtowncoder
Copy link
Member Author

cowtowncoder commented May 16, 2023

@JooHyukKim Perfect! Just look at JsonTypeInfo, there's inner class named Value. And then matching JsonTypeInfoTest. These in master branch.

@cowtowncoder
Copy link
Member Author

Fixed via #228.

I wonder if it'd be good to backport JsonTypeInfo.Value class into 2.16?

@JooHyukKim
Copy link
Member

JooHyukKim commented May 17, 2023

Is it worth backporting the JsonTypeInfo.Value class into 2.16?

I lean towards no, considering the stability of JsonTypeInfo and the lack of significant issues.

If the need arises in the future, we can always consider backporting Value class then. As far as I remember, the effort involved doesn't seem to be significantly different whether we do it now or later. How about you, @cowtowncoder ?

EDIT: Thinking back, maybe there is a big benefit of such backporting that I am missing here?

@cowtowncoder
Copy link
Member Author

Ideally we'd have Value classes for anything but simplest annotations, as it makes it easier to add things like overrides -- one cannot create instances of JsonTypeInfo, for example (for testing etc -- maybe Mock libraries can but user code not), but could easily build JsonTypeInfo.Value.
So it's more about convenient handling by AnnotationIntrospector and ability to maybe add overrides via "config override" system. It enables improvements but does not offer anything just on its own.

Does this make sense?

@JooHyukKim
Copy link
Member

JooHyukKim commented May 17, 2023

Does this make sense?

Thank you for your explanation, @cowtowncoder 🙏🏼. Big +1 for testability 🥹.

Yes, it does make sense now. Looking at 2.16 version of MutableConfigOverride and AnnotationIntrospector(tho I am sure there's couple more usages) from user's perspective, it is not awkward at all backport JsonTypeInfo.Value to 2.16.

At this point, what I assume we can start to do is...

  1. Merge Add new OptBoolean valued property in @JsonTypeInfo.. PR in databind
  2. Merged / Backporting JsonTypeInfo.Value to 2.16 in jackson-annotations (Made PR)
  3. Introduce JsonTypeInfo.Value to 2.16 in jackson-databind
    • Like in MutableConfigOverride, AnnotationIntrospector, etc...
  4. Merge jackson-databind/2.16 branch into jackson-databind/master

@cowtowncoder
Copy link
Member Author

Yes, so jackson-annotations one merged, good starting point. I agree that jackson-databind changes more challenging: can change bit by bit. I forget details of how I changed it in master (3.0) -- since there was no backwards-compatibility concerns it is not certain it can be backported as-is: 2.16 will need to be backwards-compatible.
But if possible, it is fine to split changes into multiple pieces too (AnnotationIntrospector usage, MutableConfigOverride).

@JooHyukKim
Copy link
Member

JooHyukKim commented May 19, 2023

it is fine to split changes into multiple pieces too (AnnotationIntrospector usage, MutableConfigOverride).

Sounds like a plan.👍🏻

Regards to the execution order, if possible,
- First merge FasterXML/jackson-databind#3891 into 2.16
- Then get 2.16 merged into master(3.0).
- Only then backport Value class into 2.16

Now tracked in FasterXML/jackson-databind#3943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues related to Jackson 3.x major version
Projects
None yet
Development

No branches or pull requests

2 participants