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

Configure the ObjectMappers to Use for a Class by MediaType #26212

Closed
odrotbohm opened this issue Dec 4, 2020 · 4 comments
Closed

Configure the ObjectMappers to Use for a Class by MediaType #26212

odrotbohm opened this issue Dec 4, 2020 · 4 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@odrotbohm
Copy link
Member

odrotbohm commented Dec 4, 2020

Take the following HttpMessageConverter setup in a Spring Boot application:

  • An HMC registered to handle the application/hal+json media type (via Spring HATEOAS).
  • The default MappingJackson2HttpMessageConverter registered to handle application/json and application/*+json.

An HTTP request with Accept: application/prs.hal-forms+json, application/hal+json header will produce a response with Content-Type: application/prs.hal-forms+json which I think is wrong. The returned content type should be application/hal+json.

Here's what happens: AbstractMessageConverterMethodProcessor.writeWithMessageConverters(…) iterates over all acceptable types, starting with application/prs.hal-forms+json and checks it for compatibility with the producible media types. The HAL HMC does not match but the default Jackson one matches due to the support of application/*+json.

In the next step, getMostSpecificMediaType(…) is called and that match turns from a wildcard match into a concrete match. I.e. the match is treated as if application/prs.hal-forms+json had been matched explicitly. The now following concrete match for the HAL HMC and application/hal+json ends up second in the line in mediaTypesToUse.

That "upgrade" in the match causes the subsequent MediaType.sortBySpecificityAndQuality(mediaTypesToUse) to be without effect on the ordering as the first, actually wildcard match, is treated as concrete. As a consequence, the fallback HMC gets used instead of the actual concrete match of application/hal+json and is not even able to render the object according to the media type specification.

I've prepared a reproducing example here.

@odrotbohm odrotbohm added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 4, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 15, 2021

Thanks for the sample.

I see what you're pointing out with the choice of MappingJackson2HttpMessageConverter based on a wildcard, "application/*-json", rather than picking the HATEOAS converter based on a concrete media type, "application/hal+json". However, keep in mind that from a client perspective "application/prs.hal-forms+json" is listed first and as long as that is supported it makes sense that it is picked first. In other words if we go purely based on Producible media type, I can imagine an opposite issue where the client asks for a concrete media type first followed by a wildcard media type second, but we pick the second anyway.

In terms of being supported, when MappingJackson2HttpMessageConverter lists "application/*-json" it means it can render any object as JSON but the assumption is that the object already represents the necessary format. From that perspective the converter doesn't need to know much about the format itself and therefore I see nothing wrong with the way it's configured and not much that we need to change.

Is there any support for "application/prs.hal-forms+json" and if not then perhaps HATEOAS should ensure that this media type is not treated as something we can render? There could be a number of ways. One possible direction might be customizing ContentNegotiationManager to filter it out from the list of headers when present. Or perhaps being more explicit about the list of supported media types like so:

@GetMapping(path = "/", produces = {MediaTypes.HAL_JSON_VALUE, ...})
RepresentationModel<?> index() {
	return new RepresentationModel<>().add(Link.of("/url"));
}

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 15, 2021

After a direct discussion, a summary of the problem space.

MappingJackson2HttpMessageConverter expresses it can render any Object as JSON through "application/*-json" and that is generally true but in the case of Spring HATEOAS, a RepresentationModel can be rendered differently by media type based on differently configured ObjectMapper instances. Currently Spring HATEOAS solves this through a sub-class of MappingJackson2HttpMessageConverter that targets RepresentationModel and is configured with a custom ObjectMapper and media type. So registrations may end up looking as follows:

Converter Supported Type Media Types ObjectMapper
HATEOAS MappingJackson2HttpMessageConverter RepresentationModel "application/prs.hal-forms+json", "application/json" instance1
HATEOAS MappingJackson2HttpMessageConverter RepresentationModel "application/hal+json", "application/json" instance2
... ... ...
MappingJackson2HttpMessageConverter Any object "application/*+json", "application/json" instance3
... ... ...

When a client requests both "application/prs.hal-forms+json" and "application/hal+json", if support for only the latter is enabled we fall back on the default Jackson converter whose ObjectMapper is not customized for the job.

One option would be to configure MappingJackson2HttpMessageConverter with Object types it should opt out of. This could be useful in WebFlux as well where currently the Jackson2JsonEncoder skips String but it would be useful to be able to customize that.

A second line of thought is to allow registrations of additional ObjectMapper instances through BiFunction<Class<?>, MediaType, ObjectMapper> registrations that would help select the ObjectMapper to use, or alternatively falling back on the main ObjectMapper instance used by default. Note that in this approach there is still a need to preclude falling back on the main ObjectMapper for a given type.

@rstoyanchev rstoyanchev self-assigned this Jan 15, 2021
@rstoyanchev rstoyanchev added this to the 5.3.4 milestone Jan 15, 2021
@bclozel
Copy link
Member

bclozel commented Jan 18, 2021

I think that registering additional, MediaType-specific mappers in the main JSON converter would make sense.

While ordering of converters and its effect is by design, this additional level would make the current use case more obvious in our infrastructure: we need to have specialized conversions depending on the media type, even if those are all JSON-compatible.

I'm not sure that the first solution (i.e. "opting out based on the object type") would work in other cases.

In general, I'm wondering if we shouldn't change the defaults for the JSON converter and support application/vnd.*+json (custom media types defined by the application), but not application/*+json. While it's technically right that the default converter can serialize things as JSON, it seems that there are now many JSON media types out there with specific formats that a vanilla JSON converter won't understand.

@rstoyanchev
Copy link
Contributor

Team decision: we'll proceed with a prototyping support for more than one ObjectMapper to be used.

@rstoyanchev rstoyanchev changed the title AbstractMessageConverterMethodProcessor.writeWithMessageConverters(…) chooses the wrong media type to produce Configure the ObjectMappers to Use for a Class by MediaType Feb 1, 2021
This was referenced Mar 13, 2021
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

3 participants