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

Added support for external parsers to bodyParser.json() #281

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,21 @@ specifies the number of bytes; if it is a string, the value is passed to the
[bytes](https://www.npmjs.com/package/bytes) library for parsing. Defaults
to `'100kb'`.

##### parser

The `parser` option is the function called against the request body to convert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No description of the first argument. Also doesn't describe the mechanism of how it needs to be made to construct the syntax error during the recalling mechanism with the # inserted into the original string.

it to a Javascript object. Defaults to `JSON.parse`.

##### reviver

The `reviver` option is passed directly to `JSON.parse` as the second
The `reviver` option is passed directly to the `parser` as the second
argument. You can find more information on this argument
[in the MDN documentation about JSON.parse](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Example.3A_Using_the_reviver_parameter).

##### strict

When set to `true`, will only accept arrays and objects; when `false` will
accept anything `JSON.parse` accepts. Defaults to `true`.
accept anything the `parser` accepts. Defaults to `true`.

##### type

Expand Down
9 changes: 5 additions & 4 deletions lib/types/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function json (options) {
var strict = opts.strict !== false
var type = opts.type || 'application/json'
var verify = opts.verify || false
var parser = opts.parser || JSON.parse

if (verify !== false && typeof verify !== 'function') {
throw new TypeError('option verify must be function')
Expand All @@ -80,13 +81,13 @@ function json (options) {

if (first !== '{' && first !== '[') {
debug('strict violation')
throw createStrictSyntaxError(body, first)
throw createStrictSyntaxError(parser, body, first)
}
}

try {
debug('parse json')
return JSON.parse(body, reviver)
return parser(body, reviver)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is right for custom parsers. Or at least it's not clear in the docs for the parser option that this is going to happen and how people should build their parser dunxtion to compensate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option would be to remove the reviver param and rely on people using that parameter should change to this. I didn't do that as that would break BC, and I figure a project like this wouldn't want to break BC very often.

Maybe the right way is to make the docs more clear about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I commented on the wrog line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't think I can agree with you that the docs are not clear:

The reviver option is passed directly to the parser as the second argument.

If you can suggest an update to that text I'm all ears.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved my comment down. The new comment for this line is to clarify in the docs the arguments the custom parser will be called with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the line from the reviver docs and add in the specific signature into the parser docs. The current parser section doesn't help someone build their own, I think it assumes someone is going to understand JSON.parse and copy that? If so, just write that down.

} catch (e) {
throw normalizeJsonSyntaxError(e, {
stack: e.stack
Expand Down Expand Up @@ -149,12 +150,12 @@ function json (options) {
* @private
*/

function createStrictSyntaxError (str, char) {
function createStrictSyntaxError (parser, str, char) {
var index = str.indexOf(char)
var partial = str.substring(0, index) + '#'

try {
JSON.parse(partial); /* istanbul ignore next */ throw new SyntaxError('strict violation')
parser(partial); /* istanbul ignore next */ throw new SyntaxError('strict violation')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line my above comment was supposed to be on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is right for custom parsers. Or at least it's not clear in the docs for the parser option that this is going to happen and how people should build their parser function to compensate.

} catch (e) {
return normalizeJsonSyntaxError(e, {
message: e.message.replace('#', char),
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"eslint-plugin-promise": "3.5.0",
"eslint-plugin-standard": "3.0.1",
"istanbul": "0.4.5",
"json-bigint": "0.2.3",
"methods": "1.1.2",
"mocha": "2.5.3",
"safe-buffer": "5.1.1",
Expand Down
11 changes: 11 additions & 0 deletions test/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ var assert = require('assert')
var Buffer = require('safe-buffer').Buffer
var http = require('http')
var request = require('supertest')
var JSONbig = require('json-bigint')

var bodyParser = require('..')

Expand Down Expand Up @@ -69,6 +70,16 @@ describe('bodyParser.json()', function () {
.expect(200, '{"user":"tobi"}', done)
})

it('should use external parsers', function (done) {
request(createServer({
parser: JSONbig({ storeAsString: true }).parse
}))
.post('/')
.set('Content-Type', 'application/json')
.send('{"user_id":1234567890123456789012345678901234567890123456789012345678901234567890}')
.expect(200, '{"user_id":"1234567890123456789012345678901234567890123456789012345678901234567890"}', done)
})

describe('when JSON is invalid', function () {
before(function () {
this.server = createServer()
Expand Down