Use parameters declared in consumes or produces condition to narrow the request mapping [SPR-17133]

[spring-projects-issues]

Jose Montoya opened SPR-17133 and commented

At Rossen Stoyanchev's request I'm creating this ticket with the intention to put forward a cleaner description of use cases to maybe facilitate discussion. This issue that has been argued before in #17949 and #15531 and sprung an atom specific one #21578. There's also a StackOverflow question related to this topic here.

As shown by RFC 7231 Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content Section 5.3.2 Accept, media type parameters are to be included in the content negotiation process. Even if perhaps not called out explicitly in that section it is unarguably most evident that parameters participate in the process from the example in Page 39, where text/plain;format=flowed is considered independently from text/plain. If we have consensus with that claim then we can move to some use cases. Following the logic from the RFC, but without adding quality attributes at this point:

Setup:

For a given resource the server explicitly Produces type/sub; param1=foo only, which is implicitly included in type/sub and type/*

Use case 1, sub-type wildcard:

The client Accepts type/*
The server should return the content as identified by type/sub; param1=foo, as it's the most specific one included.

Use case 2, specific sub-type:

The client Accepts type/sub
Same as UC-1.

Use case 3, specific param match:

The client Accepts type/sub; param1=foo
The server should return the content as identified by type/sub; param1=foo, as it's an exact match.

Use case 4, specific param no match:

The client Accepts type/sub; param1=bar
The server should return a 406 (Not Acceptable), the param1 values are not equal.

Use case 5, multiple specific param no match:

The client Accepts type/sub; param1=foo; param2=bar
The server should return a 406 (Not Acceptable), the param1 vales are equal, but param2 means the client only accepts something more specific than what the server produces. What the client wants does not include what the server produces.

First issue:

In spring-core/MimeType there are two methods for comparing media types isCompatibleWith and includes, none of them take into consideration parameters. UC-4 and UC-5 currently fail by returning 200 (OK) with the content of type/sub; param1=foo but headers Content-Type=type/sub; param1=bar and Content-Type=type/sub; param1=foo; param2=bar, respectively. This is obviously erroneous behavior.

Fix:

MimeType.isCompatibleWith should be enhanced to consider parameters as follows: "Parameters are incompatible only when they contain the same parameter with different values." MimeType.includes should be enhanced to consider parameters as follows: "Parameters are not included when this MimeType contains more parameters than the supplied, when this contains a parameter that the supplied does not, or when they both contain the same parameter with different values." This would not be inconsistent with both methods' current implementation.

Second issue

spring-webmvc/ProducesRequestCondition's matchMediaType method utilizes isCompatibleWith which is a symmetric comparison, ie. even if we introduced parameters into its logic, UC-5 would fail by returning 200 (OK) with the content of type/sub; param1=foo but with header Content-Type=type/sub; param1=foo; param2=bar. This should be an asymmetric comparison because even though two media types may be mutually compatible the client may receive something that is not included by what it is explicitly requesting.

Fix:

ProducesRequestCondition.matchMediaType should be modified to use the enhanced includes instead of isCompatibleWith.

---

Affects: 5.1 RC1

Reference URL: #15531

Issue Links:

• Support for the "type" parameter for Atom Feed/Entry message conversion [SPR-17040] #21578 Support for the "type" parameter for Atom Feed/Entry message conversion

Referenced from: pull request #1920

1 votes, 3 watchers

[spring-projects-issues]

Rossen Stoyanchev commented

I see the same issue with this proposal as with the other tickets that were closed. It treats all parameters as equally important which may not be the case. To quote again from RFC 7231 3.1.1.1:

The presence or absence of a parameter might be significant to the processing of a media-type, depending on its definition within the media type registry.

So if a client sends "Accept:text/plain;charset=UTF-8", under this proposal, it would fail to map to a controller declared with produces "text/plain", but the converter is perfectly capable of detecting and using the charset.

It also means you're forced to declare a parameter in order to handle a media type with it. In other words there is no way for a controller declare that it can produce "type/sub" for any value of param1.

The switch for ProducesRequestCondition to use includes from isCompatibleWith also doesn't seem right. That symmetry is deliberate. If a client sends "text/*" it should match a controller declared with "text/plain" and vice versa.

[spring-projects-issues]

Jose Montoya commented

Hey Rossen, thanks for the feedback.

1. Charset. You're right, perhaps we'd include logic special to charset parameter?. Though I'd argue the this automatic handling is more of a very nice-to-have than strict conformance, but one that I'd definitely like to not mess with.
2. "type/sub" for any value of param1. I understand that in this scenario the "presence or absence" of "param1" is not significant. I hadn't thought of this scenario in that particular way, but I did consider a similar one where let's say that there is a param1 value that acts as a wildcard or super type, that would be unsupported by this approach. That is by (opinionated) design.
3. isCompatibleWith vs includes. The problem here is that in the case that the "presence" is significant isCompatibleWith would approve "text/plain" when a client requested "text/plain;format=flowed" and on top of that would return it with a incorrect Content-Type header. Out of everything in this ticket I think that particular bug is the most straight forward one.

I did considered a different approach, to somehow allow the developer to implement and pass their own parameter matching. If the significance of parameters is dependent on the media type definition then there's no other option but to accommodate different matching logic by media type. I suppose that would be the most correct approach.

Perhaps we can make this PR the default implementation yet also allow developers to implement their own if it doesn't suit their purposes. ie. "By default all parameters are equally important, though this behavior can be overriden by providing...." or something along those lines. On the other hand, in order to minimize backwards incompatibility we can leave the current implementation as default, offer this PR logic as an optional configuration and lastly allow devs to include their own ie. "By default parameters are not included in content negotiation process, but if you do the following ... then the process will include the parameters in a all-params-are-significant manner, or you can override all behavior by providing ..."

[spring-projects-issues]

Rossen Stoyanchev commented

Charset is just one example. Special logic does not address the general point.

For the isCompatible vs includes check, I gave a fundamental example of why that is the way it is, and why it can't just change from one to the other. Conceptually, isCompatible is what we're looking to do here.

To summarize, if a requested media type has a parameter we are not in any position to make conclusions, or to insist that mappings also have it. That is not a viable path. At best I can imagine that if a @RequestMapping is declared to produce a media type with a parameter such as "foo=bar", then only match to media types that also have "foo=bar".

[spring-projects-issues]

Jose Montoya commented

Again, thank you for your feedback.

I definitely don't have half as much insight as you do into the framework, but I'm hoping we can work out a solution. Take a media type like json+ld for example, if the server only produces "app/ld+json" and the client requests "app/ld+json;profile=expanded" then the matching process should fail. There's 2 issues with what would happen currently: 1. The matching will succeed and send the contents of "app/ld+json" which is incorrect because client does not accept that. 2. It will return "Content-Type=app/ld+json;profile=extended" which is incorrect because it's misidentifying the contents.

If we agree that this might not be the correct behavior for ALL media types, and forgetting the PR, what changes could accommodate those media types for which it would be the correct behavior? Off the top of my head media types app/ld+json, app/hal+json, and app/atom+xml should behave that way.

[spring-projects-issues]

Rossen Stoyanchev commented

Good to have concrete examples! I'm not familiar with JSON-LD but the profile parameter sounds like a preference rather than a different representation. I'd expect one HttpMessageConverter in support of JSON-LD which would check for the profile parameter and do the right thing. This works just like the charset parameter actually.

The media type definition confirms it's the "same representation" and  "preference":

A profile does not change the semantics of the resource representation when processed without profile knowledge, so that clients both with and without knowledge of a profiled resource can safely use the same representation. The profile parameter may be used by clients to express their preferences in the content negotiation process. If the profile parameter is given, a server should return a document that honors the profiles in the list which are recognized by the server.

Connecting to the summary from my last comment:

• Narrowing the mapping based on parameters in the requested media type is counter intuitive and doesn't make sense because, a) the controller probably doesn't return or do anything different, and b) those parameters are supported by the underlying HttpMessageConverter.
• Using parameters in the declared producible media type to narrow the mapping however makes more sense, because it allows the controller to express what is important.

For example @GetMapping(produces="application/ld+json") should map to any request for "applicaiton/json+ld" regardless of parameters. However when the controller method declares @GetMapping(produces="application/ld+json;profile=extended") then it's expressing a preference to match only to media types that have that parameter and value. This makes sense intuitively, i.e. why else would parameters be in the request mappings if not to narrow the mapping?

[spring-projects-issues]

Jose Montoya commented

Ah, I think we're starting to achieve some understanding! I'll try to propose more concrete examples:

On profile, the definition says nothing about the representation being the same, it's more about not breaking semantics. Moreover, it's explicit that

The profile parameter may be used by clients to express their preferences in the content negotiation process.

which would only be the case if the parameter may indeed vary the content.

 

A ld+json example, a person from json-ld/playground

{
  "@context": "http://schema.org/",
  "@type": "Person",
  "name": "Jane Doe"
}

and here's the same person as ld+json;profile=expanded

[
  {
    "@type": [
      "http://schema.org/Person"
    ],
    "http://schema.org/name": [
      {
        "@value": "Jane Doe"
      }
    ]
  }
]

here it is as ld+json;profile=flattened

{
  "@context": "http://schema.org/",
  "@graph": [
    {
      "id": "_:b0",
      "type": "Person",
      "name": "Jane Doe"
    }
  ]
}

Here, if the server only produces ld+json and does not support flattened, a request for flattened should fail. However in the -opposite- scenario where it does support flattened but the client requests just ld+json the matching should succeed (I will re-confirm this with someone more in tune with ld.) In an implementation the server should then probably set the mapping to produce both media types imo.

An example with text/plain;format=flowed.

flowed rules from RFC 3676 on text/plain parameters:

This rules how text/plain;format=flowed would be generated vs text/plain

A [flowed] generating agent SHOULD:

o Ensure all lines (fixed and flowed) are 78 characters or fewer in length, counting any trailing space as well as a space added as stuffing, but not counting the CRLF, unless a word by itself exceeds 78 characters.
o Trim spaces before user-inserted hard line breaks.

A generating agent MUST:
o Space-stuff lines which start with a space, "From ", or ">".
 

 Here, if the server only produces text/plain;format=flowed and the client requests text/plain it should succeed, but not the other way around.

 

An example with app/atom+xml, server produces app/atom+xml;type=entry.

A request for app/atom+xml;type=entry should succeed.

A request for app/atom+xml;type=feed should fail.

A request for app/atom+xml should succeed. In an implementation the server should then probably set the mapping to produce both media types imo.

 

---

For those scenarios the symmetric comparison is inappropriate I think but still, let's forget the PR for now. Those are three different parameters from three different media types which change the content of the representation. We can add app/hal+json and app/phtal+xml (one that I'm working on).

What do you think? How would we accommodate those?

Now, what's to stop atom for example, to add any arbitrary negotiation rules to type? Let's say they add a better-entry value, and that requests for entry could return entry or better-entry. Any media type could have any matching rules for parameters.

[spring-projects-issues]

Rossen Stoyanchev commented

You're suggesting that any parameters in the requested media type should not match to controller methods that do not declare the same parameters. That is way too strict and doesn't really add up. Let's use those examples.

1. Looking at the Java JSON-LD library, a controller method would return the same object (e.g. Person) and then the HttpMessageConverter would call JsonLdProcessor.compact(..) or expand(..) or flatten(..). So I need just one controller method declared to produce "application/ld+json", and the converter would transparently do the right thing based on the profile flag, if present. However based on your suggestion I'd be forced to have 3, otherwise identical, controller methods (each returning Person). The only difference would be in the declared producible media type.

2. In addition to the format parameter, RFC 3676 also defines the DelSp parameter that should be present only for format=flowed, and have values of Yes or No. Based on your suggestion, if I'd be forced to declare one controller method producing each of the following combinations:

text/plain;format=flowed
text/plain;format=flowed;DelSp=Yes
text/plain;format=flowed;DelSp=No

I don't know much about how such text is typically stored, i.e. whether the "flowed" formatting is applied on the fly, but logically DelSp is a sub-option of the "flowed" format, and most likely not something for which I'd want to declare separate controller methods. It's not even clear where "flowed" vs "Fixed" would be handled in different controller methods.

3. Atom is the only example mentioned here where separate controller methods are needed, one to return Feed and the other Entry. Perhaps Mark Hobson can confirm if that's what it looks like in an actual implementation, judging from his comments under Content negotiation ignores media type parameters [SPR-10903] #15531 he has such a deployment.

So we have a mix of examples here. For some, it's a single controller method that returns the same data, with the message converter transparently handling media types parameters. For others, it's multiple controller methods returning different data based on a media type parameter value, with the message converter simply rendering whatever object it has been given.

The Spring framework has no knowledge of which parameters fall the first category that I would call "not significant" for request mapping purposes, and which fall in the second category which I would call "significant" and that should be used to narrow the request mappings. So it can't selectively choose from the parameters in the requested media type. At the same time forcing controllers to declare any combination of potential parameters and values makes zero sense too.

Instead I'm proposing for controller methods to declare what parameters they want to match on, and which should be used to narrow request mappings. This should work fine for the Atom case I think where a controller would have to declare two methods, one "application/atom+xml;type=feed" and another for "application/atom+xml;type=entry" and the framework would match to those methods only if the requested media type matches the parameter, but perhaps Mark Hobson can confirm that.

For those scenarios the symmetric comparison is inappropriate I think

It has to be symmetric for type and sub-type. The check for parameters would be applied in addition, where any parameters on the declared, producible type would have to be present on the requested media type.

[spring-projects-issues]

Jose Montoya commented

Hmm OK, I do think what you're proposing is a great start, maybe I'm just not familiar enough with HTTPMessageConverter, I'll dig into that. How would you envision the following use case working:

Let's say we have a resource offered as app/hal+xml
If the client requests app/hal+xml it should succeed.
If the client requests app/hal+xml;profile=shopping it should fail with 406.

Let's say we have a resource offered as app/hal+xml as well as app/hal+xml;profile=shopping
If the client requests app/hal+xml it should succeed.
If the client requests app/hal+xml;profile=shopping it should succeed.
If the client requests app/hal+xml;profile=amz-shopping it should fail with 406.
If the client requests app/hal+xml;profile=shopping;version=2 it should fail with 406.

HAL does not currently define a version parameter, but it's one that has been proposed frequently when discussing restful media types. Profile is something that would be defined by different applications, an unbound number of profiles could exist.

Is this still something that the MessageConverter could/should handle?

[spring-projects-issues]

Rossen Stoyanchev commented

Generally speaking, yes an HttpMessageConverter is given an Object (the return value from the controller) and a MediaType. It can examine the parameters and raise HttpMediaTypeNotAcceptableException which would be translated to a 406 response.

Does the controller return the same or different Object type for app/hal+xml vs app/hal+xml;profile=shopping? If it is the same, then it's one controller method mapped to app/hal+xml, and the converter handles the profile parameter transparently. If it is different Objects then two controller methods, one mapped to app/hal+xml and another to app/hal+xml;profile=shopping, and it would be up to the converter to reject a "profile=amz-shopping".

[spring-projects-issues]

Jose Montoya commented

Oh I see! However HttpMediaTypeException is not part of the throws signature for read or write. Those methods are where you're suggesting the parameter examination should happen, or did I misunderstand?

[spring-projects-issues]

Rossen Stoyanchev commented

Again it would help to know if the Controller is expected to have two controller methods or not? Even if it is different controller methods, I think it would be up to the converter to reject profile values that are not supported and therefore cannot be rendered.

In terms of mappings, "app/hal+xml" expresses a broad match by type and sub-type, i.e. not narrowed by profile parameter, if that makes sense -- e.g. the same controller method handles any request for the given type + sub-type regardless of parameters, or otherwise each controller method would have to narrow its mapping by profile.

[spring-projects-issues]

Rossen Stoyanchev commented

We'll try and experiment for 5.2 with support for parameters explicitly declared in the @RequestMapping. This direction makes sense because it allows the controller method to express which parameters are important for request mapping. It's something the framework does not know. 

Once an implementation is available in snapshots, it will be possible to experiment with the specific use cases mentioned here, or any others, in order to validate the approach. 

[spring-projects-issues]

Jose Montoya commented

I imagine it should be one controller method, just as we can do

method=get
value=/pet
produces=json, xml 

I thought we should be able to do

method=get
value=/pet
produces=hal+xml;profile=foo,hal+xml;profile=bar 

because I consider them to be logically different media types. But either way, your suggestion to include only the parameters explicitly declared would suffice for this use case. For more complicated matching use cases the ability to throw a HttpMediaTypeException from the converter would be great as well.

[spring-projects-issues]

Rossen Stoyanchev commented

A mapping like below, with the profile parameter declared, should work based on the proposal:

produces = {"hal+xml;profile=foo", "hal+xml;profile=bar"}

It will not match to the controller method. Then RequestMappingInfoHandlerMapping#handleNoMatch should kick in and produce a 406.

[spring-projects-issues]

Mark Hobson commented

I've commented on #15531 regarding the issues I've had with honouring Atom media type parameters in content negotiation. As promised, here's some example code showing my use-cases and my current workarounds:

https://github.com/markhobson/spring-atom-issue

I commented the code accordingly but let me know if you have any questions. It feels like the approach emerging here and on #21578 is a step in the right direction though.

[jam01]

Hey @rstoyanchev - can we possibly give this another look?

I've recently come back to an use case where this'd be incredibly useful. I think maybe we can consider something like my original PR https://github.com/spring-projects/spring-framework/pull/1920/files but done in a way that allows users to provide their own parameter negotiation logic like @markhobson showed in a previous comment. Maybe through an SPI loader when parsing media type strings...?