Adds parseUser option to control user parsing behavior #274
Conversation
| username: 'janedoe', | ||
| email: 'hello@janedoe.com', | ||
| ip_address: '127.0.0.1' | ||
| }); |
There was a problem hiding this comment.
does this check that random is -not- set?
There was a problem hiding this comment.
yes, it checks that the user property has that exact value
docs/config.rst
Outdated
|
|
||
| Defaults to ``['id', 'username', 'email']``. | ||
|
|
||
| A function can be provided for custom parsing: |
There was a problem hiding this comment.
maybe, "alternatively, a function ..."
| if ({}.hasOwnProperty.call(req.user, fieldName)) { | ||
| user[fieldName] = req.user[fieldName]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
this looks good to me but this is a big gnarly block of logic. consider labeling some of the control flow paths?
There was a problem hiding this comment.
ehh...normally id agree but i did it that way at first and it felt like borderline overcommenting since each branch has a pretty straightforward condition check, so i made sure to describe the 5 cases together above instead
| var data = req.body; | ||
| if (['GET', 'HEAD'].indexOf(method) === -1) { | ||
| if (typeof data === 'undefined') { | ||
| data = '<unavailable>'; | ||
| } | ||
| } | ||
|
|
||
| if (data && {}.toString.call(data) !== '[object String]') { | ||
| if (data && typeof data !== 'string' && {}.toString.call(data) !== '[object String]') { |
There was a problem hiding this comment.
The {}.toString.call check is really checking two cases without being obvious about it, and I sort of prefer using typeof as much as possible and avoiding checking object toString types when we don't need to be. This better delineates/escalates through the cases we care about IMO (and typeof is a cheaper check in the common case, not that we care about a trivial perf diff in a once-per-error check).
| @@ -53,123 +53,105 @@ module.exports.parseError = function parseError(err, kwargs, cb) { | |||
| }); | |||
| }; | |||
|
|
|||
| module.exports.parseRequest = function parseRequest(req, kwargs) { | |||
| kwargs = kwargs || {}; | |||
| module.exports.parseRequest = function parseRequest(req, parseUser) { | |||
There was a problem hiding this comment.
Is parseRequest a public/documented API method? If so, might want to consider how to make this signature more extendable going forward.
e.g. I'd almost prefer if we kept kwargs and you did:
parseRequest(req, {
parseUser: ['username']
});There was a problem hiding this comment.
Not public; only callsite is currently in errorHandler middleware, soon we'll add another in requestHandler and in process but all internal. The kwargs param was unused so I got rid of it.
| if (parseUser) { | ||
| var user = {}; | ||
| if (typeof parseUser === 'function') { | ||
| user = parseUser(req); |
There was a problem hiding this comment.
Is there a reason we pass req to parseUser and not just strictly the user?
(I mean, this is probably more flexible, but want to get your take.)
There was a problem hiding this comment.
In my mind the function option exists primarily as a catch-all for "well what if it's not at req.user...?"; other main case would be if we want to pull in other stuff off the req object (like maybe we want stuff from req.session), so either way I pass the entire req object for max flexibility.
Should address #231. Will also add some flexibility for both #82 and another slight improvement that we can make in the requestHandler middleware for automatically-grabbing-request.
/cc @benvinegar @MaxBittker and somewhat refactors request parsing so /cc @mattrobenolt.