Skip to content

Conversation

@lorrin
Copy link
Contributor

@lorrin lorrin commented Jul 21, 2017

This change provides a getter for the Jackson Object Mapper so that Pippo applications can customize it as they need. E.g. I have this in my onInit:

ContentTypeEngine applicationJsonEngine = getContentTypeEngine(APPLICATION_JSON);
if (applicationJsonEngine instanceof JacksonJsonEngine) {
    JacksonJsonEngine jacksonJsonEngine = (JacksonJsonEngine) applicationJsonEngine;
    ObjectMapper objectMapper = jacksonJsonEngine.getObjectMapper();
    objectMapper.registerModule(new Jdk8Module());
    objectMapper.registerModule(new GuavaModule());
} else {
    throw new IllegalStateException(String.format(
            "Cannot launch without Jackson engine registered to handle %s content.", APPLICATION_JSON));
}

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 18.362% when pulling f97b923 on lorrin:jackson_om_config into f61b7d7 on decebals:master.

@decebals
Copy link
Member

Thanks! I will take a look.
It's possible to customize Jackson Object Mapper, see #274.

@decebals
Copy link
Member

In the end, I don't see the the difference. You replaced getObjectMapper method with constructObjectMapper (by the way, I believe that createObjectMapper is a better name for a factory method) .
Correct me if I am wrong?

objectMapper.registerModule(new AfterburnerModule());
}

protected abstract ObjectMapper getObjectMapper();
Copy link
Member

Choose a reason for hiding this comment

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

It's enough to make this method public.

@lorrin
Copy link
Contributor Author

lorrin commented Jul 21, 2017

Hi, I hadn't looked at #274 before submitting this. I agree create instead of construct would be a better name for the factory method.

It is not enough to make getObjectMapper public, because it creates a new ObjectMapper every time it is called. I wanted to be able to get the already-created ObjectMapper and make my own modifications to it.

I think the #274 pattern is probably cleanest, even though a bit more verbose. I propose the following:

  1. rename the existing creation method to create
  2. add a get that returns the previously created OM
  3. leave get protected so that users are pushed to follow the customize jackson objectMapper #274 pattern

What would be different is that when I create my own ObjectMapper, I would just extend JacksonJsonEngine and override it's create method to do super, get the result, apply the changes. I don't have to care about the abstract parent class or reimplementing things it does that I want to preserve. I'll update this PR.

@lorrin
Copy link
Contributor Author

lorrin commented Jul 21, 2017

Never mind, I hadn't fully understood the #274 pattern until I tried it. You're right, the changes I submitted weren't needed to customize Jackson easily. I've kept this PR alive as a simple rename from get to create for the factory method. Thanks for the help.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 18.365% when pulling ffdc51d on lorrin:jackson_om_config into f61b7d7 on decebals:master.

@decebals
Copy link
Member

The rename is good but we have a problem because we brake the API, without a very good reason and I am sure that they are people that use this method (getObjectMapper) already.

@lorrin
Copy link
Contributor Author

lorrin commented Jul 31, 2017

Alright, I'll close this then. Thanks for your help.

@lorrin lorrin closed this Jul 31, 2017
@lorrin
Copy link
Contributor Author

lorrin commented Aug 2, 2017

BTW, if anyone stumbles across this looking for info on how to follow the #274 pattern, it's like this:

        JacksonJsonEngine customEngine = new JacksonJsonEngine(){
                @Override
                public void init(Application application) {
                    super.init(application);
                    objectMapper.registerModule(new Jdk8Module());
                    objectMapper.registerModule(new GuavaModule());
                }
            };
        customEngine.init(this);
        getContentTypeEngines().setContentTypeEngine(customEngine);

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.

3 participants