Skip to content

Commit

Permalink
res.jsonx() now ensures that Error instances passed in end up with bo…
Browse files Browse the repository at this point in the history 
…th a .stack and .message property.
  • Loading branch information
@mikermcneil
mikermcneil committed May 18, 2015
1 parent 2f4a008 commit b8c3813
Showing 1 changed file with 17 additions and 1 deletion.
18 changes: 17 additions & 1 deletion lib/hooks/responses/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,23 @@ function _mixin_jsonx(req, res) {
// (note that this guard includes arrays)
return res.send(data);
}
else if ( req.options.jsonp && !req.isSocket ) {

// When responding with an Error instance, if it's going to get sringified
// without its `.stack`, then manually add the `.stack` and `.message` back in.
if (data instanceof Error) {
var jsonSerializedErr;
try {
jsonSerializedErr = JSON.parse(JSON.stringify(data));
if (!jsonSerializedErr.stack || !jsonSerializedErr.message) {
data = {message: data.message, stack: data.stack};
}
}
catch (e){
data = {message: data.message, stack: data.stack};
}
}

if ( req.options.jsonp && !req.isSocket ) {
return res.jsonp(data);
}
else return res.json(data);
Expand Down

2 comments on commit b8c3813

@mikermcneil
Copy link
Member Author

Choose a reason for hiding this comment

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

For future reference: This was completed as originally intended in e89078d by @fernandolguevara. Now other properties besides message+stack are maintained.

However, toJSON() is not run, so any errors with a custom toJSON method do not get serialized as expected. Most of the time, this doesn't matter, since most folks don't even know you can do this. But for people who do, this feels like unexpected behavior.

See also #3462.

Re: the original problem (validation error messages from Waterline being put in a state where they can't be parsed programmatically), this is mostly fixed-- but the set of exposed information contains more information than it used to:

image

Haven't looked into this much yet, but I believe it's because of the lack of .toJSON()-ing. Will link from here when I have more info.

@mikermcneil
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the error data doesn't make it back to the client in production by default, this isn't any immediate problem, and so in order to maintain backwards compatibility, I'll leave it as-is. That said, be warned: In Sails v1.0, the implementation of error handling in blueprints will change, insofar as the default implementation of api/responses/badRequest.js will not protect the data you send through in production. This is to allow res.badRequest() to absorb support for more of the customizations that normally result in just hand-rolling responses currently. Same thing for the default implementation of res.forbidden() and res.notFound(). res.serverError() will work exactly like it does now as far as preventing any error information from touching the response in production.

The error marshaling in the responses hook in Sails core will also be pulled into the responses. This makes it easier to customize, particularly when it comes to stuff like toJSON(). The default implementations of res.forbidden() and res.notFound() will not return any information to the client at all. The default implementation of res.badRequest() will send through the provided request data verbatim. And res.serverError() will work the way it does today.

Please sign in to comment.