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 new OptBoolean valued property in @JsonTypeInfo, handling, to allow per-polymorphic type loose Type Id handling #3877

Closed
cowtowncoder opened this issue Apr 12, 2023 · 13 comments · Fixed by #3891
Labels
2.16 Issues planned for 2.16
Milestone

Comments

@cowtowncoder
Copy link
Member

(note: offshoot of #3853)

It seems reasonable and useful to allow annotation(s) to specify loose/strict handling of Type Ids for polymorphic types.
The simplest location to do that would seem to be @JsonTypeInfo; this would only allow enabling/disabling for all (or none) subtypes but alternatives are probably more difficult to implement.

@cowtowncoder cowtowncoder added to-evaluate Issue that has been received but not yet evaluated 2.16 Issues planned for 2.16 and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 12, 2023
@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 14, 2023

Here are some ideas I came up with. Let me know what you think! --personally my choice is solution#3.

Summary (overall intuition)

More specific configuration always overrides.

  • DeserializationFeature. REQUIRE_TYPE_ID_FOR_SUBTYPES = false is overridden
    by @JsonSubType.Type(looseHandling=true).
  • @JsonSubType.Type(looseHandling=true) is overridden by new annotation @JsonTypeHandling(looseHandling=true).

Solution 1: New @JsonTypeHandling annotation

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "@type")
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "dog"),
    @JsonSubTypes.Type(value = Cat.class, name = "cat"),
})
public abstract class Animal {}

@JsonTypeHandling(looseHandling = true) // <---- NEW
public class Cat extends Animal {} 

Pros:

  • More fine-grained control over loose type id handling.

Cons:

  • Requires an additional annotation on the subtype, which can be cumbersome in cases where many subtypes need to be
    configure
  • Hassle to keep track of Scattered annotations

Solution 2: Add new attribute to @JsonSubTypes.Type

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "@type")
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "dog"),
    @JsonSubTypes.Type(value = Cat.class, name = "cat", looseHandling = true), // <---- NEW
})
public abstract class Animal {} 

Pros:

  • Simple syntax that doesn't require an additional annotation.
  • Easy to use

Cons:

  • Cluttering the @JsonSubTypes.Type annotation with more attributes.
  • Cannot hide the attribute from the base class.

Solution 3: Combine both solutions

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "@type")
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "dog"),
    @JsonSubTypes.Type(value = Cat.class, name = "cat", looseHandling = true), // <---- NEW
})
public abstract class Animal {}

// ***overrides `looseHandling` attribute in @JsonSubTypes.Type
@JsonTypeHandling(looseHandling = true) // <---- NEW
public class Cow extends Animal {} 

Pros:

  • Offers both fine-grained control over loose handling with @JsonTypeHandling and a simpler syntax with
    @JsonSubTypes.Type.
  • Allows for more flexibility in configuring type id handling.

Cons:

  • More complex syntax that may be confusing to some users.

Solution 4: Add attribute to existing @JsonTypeInfo <-- original suggestion

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    requireTypeIdForSubtypes = OptBoolean.FALSE) // <---- NEW
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "dog")})
public abstract class Animal {}

public class Cow extends Animal {}

Pros:

  • Adds a new attribute to an existing annotation, providing a simple and easy-to-use syntax.
  • Does not require additional annotations or attributes to be added to subclasses.

Cons:

  • The attribute added to the existing @JsonTypeInfo annotation may not be as fine-grained as other solutions that introduce new annotations or attributes.

@cowtowncoder
Copy link
Member Author

Hmmh. I can see benefits of doing of doing both.

Although there is also the question on whether this is needed for individual subtypes, or just for base type? (that is, it always applies to all subtypes). My initial thinking was that it'd be for all subtypes of same base type, being simpler to implement and use.

I guess it goes back to question of what exactly is the use case....

@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 14, 2023

Thanks, @cowtowncoder 🙏🏽! I missed per base-type configuration using ‘@JsonTypeInfo` 🙈. Possible solutions are…

  • DeserializationFeature. REQUIRE_TYPE_ID_FOR_SUBTYPES = false is overridden by
    @JsonTypeInfo(looseHandling=true).
  • @JsonTypeInfo(looseHandling=true)is overriden by @JsonSubType.Type(looseHandling=true) .
  • @JsonSubType.Type(looseHandling=true) is overridden by new annotation @JsonTypeHandling(looseHandling=true).

You are right it becomes the question of how specific a user wants to get in the configuration. But being a heavy user of @JsonTypeInfo myself, I think supporting @JsonTypeInfo(allowLooseHandling=true) will cover most cases. It is already one level more specific than global configuration using DeserializationFeature.

@cowtowncoder
Copy link
Member Author

Much of this also depends on how exactly annotation access is handled. Overriding global default is not super difficult, but trying to combine information from different annotations is, due to annotation flattening (it is not possible to know specific level at which given annotation is specified, to compare which comes from base/super class etc).

@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 14, 2023

Overriding global default is not super difficult, but trying to combine information from different annotations is, due to annotation flattening.

Now adding attribute to @JsonTypeInfo seems more reasonable . I am leaning towards adding attribute to @JsonTypeInfo. What do u think?

@cowtowncoder
Copy link
Member Author

@JooHyukKim Yes that sounds reasonable.

@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 19, 2023

@cowtowncoder great! 🙇🏼‍♂️ Once the default branch becomes 2.16, I will try to make a PR. adding the new field to @JsonTypeInfo in jackson-annotations then work on it here

Just in case anyone wonders how to work with new attribute in jackson-annotations, build it locally.
FasterXML/jackson-annotations#202 (comment)

@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 20, 2023

I replaced all occurances of DeserializationFeature.FAIL_ON_INVALID_SUBTYPE with MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES in all comments previous to current one. Because this issue branches out from #3854

@cowtowncoder
Copy link
Member Author

@JooHyukKim Thank you for providing this!

I merged it from 2.16 to master, but looks like test is failing. I wonder if you could help figure out why -- my guess is that some of earlier merges I made might have missed something.

@JooHyukKim
Copy link
Member

I merged it from 2.16 to master, but looks like test is failing. I wonder if you could help figure out why -- my guess is that some of earlier merges I made might have missed something.

Thank you for letting me know 🙏🏼🙏🏼 No problem, will look into this today 👍🏻

@JooHyukKim
Copy link
Member

JooHyukKim commented Jun 7, 2023

Thank you for providing this!

Thank you for great guidance also 👍🏻👍🏻. Good to give back to the community.

It was kinda thrilling to see #3891 get merged, because some PRs and issues prior to this one seem to sort of connect 😄. Such great experience.

@JooHyukKim
Copy link
Member

I made PR #3970 to fix the issue. It also has explanation. Hope it all helps, @cowtowncoder ! 🙏🏼

@cowtowncoder
Copy link
Member Author

Thank you @JooHyukKim ! Will take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants