FasterXML / jackson-annotations

Core annotations (annotations that only depend on jackson-core) for Jackson data processor
https://github.com/FasterXML/jackson
Apache License 2.0
1.03k stars 330 forks source link

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

Closed cowtowncoder closed 1 year ago

cowtowncoder commented 1 year ago

(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)

JooHyukKim commented 1 year ago

@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 commented 1 year ago

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

cowtowncoder commented 1 year ago

Fixed via #228.

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

JooHyukKim commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/FasterXML/jackson-databind/pull/3891 into 2.16 - Then get 2.16 merged into master(3.0). - Only then backport Value class into 2.16

Now tracked in https://github.com/FasterXML/jackson-databind/issues/3943