Add streamlined sealed class polymorphic de/serialization

[sigpwned]

Add streamlined support for sealed class polymorphic de/serialization.

Java 17 sealed classes are required to enumerate their allowed subclasses explicitly. This metadata should allow Jackson to infer the subtypes of a sealed class for polymorphic serialization automatically. Specifically, the following sealed class should handle polymorphic serialization as written:

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

Note the absence of @JsonSubTypes annotation and any (Jackson-specific) repetition of any class names in the code above.

This PR should work for JDK8+ through the clever use of reflection. (There was an excellent model to follow in JDK14Util!) Sealed classes are detected and handled automatically if run on Java17+.

The current build is broken on Java 17. I made the following changes to get the build working on JDK17.

• 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 this PR (e.g., including moving them to a separate upstream PR and rebasing this PR), but this at least demonstrates that the sealed classes work and tests pass on JDK17.

Per the issue FasterXML/jackson-future-ideas#61

[pjfanning]

#3102 seems to be the issue that is breaking the Record tests

[pjfanning]

only the first of these tests fails - can you just ignore the one that fails?

[sigpwned]

Agreed, and that was the first thing I tried. But the @Ignore only seems to have any effect if it's applied to the whole class. I admit that I don't understand why. I just tried it again and got the same result. Please check my math here because I agree that would be the better solution. I would be happy to be wrong!

[pjfanning]

So, jackson still uses junit3 as opposed to junit4 (or even junit5) - so that might explain why the Ignore annotation doesn't work at the test level - it works well in junit4 style (where you annotate methods with @Test).

Could you move the failing tests to the com.fasterxml.jackson.databind.failing package of test-jdk14? Just the failing tests, not the full test classes. So you can copy the full class to com.fasterxml.jackson.databind.failing but then remove the tests that work and go back to the original class and remove the broken test(s).

The tests in the 'failing' package should be ignored when you run the build with mvn from command line.

[pjfanning]

again - can you ignore just the individual tests that fail?

[pjfanning]

can you use @Ignore like the other tests you disabled?

[pjfanning]

can you move this single test (not full class) to failing package?

[pjfanning]

and uncommented when it is moved

[pjfanning]

Use @Ignore

[pjfanning]

can you move this single test to the failing package?

[sigpwned]

Thanks for the feedback, @pjfanning! That was an eagle-eyed code review.

Regarding the other issues, I think we agree much more than we disagree. I was excited to get the changes in front of you all last night, so I communicated poorly. Let me try again with the benefit of a good night's sleep.

• I wholly agree that we can't just disable tests that fail. Otherwise, what's the point of tests! 😄
• This feature deals with sealed classes. Sealed classes are not supported by the current main Jackson build environments, JDK8 and JDK11. Therefore, in order to run my tests, I needed to get the code to build on a later JDK, at least JDK15 with previews enabled. There was already a JDK17 profile and test source folder, so I chose JDK17.
• There are some issues with building this project on JDK17 on HEAD. (So, before my code is even added.) I suspect this is not news to anyone on this thread!
• I had to make two kinds of changes to get the build working on JDK17.
  • First, I had to open a couple of modules, namely java.lang and java.util, with small changes to the POM. I think these changes are The Right Thing, at least until Jackson decides to package itself as a named module, and should be considered for merging, but probably under a separate PR.
  • Second, I had to disable some tests that are failing at runtime due to changes in JDK17. I think these changes are not The Right Thing, but I chose to disable them so I could get my tests running to demonstrate that the new feature code works. I think these bugs should be addressed outside the scope of this PR, but otherwise it's not obvious to me what the best approach would be. In any case, I agree that the "core" issue is @JsonNaming does not work with Java 16 record types #3102, and added comments to that point in the code.

So we're in a little bit of a weird situation on this PR, mostly because the project is already in a (slightly, but significantly) weird situation on JDK17. Namely, record deserialization appears to be broken on JDK17 due to platform changes starting in JDK17 that are completely outside this project's control.

Here are the options I see for moving forward, FWIW. You guys may (read: probably will) have a better idea.

1. We leave the record deserialization tests disabled for now and open a ticket to re-enable those tests, and mention @JsonNaming does not work with Java 16 record types #3102 in that issue. In my opinion, this is risk, since we'll have a feature (record deserialization) without test coverage. We could reduce the pain somewhat by figuring out a way to disable the tests only for JDK17 (even if it's just a heavily-commented if statement), but it's still not perfect.
2. We leave the JDK17 code in the build but don't run it, and call it a day. The sealed class feature works in JDK8 (since we use reflection) today, as proven by the tests that run right now, but if we remove the JDK17 profile in the build, then the tests won't run and the code could drift. In my opinion, this is risky, since we'll have a feature (sealed class subtype discovery) without test coverage.
3. Sealed classes exist in JDK15 and JDK16 as a preview feature. Record deserialization works fine for JDK16 and earlier. We set up a JDK15 and/or JDK16 profile in the build with preview features enabled and run the tests in that JDK version using a new test source folder. There is precedent for that in the JDK14 profile. That would allow us to get a working build with (passing!) meaningful tests for all features while sidestepping the JDK17 issue entirely.
4. We move this code out of the jackson-databind project and into a JDK17 module.

In my judgment, for what it's worth, Option 3 above is the best way forward. It allows us to divide and conquer what are really two separate problems here: the new sealed class feature and JDK17 support. Practically, I would roll back the changes to the tests, and we would (a) disable the JDK17 build profile temporarily and open an issue about JDK17 if one doesn't exist, and (b) add a build for JDK15/16 in GitHub actions, depending on what we chose above. But I will happily defer to the maintainers' judgment on this (and all) issues.

FYI, @cowtowncoder

[cowtowncoder]

One quick note: I think I'd prefer only testing on LTS versions if at all possible. Use of preview never seems to work very well across various tools.

Other than this, yeah, challenges seen do suggest that maybe my initial wish for inclusion in core databind is misguided, and that this really should be in a module.
Whether that module would be for this specific feature, or more general jackson-module-java17 (as I said, to me versions of interest are 11, 14 (maybe) and 17) which could acquire more functionality, is open for discussion.
For java 8 we had multi-module approach, but for 17 perhaps single one would work best.

[sigpwned]

Got it. If the goal is to stay on LTS releases, then I think the plan to publish JDK17 features as a module for 2.x makes total sense. I have a module version of this ready. I'll clean it up a little bit and share it on one of these threads. I'll happily make any changes to that you guys like, e.g., single vs multiple JDK17 modules, etc. Thank you all!

[sigpwned]

Thank you for the (very) quick review, @pjfanning! I will get on this feedback first thing in the morning!

[sigpwned]

Thank you again for the quick review, @pjfanning! I believe I have addressed your comments in the latest commits.

I did find one issue, though: at least on my machine, while tests in the package com.fasterxml.jackson.failing were ignored, tests in the com.fasterxml.jackson.databind.failing package were not, so I added one line to the POM to ignore tests in that package. Certainly, I can move my tests to com.fasterxml.jackson.failing instead, if that's preferred.

Please let me know if you disagree with that change (or see any other issues, for that matter), and I'll address them ASAP!

[cowtowncoder]

@sigpwned Let me change the failing matching to allow other locations.

Done. Should allow placement wherever, tests under **/failing should now be ignored by junit runs (but can run manually from IDE)

[sigpwned]

Oof. That was embarassing. I've removed the reference to the Java16 idiom Stream#toList. I reconfigured my IDE to use Java8 and everything else looks good, outside of the test folders explicitly intended to run against later Java versions. Next build should work. 🤞

[cowtowncoder]

Aside from tests I think the approach to take has to be that of stand-alone separate module, starting with 2.x.

The practical challenge is then that of spinning up a new one. One approach is that someone creates up a new repo, to look like jackson-modules-java8 under whatever org; and once we get things ready I can eventually move it under FasterXML.
It'd be nice if we could just add it under one of existing multi-maven-module projects, but due to needing JDK 17 to compile I am not sure this is doable, or at least easily so.

[sigpwned]

@cowtowncoder I'm comfortable any which way you and @pjfanning want to approach, and will abide by that decision and help however I can. FWIW, I also have a Java 17 module-style approach up and running at sigpwned/jackson-modules-java-17-sealed-classes, in case that's useful.

[sigpwned]

And I've probably missed the big question, which is should I be pursuing a PR for the 3.x branch?

[cowtowncoder]

@sigpwned At this point no need for 3.0; I think stand-alone module does for now and later on we can consider what to do there (keep separate or add integrated -- most likely if JDK baseline was 17)

[SimSonic]

Hi! What about this PR?

[pjfanning]

@SimSonic there is no plan to merge. Use
https://github.com/sigpwned/jackson-modules-java-17-sealed-classes instead.

[SimSonic]

Use https://github.com/sigpwned/jackson-modules-java-17-sealed-classes instead.

Thanks a lot, I did not see that module at all...

[cowtowncoder]

No plans to merge in its current form, closing.