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

Support for sealed classes in Java #61

Open
nlisker opened this issue Jul 11, 2021 · 33 comments
Open

Support for sealed classes in Java #61

nlisker opened this issue Jul 11, 2021 · 33 comments

Comments

@nlisker
Copy link

nlisker commented Jul 11, 2021

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:

@JsonSubTypes({
	@JsonSubTypes.Type(value = A.class),
	@JsonSubTypes.Type(value = B.class)
})
public interface L {

	record A(int a) implements L {}

	record B(int b) implements L {}
}

can be

public sealed interface L {

	record A(int a) implements L {}

	record B(int b) implements L {}
}
@cowtowncoder
Copy link
Member

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.
So I suppose in above examples one should still use @JsonTypeInfo to opt-in and specify mechanism (type id to use (Class name vs Type name); inclusion mechanism (property, as-wrapper-array, as-wrapper-object)).
But it is true that all subtypes would be easily discoverable without explicit @JsonSubTypes annotation.

@nlisker
Copy link
Author

nlisker commented Jul 11, 2021

So I suppose in above examples one should still use @JsonTypeInfo

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.

@slutmaker
Copy link

@cowtowncoder any updates on this?

@cowtowncoder
Copy link
Member

@AbstractCoderX no

@almson
Copy link

almson commented Dec 16, 2021

I don't think it would be a backwards-incompatible change to have @JsonSubTypes take priority, but for sealed classes that have @JsonTypeInfo but not @JsonSubTypes for the sub types to be inferred.

@sigpwned
Copy link

sigpwned commented Jul 24, 2022

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!

@pjfanning
Copy link
Member

My 2 cents would be to try your own standalone module and Jackson can possibly take over the module later.

@sigpwned
Copy link

Good suggestion, @pjfanning. I think that's a much more sane approach. I'll report any progress back here.

@sigpwned
Copy link

sigpwned commented Jul 24, 2022

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 Features

This module allows the following (simpler) code to work for serializing sealed classes:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
public sealed class SealedExample permits AlphaSealedExample, BravoSealedExample {
}

@JsonTypeName("alpha")
public final class AlphaSealedExample extends SealedExample {
}

@JsonTypeName("bravo")
public final class BravoSealedExample extends SealedExample {
}

In particular, I'm pleased that there is no repetition of type names or any such thing. The implementation works without the @JsonTypeName annotations, too, but falls back on (potentially awkward) default names.

Implementation

For convenience, the (abridged, for clarity) crux of the implementation is here:

public class Jdk17SealedClassesAnnotationIntrospector extends JacksonAnnotationIntrospector {
  @Override
  public List<NamedType> findSubtypes(Annotated a) {
    if (a.getAnnotated() instanceof Class<?> klass && klass.isSealed()) {
      Class<?>[] permittedSubclasses = klass.getPermittedSubclasses();
      if (permittedSubclasses.length > 0) {
        return Arrays.stream(permittedSubclasses).map(NamedType::new).toList();
      }
    }
    return null;
  }
}

This implements a new AnnotationInspector for discovering subtypes that is appended as a new "last resort" option by the module. If I were trying to integrate the new code into JacksonAnnotationIntrospector, though, it would probably look like this earlier draft:

public class Jdk17AnnotationIntrospector extends JacksonAnnotationIntrospector {
  @Override
  public List<NamedType> findSubtypes(Annotated a) {
    // This is the "existing" subtype implementation, which uses the @JsonSubTypes annotation to
    // allow for polymorphic de/serialization. We
    List<NamedType> result = findSubtypesByAnnotation(a);
    if (result == null)
      result = findSubtypesByPermittedSubtypes(a);
    return result;
  }

  protected List<NamedType> findSubtypesByAnnotation(Annotated a) {
    return super.findSubtypes(a);
  }

  protected List<NamedType> findSubtypesByPermittedSubtypes(Annotated a) {
    if (a.getAnnotated() instanceof Class<?> klass && klass.isSealed()) {
      Class<?>[] permittedSubclasses = klass.getPermittedSubclasses();
      if (permittedSubclasses.length > 0) {
        return Arrays.stream(permittedSubclasses).map(NamedType::new).toList();
      }
    }
    return null;
  }
}

Possible Next Steps

All 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!

@cowtowncoder
Copy link
Member

Very cool.

I think this could potentially even be included in core databind, but that would require doing something similar to detection of record types: use basic reflection to find methods dynamically so that things still work on JDK 8 (but obviously sealed classes wont' be there).

Aside from this, one thing that we would need would be one more MapperFeature to allow disabling functionality -- just in case someone somewhere did not want this auto-detection.

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).

@sigpwned
Copy link

@cowtowncoder awesome!

If I'm tracking, then the record support in core databind is in JDK14Util. I grok that code, and I can absolutely take a swing at implementing it for the sealed classes feature. I'll update with progress here.

@pjfanning
Copy link
Member

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.

@cowtowncoder
Copy link
Member

Good point @pjfanning. I am bit torn here: you are right, reflection is kind of frowned upon.
Then again we already do use it for Records so in a way damage is done, and I don't see 2.x moving to Java 17 baseline any time soon. So functionality needs to be maintained for quite a while.

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 (master branch) should require JDK 17 as the baseline. Initially this was to get Record types in cleanly.
If this was done we could then support this aspect natively in 3.x and via Reflection in 2.x.

@sigpwned
Copy link

For my part, I'm happy to tackle it any which way you guys decide!

@cowtowncoder
Copy link
Member

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 MapperFeature to gate the functionality should avoid possible issues in cases where some runtime would have problems (as long as feature is checked in before attempting to load the functionality, which I think should be doable).

@sigpwned
Copy link

@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.

@pjfanning
Copy link
Member

@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.

See https://endoflife.date/java

@sigpwned
Copy link

sigpwned commented Jul 27, 2022

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:

  • add module opens to pom.xml
  • The following tests are disabled because Java17 breaks the use of setAccessible to update (implicitly immutable) record field values
    • RecordWithJsonNaming3102Test
    • RecordWithJsonSetter2974Test
    • RecordBasicsTest#testNamingStrategy
    • RecordUpdate3079Test#testDirectRecordUpdate

It appears that JDK17 has a change that breaks (at least some cases of) record deserialization, ostensibly related to disallowing the use of setAccessible to change a record instance's (implicitly final) field values. It's not clear to me if and how this can be fixed. I'm happy to handle these broken tests another way in that PR (e.g., including moving them to a separate upstream PR and rebasing that PR), but the PR at least demonstrates that the sealed classes work and tests pass on JDK17.

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.

@pjfanning
Copy link
Member

@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.

@pjfanning
Copy link
Member

pjfanning commented Jul 27, 2022

@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.

@cowtowncoder
Copy link
Member

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 setAccessible() but the question is that of how to disable it in specific places. I haven't had time to look into this, and have been hoping to rather find time for the full overhaul of POJO introspection that would also consider Records as a distinct "POJO-like" type.
But my thinking at tactical level has been that:

  1. With Records, basic introspection should ignore Fields and setters altogether (wrt general discovery) and just proceed with Record-specific introspection of Constructor, "getters" (with no get-prefix)
  2. Not try to call setAccessible() on getters, Constructor for Records

I guess one bigger question is whether setAccessible() should ever be called on JDK 17+. If not maybe it'd be easier to try to detect JDK version -- not something I'd like to do, since this is fragile by definition, but maybe necessary.

Also, given these complexities I guess it does make more sense to go with Module route with 2.x.
This would simplify implementation and users would then explicitly register it when they know it may be used (as in, running on JDK 17+), or handle optional loading on their side.

@pjfanning
Copy link
Member

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.

@sigpwned
Copy link

sigpwned commented Jul 27, 2022

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!

  • Jackson 17 Module -- Available at sigpwned/jackson-modules-java-17-sealed-classes. I see a few options:
    • It is currently structured as a single catch-all JDK17 module.
    • It could easily be converted to a module-per-feature instead.
    • It could also use a hybrid approach with one module per feature, and then one "super module" that depends on all the feature modules and adds everything for users who just want it all.
    • I can continue to maintain this feature as a standalone module -- it's already in maven central -- or it can be integrated as a first-class Jackson module. For my part, I would prefer the latter just because I think it will be a better experience for the community, but will defer to the group.
  • Jackson Databind -- Available at FasterXML/jackson-databind/pull/3549. I see a few options here, too:
    • We want to stay on LTS builds, so using JDK15/16 to solve the problem is off the table. (@cowtowncoder this sounds like the voice of experience talking, so I think that's good guidance!)
    • We can use a separate PR to address version 2.x JDK17 record issues. (The JDK17 build succeeds (with minor changes) once about four record tests are disabled, at least in JDK17.) Then, I can rebase this PR and merge into 2.x.
    • We can ignore the 2.x branch, preferring a module instead, and merge into the 3.x branch. I can make whatever adjustments are needed.

I'm happy to jump any which way you guys like!

@sigpwned
Copy link

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).

@sigpwned
Copy link

sigpwned commented Aug 2, 2022

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.

@almson
Copy link

almson commented Jun 5, 2023

@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 JacksonAnnotationIntrospector::findSubtypes semantics need to change.

@sigpwned
Copy link

sigpwned commented Jun 5, 2023

@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.

@almson
Copy link

almson commented Jun 5, 2023

@sigpwned 2.13.0 I should update it. Do you think it makes a difference?

@almson
Copy link

almson commented Jun 5, 2023

@sigpwned Did you test it with deeper class hierarchies that include abstract types?

@Alathreon
Copy link

Hello,
may I ask if it is possible to make so jackson tries to serialize automatically from the list of implementations, and if one doesn't work, it tries the next ?
Because I found myself often using sealed interfaces with records, they do not have any logic but only data, and so itsn't a stretch to think that in most cases, different implementations won't conflict.
Note that I am asking this, because each time, I basically have to modify the json structure just so jackson can understand it, and not the reverse...

@hjohn
Copy link

hjohn commented Jul 14, 2023

Hello, may I ask if it is possible to make so jackson tries to serialize automatically from the list of implementations, and if one doesn't work, it tries the next ? Because I found myself often using sealed interfaces with records, they do not have any logic but only data, and so itsn't a stretch to think that in most cases, different implementations won't conflict. Note that I am asking this, because each time, I basically have to modify the json structure just so jackson can understand it, and not the reverse...

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 {
    }
  }
}

@Alathreon
Copy link

Alathreon commented Jul 15, 2023

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:

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 ?

@cowtowncoder
Copy link
Member

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:

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.
@hjohn suggested this since it might work for your use case, being closest relevant feature (I think).

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.
Jackson is unlikely to ever support set of multiple potential deserializers per type: there is no support for such setup nor do I think it would be worth the complexity.

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".

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

No branches or pull requests

8 participants