Skip to content

serde framework detection should be centralized and should not be hard-coded #32384

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

Closed
volkert-fastned opened this issue Mar 6, 2024 · 4 comments
Assignees
Labels
status: duplicate A duplicate of another issue

Comments

@volkert-fastned
Copy link

volkert-fastned commented Mar 6, 2024

In Spring Framework 6.1.4, found five different locations that have a hard-coded serde framework lookup mechanism like this, in a static block:

	static {
		ClassLoader classLoader = [CLASS NAME].class.getClassLoader();
		jaxb2Present = ClassUtils.isPresent("jakarta.xml.bind.Binder", classLoader);
		jackson2Present = ClassUtils.isPresent("com.fasterxml.jackson.databind.ObjectMapper", classLoader) &&
						ClassUtils.isPresent("com.fasterxml.jackson.core.JsonGenerator", classLoader);
		jackson2XmlPresent = ClassUtils.isPresent("com.fasterxml.jackson.dataformat.xml.XmlMapper", classLoader);
		jackson2SmilePresent = ClassUtils.isPresent("com.fasterxml.jackson.dataformat.smile.SmileFactory", classLoader);
		gsonPresent = ClassUtils.isPresent("com.google.gson.Gson", classLoader);
		jsonbPresent = ClassUtils.isPresent("jakarta.json.bind.Jsonb", classLoader);
		kotlinSerializationCborPresent = ClassUtils.isPresent("kotlinx.serialization.cbor.Cbor", classLoader);
		kotlinSerializationJsonPresent = ClassUtils.isPresent("kotlinx.serialization.json.Json", classLoader);
		kotlinSerializationProtobufPresent = ClassUtils.isPresent("kotlinx.serialization.protobuf.ProtoBuf", classLoader);
	}

This is duplicated in the following classes, as far as I could find:

  • AllEncompassingFormHttpMessageConverter.java
  • BaseDefaultCodecs.java
  • DefaultRestClientBuilder.java
  • RestTemplate.java
  • WebMvcConfigurationSupport.java

In my experience, this poses multiple problems:

  • This behavior is very hard to intercept, override and/or customize
  • Different Spring components require this to be overridden separately

The fact that these are hard-coded classpath lookups means that it's very hard to explicitly select a preferred serde framework, when more than one of these happen to live in the classpath, for instance, as a pulled-in transative dependency in a non-Spring library. As a result, we are dependent on whatever opinionated preference the Spring Framework itself ends up having, depending on what it finds in the classpath.

In Spring Boot, there appears to be a configuration property spring.mvc.converters.preferred-json-mapper that should allow developers to explicitly select a preferred serde framework, overriding this autodetection mechanism. However, this is specific to Spring Boot, and even then, It's not clear whether all Spring Components that depend on any of the 5 aforementioned classes would actually respect this.

Also, in integration tests, it appears that MockMvc does not respect this configuration property, at least not by default.

Although it might eventually be possible to figure out some kind of workaround, the fact that there is hard-coded classpath lookup going on for this in several parts of the Spring Framework, and in a static block upon class instantiation, makes it very hard to override this behavior. You can't simply exclude any of the hard-coded package names from the Component Scanner for instance, since the lookup is done directly in the classpath, and the frameworks themselves aren't loaded as Spring Components. (Perhaps having annotated Spring Components that wrap each supported serde framework would be a good approach.)

So as things seem right now, you'd have to extend, wrap or write alternative implementations of all these separate classes, which is far from trivial. And then you'd have to figure out how to have these classes properly and consistently replace the classes they are meant to replace in the Spring Configuration.

It seems better to have one general Spring-wide and well-documented serde framework lookup/selection mechanism that all these classes would make use of, and which developers could easily configure or customize in one place, when the defaults happen not to suit them.

Thanks for considering this improvement.

@sdeleuze
Copy link
Contributor

sdeleuze commented Mar 6, 2024

From my POV, this issue is a duplicate of #32382, so please see my comments on this issue.

@sdeleuze sdeleuze closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
@sdeleuze sdeleuze added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 6, 2024
@volkert-fastned
Copy link
Author

@sdeleuze Sorry for my previous (now deleted) response, I didn't read your latest response in the other ticket until now. I'll look into that further.

But doesn't the point about the duplicate code in the static blocks of all these classes still stand?

@volkert-fastned
Copy link
Author

@sdeleuze Also, the more I think about it, the more I disagree with how the framework changes behavior, depending on what it finds in the classpath. Just because I added some library that pulled in some transitive dependencies, I suddenly have to figure out what configuration changes and such to apply to my Spring project, just to restore the previous behavior. That just doesn't feel right to me.

I really didn't mean to use GitHub issues as a support channel like StackOverflow, and sorry if I gave you that impression. But I think a discussion on whether the existing behavior is desirable is a perfectly reasonable one to have.

@volkert-fastned
Copy link
Author

For anybody stumbling upon this thread through a search engine while looking for a solution to this problem in Spring Boot, see these answers for a workaround:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

3 participants