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

sdellysse · 2017-11-21T16:46:25Z

Added the option to use a function other than JSON.parse to do the actual parsing. This fixes #278; contrary to one of the suggestions in that issue I left the reviver option as-is for backward compatibility. Also see #42, where adding a similar option for query string parsing was the preferred choice.

dougwilson

I think that ultimately that issue should be resolved by implementing to a long standing issuehttps://github.com//issues/22 instead of this hakf way one. Or at the very least add a parser option to every parser type because I'm sure the same argument for needig custom applies across the board.

dougwilson · 2017-11-21T16:52:41Z

lib/types/json.js

      }
    }

    try {
      debug('parse json')
-      return JSON.parse(body, reviver)
+      return parser(body, reviver)


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.

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?

Sorry, I commented on the wrog line.

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.

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.

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.

dougwilson · 2017-11-21T16:59:53Z

lib/types/json.js

  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')


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

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.

dougwilson · 2017-11-21T17:04:10Z

lib/types/json.js

  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')


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.

dougwilson · 2017-11-21T17:04:54Z

lib/types/json.js

      }
    }

    try {
      debug('parse json')
-      return JSON.parse(body, reviver)
+      return parser(body, reviver)


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.

dougwilson · 2017-11-21T17:08:46Z

Ok, just wanted to give feedback real quick. I'll be gone for tge holiday and back next week, so see how far you can get on the changes by then or let me know asap if you have comments on the overall request I made instead of just those line comments.

sdellysse · 2017-11-21T19:01:10Z

Ah, I see what you're saying now. I'll work on the rest, hope you have a good holiday!

dougwilson · 2017-11-21T21:13:49Z

I love the changes! I'm not sure if you responded to my general comment, maybe I missed it. Why not implement #22 to resolve the underlying issue? I'm just curious because that's still needed and if course that eould replace this causing more API churn in the log run.

dougwilson

In general great!

dougwilson · 2017-11-21T21:15:17Z

README.md

@@ -86,16 +86,27 @@ 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


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.

dougwilson · 2017-11-21T21:16:26Z

lib/types/raw.js

@@ -49,7 +50,7 @@ function raw (options) {
    : type

  function parse (buf) {
-    return buf
+    return parser(buf)


We probably don't need two levels of functions here. Just replace parse with a variable.

dougwilson · 2017-11-21T21:16:43Z

lib/types/text.js

@@ -51,7 +52,7 @@ function text (options) {
    : type

  function parse (buf) {
-    return buf
+    return parser(buf)


We probably don't need two levels of functions here. Just replace parse with a variable.

dougwilson · 2017-11-21T21:17:48Z

README.md

@@ -159,6 +170,15 @@ The `verify` option, if supplied, is called as `verify(req, res, buf, encoding)`
 where `buf` is a `Buffer` of the raw request body and `encoding` is the
 encoding of the request. The parsing can be aborted by throwing an error.

+##### parser
+
+The `parser` option, if supplied, is used to transform the body of a request


Should probably describe the type and number of arguments it will be called with.

dougwilson · 2017-11-21T21:18:02Z

README.md

@@ -208,6 +228,15 @@ The `verify` option, if supplied, is called as `verify(req, res, buf, encoding)`
 where `buf` is a `Buffer` of the raw request body and `encoding` is the
 encoding of the request. The parsing can be aborted by throwing an error.

+##### parser
+
+The `parser` option, if supplied, is used to transform the body of a request


Should probably describe the type and number of arguments it will be called with.

dougwilson · 2017-11-21T21:19:48Z

README.md

@@ -274,6 +303,16 @@ The `verify` option, if supplied, is called as `verify(req, res, buf, encoding)`
 where `buf` is a `Buffer` of the raw request body and `encoding` is the
 encoding of the request. The parsing can be aborted by throwing an error.

+##### parser
+
+The `parser` option, if supplied, is used to in place of the default parser to


Should probably mention the type of the argument and also that it is not called at all for zero length bodies (though, should it be always called because what if they want a zero length result to be something other than {}?). The other parsers seem to be called on zero length, so this seems odd not to.

dougwilson · 2017-11-21T21:21:42Z

README.md

+
+The `parser` option, if supplied, is used to in place of the default parser to
+convert the request body into a Javascript object. If this option is supplied,
+both the `extended` and `parameterLimit` options are ignored.


For the json parser, the reviver is not ignore but these are. Should json work like this and ignore the arguments that were originally going to the underlying parser that is now replaced, or should these be passed to the new urlencoded parser somehow or neither and a warning is a user gave these? Some thoughts.

sdellysse · 2017-11-21T23:22:59Z

Closing, superceded by PR #282

Added support for external parsers to bodyParser.json()

d874969

dougwilson suggested changes Nov 21, 2017

View reviewed changes

removed test dependency on json-bigint

4f18d51

dougwilson suggested changes Nov 21, 2017

View reviewed changes

Shawn Dellysse added 5 commits November 21, 2017 15:43

reworked doc to describe json parser() func better

1d47892

added parser() option and doc for .text()

b9823e5

added parser() option and doc for .raw()

4b91d93

added parser() option and doc for .urlencoded()

e786697

cleanup to satisfy linter

612e4da

dougwilson suggested changes Nov 21, 2017

View reviewed changes

sdellysse mentioned this pull request Nov 21, 2017

Generic Body Parser implemented #282

Closed

sdellysse closed this Nov 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sdellysse commented Nov 21, 2017 •

edited

Loading

dougwilson left a comment

dougwilson Nov 21, 2017

sdellysse Nov 21, 2017

dougwilson Nov 21, 2017

sdellysse Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson commented Nov 21, 2017

sdellysse commented Nov 21, 2017

dougwilson commented Nov 21, 2017

dougwilson left a comment

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

dougwilson Nov 21, 2017

sdellysse commented Nov 21, 2017

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

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

Conversation

sdellysse commented Nov 21, 2017 • edited Loading

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Nov 21, 2017

sdellysse commented Nov 21, 2017

dougwilson commented Nov 21, 2017

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdellysse commented Nov 21, 2017

sdellysse commented Nov 21, 2017 •

edited

Loading