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: allow json-jackson to auto-discover modules #4447

Merged
merged 1 commit into from
May 11, 2020
Merged

feat: allow json-jackson to auto-discover modules #4447

merged 1 commit into from
May 11, 2020

Conversation

tirz
Copy link
Contributor

@tirz tirz commented Apr 22, 2020

Currently, we cannot register module to jersey-media-json-jackson.
This MR will allow json-jackson to auto-discover the modules in use.

We may for example add to pom.xml:

<dependency>
    <groupId>com.fasterxml.jackson.module</groupId>
    <artifactId>jackson-datatype-jdk8</artifactId>
    <version>2.8.4</version>
</dependency>

Will fix: #4311

@jansupol
Copy link
Contributor

jansupol commented Apr 24, 2020

I am not sure about this. The thing is that the JsonMapperConfigurator code is repackaged from Jackson 2.10.1. As long as a new version of Jackson is repackaged, the contributed code of this PR could be replaced again (and hence lost).

Ideally, the change would be made to Jackson itself.

On the other hand, this is not the first PR we register that asks for changing the Jackson classes (Note that for any change like this in the 3rd party content we probably need to ask Eclipse Legal to approve). We ought to think about some kind of wrapper, if possible.

@jansupol
Copy link
Contributor

It looks like it is possible to pass the ObjectMapper to JacksonJsonProvider. We use it directly in JacksonFeature as well as in Jersey FilteringJacksonJaxbJsonProvider.
Assumingly the solution is to

  • Create a DirectJacksonJsonProvider class that has a constructor that passes the ObjectMapper to Jackson's JacksonJsonProvider. Use the DirectJacksonJsonProvider in JacksonFeature.
  • Create a constructor in Jersey FilteringJacksonJaxbJsonProvider that passes the ObjectMapper to Jackson's JacksonJsonProvider.

Create

package org.glassfish.jersey.jackson.internal;

import com.fasterxml.jackson.databind.ObjectMapper;

public class DefaultJacksonObjectMapper extends ObjectMapper {
    public DefaultJacksonObjectMapper() {
       findAndRegisterModules();
    }
}

And create the constructor, such as (also in DirectJacksonJsonProvider):

public final class FilteringJacksonJaxbJsonProvider extends JacksonJaxbJsonProvider {

    public FilteringJacksonJaxbJsonProvider() {
        super(new DefaultJacksonObjectMapper(), DEFAULT_ANNOTATIONS);
    }

Would that work?

@tirz
Copy link
Contributor Author

tirz commented Apr 26, 2020

Unfortunately, with:

@Singleton
public final class DirectJacksonJaxbJsonProvider extends JacksonJsonProvider {

    public DirectJacksonJaxbJsonProvider() {
        super(new DefaultJacksonObjectMapper(), BASIC_ANNOTATIONS);
    }

    public DirectJacksonJaxbJsonProvider(final Annotations... annotationsToUse) {
        super(new DefaultJacksonObjectMapper(), annotationsToUse);
    }
}
final class DefaultJacksonObjectMapper extends ObjectMapper {

    DefaultJacksonObjectMapper() {
        findAndRegisterModules();
    }
}

We are setting ObjectMapper by an instance of DefaultJacksonObjectMapper at each time, which mean we cannot customize the injection like on this example:

public class ObjectMapperContextResolver implements ContextResolver<ObjectMapper> {

    @Context
    private HttpHeaders headers;

    private final ObjectMapper mapper;

    public ObjectMapperContextResolver() {
        mapper = new ObjectMapper();
        mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY);
    }

    @Override
    public ObjectMapper getContext(Class<?> type) {
        return mapper;
    }
}

I tried a simple:

@Singleton
public class DefaultJacksonJaxbJsonProvider extends JacksonJaxbJsonProvider {

    public DefaultJacksonJaxbJsonProvider() {
        findAndRegisterModules();
    }

    public DefaultJacksonJaxbJsonProvider(final Annotations... annotationsToUse) {
        super(annotationsToUse);
        findAndRegisterModules();
    }

    private void findAndRegisterModules() {
        final ObjectMapper defaultMapper = _mapperConfig.getDefaultMapper();
        if (Objects.nonNull(defaultMapper)) {
            defaultMapper.findAndRegisterModules();
        }

        final ObjectMapper mapper = _mapperConfig.getConfiguredMapper();
        if (Objects.nonNull(mapper)) {
            mapper.findAndRegisterModules();
        }
    }
}

But it seams to break the JDK8 (-Ptravis_e2e_skip) and JDK15 (-Ptravis_e2e) tests :/

I will investigate it.

@jansupol jansupol requested a review from senivam April 27, 2020 13:14
@jansupol
Copy link
Contributor

The Travis is good at exposing imperfections in tests, so the tests fail intermittently. I can see all the tests passed now with this PR.

@jansupol
Copy link
Contributor

It looks good. Please update the copyright years to 2020 and add a copyright header to the new file.

I would not mind a test for the provided functionality, too.

Signed-off-by: Théo Gaillard <theo.gaillard@protonmail.com>
@jansupol
Copy link
Contributor

jansupol commented May 7, 2020

Filed CQ 22138 for jackson-modules-java8.

@jansupol
Copy link
Contributor

The CQ has been approved

@jansupol jansupol merged commit 31bd872 into eclipse-ee4j:master May 11, 2020
@jansupol jansupol added this to the 2.31 milestone May 11, 2020
@tirz tirz deleted the feature-json_jackson_register_modules branch May 12, 2020 23:28
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