Updated body parser to handle extended content types #1663
Conversation
lib/plugins/bodyParser.js
Outdated
| case 'application/json': | ||
| var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json'); | ||
|
|
||
| switch (true) { |
There was a problem hiding this comment.
Switching on a constant is kind of uncommon - I think I would prefer we do something like map any +json type into application/json (if we find a match) and keep the existing switch statement as is.
lib/plugins/bodyParser.js
Outdated
| var type = req.contentType().toLowerCase(); | ||
| //var type = req.contentType().toLowerCase(); | ||
| var type = req | ||
| .contentType() |
There was a problem hiding this comment.
req.contentType() should already support handling ;, did you find out otherwise?
re: https://github.com/restify/node-restify/blob/master/lib/request.js#L210
| @@ -28,8 +28,13 @@ function jsonBodyParser(options) { | |||
| // save original body on req.rawBody and req._body | |||
| req.rawBody = req._body = req.body; | |||
|
|
|||
| if (req.getContentType() !== 'application/json' || !req.body) { | |||
| return next(); | |||
| var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json'); | |||
There was a problem hiding this comment.
Since we're already doing this match here before we call into it jsonBodyParser, we can remove this content type check entirely.
test/plugins/jsonBodyParser.test.js
Outdated
| @@ -490,3 +490,947 @@ describe('JSON body parser', function() { | |||
| client.end(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('JSON body parser with content type application/hal+json', function() { | |||
There was a problem hiding this comment.
Wowza, this is a huge test! Thanks for adding all of these but I don't think we need to duplicate all of the existing tests. I'd just add a test or two to make sure that */*+json gets matched as application/json.
|
Hello @DonutEspresso, the changes have been made |
|
Thanks @ifiok ! I just realized I'm dumb and that jsonBodyParser can be used independent of bodyParser - I think that is why you added the regex test in jsonBodyParser directly too. :( Sorry about that! Do you mind re-adding that check into jsonBodyParser? Once that's in I think we're ready to roll. Otherwise, this looks great! |
… it can used independently
|
@DonutEspresso I have added the check back to the jsonBodyParser |
|
Thanks @ifiok! Node 10 seems to be failing CI but that seems to be a problem on master. We can follow up on that elsewhere. Thanks for the contrib! 🎉 |
| @@ -28,8 +28,13 @@ function jsonBodyParser(options) { | |||
| // save original body on req.rawBody and req._body | |||
| req.rawBody = req._body = req.body; | |||
|
|
|||
| if (req.getContentType() !== 'application/json' || !req.body) { | |||
| return next(); | |||
| var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json'); | |||
There was a problem hiding this comment.
This Regexp is used in two different places. Perhaps make it a const and share?
Issues
Closes:
Changes
Updates the validator check for content types to allow for extended types such as outlined in:
https://www.iana.org/assignments/media-types/media-types.xhtml.
It also covers for types such as:
'application/json; charset=utf-8'