Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Match accept type exactly #1297

Closed
mstade opened this Issue Aug 20, 2012 · 22 comments

Comments

Projects
None yet
6 participants

mstade commented Aug 20, 2012

I'm probably not quite understanding how to use request.accepts() so any input here would be awesome. Basically, I'm trying to write some middleware that only deals with requests where the accept type matches something exactly. Looking at the docs, it seems request.accepts() isn't quite useful for this purpose, and I end up having to do this:

var acceptsType = (req.accepted || []).some(function(type) {
    return type.value === "my/type"
})

It's not a huge deal, but if I don't do that, things like */* seems to match, and browsers likes sending that it seems.

If request.accepts() already does this somehow, I'd love to know. If not, I suggest changing the logic of request.accepts() so that it returns the actual best match, meaning for a request that has Accept: */* it'd actually return */* if that is indeed the best match, rather than the input mime type. Thus, if you then want to do an exact match, the code would look something like:

if (req.accepts("application/json") == "application/json") {
    // Exact match
}

For cases when you just want to know if the client accepts the mime type, but isn't necessarily an exact match, just drop the quality check:

if (req.accepts("application/json")) {
    // Returns */* or whatever the match is, but truthy value anyhow
}

I'm probably not understanding the accepts function correctly, but it seems to me it's pretty much useless whenever a client sends Accept: */* and you have to resort to looking through the accepts array. Please do correct me if I'm wrong!

Owner

tj commented Aug 20, 2012

yeah there is a debate going on about this in one of the other issues (or a commit, i cant remember). Technically if a browser or client gives you "/" then it's really willing to accept anything, in which case Express is doing the correct thing. One thing we could maybe do since there seems to be a consensus that people want Express to do the wrong thing here, maybe we can pass a flag in which case Express would ignore "/"

mstade commented Aug 20, 2012

What I'm suggesting isn't that express do the wrong thing, just that it be a little bit clearer about what the client actually said it accepts. I think it's incorrect in saying that application/json is the best match to */*, because in that case */* is the best match. It's also a more useful return value, since then I can look at the return value and make a clever choice as to what to do next (i.e. is it an exact match or a wildcard?)

It's not lying, it still returns a truthy value to the question. Think of it like a conversation:

me: Does the client accept application/json?
express: Yes, because the client said it's fine with */* which matches application/json.

Rather than:

me: Does the client accept application/json?
express: Yes, the client accepts application/json.

See, infinitely more useful! :)

Owner

tj commented Aug 20, 2012

well partial matches are still matches, image/* etc

Owner

tj commented Aug 20, 2012

it already has a return value though, which is used internally for res.format()

mstade commented Aug 21, 2012

I think I'm being confusing, my apologies. I'm saying (I think) that partial matches are definitely matches, but that the return value from accept() is currently not very interesting. This because all it tells me is that the input is matched by the request's list of accepted types, but it doesn't actually tell me why. If it would return the best match from the request (image/* or */* or whatever) then not only will I know it was a match, I can also determine if it was partial by the use of a simple equality check.

Granted, the need for checking for exact matches is probably less than the need for checking for any match, but I don't think it's unwarranted. Having middleware that deals with one set of media types but calls next() for anything else, for instance.

Owner

tj commented Aug 21, 2012

gotcha, yeah we may be able to alter the return value a bit. currently it's returning the match because for example you can do req.accepts('html, json') in which case it might return "json". res.format() uses this to determine which callback to invoke. I think that's still a useful return value but perhaps we can return an object or maybe a different method to see which pattern matches

Owner

dougwilson commented Oct 9, 2012

It sounds like @mstade would be interested in a method that would return all the media types as sent in Accept but order them by their precedence, possibly with the one at [0] being the most wanted.

req.accepts(); // Perhaps no args returns an ordered list as-is?
// Accept: text/*, text/html, text/html;level=1, */*
// -> ['text/html;level=1', 'text/html', 'text/*', '*/*']

// Accept: text/*;q=0.3, text/html;q=0.7, text/html;level=1, text/html;level=2;q=0.4, */*;q=0.5
// -> ['text/html;level=1', 'text/html', '*/*', 'text/html;level=2', 'text/*']

Perhaps something else they are looking for is a mime => quality mapping.

req.acceptsMap(); // ??
// Accept: text/*, text/html, text/html;level=1, */*
// -> {'text/html;level=1': 1, 'text/html': 1, 'text/*': 1, '*/*': 1}

// Accept: text/*;q=0.3, text/html;q=0.7, text/html;level=1, text/html;level=2;q=0.4, */*;q=0.5
// -> {'text/html;level=1': 1, 'text/html': 0.7, '*/*': 0.5, 'text/html;level=2': 0.4, 'text/*': 0.3]
Owner

dougwilson commented Oct 9, 2012

Ah, didn't know that. i.m.o req.accepted contains all the information possible, then, so that should solve the issues I've seen where people are talking about res.accepts(). If they wanted an ordered list from highest to lowest:

req.accepted.sort(function (a, b) { return b.quality - a.quality; }).map(function (a) { return a.value; });
Owner

tj commented Oct 9, 2012

req.accepted is already sorted for you, but yeah it could be something more convenient built on top. To me */* is fine but I certainly can understand that since everything fires */* at you that it's maybe not ideal

Member

jonathanong commented Sep 10, 2013

this issue seems to be solved, and express is doing the correct thing (hopefully). closing! you may also be interested in https://github.com/federomero/negotiator.

EDIT: Lots of good information below. Take my comments with a grain of salt.

I'd really like to see this issue re-opened.

Here are the accepts headers for two requests in Chrome.

  • Ajax request via JQuery, expliciting setting Accept Header to application/json
    • Accept:application/json, text/javascript, */*; q=0.01
  • Fresh page request
    • Accept:text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8

Based on the current implementation of req.accepts, there's no way I can implement a smart route that correctly sends JSON when I want to, but HTML the rest of the time (assuming it's either */* or text/html in the HTML case).

I could see a couple ways to make this more flexible for devs:

  1. Allow an optional quality argument to req.accepts.
    • req.accepts('json', 0.5)
  2. Return the accepts header from req.accepts so that you can decide for yourself
if (req.accepts('json') && req.accepts('json').quality > 0.5 && req.accepts('json').type != '*') { 
  // handle for json case
}

Thoughts?

I should have said:

There's no way I can implement a smart route without doing this logic myself. I.e. parse the req.accepted array and decide for myself. This seems like a pretty straightforward and basic use case though, so I'm inclined to believe it should behave as above for all parties without having to write accepted parsing logic.

Contributor

defunctzombie commented Feb 28, 2014

I think what you want is the array form of the accepts api:

http://expressjs.com/api.html#req.accepts

req.accepts(['html', 'json']);

Which will return the one with the highest quality from an array you specify. This way you can say, I will be handling these two so tell me which one wins in quality.

Thanks @defunctzombie, I see that this works, it still feels sloppy however:

if (req.accepts('html','json') === 'html') {
  // send html here
}
else if (req.accepts('html','json') === 'json') {
 // jsonify me
}
Contributor

defunctzombie commented Feb 28, 2014

Depending on your default this could be simpler with less repeating

var accepted = req.accepts(['html', 'json']);
if (accepted === 'html') {
    ///
    return;
}

// default is json

Yes, of course I could clean that up, it was more making the comment that I have to put in all of the possible header matches into the array. I guess this is just the tax of every fucking client on earth appearing to send */*.

Contributor

defunctzombie commented Feb 28, 2014

Yea, everything sends */*. In reality most apps either respond with html or json so it isn't gonna be that much to handle. In my apps I actually split API servers/routes from HTML ones so don't even respect anything the browser requests and respond according to the function of the route/subsystem typically.

Member

jonathanong commented Mar 1, 2014

Not sure how that's sloppy.accepts() is how content negotiation is supposed to work. Your method is much sloppier (unless I'm missing something) and does not honor the client's headers

It's a function of the fact that basically every browser sends *.* with all of the requests. That's all I meant by sloppy.

Owner

dougwilson commented Mar 1, 2014

FYI @kenperkins jQuery is adding the */* when you use it (see https://github.com/jquery/jquery/blob/0d68b7877f761264bfe4950e4df156b854925a6b/src/ajax.js#L597). If you invoked XHR yourself, you can have it not include */* all you want.

Really appreciate that legwork @dougwilson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment