spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.94k stars 40.65k forks source link

Reconsider whether spring-boot-starter-json should bring in spring-web #18639

Closed elefeint closed 5 years ago

elefeint commented 5 years ago

Would you consider revisiting the decision to have JSON starter depend on spring-web (original discussion in #10031)?

With microservices and spring-cloud-stream gaining adoption, JSON deserialization is becoming more useful for non-web contexts.

wilkinsona commented 5 years ago

As discussed in #10031, removing spring-web would have the significant downside of preventing us from auto-configuring Jackson2ObjectMapperBuilder. That would mean that none of the spring.jackson.* configuration properties have any effect.

What benefit do you see in removing the dependency?

elefeint commented 5 years ago

My current context is in writing a spring-cloud-stream sample Sink application that receives serialized objects from Cloud Pub/Sub. I was surprised to see that spring-web got brought in as a dependency, as it is not needed for the application's purpose.

I understand that just taking the dependency out of spring-boot-starter-json starter would break the functionality entirely. The suggestion was more to extract object mapping auto-configuration out of Spring Web into a reusable json-mapping-only auto-configuration.

wilkinsona commented 5 years ago

Unfortunately, that can't happen while Jackson2ObjectMapperBuilder remains part of spring-web. Its location is out of the control of the Spring Boot team as spring-web is part of Spring Framework. If you'd like to pursue this, please open a Spring Framework issue to see if Jackson2ObjectMapperBuilder could be moved to another module and comment here with a link to it. I'll close this issue for now. It can be re-opened if a change is made in Spring Framework that would allow us to proceed.

pgv commented 1 year ago

You should drop Jackson2ObjectMapperBuilder and use JsonMapper.builder() instead in JacksonAutoconfiguration.

wilkinsona commented 1 year ago

Thanks for the suggestion but the two aren't really equivalent. Jackson2ObjectMapperBuilder provides quite a lot of additional functionality that we use when processing the spring.jackson.* properties.

pgv commented 1 year ago

I disagree with you there. In my opinion, the main work in is done in the JacksonAutoConfiguration class. It configures all the right values in the Jackson2ObjectMapperBuilder which then uses those values to configure the ObjectMapper. If you look at the code most (not all, but most) of the properties, features, and modules are just "copied" exactly as they come from the JacksonAutoConfiguration into the ObjectMapper. In fact I have implemented a custom version of JacksonAutoConfiguration using JsonMapper.builder() and my own JsonMapperBuilderCustomizer. I have done it quite fast and haven't checked to see that everything is replicated exactly, but it shouldn't be too difficult to provide a @ConditionalOnMIssingClass(Jackson2ObjectMapperBuilder.class) configuration that builds an ObjectMapper using JsonMapper.builder(). Maybe I'm missing something and there is something very important that can only be done using Jackson2ObjectMapperBuilder, but I honestly don't see it.

pgv commented 1 year ago

You are right with your comment in #35287, I think spring-boot should deprecate its use of Jackson2ObjectMapperBuilder entirely. And rewrite the JacksonAutoConfiguration without it. Just in case, I'm attaching a quick copy&paste that I made in 20 minutes replacing Jackson2ObjectMapperBuilder with JsonMapper.builder(). There are a couple of missing features that are configured by default in Jackson2ObjectMapperBuilder (those could be added easily here, but why?) and the module lookup using ServiceLoader and the jdk8, javatime, joda, and kotlin modules (again, is that really a sensible default?). All that could be easily added here as well. Or... they could be added as beans if the developer needs them.

Just to give an idea of how nice it could be without that nasty spring-web dependency. On top of that I found that JsonMapper.builder() actually gives you access to more options from the jackson library that are not available in Jackson2ObjectMapperBuilder. Just so you know, I did all this because spring boot detects the web component and launches a web server automatically and I have to configure it so it doesn't do that.

jsonmapper.zip

wilkinsona commented 1 year ago

Just so you know, I did all this because spring boot detects the web component and launches a web server automatically and I have to configure it so it doesn't do that

It shouldn't do that. spring-web isn't sufficient for Boot to start a web server. We need spring-webmvc or spring-webflux and an embedded web server (Jetty, Netty, Tomcat, or Undertow) to start a server.

pgv commented 1 year ago

So, I'm using debezium to get database changes from one place and update it in another place as json objects. So, I have the JacksonAutoconfiguration and a jetty server in the same app. So the web application type is set to SERVLET, and then I get an exception because there is some class missing, I guess from webmvc (Well, not me, just some other team using my module). So I have to tell them to set .web(NONE) and that's it. So the question is having the standard and official and more complete JsonMapper.builder() why reinvent the wheel and use the Jackson2ObjectMapperBuilder which is basically a bad copy?