AllEncompassingFormHttpMessageConverter prioritizes Jackson 2 XML over JSON [SPR-13309]

[spring-projects-issues]

Abhijit Sarkar opened SPR-13309 and commented

AllEncompassingFormHttpMessageConverter forces Jackson XML without any option given to the user to choose an implementation. Until 4.1.7.RELEASE, Jackson JSON used to be the default and the code didn't set part converters exclusively (as it does now using if-else instead of if as was before). There's no log statements indicating what converters are chosen by default causing a whole lot of pain for people like me whose code is now broken.

---

Affects: 4.2 GA

Attachments:

• AllEncompassingFormHttpMessageConverter.jpg (234.54 kB)

Issue Links:

• AllEncompassingFormHttpMessageConverter does not prefer Jackson2 over JAXB for XML marshalling [SPR-13240] #17831 AllEncompassingFormHttpMessageConverter does not prefer Jackson2 over JAXB for XML marshalling
• AllEncompassingFormHttpMessageConverter ignores JAXB XML when Jackson used for JSON [SPR-14336] #18908 AllEncompassingFormHttpMessageConverter ignores JAXB XML when Jackson used for JSON

Referenced from: commits 257cc63

[spring-projects-issues]

Rossen Stoyanchev commented

Can you elaborate what the actual end result error is?

I think we are talking about #17831 which ensured that Jackson XML is preferred (over JAXB2) if it is available on the classpath. This was only intended to impact XML serialization however. Prior to that the AllEncompassingFormHttpMessageConverter used to register JAXB2 and Jackson JSON converters and that hasn't changed.

I'm wondering if the issue you're running into is with using the RestTemplate and the order of registration of converters where the first converter to succeed is how the request body gets serialized and the Content-Type decided upon?

If so we can probably update the order to preserve the order and which converters were registered at the front. This will avoid regressions where RestTemplate#postXxx and other such methods is involved since they can't specify a preferred Content-Type. At the same time it does make sense to register Jackson XML still since that helps where RestTemplate#exchange(Requestentity) and other such methods is involved and where the preferred Content-Type can be specified making it possible to pick JSON vs XML for example.

[spring-projects-issues]

Abhijit Sarkar commented

The actual end result depends on the client, but in our case the request was rejected because the content type changed from JSON to XML.
I think the issue at hand is related but not exactly the same as #17831. The order of converters, as you mentioned, has changed and is a problem. What also has changed is that instead of registering all converters if they're present on the class path, the code was changed to add only one that's found first (if vs if-else).
I'm not sure why the framework imposes a preference for any kind of converter based on the class path. It seems like a bad idea. In a large project, there may be more than one libraries on the class path and keeping the order straight like the way Spring wants it is a fragile and ugly process. Why not have the options to let the developer choose an implementation, like preferJackson or preferJAXB. If they don't specify, default to the previous behavior. I also don't see the value in the change made to register converters exclusively instead of all (if-else). The converters are tried in order and it seems perfectly reasonable to have multiple that can handle same content and the first one wins. That's the whole point of having a chain of converters.
I love Spring but the changes made to AllEncompassingFormHttpMessageConverter in 4.2.0.RELEASE don't seem to be well thought out.

[spring-projects-issues]

Rossen Stoyanchev commented

The actual end result depends on the client, but in our case the request was rejected because the content type changed from JSON to XML.

Right, this is what I thought. Although I'm not sure what you mean by "depends on the client" (who/what is the client?). Either way I presume that jackson-dataformat-xml is on the classpath. I assume that is intentional (it's only necessary if you care about XML serialization via Jackson)?

What also has changed is that instead of registering all converters if they're present on the class path, the code was changed to add only one that's found first (if vs if-else).

Wrong. Jackson JSON is still registered and preferred.

As for why we register converters based on classpath, note that this is being done very carefully where the intent is very clear. In this case the effect is that (1) if jackson-dataformat-xml is on the classpath, it will be preferred over JAXB2, and (2) if Jackson is on the classpath it's preferred overy Gson. These are very reasonable assumptions that furthermore are very easy to override if you are in a situation where they don't work.

The key issue I see here is the order of registration. I will provide a fix that you can try out.

[spring-projects-issues]

Abhijit Sarkar commented

Wrong. Jackson JSON is still registered and preferred.

You're right, I missed to see that the if-else is only within converters that can handle the same content type.

if jackson-dataformat-xml is on the classpath, it will be preferred over JAXB2

I still contend that this should be left up to the user. Jackson XML has several limitations and I'm not entirely sure if Jackson JSON doesn't bring in some transitive dependencies on Jackson XML. In our case, we prefer JAXB over Jackson and the way AllEncompassingFormHttpMessageConverter is coded, there's no option to alter that behavior. We ended up creating our own converter and registered Jackson and JAXB regardless of classpath. As I suggested earlier, if you provide options to control that, we could avoid having to maintain code that we don't need.
What I'm suggesting is something like the following:

private HttpMessageConverter xmlMessageConverter; // setter, getter private HttpMessageConverter jsonMessageConverter; // setter, getter
private boolean favorJsonOverXml = true; // setter, getter

HttpMessageConverter<?> xmlMessageConverter = // figure out a sensible default if user didn't provide one
HttpMessageConverter<?> jsonMessageConverter = // figure out a sensible default if user didn't provide one

if (favorJsonOverXml) {
     addPartConverter(jsonMessageConverter);
     addPartConverter(xmlMessageConverter);
} else {
     addPartConverter(xmlMessageConverter);
     addPartConverter(jsonMessageConverter);
}

[spring-projects-issues]

Rossen Stoyanchev commented

I still contend that this should be left up to the user. Jackson XML has several limitations and I'm not entirely sure if Jackson JSON doesn't bring in some transitive dependencies on Jackson XML.

Wrong. Jackson JSON and XML are separate dependencies. You need to remove jackson-dataformat-xml from your dependencies if you don't want it. Or find out what's bringing it with mvn dependency:tree or gradle dependencies.

The idea is to provide sensible out-of-the-box defaults. The suggested setters are unnecessary. If you don't like the defaults, just set the converters explicitly. What we need to do is fix the regression by ensuring that Jackson JSON comes before Jackson XML. Then all is good.