Closed FredrikFolkesson closed 6 years ago
Hi @FredrikFolkesson thanks for the report! I'm not quite following here. Would you be able to do one of the following? (a) make a pull request with the change needed to make in the accepts
module or (b) provide a PoC app that demonstrates the issue and I can dig in. Thanks!
Hi,
This "app" shows the error
const accepts = require('accepts');
const req = {}
req.headers = {}
req.headers.accept = "application/json; charset=utf-8"
const accept = accepts(req);
console.log(accept.types()) //gives back the list of accepted types correctly. ([ 'application/json' ])
console.log(accept.type(['application/json'])) // gives false instead of giving back the type 'application/json'
Ah, thanks. Yes, this is working as expected and according to the RFC. In your example, the client doesn't accept application/json
, it only accepts application/json
that has the charset
of utf-8
. Your server spec doesn't support that, so it's not chosen. This is defined by RFC 7231.
The English version of your code is the client says:
"I only accept a response in the representation of application/json; charset=utf-8"
And the server knows:
"I can produce a response in the representation application/json"
The reason false
is returned is because application/json
is not a match against application/json; charset=utf-8
. What charset would that application/json
be in? The negoiator has no idea, so doesn't match it up. If you know you're going to output in the charset utf-8
, then that's what you need to specify in the type
arguments:
const accepts = require('accepts');
const req = {}
req.headers = {}
req.headers.accept = "application/json"
const accept = accepts({ headers: { accept: 'application/json; charset=utf-8' }});
// => application/json;charset=utf-8
console.log(accept.type(['application/json;charset=utf-8']))
const accept2 = accepts({ headers: { accept: 'application/json' }});
// => application/json;charset=utf-8
console.log(accept2.type(['application/json;charset=utf-8']))
@dougwilson But then the documentation is wrong?
https://github.com/jshttp/accepts#types
Return the types that the request accepts, in the order of the client's preference (most preferred first).
And accept.types()
gives back application/json
It could probably be a bug with the getting the list operation.
Sorry to necromance, but stumbled upon this and don't know whether this has changed in a higher express version:
I solved this by calling negotiator directly
Node repl
var Negotiator = require('negotiator')
new Negotiator({ headers: { accept: 'application/json; charset=utf-8' }}).mediaTypes()
> [ 'application/json' ]
This would be compliant with what I would expect from accepts, but could not find that behaviour to be working.
Unless there is no specific support for this wanted I believe it to be a subtle issue in /index.js in the sequence around the filtering and gonna attempt an investigation. No expert on RFCs, but looking at comparable libs I think it's rather wanted to parse out the charset and seems accidental(?). In any case thanks for all the efforts
@dougwilson obviously you are right to mention the RFC, however I think the mentioned section is a bit confusing and this one would take precedence no? https://tools.ietf.org/html/rfc7231#section-3.1.1.1 that section mentions the wanted extensibility so an aggressive filter might not appropriate(?)
Digging deeper I am pretty sure this is unwanted. The error arises from the Negotiator interface providing a priority prop, that compares parsed mediaTypes with the header, but then also then maps only those priorities around here priorities https://github.com/jshttp/negotiator/blob/master/lib/mediaType.js#L162-L182
Originated here https://github.com/jshttp/accepts/blob/master/index.js#L105
Hi @eljefedelrodeodeljefe , I'm sorry you are having issue. Unfortunately I am not really following along here. If you are having the same issue as the OP (I assume so, because this is the same thread), I think what I said at https://github.com/jshttp/accepts/issues/17#issuecomment-393961360 still stands. It may help if maybe you can take that statement I made there are make adjustments to what it should be and provide references to the RFC to support the changes you are asking for.
Thanks for replying @dougwilson, also no follow up needed, but as an explanation: Not having a hard issue, as I workaround it anyhow and I know Fastify does too https://github.com/fastify/fastify-accepts. The original problem rather arises the caller of req.accepts('application/json')
to be wanting to accept any kind of charset, setting the charset in Content-Type, but they get returned false
if the header is application/json; charset=utf-8
.
Reading the RFC for my taste it should allow that. But again, no strong feelings.
A header like this
application/json; charset=utf-8
Works correctly with
accept.types()
=>[ 'application/json' ])
But
accept.type(['text', 'application/json']
oraccept.type(['text', 'json']
givesfalse
It seems to be a problem here: https://github.com/jshttp/negotiator/blob/master/lib/mediaType.js#L141
Since we then compare the keys and our accepted type
application/json
does not have the charset key.