Media Type: Expose more-complete, unstringified parse results

[ethanresnick]

For certain advanced negotiation cases that the library can't handle out of the box (e.g. the stuff with parameter semantics that we've discussed elsewhere), it's really useful for the user to be able to get at the parsed media types as {type, subtype, params, q} objects.

This PR adds an optional third argument, detailed that applies to .mediaTypes only when no types to match are provided. When detailed is true, the user gets back the full list of media types as the objects described above.

[ethanresnick]

@dougwilson Any thoughts on this? It would really help me out with implementing the negotiation requirements here.

Edit: I'd be happy to have the object contain a single type member, with a type/subtype string, rather than two members, to make this API line up with that of the content-type module too.

[dougwilson]

I'm thinking about it. The main concerns is that once we put it out there, it's hard to ever take it back, so I don't want to rush the decision too much.

I don't really like arguments that are just a Boolean, because if you come across the code blah(42, true), it's hard to know what true even is. Can it be changed to an options object instead (the option can still be a Boolean)?

We probably need to fix the parsing of params if we are going to expose them directly. The main issue right now is that text/html;foo=bar;q=1;fizz=buzz incorrectly returns { foo: "bar", fizz: "buzz" } (stuff after the q have a different meaning). I'd be interesting to see that fixed (probably pretty simple).

Otherwise, let me just think on it for a couple days :)

If you can, would you be willing to provide an example of what some real-world code would look like utilizing this new option? That would help me a lot as well.

[ethanresnick]

Hey Doug! Sorry it's taken me a while to get back to this :)

If you can, would you be willing to provide an example of what some real-world code would look like utilizing this new option?

Sure, I'll try. I hear you about not wanting to add to the public API prematurely. Unfortunately, though, my actual use case is distressingly complex—a function of trying to comply with a semi-arbitrary (and probably RFC2046-voliating) requirement in the JSON API spec, which is what I'm implementing, while simultaneously trying to provide a sensible content fallback instead of a 406 where possible. So, rather than go into the weeds with my use case, let me offer a related hypothetical example that'll hopefully seem plausible:

Imagine a media type (application/vnd.example.com) that uses the standard profile parameter to signal additional semantics that apply to a particular response's content (as json-ld does, for example). The media type defines five explicit profile values, all of which implementations are allowed, but not required, to support. These allowed profile values, because they each define additional semantics (per RFC 6906) can be safely used together or separately, in any combination. Therefore, the media type defines the content negotiation rule for this parameter as follows:

1. The client should include all the profile values that it supports (or is interested in) in its Accept header, to request the richest response possible.
2. And the server should return a response with a Content-Type whose profile parameter lists whatever subset of the Acceptable profile values the server knows how to support.

So, the client sends a request with Accept: application/vnd.example.com; profile=http://example.com/profiles/1 http://example.com/profiles/3 http://example.com/profiles/4. Now, the server knows how to support profiles 1 and 4, so it responds with Content-Type: application/vnd.example.com; profile=http://example.com/profiles/1 http://example.com/profiles/4.

But, to respond in this way, it needs to be able to inspect the parameters provided with the application/vnd.example.com media type in the Accept header, which is what this PR aims to do.

We probably need to fix the parsing of params if we are going to expose them directly.

Yup. I can look into fixing this if the basic value of the PR makes more sense now. We could also just not expose the parameters after the q-value since, if I remember correctly, they actually have a different semantic meaning, so it probably wouldn't make sense to lump them into the same object anyway.

I don't really like arguments that are just a Boolean... Can it be changed to an options object instead (the option can still be a Boolean)?

Updated!

[dougwilson]

(P.S. I have not read any of the text yet, but I wanted to let you know don't worry about the 0.8 failure on Travis CI right now)

[ethanresnick]

Awesome. I was gonna ask about that :) Looks like a package.json thing

[dougwilson]

Ok, I have read your response (though I'm about to head to sleep ;) ) and it makes sense to be. It seems to (generally) be yet another example of the issues in #35 and I think it, as a problem, definitely makes sense.

[ethanresnick]

@dougwilson Besides the parameter parsing issue, is this ready to merge, now that it works without provided?

And about the parsing issue...I actually couldn't reproduce it. When I set Accept: application/json;q=0.5;fizz=buzz, or do the same thing with quotes around buzz, the parsed parameter value never contains any quote characters. Am I missing something?

Regardless, maybe we shouldn't include the parameters after q in the parameters object at all? From the HTTP spec, it looks like these parameters name extensions to the Accept header that should be taken into account for each media range (analogous to the way the Cache Control header can be extended), rather than parameters of the media type. Therefore, it seems wrong to lump them in with the parameters object; in some sense, they're more analogous to the q parameter, in that their meaning would be standardized independently of any media type.

So, I think the "proper" thing to do would be to output a {type, parameters, q, extensions} object, where extensions holds all these post-q parameters. However, since I couldn't find any registered Accept header extensions, nor have I ever seen such parameters used, I think it would be fine to just output {type, parameters, q} for now, where parameters doesn't include the extensions, and then add an extensions key later in the (unlikely) event that someone needs it.

[dougwilson]

Regardless, maybe we shouldn't include the parameters after q in the parameters object at all? From the HTTP spec, it looks like these parameters name extensions to the Accept header that should be taken into account for each media range (analogous to the way the Cache Control header can be extended), rather than parameters of the media type. Therefore, it seems wrong to lump them in with the parameters object; in some sense, they're more analogous to the q parameter, in that their meaning would be standardized independently of any media type.

Yes, this is exactly what I said: if we are going to expose the parameters, we cannot start with exposing a bad object from the get-go. The stuff after the first q should not be part of the returned parameters object.

[ethanresnick]

Ahh, ok, my mistake. I think your original post had a missing quotation mark, and we've had issues with quotation mark parsing in the past, so I just assumed that was still the problem (rather than the semantic one).

Anyway, I've updated the PR now to parse the extensions into a separate object, which I decided to expose after all (unless there's a reason not to?). Let me know what you think :)

[dougwilson]

Awesome, will do :) I plan to look it over tomorrow, and a quick glance right now doesn't reveal any real issues :)

[ethanresnick]

@dougwilson Your ping on expressjs/compression#44 reminded me of this issue too. Let me know if you have a chance to look this over soon :)

[dougwilson]

Hi @ethanresnick , I'm sorry I let this sit so long; I'm working on integrating this (and for it to be a uniform option across all methods) currently. I plan to have this (or something similar) released in the next minor. You should be able to drop your fork for the module directly in https://github.com/ethanresnick/json-api/blob/4f710eb0dc8e8a7fcdc2a698c1a00d161cd6ba0f/src/steps/http/content-negotiation/negotiate-content-type.js with the next minor, which is my goal.

[dougwilson]

Well, looks like there are still a lot of edge-cases that need to be worked out regarding this interface. For example, I'm wondering about the following cases, if you can provide your input, @ethanresnick :

1. Given the header Accept: text/html; level=1, text/html; level=2, what should negotiator.mediaTypes(['text/html'], {detailed: true}) return?
2. Given the header Accept: text/*; level=1, what should negotiator.mediaTypes(['text/html'], {detailed: true}) return?
   1. Should the subtype be * or html in the returned object?
   2. How would this be different if the call was negotiator.mediaTypes(['text/html', 'text/plain'], {detailed: true})?

[ethanresnick]

Hey Doug, sorry it's taken me a while on this! To your cases...

Given the header Accept: text/html; level=1, text/html; level=2, what should negotiator.mediaTypes(['text/html'], {detailed: true}) return?

negotiator.mediaTypes(['text/html']) currently returns an empty array for that Accept (test), and I'd imagine that {detailed: true} wouldn't affect that. Maybe the current behavior of returning an empty array is problematic (that's the debate in #35), but I think that's independent of the detailed option. The detailed option is just meant to indicate how to format the matches, so if there aren't any matches, it doesn't have any effect. (This is already tested here.)

Given the header Accept: text/*; level=1, what should negotiator.mediaTypes(['text/html'], {detailed: true}) return?

Here also I think negotiator.mediaTypes(['text/html']) currently returns an empty array, so adding detailed wouldn't change that. Were negotiator.mediaTypes(['text/html']) to start returning some media types in this case, the subtype in the detailed version would match whatever subtype we decide to return in the non-detailed version, which would presumably be html or plain, rather than *.

Because of these issues with parameter matching on the negotiatior.mediaTypes(array) form, I imagine the detailed option will primarily be used with negotiatior.mediaTypes(undefined, {detailed: true}). For consistency, I think it's nice to allow the detailed option when there are provided types in the first argument, but I don't think it should affect the behavior beyond changing the result formatting.

[dougwilson]

It's no problem! And my bad, I guess I was not thinking straight when I wrote that :) Would you mind adding more tests to this PR, specifically tests for negotiator.mediaType with detailed true, which are missing?

[ethanresnick]

@dougwilson Done! I've also rebased everything so that it's much easier to follow :)

[ethanresnick]

@dougwilson Any update on this?

[ethanresnick]

@dougwilson Any chance we can finally get this merged?

[acg]

We ran into #35 with an Accept: application/json; version=1 header and we're looking forward to the fix here!