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

feat: More work and docs needed for jackson in native-image #32326

Merged
merged 12 commits into from Feb 27, 2024

Conversation

johanandren
Copy link
Member

@johanandren johanandren commented Feb 22, 2024

Not entirely great but I think this improves on things quite a bit

final case class Compass(@JsonProperty("currentDirection") currentDirection: Direction) extends JsonSerializable

// scala enums
// FIXME this one needs some very specific stuff for Scala enums, might not be worth it
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails with reflective access to $outer so to support it we'd need to detect Enumeration subtype in AkkaJacksonSerializationFeature and handle specifically

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to give up on Scala stdlib enums for now and documented that those doesn't work

// from the akka-serialization-jackson docs

// type hierarchy / ADT
// FIXME: Still haven't figured out why @JsonCreator cannot be used here
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really strange, I have verified that the constructor and parameter names are available for reflection, and for multi-param constructors it just works with nothing extra, but for some reason just @JsonCreator doesn't cut it here and a @JsonProperty("fieldName") is required.

I also ran with agent and tried with the generated reflect-config.json (which I expect to add the same or a bit less for the same class) and get the same error so it is something specifically about how Jackson reflectively finds single-param constructors with that annotation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out what works, but not why:

When declared like in our docs @JsonCreator case class MyClas(field: String) the annotation is on the class, not on the constructor, if the class instead is annotated on the constructor case class MyClass @JsonCreator() (field: String) it works as expected with native-image.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is still weird is that for regular JDK the @JsonCreator is not needed either, but for lots or possibly all single param constructors in native-images it is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, feels like the Jackson Scala module isn't used? Could it be that it's not included in native image and then falls back to regular Java Jackson, which requires the JsonCreator and has the param name problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, maybe something missing in the reflective stuff for modules, I'll investigate that a bit further.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that's not it, the objectMapper lists the same modules for normal JDK run as on native image.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained elsewhere but it turns out that the Scala Jackson support reads/parses the actual classfiles as resources from the classpath, so needs a resource-config.json entry for graal. The AkkaJacksonSerializationFeature now automatically adds that for classes that should be serialisable with Jackson.


// Note: native image needs additional marker trait on ADT supertype here or else
// AkkaJacksonSerializationFeature has no way to detect and register the subtypes for
// reflective access
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really annoying, but can't see a way around except possibly doing classpath scanning, which we cannot do in a native image Feature so would have to be some other test infrastructure that users run to generate metadata files (a bit like the one I have in place internally in Akka).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like current state of art means that it is acceptable for users to have some trouble to solve themselves. Spring docs mention quite some caveats https://docs.spring.io/spring-boot/docs/current/reference/html/native-image.html#native-image.advanced.known-limitations

There I also noticed the link to https://github.com/oracle/graalvm-reachability-metadata
Is that something we can point at? I guess it's more related to libraries than user code, which is what you are trying to solve here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did figure out a way around it today, apparently you can register more type reachability callbacks from a reachability callback, so it now looks for interface constructor parameters and registers a new callback handler to find and register concrete types. Seems to work fine from the test coverage in local-scala native image test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the reachability metadata I've gone with including what applies from the metadata repo, but coverage there is very spotty, so I think for now it is better, for user experience, with our own verified reflection metadata for transitive dependencies we say we support (unless they provide it themselves in their jars like Netty, which then just works tm).

RuntimeReflection.registerAllDeclaredConstructors(clazz)
// we still need explicit register of each constructor for some reason
util.Arrays.stream(clazz.getDeclaredConstructors).forEach { constructor =>
// FIXME this could probably be more selective
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But unclear to me how Jackson chooses what constructor to use in the face of several, perhaps based on the incoming data and we really need to list all?

@johanandren johanandren force-pushed the wip-more-jackson-native-image-stuff branch from 5dcc7e6 to 90f6f26 Compare February 26, 2024 12:33
Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. That was more complicated, but great to have built-in support for this 👍🏼

akka-docs/src/main/paradox/additional/native-image.md Outdated Show resolved Hide resolved
akka-docs/src/main/paradox/additional/native-image.md Outdated Show resolved Hide resolved

private def registerClassResourceForAccess(access: Feature.DuringAnalysisAccess, clazz: Class[_]): Unit = {
if (!alreadyRegisteredClassResources.contains(clazz.getName)) {
// Scala Jackson support needs the class files as resources as well
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@johanandren
Copy link
Member Author

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johanandren johanandren merged commit f0f5b28 into main Feb 27, 2024
6 checks passed
@johanandren johanandren deleted the wip-more-jackson-native-image-stuff branch February 27, 2024 12:25
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

Successfully merging this pull request may close these issues.

None yet

3 participants