From 8ab8e9780bdee1d12767166e2a616a6aee15dc4a Mon Sep 17 00:00:00 2001 From: Shelby Sanders Date: Fri, 5 Sep 2014 21:29:28 -0700 Subject: [PATCH 1/5] Changed errorHandler() to honor options.remoting.errorHandler.handler in order to replace restErrorHandler --- lib/rest-adapter.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index e116597..499e926 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -305,6 +305,10 @@ RestAdapter.urlNotFoundHandler = function() { RestAdapter.errorHandler = function(options) { options = options || {}; return function restErrorHandler(err, req, res, next) { + if (options.handler) { + return options.handler(err, req, res, next) + } + if(typeof err === 'string') { err = new Error(err); err.status = err.statusCode = 500; From f79a727954df2ac687c4dcc9713f8d59a0057c4c Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 22 Oct 2014 14:34:44 -0700 Subject: [PATCH 2/5] Ensure next() calls the default error handler in custom handlers --- lib/rest-adapter.js | 50 +++++++++++++++++++++++---------------- test/rest-adapter.test.js | 4 +++- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index 499e926..b89bd2c 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -100,7 +100,7 @@ RestAdapter.prototype.connect = function(url) { RestAdapter.prototype.invoke = function(method, ctorArgs, args, callback) { assert(this.connection, 'Cannot invoke method without a connection. See RemoteObjects#connect().'); assert(typeof method === 'string', 'method is required when calling invoke()'); - + var lastArg = arguments[arguments.length - 1]; callback = typeof lastArg === 'function' ? lastArg : undefined; @@ -306,32 +306,42 @@ RestAdapter.errorHandler = function(options) { options = options || {}; return function restErrorHandler(err, req, res, next) { if (options.handler) { - return options.handler(err, req, res, next) + return options.handler(err, req, res, defaultHandler); + } else { + defaultHandler(); } - if(typeof err === 'string') { - err = new Error(err); - err.status = err.statusCode = 500; - } + function defaultHandler(handlerError) { + if(handlerError) { + if(err !== handlerError) { + handlerError.originalError = err; + } + err = handlerError; + } + if(typeof err === 'string') { + err = new Error(err); + err.status = err.statusCode = 500; + } - res.statusCode = err.statusCode || err.status || 500; + res.statusCode = err.statusCode || err.status || 500; - debug('Error in %s %s: %s', req.method, req.url, err.stack); - var data = { - name: err.name, - status: res.statusCode, - message: err.message || 'An unknown error occurred' - }; + debug('Error in %s %s: %s', req.method, req.url, err.stack); + var data = { + name: err.name, + status: res.statusCode, + message: err.message || 'An unknown error occurred' + }; - for (var prop in err) { - data[prop] = err[prop]; - } + for (var prop in err) { + data[prop] = err[prop]; + } - data.stack = err.stack; - if (process.env.NODE_ENV === 'production' || options.disableStackTrace) { - delete data.stack; + data.stack = err.stack; + if (process.env.NODE_ENV === 'production' || options.disableStackTrace) { + delete data.stack; + } + res.send({ error: data }); } - res.send({ error: data }); }; }; diff --git a/test/rest-adapter.test.js b/test/rest-adapter.test.js index b10d813..232e2a3 100644 --- a/test/rest-adapter.test.js +++ b/test/rest-adapter.test.js @@ -73,7 +73,7 @@ describe('RestAdapter', function() { return new RestAdapter(remotes).getClasses(); } }); - + describe('path normalization', function() { it('fills `routes`', function() { remotes.exports.testClass = factory.createSharedClass(); @@ -307,7 +307,9 @@ describe('RestAdapter', function() { ]); }); + }); + describe('errorHandler', function() { }); }); From f9807dc8c092187c4ad457d2ec21c8694af83368 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 22 Oct 2014 14:45:32 -0700 Subject: [PATCH 3/5] Ensure handler errors are caught --- lib/rest-adapter.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index b89bd2c..56d109d 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -306,16 +306,19 @@ RestAdapter.errorHandler = function(options) { options = options || {}; return function restErrorHandler(err, req, res, next) { if (options.handler) { - return options.handler(err, req, res, defaultHandler); + try { + options.handler(err, req, res, defaultHandler); + } catch(e) { + defaultHandler(e); + } } else { - defaultHandler(); + return defaultHandler(); } function defaultHandler(handlerError) { if(handlerError) { - if(err !== handlerError) { - handlerError.originalError = err; - } + // ensure errors that occurred during + // the handler are reported err = handlerError; } if(typeof err === 'string') { From e387db2ad8e5d9da822e56e10a337979ae291ef6 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 22 Oct 2014 14:53:50 -0700 Subject: [PATCH 4/5] Add custom handler test --- test/rest-adapter.test.js | 4 ---- test/rest.test.js | 44 ++++++++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/test/rest-adapter.test.js b/test/rest-adapter.test.js index 232e2a3..6830508 100644 --- a/test/rest-adapter.test.js +++ b/test/rest-adapter.test.js @@ -308,10 +308,6 @@ describe('RestAdapter', function() { }); }); - - describe('errorHandler', function() { - - }); }); function someFunc() { diff --git a/test/rest.test.js b/test/rest.test.js index de3b168..9a66a53 100644 --- a/test/rest.test.js +++ b/test/rest.test.js @@ -13,7 +13,7 @@ describe('strong-remoting-rest', function(){ var objects; var remotes; var adapterName = 'rest'; - + before(function(done) { app = express(); app.disable('x-powered-by'); @@ -32,7 +32,7 @@ describe('strong-remoting-rest', function(){ objects = RemoteObjects.create({json: {limit: '1kb'}, errorHandler: {disableStackTrace: false}}); remotes = objects.exports; - + // connect to the app objects.connect('http://localhost:' + server.address().port, adapterName); }); @@ -75,6 +75,30 @@ describe('strong-remoting-rest', function(){ .expect(413, done); }); + it('should allow custom error handlers', function(done) { + var called = false; + var method = givenSharedStaticMethod( + function(cb) { + cb(new Error('foo')); + } + ); + + objects.options.errorHandler.handler = function(err, req, res, next) { + expect(err.message).to.contain('foo'); + var err = new Error('foobar'); + called = true; + next(err); + } + + request(app).get(method.url) + .expect('Content-Type', /json/) + .expect(500) + .end(expectErrorResponseContaining({message: 'foobar'}, function(err) { + expect(called).to.eql(true); + done(err); + })); + }); + it('should disable stack trace', function(done) { objects.options.errorHandler.disableStackTrace = true; var method = givenSharedStaticMethod( @@ -1022,7 +1046,7 @@ describe('strong-remoting-rest', function(){ returns: { arg: 'msg', type: 'string' } } ); - + var msg = 'hello'; objects.invoke(method.name, [msg], function(err, resMsg) { assert.equal(resMsg, msg); @@ -1048,7 +1072,7 @@ describe('strong-remoting-rest', function(){ objects.invoke(method.name, [1, 2], function(err, n) { assert.equal(n, 3); done(); - }); + }); }); it('should allow arguments in the query', function(done) { @@ -1065,7 +1089,7 @@ describe('strong-remoting-rest', function(){ http: { path: '/' } } ); - + objects.invoke(method.name, [1, 2], function(err, n) { assert.equal(n, 3); done(); @@ -1086,7 +1110,7 @@ describe('strong-remoting-rest', function(){ ] } ); - + objects.invoke(method.name, [], function(err) { assert(called); done(); @@ -1106,7 +1130,7 @@ describe('strong-remoting-rest', function(){ http: { path: '/' } } ); - + var obj = { foo: 'bar' }; @@ -1152,7 +1176,7 @@ describe('strong-remoting-rest', function(){ http: { path: '/' } } ); - + objects.invoke(method.name, [1, 2], function(err, n) { assert.equal(n, 3); done(); @@ -1182,7 +1206,7 @@ describe('strong-remoting-rest', function(){ done(); }); }); - + describe('uncaught errors', function () { it('should return 500 if an error object is thrown', function (done) { var errMsg = 'an error'; @@ -1191,7 +1215,7 @@ describe('strong-remoting-rest', function(){ throw new Error(errMsg); } ); - + objects.invoke(method.name, function(err) { assert(err instanceof Error); assert.equal(err.message, errMsg); From 3b9c88686c9838d992d98da0b255683111d50bdc Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Wed, 22 Oct 2014 15:10:52 -0700 Subject: [PATCH 5/5] Ensure errorHandler.handler is a function --- lib/rest-adapter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index 56d109d..d39b3ec 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -305,7 +305,7 @@ RestAdapter.urlNotFoundHandler = function() { RestAdapter.errorHandler = function(options) { options = options || {}; return function restErrorHandler(err, req, res, next) { - if (options.handler) { + if (typeof options.handler === 'function') { try { options.handler(err, req, res, defaultHandler); } catch(e) {