-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 support for automatic detection of subtypes (like @JsonSubTypes
) from Java 17 sealed types
#5025
Conversation
*/ | ||
public class TestSubtypesWithSealedTypes extends DatabindTestUtil | ||
{ | ||
@JsonTypeInfo(use=JsonTypeInfo.Id.NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to test with more use=JsonTypeInfo.Id.xxx
values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course! I'll get that in today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some tests, and have ensured that the proposed change has tests for the following values:
JsonTypeInfo.Id.CLASS
JsonTypeInfo.Id.MINIMAL_CLASS
JsonTypeInfo.Id.NAME
JsonTypeInfo.Id.SIMPLE_NAME
JsonTypeInfo.Id.DEDUCTION
JsonTypeInfo.As.PROPERTY
JsonTypeInfo.As.WRAPPER_OBJECT
JsonTypeInfo.As.WRAPPER_ARRAY
JsonTypeInfo.As.EXISTING_PROPERTY
The following values still do not have tests:
JsonTypeInfo.Id.NONE
-- Based on my read ofNoTypeInfoTest
, this seems to concern how to deserialize interfaces as concrete types on a one-to-one basis, as opposed to subtypes.JsonTypeInfo.Id.CUSTOM
-- Based on my read ofCustomTypeIdResolverTest
, this seems to allow users to manage subtype resolution on their own, as opposed ot using annotations/sealed types/other approaches.JsonTypeInfo.As.EXTERNAL_PROPERTY
-- I was not quickly able to find an existing tests for this value. If you think testing this value is important, then I will take a deeper look!
@sigpwned Quick question, aside from PR itself -- do we have a CLA from you? I thought we might have, but couldn't find one in storage. Before merging we need one -- if you sent one earlier LMK (or if you are under one of CCLAs?) -- if not, doc to fill is here: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and is usually just printed, filled & signed, scan/photo, email to |
src/main/java/tools/jackson/databind/introspect/JacksonAnnotationIntrospector.java
Show resolved
Hide resolved
src/test/java/tools/jackson/databind/jsontype/ExistingPropertyWithSealedTypesTest.java
Outdated
Show resolved
Hide resolved
src/test/java/tools/jackson/databind/jsontype/TestTypedSerializationWithSealedTypes.java
Outdated
Show resolved
Hide resolved
src/test/java/tools/jackson/databind/jsontype/TestTypedSerializationWithSealedTypes.java
Outdated
Show resolved
Hide resolved
src/test/java/tools/jackson/databind/jsontype/TestTypedSerializationWithSealedTypes.java
Outdated
Show resolved
Hide resolved
src/test/java/tools/jackson/databind/jsontype/TestTypedSerializationWithSealedTypes.java
Outdated
Show resolved
Hide resolved
So far looks good: implementation as simple as expected. Added a note on test class naming (newer pattern to end class name with "Test", not Start; using name prefix to group together). Hoping to go over PR with more thought later on but wanted to give some quick feedback despite time crunch on my end. |
All comments should be addressed by newest commits! Will send CLA over later this evening. |
@sigpwned If this is ready to move from draft please mark as Ready for Review. |
CLA submitted! |
Whops. Merged it accidentally already. Need to update release notes next then. |
@JsonSubTypes
) from Java 17 sealed types
This (draft) PR adds support for detecting subtypes for serialization automatically using Java 17 sealed types/permitted subclasses, per the discussion in jackson-future-ideas.
The implementation is pretty simple. (This is largely due to Jackson’s modular design, so kudos to the team!) Happily, it also turned out to be essentially a direct port from the module I wrote for this feature for Jackson 2.x versions, which increases my confidence that the implementation is at least heading in the right direction.
Hopefully the changes are pretty easy to grok, but I’ll add a little voiceover here, just in case the code I’ve written isn’t as clear as I hope it is.
Design
The feature is designed to interpret a sealed class declaration like this:
As equivalent to the following Jackson 2.14+ structure:
The
@JsonTypeInfo
and@JsonTypeName
annotations should be honored by the implementation as well, so this code should work like users expect, too:That is: (a) serialization should store type names in serialized objects as a property named
type
; and (b) theExampleChildA
andExampleChildB
classes should use names"a"
"b"
, respectively.I chose this design because removing redundant
@JsonSubTypes
declarations was what originally drove me to explore this feature as a module back in 2022. However, I'm very happy to revisit if desired!To @pjfanning’s point, this feature should provide a clearer, cleaner, simpler path towards polymorphism for closed type hierarchies. To @cowtowncoder’s point, I think this approach is also quite ergonomic. I particularly like how DRY it is!
Implementation
Since Jackson 3.x requires Java 17+, my strategy was to treat subtype detection from sealed types as a first-class citizen. Therefore, I added the feature directly into the
JacksonAnnotationIntrospector
class, as opposed to putting it into a module or something. TheAnnotationIntrospector#findSubtypes
hook made this very easy.Also, I used Java 17 syntax in places where I thought it made the code clearer. Obviously, this does not match the style of code around it, since the 3.x branch is the first one to allow Java 17. I will happily match style if preferred, just let me know.
Testing
For testing, my goal was to identify the “core” unit tests for subtype detection using the
@JsonSubTypes
annotation, and modify those tests for sealed class subtype detection. I elected to usetools.jackson.databind.jsontype.TestSubtypes
for this. I will happily add more tests as needed, just point me in the right direction.Open Questions
I have opened a draft PR specifically to address the following open questions:
JacksonAnnotationIntrospector#findSubtypes
and its uses, I think the implementation is complete, but obviously you guys are the experts!TestSubtyptes
tests that I adapted for this feature, the code seemed to register subtypes usingJsonMapper.Builder#registerSubtypes
, sometimes redundantly with@JsonSubTypes
annotations in many cases. I commented those (redundant) calls out because I wanted to ensure that the JacksonAnnotationIntrospector is registering the subtypes itself from the sealed types. The tests pass, but please review, and let me know if I missed the point.Thank you all for continuing to maintain this wonderful library! The ease of integration of this new feature shows how well-designed this library continues to be, even as it has grown and changed over the last 10+ years!