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

Allow Jackson2ObjectMapperBuilder configuration to override the well-known modules' configuration [SPR-12634] #17235

Closed
spring-projects-issues opened this issue Jan 16, 2015 · 14 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jan 16, 2015

Andy Wilkinson opened SPR-12634 and commented

My specific use case is that I'd like to be able to override JodaModule's serialiser for DateTime with one configured with a custom format. I can't do this at the moment by adding the serialiser to Jackson2ObjectMapperBuilder as its module that registers the serialiser is registered before the well-known modules are configured. This means that JodaModule overwrites my custom serialiser.

I've discussed this a little with Sebastien. One suggestion was to add a module to Jackson2ObjectMapperBuilder. This would work, but only because it would then switch off the registration of the well-known modules. I'd like to keep the well-known module registration and be able to apply my own configuration after those modules have been registered.

Would it be possible to change the ordering in Jackson2ObjectMapperBuilder.configure so that the well-known modules are registered with the ObjectMapper before the builder's own module is registered? This would allow the builder's configuration to override the configuration provided by the well-known modules, but could break an existing (slightly strange) use the was expecting their custom configuration to be overridden by a well-known module.


Affects: 4.1.4

Reference URL: spring-projects/spring-boot#2225

Issue Links:

Referenced from: commits 77fcf21, fab8dd3, b215d4c, ccb1c13, 5fb6d6d

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

For the fix targeting 4.1.5, my proposal would be to stay rather conservative and only allow (as proposed by Andy) to customize well-known modules by user provided modules specified by class (Jackson2ObjectMapperBuilder#modulesToInstall) as implemented in this commit.

For 4.2 only, I would add this additional commit that is a less backward compatible change, in order to allow to customize modules configurations with, for example, builder serializers or deserializers, since it seems to me the most logical behavior (modules are bundles, usually predefined, of multiple configurations that are likely to be customized by more specific ones like serializers or deserializers).

Any thoughts ?

@spring-projects-issues
Copy link
Collaborator Author

Andy Wilkinson commented

The modulesToInstall being handled as Class es means that I don't think the first commit is sufficient to address my use case. I'd like to be able to add a module that registers a serialiser that's configured using Boot's JacksonProperties bean. I can't see how to do that given the use of BeanUtils.instantiate. I can add a Module instance but that's back to the problem of it turning off the registration of the well-known modules.

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Andy Wilkinson Ok, so it seems you would like to have in 4.1.5 also the behavior introduced by my second commit, right ?
Spring Boot issue #2225 has been recently closed as invalid, is this change needed to fix another Spring Boot 1.2.x issue ?

@spring-projects-issues
Copy link
Collaborator Author

Andy Wilkinson commented

so it seems you would like to have in 4.1.5 also the behavior introduced by my second commit, right ?

Yes, please

Spring Boot issue #2225 has been recently closed as invalid

Oops, that was me getting a little trigger happy. One part of the issue was user error, but the other part (configuring the format of a serialised DateTime) requires the change we're discussing here. I've reopened the Boot issue.

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Andy Wilkinson OK, thanks for your feedback.

Rossen Stoyanchev Juergen Hoeller Do you think we could introduce this change (combination of my 2 commits) in the 4.1.x branch ? Registering serializers and deserializers after modules make sense from my POV but it may break some applications for people using Jackson2ObjectMapperBuilder or Jackson2ObjectMapperFactoryBean and relying on modules and serializers/deserializers registration order.

On the other hand, the current behavior makes it impossible to fix this Spring Boot bug and waiting a Spring Framework 4.2 release to be used in Spring Boot will take a long time, so I perfectly understand Andy request ...

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

I'm ok with this in 4.1.5. It arguably should have worked that way right from the beginning, and we're finally fixing this now.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Juergen Hoeller This change is now commited in master (main commit + 2nd commit in order to make tests not dependent of the local timezone).

The 4.1.x backport will require a small update, since there has been a breaking change (only in tests) between Jackson Joda module 2.4 (used in 4.1.x branch) and 2.5 (used in master): com.fasterxml.jackson.datatype.joda.ser.JacksonJodaFormat has been renamed to com.fasterxml.jackson.datatype.joda.cfg.JacksonJodaDateFormat. Please let me know to create an updated commit via a PR on 4.1.x branch.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Reopening for a variant that preserves our well-known modules and adds specified Module instances in addition to them.

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Juergen Hoeller Andy Wilkinson What about adding an overloaded modulesToInstall(Modules...) method to the builder and the factory as implemented in this commit?

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

As discussed, that's ok by me, except for the Jackson2ObjectMapperFactoryBean case where we'd end up with overloaded setModulesToInstall setter methods which is invalid according to the JavaBeans rules - since the "modulesToInstall" property needs to have a unique property type and a unique setter associated with it.

So the overloading in Jackson2ObjectMapperBuilder is fine that way but we'd need to find a unique name for the new setter method in Jackson2ObjectMapperFactoryBean... I'm afraid I can't think of a good name for it though. Which leaves me wondering whether we even need it in Jackson2ObjectMapperFactoryBean? Would Boot be alright with that variant only existing in Jackson2ObjectMapperBuilder, where passing in pre-configured Module instances is more natural anyway?

Juergen

@spring-projects-issues
Copy link
Collaborator Author

Andy Wilkinson commented

Would Boot be alright with that variant only existing in Jackson2ObjectMapperBuilder, where passing in pre-configured Module instances is more natural anyway?

Yes, that'd work nicely for Boot. We don't make any use of Jackson2ObjectMapperFactoryBean at the moment.

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Thanks for your feedback. Overloading only in Jackson2ObjectMapperBuilder is also fine to me, I have updated my commit.

@spring-projects-issues
Copy link
Collaborator Author

Sébastien Deleuze commented

Juergen Hoeller It is now in master. I have modified the tests in order to make them more relevant and to avoid to depend on classes that have been renamed between Jackson 2.4 and Jackson 2.5, so backporting should be straightforward.

The 3 commits to backport are:

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Feb 11, 2015

Juergen Hoeller commented

Backported now along with the TimeZone changes in #17195.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants