micronaut-core icon indicating copy to clipboard operation
micronaut-core copied to clipboard

Implement content negotiation for Accept header

Open yawkat opened this issue 3 years ago • 6 comments

This patch adds support for proper content negotiation using the Accept header. This includes new support for:

  • "Partial" matches, like text/*
  • Quality parameters
  • Correct matching by precedence, e.g. matching text/plain over */*.

Compatibility considerations:

  • MediaType now considers parameters in matches, if specified
  • Overloaded routes that relied on the old, incorrect matching behavior will now match differently.

Resolves #6637

yawkat avatar Dec 13 '21 11:12 yawkat

The ContentNegotiationSpec still fails because of a test case that has Accept: application/json, application/xml, and expects that to only match the controller for application/json. So basically it relies on the order of Accept headers.

I don't think we should support this. The spec makes no mention of order being relevant. Browsers seem to use quality values properly and not rely on order either. Edge is the exception there, I will look around if I can find more details on Edge, but even that browser will work with this PR (I try to match the "most specific" controller).

@graemerocher thoughts?

yawkat avatar Dec 13 '21 12:12 yawkat

Apparently others have noticed the Edge thing too, and they come to the same conclusion: https://github.com/willdurand/Negotiation/issues/98#issuecomment-390625746

They also propose the same approach of prioritizing non-wildcard accept types that I use in this patch. That fixes edge, but it does not fix the test case that is failing for this PR.

yawkat avatar Dec 13 '21 12:12 yawkat

@yawkat FWIW many frameworks implement it this way (based on order), both Rails and Grails use the order to prioritize. Also because of how inconsistently this is implemented across browsers and there being no real standard this was in fact disabled all together by default in Grails (see https://docs.grails.org/3.0.x/ref/Controllers/withFormat.html) with users having to enable explicitly

graemerocher avatar Dec 13 '21 12:12 graemerocher

Without being gated behind a config option, this would have to wait for 4.x

jameskleeh avatar Dec 13 '21 15:12 jameskleeh

yea i think it's not worth a config option. I'll let the PR sit and we can revisit it for 4.0

yawkat avatar Dec 13 '21 15:12 yawkat

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 07 '24 21:02 CLAassistant