FasterXML / jackson-future-ideas

Repository for SOLE PURPOSE of issue tracker and Wiki for NEW IDEAS. Please: NO BUG REPORTS.
18 stars 6 forks source link

Support for sealed classes in Java #61

Open nlisker opened 3 years ago

nlisker commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@cowtowncoder any updates on this?

cowtowncoder commented 3 years ago

@AbstractCoderX no

almson commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

sigpwned commented 2 years ago

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

sigpwned commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

cowtowncoder commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 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 commented 2 years ago

@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 commented 2 years ago

@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. https://github.com/FasterXML/jackson-databind/issues/3102 seems to describe some workarounds.

cowtowncoder commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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!

sigpwned commented 2 years ago

OK, the last CR on https://github.com/FasterXML/jackson-databind/pull/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 https://github.com/FasterXML/jackson-future-ideas/issues/61#issuecomment-1197241383.

sigpwned commented 2 years ago

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 https://github.com/FasterXML/jackson-databind/pull/3549.

almson commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

almson commented 1 year ago

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

Alathreon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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