Skip to content

possibility to not require JsonTypeInfo annotation for sealed classes? #3596

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

Open
pjfanning opened this issue Sep 8, 2022 · 15 comments
Open
Labels
3.x Issues to be only tackled for Jackson 3.x, not 2.x

Comments

@pjfanning
Copy link
Member

Scala, Kotlin and Java 17 have sealed classes. It is possible to write introspectors that find the subtypes.
It would be great if jackson-databind could infer a default JsonTypeInfo setting for sealed traits/interfaces/abstract-classes

A default like this, perhaps:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
@pjfanning pjfanning added the to-evaluate Issue that has been received but not yet evaluated label Sep 8, 2022
@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 9, 2022

Yes, I agree. See #3549 for related work.

I do think this probably needs to be somehow opt-in since there are probably some security concerns (I can't think of specifics but this tingles my spider sense wrt past experiences).
Just a simple MapperFeature or something.

@pjfanning
Copy link
Member Author

@cowtowncoder a MapperFeature to enable a default JsonTypeInfo for sealed classes would be fine. Once enabled, any class that has subtypes would automatically support be deserialized as if it has the default JsonTypeInfo annotation on it.

@cowtowncoder
Copy link
Member

Right. Now that I think about this, however, I realize there is the big question of what @JsonTypeInfo defaults are. Or rather how to allow configuration -- a default like shown above is fine but there probably should be a way to customize it. Unfortunately we do not have JsonTypeInfo.Value type (unlike for some of annotations), although I guess one could be added.
Specifying a class/interface with annotation could work I suppose.

Another thing to add to AnnotationIntrospector -- or, perhaps, this would be the main thing that exposes everything? -- would be a method to call to ask if given type is a "sealed type".
This is where we have some options; one would be "yes/no/don't know" result, after which introspector (or something) would need to figure out @JsonTypeInfo parameterization.
Another that it's this method that would actually return the parameterization.

Yet another is of course to instead bolt this in via existing handling of @JsonTypeInfo... but it seems cleaner to separate this out.

Regardless, modules (Kotlin, Scala, Java-17?) would provide their AnnotationIntrospectors to detect sealed types they know of and handle.

Does this sound good?

/cc @sigpwned apologies for your sealed class PR falling through the cracks -- but I think it might be possible to join forces here. The concept of sealed class is indeed a common shared concern.

@cowtowncoder
Copy link
Member

I think #5025 implemented this?

@pjfanning
Copy link
Member Author

I failed to find any tests in #5025 that do not require the JsonTypeInfo annotation. The point of this issue is to allow users to omit the JsonTypeInfo annotation altogether. That is support polymorphism where no annotations are used.
With sealed classes, we can infer that users want JsonTypeInfo support even if it is not declared as an annotation. We can also infer the JsonSubTypes of the sealed class using Java reflection.

@cowtowncoder
Copy link
Member

Ahhh. Ok yes. Teaches me not to actually read the discussion before commenting.
So #5025 could help slightly but is not the main thing here.

So my earlier comment still stands: I would be against automatically assuming @JsonTypeInfo for sealed classes as default setting, but opt-in feature sounds like a good idea.
And wrt defaulting, probably needs to be something like JsonTypeInfo.value as opposed to simple MapperFeature.

@sigpwned
Copy link

sigpwned commented Mar 31, 2025

For my part, that seems smart, @cowtowncoder. There are way too many variables involved in serializing sealed types without a guiding @JsonTypeInfo annotation. For example:

  • Should the type name be included as a parameter? If so, what name? Whatever the default name is, what happens if the record defines a parameter with the same name? If the type could simply be inferred from context, would that be preferable?
  • What value should be used? The simple name seems like a reasonable default for the value, since sealed classes have to exist in the same package and therefore must have unique names, but I haven't thought about things like nested types, which may allow duplicate names.

Baking in #5025 dramatically reduces the amount of annotation required to serialize a sealed type, so it's at least much less painful, even if it doesn't eliminate the boilerplate entirely. Only the top-level @JsonTypeInfo is required, as opposed to declaring all the subtypes.

Regarding eliminating the annotation entirely, I suspect it would be possible to create a module that provides default behavior for sealed classes. But I think that's where that kind of functionality belongs -- in a module -- at least for now. Maybe it'll make sense to upstream it one day.

Just my two cents, for whatever that's worth!

And sorry I missed your mention earlier in the thread! At least we got there in the end. :)

@cowtowncoder
Copy link
Member

Agree on those points (name clashes etc). And while I am sympathetic to aim of reducing annotations, I also think defaults should have minimal chance of adding hard-to-find problems; and enabling of polymorphic handling where one is not requested seems like one.

@pjfanning
Copy link
Member Author

Let's start with something that is not enabled by default. But in my head, if someone has a sealed class or interface or abstract class as the type for a field, then I think they expect polymorphism support even if you may need to supply info at mapper config level as to how to include typing info in the serialised data and how to deserialize that info.

@cowtowncoder
Copy link
Member

While I can see polymorphic handling as commonly expected, I think there will also be usage for serialization-only cases where PM is not desired.

But the configurability aspect is big too: maybe as-property style is dominant, but how about chosen type id property -- what would be the default?

@pjfanning
Copy link
Member Author

While I can see polymorphic handling as commonly expected, I think there will also be usage for serialization-only cases where PM is not desired.

But the configurability aspect is big too: maybe as-property style is dominant, but how about chosen type id property -- what would be the default?

@cowtowncoder
Copy link
Member

@pjfanning That sounds like a plan -- since AnnotationIntrospector allows overriding finding @JsonTypeInfo (or equivalent):

    @Override
    public JsonTypeInfo.Value findPolymorphicTypeInfo(MapperConfig<?> config, Annotated ann);

it should not be too hard to implement defaulting.

@cowtowncoder
Copy link
Member

One other thought: to allow per-class @JsonTypeInfo override (change/removal/add) for base types, we could quite easily add ability in ConfigOverride -- it already allows overrides for:

  • @JsonFormat
  • @JsonInclude
  • @JsonIgnoreProperties
  • @JsonSetter (null mapping)
  • @JsonAutoDetect
  • is-ignored-type (@JsonIgnoreType)
  • is-mergeable (@JsonMerge)

this would not allow completely config-free set up, but would avoid use of annotations.

@cowtowncoder cowtowncoder added 3.x Issues to be only tackled for Jackson 3.x, not 2.x and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 1, 2025
@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 6, 2025

Wrote #5070 failing-test so we have something to start with. We can also add config-override code along the way.

@cowtowncoder Do you recall we had some work done for config-overrride for @JsonTypeInfo?
Also I vaguely remember I was working on it wayyyy back.
I stopped for some reason, but think we can pick that up again?

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 6, 2025

@JooHyukKim We can, although I am not sure this is amongst higher priority things for 3.0. But it is something that could be done.

Note tho that my reference of config-overrides does not directly address doing #3596 itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues to be only tackled for Jackson 3.x, not 2.x
Projects
None yet
Development

No branches or pull requests

4 participants