Skip to content

[SEMVER-MAJOR] Implement strong error handler for rest-adapter #302

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.

Sign up for GitHub

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

Merged
davidcheung merged 1 commit into from
Jun 7, 2016
Merged

[SEMVER-MAJOR] Implement strong error handler for rest-adapter #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
86 changes: 86 additions & 0 deletions 3.0-RELEASE-NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,89 @@ To disable a static remote method

To disable a prototype remote method:
`disableMethodByName('prototype.updateAttributes')`

## New error-handler for rest-adapter

The REST adapter (typically created via `loopback.rest()`) uses a new error
handler implementation that provides more secure configuration out of the box.

The `error.status` has been removed and will be replaced by `error.statusCode`,
`statusCode` is more descriptive and avoids ambiguity, users relying on `status`
will need to change implementation accordingly.

To replicate old behavior user can specify `config.local.js` as follow:
```js
module.exports = {
remoting : {
errorHandler: {
handler : function(err, req, res, defaultHandler) {
err.status = err.statusCode;
defaultHandler();
}
}
}
}
```

The environment setting `NODE_ENV='production'` is no longer supported,
production vs. debug mode is controlled exclusively by configuration.
Production environment is assumed by default, the insecure debug mode
must be explicitely turned on.

You can learn more about the rationale behind the new handler in
[this comment](https://github.com/strongloop/loopback/issues/1650#issuecomment-161920555)

User can specific options in their applications in `config.json` as follow:

@bajtos bajtos May 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure about this? I think a better option is to configure strong-remoting's error handler via server/middleware.json, but we may not be there yet.

{
  "routes": {
    "loopback#rest": {
      "params": {
        "debug": true,
        "log": true
      }
   }
}

@davidcheung davidcheung May 18, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm it works in my sand-box
i think the behavior comes from

strong-remoting/lib/rest-adapter.js

Line 315 in aa391d2

root.use(RestAdapter.errorHandler(this.remotes.options.errorHandler));

@davidcheung davidcheung May 19, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maybe we can update this when we change it to use middleware.json?

@bajtos bajtos May 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We are already using middleware.json. But never mind, let's leave this out of scope of this pull request and stay consistent with the current state of affairs (i.e. configure it via server/config.json).

```json
{
"restApiRoot": "/api",
"host": "0.0.0.0",
"port": 3000,
"remoting": {
"errorHandler": {
"debug": false,
"log": false
}
}
}
```

@bajtos bajtos May 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a link to strong-error-handler GH repository, so that people can easily find description of the config options available.

@bajtos bajtos May 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see the link is already included below, ignore this comment.


#### Production mode

Stack trace is never returned in HTTP responses.

Bad Request errors (4xx) provide the following properties copied from the
error object: `name`, `message`, `statusCode` and `details`.

All other errors (including non-Error values like strings or arrays) provide
only basic information: `statusCode` and `message` set to status name from HTTP
specification.

#### Debug mode

When in debug mode, HTTP responses include all properties provided by the error.

For errors that are not an object, their string value is returned in
`message` field.

When a method fails with an array of errors (e.g. bulk create), HTTP the response
contains still a single wrapper error with `details` set to the original array
of errors.

An example of an error response when an array of errors was raised:

```js
{
error: {
statusCode: 500,
name: 'ArrayOfErrors',
message: 'Failed with multiple errors, see `details` for more information.',
details: [
{ name: 'Error1', message: 'expected error', statusCode: 500, stack: '<stacktrace>' },
{ name: 'Error2', message: 'expected error2', statusCode: 500, stack: '<stacktrace>'}
]
}
}
```

@bajtos bajtos May 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a link to this pull request so that people can find the discussion that led to this change (see other release note entries) and also a pointer to strong-error-handler so that people can find more information about the settings available in this new error handler.


Please see [Related code change](https://github.com/strongloop/strong-remoting/pull/302) here, and new [`strong-error-handler` here](https://github.com/strongloop/strong-error-handler/).
57 changes: 12 additions & 45 deletions lib/rest-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ var async = require('async');
var HttpInvocation = require('./http-invocation');
var ContextBase = require('./context-base');
var HttpContext = require('./http-context');
var strongErrorHandler = require('strong-error-handler');

var json = bodyParser.json;
var urlencoded = bodyParser.urlencoded;
Expand Down Expand Up @@ -333,7 +334,7 @@ RestAdapter.remoteMethodNotFoundHandler = function(className) {
var message = 'Shared class "' + className + '"' +
' has no method handling ' + req.method + ' ' + req.url;
var error = new Error(message);
error.status = error.statusCode = 404;
error.statusCode = 404;
next(error);
};
};
Expand All @@ -342,13 +343,21 @@ RestAdapter.urlNotFoundHandler = function() {
return function restUrlNotFound(req, res, next) {
var message = 'There is no method to handle ' + req.method + ' ' + req.url;
var error = new Error(message);
error.status = error.statusCode = 404;
error.statusCode = 404;
next(error);
};
};

RestAdapter.errorHandler = function(options) {
options = options || {};
if (options.hasOwnProperty('disableStackTrace')) {
console.warn(
'strong-remoting no longer supports "errorHandler.disableStackTrace" option. ' +
'Use the new option "errorHandler.debug" instead.');
}

var strongErrorHandlerInstance = strongErrorHandler(options);

return function restErrorHandler(err, req, res, next) {
if (typeof options.handler === 'function') {
try {
Expand All @@ -366,49 +375,7 @@ RestAdapter.errorHandler = function(options) {
// the handler are reported
err = handlerError;
}
if (typeof err === 'string') {
err = new Error(err);
err.status = err.statusCode = 500;
}

if (res.statusCode === undefined || res.statusCode < 400) {
res.statusCode = err.statusCode || err.status || 500;
}

if (Array.isArray(err)) {
var details = err.map(function(it) {
var data = generateResponseError(it);
delete data.statusCode;
return data;
});

var msg = 'Failed with multiple errors, see `details` for more information.';
err = new Error(msg);
err.details = details;
}

res.send({ error: generateResponseError(err) });

function generateResponseError(error) {
debug('Error in %s %s: %s', req.method, req.url, error.stack);

var data = {
name: error.name,
status: res.statusCode,
message: error.message || 'An unknown error occurred',
};

for (var prop in error) {
data[prop] = error[prop];
}

data.stack = error.stack;
if (process.env.NODE_ENV === 'production' || options.disableStackTrace) {
delete data.stack;
}

return data;
}
return strongErrorHandlerInstance(err, req, res, next);
}
};
};
Expand Down
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"qs": "^2.4.2",
"request": "^2.55.0",
"sse": "0.0.6",
"strong-error-handler": "^1.0.0",
"traverse": "^0.6.6",
"xml2js": "^0.4.8"
},
Expand Down Expand Up @@ -73,7 +74,8 @@
"express": false,
"body-parser": false,
"cors": false,
"js2xmlparser": false
"js2xmlparser": false,
"strong-error-handler": false
},
"license": "Artistic-2.0"
}
4 changes: 4 additions & 0 deletions test/rest.browser.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,10 @@ describe('strong-remoting-rest', function() {
});

describe('uncaught errors', function() {
beforeEach(function() {
var optsErrorHandler = { errorHandler: { debug: true, log: false }};
extend(objects.options, optsErrorHandler);
});
it('should return 500 if an error object is thrown', function(done) {
var errMsg = 'an error';
var method = givenSharedStaticMethod(
Expand Down
37 changes: 8 additions & 29 deletions test/rest.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,8 @@ describe('strong-remoting-rest', function() {

// setup
beforeEach(function() {
if (process.env.NODE_ENV === 'production') {
process.env.NODE_ENV = 'test';
}
objects = RemoteObjects.create({ json: { limit: '1kb' },
errorHandler: { disableStackTrace: false }});
errorHandler: { debug: true, log: false }});
remotes = objects.exports;

// connect to the app
Expand Down Expand Up @@ -131,36 +128,18 @@ describe('strong-remoting-rest', function() {
}));
});

it('should disable stack trace', function(done) {
objects.options.errorHandler.disableStackTrace = true;
it('should exclude stack traces by default', function(done) {
var method = givenSharedStaticMethod(
function(cb) {
cb(new Error('test-error'));
}
);
function(cb) { cb(new Error('test-error')); });

// Send a plain, non-json request to make sure the error handler
// always returns a json response.
request(app).get(method.url)
.expect('Content-Type', /json/)
.expect(500)
.end(expectErrorResponseContaining({ message: 'test-error' }, ['stack'], done));
});
// reset the errorHandler options
objects.options.errorHandler = {};

it('should disable stack trace', function(done) {
process.env.NODE_ENV = 'production';
var method = givenSharedStaticMethod(
function(cb) {
cb(new Error('test-error'));
}
);

// Send a plain, non-json request to make sure the error handler
// always returns a json response.
request(app).get(method.url)
.expect('Content-Type', /json/)
.expect(500)
.end(expectErrorResponseContaining({ message: 'test-error' }, ['stack'], done));
.end(expectErrorResponseContaining(
{ message: 'Internal Server Error' }, ['stack'], done));
});

it('should turn off url-not-found handler', function(done) {
Expand Down Expand Up @@ -2072,7 +2051,7 @@ describe('strong-remoting-rest', function() {
it('returns 404 with standard JSON body for unknown URL', function(done) {
json('/unknown-url')
.expect(404)
.end(expectErrorResponseContaining({ status: 404 }, done));
.end(expectErrorResponseContaining({ statusCode: 404 }, done));
});
});

Expand Down