-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support for sealed classes in Java #61
Comments
This sounds similar to what Kotlin does; would make sense I think. One possible concern is that of whether users sometimes might not want to automatically enable polymorphic handling... and come to think of that, it is not clear which inclusion mechanism should be used. |
Correct. Sealed classes are in preview right now, but soon will be released as a core feature. I suggest looking into this in preparation for their release. |
@cowtowncoder any updates on this? |
@AbstractCoderX no |
I don't think it would be a backwards-incompatible change to have |
I'm considering taking a stab at this. @cowtowncoder, is there any place in particular you'd like to see in a PR? On face, this feels like a FasterXML/jackson-databind concern, but obviously records aren't available until Java 15 (at the earliest), so that probably isn't feasible. I see a project FasterXML/jackson-modules-java8. Would it make sense to create an implementation in a new public repository (of my own) titled "jackson-modules-java17" (that targets Java 17) modeled after jackson-modules-8, and share it here? And then if it passes muster, you could fork it/create a new repo and I would PR into it/whatever? Or is that a bad approach? Happy to tackle this in whatever way you think is best! |
My 2 cents would be to try your own standalone module and Jackson can possibly take over the module later. |
Good suggestion, @pjfanning. I think that's a much more sane approach. I'll report any progress back here. |
OK, I've got a module for simplified serialization of Java 17 sealed classes that seems to work pretty well up at sigpwned/jackson-modules-java-17-sealed-classes. (Thanks again, @pjfanning, this was definitely the right approach to take.) The module is based on some reading from this issue in the Kotlin module. The build includes a (reasonably) good battery of serialization and deserialization tests that ensure the new feature works and the existing functionality hasn't changed. New FeaturesThis module allows the following (simpler) code to work for serializing sealed classes:
In particular, I'm pleased that there is no repetition of type names or any such thing. The implementation works without the ImplementationFor convenience, the (abridged, for clarity) crux of the implementation is here:
This implements a new
Possible Next StepsAll feedback welcome. If this seems like a worthy addition to the library proper, then I'd be happy to submit a PR, preferably with a little guidance on how best to package it. Thanks again all for the help! |
Very cool. I think this could potentially even be included in core databind, but that would require doing something similar to detection of Aside from this, one thing that we would need would be one more I would be happy to help get things merged if you @sigpwned wanted to work on something like this. Alternatively there might be a way to add a new JDK 17 module somehow. I guess we'd want new "jackson-modules-java17" or something; perhaps there might be other JDK 17 functionality to support (I am not fully up to what all is included, aside from it being first LTS with records). |
@cowtowncoder awesome! If I'm tracking, then the record support in core databind is in |
I don't want to nix any dev work here but with recent JDKs, reflection is frowned on. I would have preference for a separate module that does not use reflection - leaving jackson-databind as is. But I'll accept the consensus on this. |
Good point @pjfanning. I am bit torn here: you are right, reflection is kind of frowned upon. Now... one other thing to consider: I haven't yet brought this up on mailing lists, but I am thinking that maybe Jackson 3.x ( |
For my part, I'm happy to tackle it any which way you guys decide! |
Thank you @sigpwned! I think going with Record-like handling makes sense, encapsulating functionality. Use of Reflection is what it is but I think this would not make problem any worse. And using a |
@cowtowncoder ok, sounds good. I'll start on that. If we change our minds later, that's fine. More options is always better! Update soon. |
@cowtowncoder just on the supported Java version issue. Java 11 is still fully supported by the vendors. Java 8 is still supported for security fixes. |
OK, I think I have this working! For convenience, I opened a PR against FasterXML/jackson-databind to make it easy to see the changes. Independent of my code changes, there were a couple of problems in the existing code that broke the build on JDK17. To that end, I made the following changes to get the build working:
It appears that JDK17 has a change that breaks (at least some cases of) record deserialization, ostensibly related to disallowing the use of Hopefully that makes sense! All questions, comments, and feedbacks welcome. Also, I'm happy to continue the conversation here, or on that PR thread, as you all prefer. |
@sigpwned we can't really turn off those tests. They are there to test the functionality in older JDKs too. I think the fact that it appears that record deserialization is broken in some cases with Java 17 may be an indication that we should create a Java 17 module. Would you be able to see if you can fix the broken tests? If they were fixed, then the issue with how to proceed is resolved but while they remain broken, there is a general issue with whether adding your code to jackson-databind is the right approach. |
@cowtowncoder based on https://stackoverflow.com/questions/71040780/java-record-tests-modify-fields and other reading, it just looks like Record deserialization support is broken and can't be fixed using Java reflection. FasterXML/jackson-databind#3102 seems to describe some workarounds. |
Sigh. Yes, I have been wondering about this -- Jackson with Java 17 does seem problematic. Which is not great, considering that is the LTS with first Record support. At practical level, we should not necessarily need
I guess one bigger question is whether Also, given these complexities I guess it does make more sense to go with |
I've reconsidered - this PR is probably safe to add to jackson-databind. I thought the Record issues were more global but they actually seem very specific to use cases like naming strategies. The solution would probably involve treating Records as a special case and trying to have the deserialization code convert the input field names to the ones that match the record constructor and then treating the deserialization like it is a vanilla record (vanilla records without annotations work) - anything to avoid using the java.lang.reflect.Field#set which fails for records. |
Okey dokey, I have both styles of implementation ready at the following locations. I'm going to attempt to summarize our conversation around this feature to date. But keep me honest!
I'm happy to jump any which way you guys like! |
OK, the last CR on FasterXML/jackson-databind#3549 should now be clean. How do you think we should proceed here, @pjfanning @cowtowncoder? I think we have all options discussed ready for review, per #61 (comment). |
Just checking in, @pjfanning @cowtowncoder. Any thoughts on this? There is an implementation ready for a Java 17 module https://github.com/sigpwned/jackson-modules-java-17-sealed-classes, and in databind core FasterXML/jackson-databind#3549. |
@sigpwned I tried your module. It doesn't really work. It includes abstract classes and interfaces, and doesn't sort. I tried rewriting it. It's not possible to simply filter abstract classes, or else their children get ignored. To filter abstract classes, I had to enumerate the entire tree each time, which turned into an N^2 operation. I couldn't get sorting to work at all. I think this might need to be implemented at the level of Jackson, or the |
@almson can you share a little more about your setup? In particular, what version of Jackson were you using? I'll be the first to admit that I haven't looked at this code in a while, but it did work at the time of the above writing, to the best of my knowledge. |
@sigpwned 2.13.0 I should update it. Do you think it makes a difference? |
@sigpwned Did you test it with deeper class hierarchies that include abstract types? |
Hello, |
Jackson has the option now to use deduction (JsonTypeInfo.Id.DEDUCTION) to see which class in a list of classes is the best fit. If you combine that with the list of permitted subclasses (Class#getPermittedSubclasses) you can I think do this yourself. I've had some success with this Module: /**
* A module for Jackson which allows it to determine the subtype during deserialization
* based on a sealed class hierarchy.
*/
public class SealedClassesModule extends SimpleModule {
@Override
public void setupModule(SetupContext context) {
context.appendAnnotationIntrospector(new SealedClassesAnnotationIntrospector());
}
private static class SealedClassesAnnotationIntrospector extends JacksonAnnotationIntrospector {
@Override
public List<NamedType> findSubtypes(Annotated annotated) {
if(annotated.getAnnotated() instanceof Class<?> cls && cls.isSealed()) {
Class<?>[] permittedSubclasses = cls.getPermittedSubclasses();
if(permittedSubclasses.length > 0) {
return Arrays.stream(permittedSubclasses).map(NamedType::new).toList();
}
}
return null;
}
@Override
protected TypeResolverBuilder<?> _findTypeResolver(MapperConfig<?> config, Annotated annotated, JavaType baseType) {
if(annotated.getAnnotated() instanceof Class<?> cls && cls.isSealed()) {
return super._findTypeResolver(config, AnnotatedClassResolver.resolveWithoutSuperTypes(config, Dummy.class), baseType);
}
return super._findTypeResolver(config, annotated, baseType);
}
@JsonTypeInfo(use = JsonTypeInfo.Id.DEDUCTION)
private static class Dummy {
}
}
} |
So does that mean when this issue will be resolved, JsonTypeInfo.Id.DEDUCTION will automatically be able to find the best fit without needing to provide the permitted classes in a custom module ? |
No. DEDUCTION is a heuristic option that can try to determine likely subtype on presence (but not absence) of specific properties by subtypes. It is not a replacement for supporting sealed classes. Jackson has no way of asking deserializers whether they might be able deserialize specific value: deserializers are looked up by type so that a deserializer that claims to support type X MUST be able to deserialize any and all representations. One challenge is that all deserialization is based on incremental/streaming input so deserializers do not have access to full value representation (like all propeties), without explicit reading all content (which for structured types also means all children values). So once deserializer starts deserializing value, it absolutely must complete the process; it has no way of saying "oops, cannot do it, someone else try". |
Sealed classes are coming to Java soon. Jackson can use them to remove the need to specify
@JsonSubTypes
because they are known from the sealing class:can be
The text was updated successfully, but these errors were encountered: