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

Support Jackson based XML serialization #1580

Closed
sdeleuze opened this issue Sep 18, 2014 · 14 comments
Closed

Support Jackson based XML serialization #1580

sdeleuze opened this issue Sep 18, 2014 · 14 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@sdeleuze
Copy link
Contributor

Spring 4.1 now supports Jackson for XML serialization based on Jackson XML extension.
It is automatically enabled when using <mvc:annotation-driven /> or @EnableWebMvc if Jackson XML extension is detected on the classpath.

Like what it is done for ObjectMapper, Spring Boot could provide easy MappingJackson2XmlHttpMessageConverter customization by creating a conditional XmlMapper bean in JacksonAutoConfiguration.

I will provide a PR for that.

@sdeleuze sdeleuze added the type: enhancement A general enhancement label Sep 18, 2014
@sdeleuze sdeleuze self-assigned this Sep 18, 2014
@philwebb philwebb added the status: ideal-for-contribution An issue that a contributor can help us with label Sep 23, 2014
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

I have began to work on this issue by adding a new JacksonXmlMapperAutoConfiguration inner static class to JacksonAutoConfiguration (see this gist for more details). It declares a XmlMapper bean if this class is detected on the classpath.

The issue here is that the @ConditionalOnMissingBean on ObjectMapper jacksonObjectMapper() bean declaration take in account subclasses, so as soon as you register a XmlMapper bean (which is a subclass of ObjectMapper), the ObjecMapper bean is not registered anymore.

How do you think I should handle that ? Should I implement an additional includeSubclasses boolean attribute on @ConditionalOnMissingBean, @ConditionalOBean and OnBeanCondition ?

@wilkinsona
Copy link
Member

Thinking aloud: rather than adding the includeSubclasses attribute, it may be possible to order the auto-configuration such that the ObjectMapper is auto-configured first and the auto-configuration for XmlMapper can then check for an existing XmlMapper.

Taking a step back, how do you expect an app to make use of both an ObjectMapper and an XmlMapper? If both beans exist, any injection of an ObjectMapper will have two candidates and they'll have to distinguish between them. That feels a bit clunky but I guess it's a solved problem in Spring 4.1?

@snicoll
Copy link
Member

snicoll commented Oct 1, 2014

I concur with Andy here. I also find it weird that the app can work with both an ObjectMapper and an XmlMapper as type injection would not be deterministic enough?

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

I agree too, we may find a better solution.

In Spring Framework, we don't deal with ObjectMapper beans. If we need a custom ObjectMapper we need to customize HttpMessageConverters.

If we can do some breaking change in Spring Boot 1.2, my proposal would be de register a Jackson2ObjectMapperBuilder bean (it has been introduced in Spring Framework 4.1) instead of an ObjectMapperbean. This builder has been designed to allow creating ObjectMapper and XmlMapper instances from the same configuration.

Any thought ?

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

We may perhaps be able to avoid breaking changes by adding @ConditionalOnMissingBean(Jackson2ObjectMapperBuilder.class) to ObjectMapper JacksonObjectMapperAutoConfiguration#jacksonObjectMapper() ...

@wilkinsona
Copy link
Member

I think we may be over-thinking this. Why not auto-configure a single bean: an ObjectMapper by default and an XmlMapper when the app depends on com.fasterxml.jackson.dataformat:jackson-dataformat-xml?

Going a step further, we could perhaps use a Jackson2ObjectMapperBuilder under the covers and apply the configuration from the environment to it. We'd then use this to create the ObjectMapper or XmlMapper. If we exposed the builder as a bean, users that wanted both an ObjectMapper and an XmlMapper could have the builder injected and use it to create the other mapper? This would create the same injection problem for ObjectMapper dependencies, but it would now be opt in.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

I don't think we should limit auto-configuration to a single bean since XmlMapper should not be used for JSON serialization and using Jackson for JSON and XML serialization at the same time is a common use case.

In Spring Framework 4.1, @EnableWebMvc register both MappingJackson2HttpMessageConverter and MappingJackson2XmlHttpMessageConverter (instead of JAXB2 converter), so if you want to customize their objectMapper property you will need both ObjectMapperand XmlMapperbeans.

About the Jackson2ObjectMapperBuilder bean proposal, Spring Boot could use directly this "common" builder to create ObjectMapper and XmlMapper instances without creating related beans, so no more injection problem. When Jackson2ObjectMapperBuilder bean is missing, we could keep the current ObjectMapper bean management for compatibility reasons or for people using just JSON serialization. Would you be ok with this kind of mechanism ?

@wilkinsona
Copy link
Member

I don't think we should stop exposing the ObjectMapper as a bean as it may break existing applications.

As an alternative, how about this:

  • Update JacksonAutoConfiguration to create a Jackson2ObjectMapperBuilder, configure it from the environment, and expose it as a bean
  • Update JacksonObjectMapperAutoConfiguration to create its ObjectMapper from the builder
  • Update HttpMessageConvertersAutoConfiguration to add auto-configuration for MappingJackson2XmlHttpMessageConverter that consumes the Jackson2ObjectMapperBuilder and uses it to create the converter's XmlMapper. The XmlMapper is never exposed as a bean.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

Yes, I agree that we should keep ObjectMapper bean to avoid breaking existing application.

About the alternative, from a user perpective I would expect the following behavior:

  1. If a user defines just an ObjectMapper bean, it should be used to configure the MappingJackson2HttpMessageConverter.
  2. If a user defines a Jackson2ObjectMapperBuilder bean, it should be used to configure both MappingJackson2HttpMessageConverter and MappingJackson2XmlHttpMessageConverter.

Do you agree with 2) ?

A possible implementation could be:

  • No Jackson2ObjectMapperBuilder bean created by default.
  • Update JacksonObjectMapperAutoConfiguration to create its ObjectMapper from the builder
  • Update HttpMessageConvertersAutoConfiguration to:
    • Add auto-configuration for MappingJackson2XmlHttpMessageConverter that consumes the Jackson2ObjectMapperBuilder bean (or create a new Jackson2ObjectMapperBuilder instance if no bean exists) and uses it to create the converter's XmlMapper. The XmlMapper is never exposed as a bean.
    • Update auto-configuration for MappingJackson2HttpMessageConverter that consumes an optional Jackson2ObjectMapperBuilder bean and uses it to create the converter's ObjectMapper. If no Jackson2ObjectMapperBuilder bean exists, then it should uses the ObjectMapper bean.

Does it make sense from you point of view ?

@wilkinsona
Copy link
Member

I don't entirely agree with 2. In 1.1, MappingJackson2HttpMessageConverter is configured using an existing ObjectMapper bean (either created by the user or by the auto-configuration). I don't think we should change this behaviour unless there's a good reason to do so.

I can't see any harm in creating a Jackson2ObjectMapperBuilder if the user hasn't provided one, i.e. it should be conditional on a missing Jackson2ObjectMapperBuilder bean. This bean can then be injected into the auto-configuration that creates an ObjectMapper (if there isn't such a bean already) and that creates the new MappingJackson2XmlHtppMessageConverter (again, if there isn't such a bean already). Taking this approach, there's no need to change the existing auto-configuration for MappingJackson2HttpMessageConverter.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

Indeed, if the ObjectMapper bean is created thanks to the Jackson2ObjectMapperBuilder one, it will indirectly use its configuration, so that's perfect.

I will send a PR with this implementation if you agree.

@wilkinsona
Copy link
Member

Sounds good to me. Thanks!

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 1, 2014

Thanks for your feedback!

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 3, 2014

Hi @wilkinsona, could you have a look to my PR #1661 and send me your feedbacks ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants