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 DefaultTyping.NON_FINAL_AND_ENUMS to allow Default Typing for Enums #4159

Merged
merged 6 commits into from Oct 18, 2023

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Oct 15, 2023

As title says, allow default type handler for Enum's.
Fixes #3569

@JooHyukKim JooHyukKim changed the title Allow default type handler use for Enums Allow default type handler for Enums Oct 15, 2023
@JooHyukKim
Copy link
Member Author

This PR is against '2.15' branch at the moment, but with possible rebasing to '2.16' branch in mind.

I am leaning toward merging into '2.16', since version '2.15' is sort of mature already 🤔.. WDYT @cowtowncoder ?

@cowtowncoder
Copy link
Member

This PR is against '2.15' branch at the moment, but with possible rebasing to '2.16' branch in mind.

I am leaning toward merging into '2.16', since version '2.15' is sort of mature already 🤔.. WDYT @cowtowncoder ?

If this was to be applied, definitely 2.16: seems quite risky change to existing behavior.
(also not sure this makes sense in general but that aside)

@JooHyukKim JooHyukKim changed the base branch from 2.15 to 2.16 October 16, 2023 23:11
@JooHyukKim JooHyukKim marked this pull request as draft October 16, 2023 23:25
@JooHyukKim
Copy link
Member Author

If this was to be applied, definitely 2.16: seems quite risky change to existing behavior.

Rebased to 2.16, thanks! 🙏🏼

@JooHyukKim
Copy link
Member Author

JooHyukKim commented Oct 16, 2023

My apologies, after re-reading #4159 (comment), I realized that if I called activateDefaultTyping() configuration with DefaultTyping.EVERYTHING, the test passes, as per #3569 (comment).

@JooHyukKim JooHyukKim closed this Oct 16, 2023
@cowtowncoder
Copy link
Member

@JooHyukKim Btw, if for some reason we do want to enable inclusion of polymorphic type for Enums, we could and should simply add a new DefaultTyping value -- not modify NON_FINAL but add something like NON_FINAL_AND_ENUMS.
I'd have no problem with that.

@JooHyukKim
Copy link
Member Author

@JooHyukKim Btw, if for some reason we do want to enable inclusion of polymorphic type for Enums, we could and should simply add a new DefaultTyping value -- not modify NON_FINAL but add something like NON_FINAL_AND_ENUMS. I'd have no problem with that.

Oh, I thought adding another DefaultTyping would be too much :))) NON_FINAL_AND_ENUMS sounds perfect 👍🏼 Will work on it later today and let's see how it looks. Thank you again! @cowtowncoder

@cowtowncoder
Copy link
Member

@JooHyukKim Btw, if for some reason we do want to enable inclusion of polymorphic type for Enums, we could and should simply add a new DefaultTyping value -- not modify NON_FINAL but add something like NON_FINAL_AND_ENUMS. I'd have no problem with that.

Oh, I thought adding another DefaultTyping would be too much :))) NON_FINAL_AND_ENUMS sounds perfect 👍🏼 Will work on it later today and let's see how it looks. Thank you again! @cowtowncoder

Yeah, realistically changing behavior of an existing choice is bigger change than adding a new one (unless I am missing something). We don't want dozens of DefaultTyping options, and users can quite easily create their own alternatives.
But if there is need to replace some use cases of DefaultTyping.EVERYTHING it's worth it.

@JooHyukKim JooHyukKim reopened this Oct 17, 2023
@JooHyukKim JooHyukKim changed the title Allow default type handler for Enums Add new DefaultTyping.NON_FINAL_AND_ENUMS to allow default type handler for Enums Oct 17, 2023
@JooHyukKim JooHyukKim marked this pull request as ready for review October 17, 2023 15:23
Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will try to get merged ASAP.

@cowtowncoder cowtowncoder merged commit 9065be2 into FasterXML:2.16 Oct 18, 2023
3 checks passed
@cowtowncoder cowtowncoder changed the title Add new DefaultTyping.NON_FINAL_AND_ENUMS to allow default type handler for Enums Add new DefaultTyping.NON_FINAL_AND_ENUMS to allow Default Typing Enums Oct 18, 2023
@cowtowncoder cowtowncoder changed the title Add new DefaultTyping.NON_FINAL_AND_ENUMS to allow Default Typing Enums Add new DefaultTyping.NON_FINAL_AND_ENUMS to allow Default Typing for Enums Oct 18, 2023
cowtowncoder added a commit that referenced this pull request Oct 18, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Oct 18, 2023
@cowtowncoder
Copy link
Member

Also realized that the only reason to need NON_FINAL_AND_ENUMS is this complicated combination of generic types as root values, with polymorphic typing.

In general, use of generic (parameterized) types as root values as an Anti-Pattern for Jackson and should be avoided; particularly so if polymorphic type handling is needed.
Proper way to handle this is to remove generic type from root value, either by:

  1. Sub-classing generic type to make it concrete, OR
  2. Using a non-generic wrapper -- parametric types are perfectly fine anywhere below root value.

So instead of trying read and write MyWildBaseType<T> itself, wrap it in something like:

public class MyWrapper {
   @JsonTypeInfo // no longer any need for DefaultTyping!
   public MyWildBaseType<T> value;
}

// or even
public class MyPlainWrapper {
   @JsonTypeInfo // no longer any need for DefaultTyping!
   public Object value;
}

and things work without special handling -- including NO DEFAULT TYPING needed! -- when we avoid generics and polymorphism for root value, and get reliable base type from Field/Method declaration

@JooHyukKim JooHyukKim deleted the 3569-fix branch October 22, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants