Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No parameter support in Content-Type and Accept Header #528

Closed
awenger opened this issue Feb 11, 2014 · 10 comments
Closed

No parameter support in Content-Type and Accept Header #528

awenger opened this issue Feb 11, 2014 · 10 comments

Comments

@awenger
Copy link

awenger commented Feb 11, 2014

It's impossible to use parameterized [1] Content-Type and Accept Headers

var restify = require('restify');
var util = require('util');

var server = restify.createServer({
  formatters: {
    'application/vnd.test+json': function(req, res, body) {
      console.log('custom formatter called')
      return JSON.stringify(body);
    }
  }
});
server.pre(restify.pre.userAgentConnection());
server.use(restify.acceptParser(server.acceptable));
server.use(restify.authorizationParser());
server.use(restify.bodyParser({ mapParams: false }));
server.use(restify.queryParser());

server.get('/test/:1', function(req, res, next) {
  res.setHeader('content-type', 'application/vnd.test+json');
  res.send({a:1,b:2});
})

server.listen(5000, function() {});

curl http://localhost:5000/test/1 -H 'Accept: application/vnd.test+json;param=myparam'
Causes the following error:
{"code":"NotAcceptableError","message":"Server accepts: application/vnd.test+json,application/json,text/plain,application/octet-stream,application/javascript"}

Replacing negotiator with the current version (0.4.1) fixes this problem:
custom formatter called
{"a":1,"b":2}

Is there a possibility to set a parameterized Content-Type in the response?

server.get('/test/:1', function(req, res, next) {
  res.setHeader('content-type', 'application/vnd.test+json;param=myparam');
  res.send({a:1,b:2});
})

The Content-Type is truncated to application/vnd.test+json

[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7

@micahr
Copy link
Member

micahr commented Jun 17, 2015

Looks like we are using a more recent version of negotiator now, than when this was filed. @awenger can you confirm that this is still a problem?

@awenger
Copy link
Author

awenger commented Jun 17, 2015

This is still impossible with the recent version @micahr . However the source of the problem is different this time.

Negotiator seems to negotiate the content-type by also checking the parameter of the content type (application/vnd.test+json != Accept: application/vnd.test+json;param=myparam) [1].
Therefor I tried to add another formatter:

'application/vnd.test+json;param=myparam': function(req, res, body) {
    console.log('custom formatter called')
    return JSON.stringify(body);
}

However restify strips the param part and passes only application/vnd.test+json to Negotiator [2]. This, as descirbed, is not matched by Negotiator.

This also leads to restify listing application/vnd.test+json twice in the acceptable content-types:

{"code":"NotAcceptableError","message":"Server accepts: application/vnd.test+json,application/vnd.test+json,application/json,text/plain,application/octet-stream,application/javascript"}

[1] https://github.com/jshttp/negotiator/blob/master/lib/mediaType.js#L106
[2]

if (k.indexOf(';') !== -1) {
var tmp = k.split(/\s*;\s*/);
t = tmp[0];
if (tmp[1].indexOf('q=') !== -1) {
q = parseFloat(tmp[1].split('=')[1]);
}
}

@micahr micahr added this to the Release: 3.1.x milestone Jun 17, 2015
@micahr
Copy link
Member

micahr commented Jun 17, 2015

Thank you for confirming that this is still an issue. I'll get it on our list for the next release.

@awenger
Copy link
Author

awenger commented Jun 17, 2015

Thank you for your effort. I'm not 100% sure whether it's a good idea to enforce matching content type parameters during content negotiation. This is what the spec says about it:

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 [1]

What do you think @micahr

[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7

@micahr
Copy link
Member

micahr commented Jun 17, 2015

I think we shouldn't be using parameters when matching content-type as they are meant as additional metadata. It would be better to match against the root content-type (application/vnd.test+json in this case) and then allow the consumer to use the parameters to further determine actions needed for a given route.

@awenger
Copy link
Author

awenger commented Jun 17, 2015

Sounds good. This is also what I had in mind. But in this case I guess we have to fix it in Negotiator, or did I miss something?

@micahr
Copy link
Member

micahr commented Jun 17, 2015

I think we should strip the params before sending the content types for comparison to negotiator.

@rzio
Copy link

rzio commented Oct 10, 2015

Hi.
We're having an probably related issue:
When a request is sent with accept header which has a parameter (such as application/json;odata=none), the request hangs for a long time and returns 502.
Should we just strip the paremeters in server.pre handler, and handle it ourselves, or we're missing something, and it should work ?

Restify version 4.0.3 on Node 4.0.0

Update:
We've found why nodejs hangs when accept header has parameters:
restify code sets the response code to 406, but res.end() is never called. Since it was the last handler in the processing chain, and it called a res.send method, we have assumed that something would be sent (a response; an error; ...).
Is this a bug or something we should have implemented by ourselves ?

@retrohacker
Copy link
Member

Closing this in favor of #1219

Please correct me if I'm missing a nuance ❤️

@retrohacker
Copy link
Member

Hey @rzio, I know this was from back in 2015, but if you are still experiencing this problem please open an issue with a repro case and we will tackle it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants