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

Automatically serialize/deserialize Java 8 datatype #4311

Closed
wants to merge 1 commit into from

Conversation

Praveen2112
Copy link

This patch adds support for automatically serializing and deserialzing for new "datatypes" introduced in Java 8 for jersey-media-json-jackson module.

Fixes #3637.

This patch adds support for automatically serializing and deserialzing for new "datatypes" introduced in Java 8 for jersey-media-json-jackson module.

Signed-off-by: praveenkrishna <praveenkrishna@tutanota.com>
@jansupol
Copy link
Contributor

This PR does not look right:

  • the newly used module jackson-datatype-jdk8 is deprecated.
  • the class JsonMapperConfigurator.java which is being modified is repackaged JsonMapperConfigurator.java from Jackson - and Jackson itself does not do this in 2.9 nor in 2.10 branch. Have you considered to create the PR to Jackson itself?
  • this PR cries for a test - adding new functionality which allows for using Optional
  • the PR breaks jersey-tests-osgi-functional tests

@Praveen2112
Copy link
Author

Praveen2112 commented Nov 14, 2019

hi, thanks for your comment

the newly used module jackson-datatype-jdk8 is deprecated.

I have checked it from Jackson modules jdk8 it looks like they share a same group and artifactID.

I'll add test for the same and debugging on why jersey-tests-osgi-functional test fails

@jansupol
Copy link
Contributor

jersey-tests-osgi-functional likely failed because you need to modify the JsonJacksonTest to contain mavenBundle().groupId("com.fasterxml.jackson.core").artifactId(jackson-datatype-jdk8").versionAsInProject().

However, I am not convinced we should modify the repackaged JsonMapperConfigurator.

@Praveen2112
Copy link
Author

Actually we can modify the JsonMapperConfigurator in the jackson library but the problem is Jackson-Jaxrs library can also run in Java version before 1.8 so adding them there might break it. And I think from 3.0 version Jackson runs only Java version above 8 so Jdk-8 module would be a part of it. (But it is not released ). So once we move to the 3.0 of Jackson we wouldn't require this dependency

@jansupol
Copy link
Contributor

The preferred way to do this should be to extend JacksonJsonProvider and pass the ObjectMapper with predefined Jdk8Module to be registered. But I am not sure this should be a class in Jersey, it sounds more like a user-specific class, especially when Jackson 3 would not need this.

What might be better is an extended JacksonJsonProvider that would get com.fasterxml.jackson.databind.Module from the @Context Configuration and pass it to JacksonJsonProvider, so that you could use the configuration mechanism to specify the needed modules.

@Praveen2112
Copy link
Author

Okay. Will close it and work on the latter approach. Thank you for your insights.

@jansupol
Copy link
Contributor

Actually, now I look at their repo, they have it as a service. So it should be automatically loaded when on classpath by Jackson somehow.

@jansupol
Copy link
Contributor

I cannot find a code that uses ServiceLoader and in the tests, the Module is registered manually. If that is not handled by Jackson itself, maybe it can be done in Jersey JacksonFeature.

@Praveen2112
Copy link
Author

But aren't those features are a part of Jackson 3.0 ?

@jansupol
Copy link
Contributor

Maybe. But 3.0 may have a few months before release and the services are available for Jackson 2.x.

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.

Automatically serialize/deserialize Java 8 datatype with jersey-media-json-jackson
2 participants