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
Updated body parser to handle extended content types #1663
Changes from 4 commits
ce7bc65
1ac6008
18ed8bd
560b2d7
6ede98e
ef5a15e
3b90f9e
c302a12
f3517f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,25 +160,34 @@ function bodyParser(options) { | |
} | ||
|
||
var parser; | ||
var type = req.contentType().toLowerCase(); | ||
//var type = req.contentType().toLowerCase(); | ||
var type = req | ||
.contentType() | ||
.toLowerCase() | ||
.split(';')[0]; | ||
|
||
switch (type) { | ||
case 'application/json': | ||
var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json'); | ||
|
||
switch (true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching on a constant is kind of uncommon - I think I would prefer we do something like map any |
||
case jsonPatternMatcher.test(type): | ||
parser = parseJson[0]; | ||
break; | ||
case type === 'application/json': | ||
parser = parseJson[0]; | ||
break; | ||
case 'application/x-www-form-urlencoded': | ||
case type === 'application/x-www-form-urlencoded': | ||
parser = parseForm[0]; | ||
break; | ||
case 'multipart/form-data': | ||
case type === 'multipart/form-data': | ||
parser = parseMultipart; | ||
break; | ||
case 'text/tsv': | ||
case type === 'text/tsv': | ||
parser = parseFieldedText; | ||
break; | ||
case 'text/tab-separated-values': | ||
case type === 'text/tab-separated-values': | ||
parser = parseFieldedText; | ||
break; | ||
case 'text/csv': | ||
case type === 'text/csv': | ||
parser = parseFieldedText; | ||
break; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're already doing this match here before we call into it jsonBodyParser, we can remove this content type check entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Regexp is used in two different places. Perhaps make it a |
||
var contentType = req.getContentType().split(';')[0]; | ||
|
||
if (contentType !== 'application/json' || !req.body) { | ||
if (!jsonPatternMatcher.test(contentType)) { | ||
return next(); | ||
} | ||
} | ||
|
||
var params; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
req.contentType()
should already support handling;
, did you find out otherwise?re: https://github.com/restify/node-restify/blob/master/lib/request.js#L210