From 02199c6b02589b12324d8191c69ea80aad6b907a Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 17 Feb 2016 14:51:57 +0000 Subject: [PATCH] Disambiguate between error code & status code refs #6526 - Change our errors to use `statusCode` for the status code (like res.statusCode) - Use statusCode for anything that's supposed to be the statusCode, rather than an error idenfier/code - Update all the tests that check the key - Route tests don't need fixing as the status codes are still returned correctly --- core/server/errors/bad-request-error.js | 2 +- core/server/errors/data-import-error.js | 2 +- core/server/errors/email-error.js | 2 +- core/server/errors/index.js | 58 ++++++++++++++----- core/server/errors/internal-server-error.js | 2 +- .../server/errors/method-not-allowed-error.js | 2 +- core/server/errors/no-permission-error.js | 2 +- core/server/errors/not-found-error.js | 2 +- core/server/errors/request-too-large-error.js | 2 +- core/server/errors/too-many-requests-error.js | 2 +- core/server/errors/unauthorized-error.js | 2 +- .../errors/unsupported-media-type-error.js | 2 +- core/server/errors/validation-error.js | 2 +- core/server/helpers/ghost_head.js | 2 +- .../api/api_authentication_spec.js | 16 ++--- core/test/integration/api/api_posts_spec.js | 4 +- core/test/integration/api/api_upload_spec.js | 6 +- .../controllers/frontend/channels_spec.js | 2 +- core/test/unit/error_handling_spec.js | 12 ++-- core/test/unit/rss_spec.js | 4 +- 20 files changed, 79 insertions(+), 49 deletions(-) diff --git a/core/server/errors/bad-request-error.js b/core/server/errors/bad-request-error.js index 92248bf8a8..bdb1825e8c 100644 --- a/core/server/errors/bad-request-error.js +++ b/core/server/errors/bad-request-error.js @@ -4,7 +4,7 @@ function BadRequestError(message) { this.message = message; this.stack = new Error().stack; - this.code = 400; + this.statusCode = 400; this.errorType = this.name; } diff --git a/core/server/errors/data-import-error.js b/core/server/errors/data-import-error.js index 75b0909c1c..48343435b5 100644 --- a/core/server/errors/data-import-error.js +++ b/core/server/errors/data-import-error.js @@ -4,7 +4,7 @@ function DataImportError(message, offendingProperty, value) { this.message = message; this.stack = new Error().stack; - this.code = 500; + this.statusCode = 500; this.errorType = this.name; this.property = offendingProperty || undefined; this.value = value || undefined; diff --git a/core/server/errors/email-error.js b/core/server/errors/email-error.js index cb9e7b8ec3..4bf1d6722e 100644 --- a/core/server/errors/email-error.js +++ b/core/server/errors/email-error.js @@ -4,7 +4,7 @@ function EmailError(message) { this.message = message; this.stack = new Error().stack; - this.code = 500; + this.statusCode = 500; this.errorType = this.name; } diff --git a/core/server/errors/index.js b/core/server/errors/index.js index 2cd5bab7d8..f7f49c3f14 100644 --- a/core/server/errors/index.js +++ b/core/server/errors/index.js @@ -34,6 +34,29 @@ function getConfigModule() { return config; } +function isValidErrorStatus(status) { + return _.isNumber(status) && status >= 400 && status < 600; +} + +function getStatusCode(error) { + if (error.statusCode) { + return error.statusCode; + } + + if (error.status && isValidErrorStatus(error.status)) { + error.statusCode = error.status; + return error.statusCode; + } + + if (error.code && isValidErrorStatus(error.code)) { + error.statusCode = error.code; + return error.statusCode; + } + + error.statusCode = 500; + return error.statusCode; +} + /** * Basic error handling helpers */ @@ -197,7 +220,7 @@ errors = { var errorContent = {}; // TODO: add logic to set the correct status code - statusCode = errorItem.code || 500; + statusCode = getStatusCode(errorItem); errorContent.message = _.isString(errorItem) ? errorItem : (_.isObject(errorItem) ? errorItem.message : i18n.t('errors.errors.unknownApiError')); @@ -227,7 +250,7 @@ errors = { if (error.code && (error.errno || error.detail)) { error.db_error_code = error.code; error.errorType = 'DatabaseError'; - error.code = 500; + error.statusCode = 500; return this.rejectError(error); } @@ -243,7 +266,7 @@ errors = { res.status(httpErrors.statusCode).json({errors: httpErrors.errors}); }, - renderErrorPage: function (code, err, req, res, next) { + renderErrorPage: function (statusCode, err, req, res, next) { /*jshint unused:false*/ var self = this, defaultErrorTemplatePath = path.resolve(getConfigModule().paths.adminViews, 'user-error.hbs'); @@ -281,17 +304,22 @@ errors = { function renderErrorInt(errorView) { var stack = null; - if (code !== 404 && process.env.NODE_ENV !== 'production' && err.stack) { + if (statusCode !== 404 && process.env.NODE_ENV !== 'production' && err.stack) { stack = parseStack(err.stack); } - res.status(code).render((errorView || 'error'), { + res.status(statusCode).render((errorView || 'error'), { message: err.message || err, - code: code, + // We have to use code here, as it's the variable passed to the template + // And error templates can be customised... therefore this constitutes API + // In future I recommend we make this be used for a combo-version of statusCode & errorCode + code: statusCode, + // Adding this as being distinctly, the status code, as opposed to any other code see #6526 + statusCode: statusCode, stack: stack }, function (templateErr, html) { if (!templateErr) { - return res.status(code).send(html); + return res.status(statusCode).send(html); } // There was an error trying to render the error page, output the error self.logError(templateErr, i18n.t('errors.errors.errorWhilstRenderingError'), i18n.t('errors.errors.errorTemplateHasError')); @@ -303,12 +331,12 @@ errors = { '

' + i18n.t('errors.errors.encounteredError') + '

' + '
' + hbs.handlebars.Utils.escapeExpression(templateErr.message || templateErr) + '
' + '

' + i18n.t('errors.errors.whilstTryingToRender') + '

' + - code + ' ' + '
'  + hbs.handlebars.Utils.escapeExpression(err.message || err) + '
' + statusCode + ' ' + '
'  + hbs.handlebars.Utils.escapeExpression(err.message || err) + '
' ); }); } - if (code >= 500) { + if (statusCode >= 500) { this.logError(err, i18n.t('errors.errors.renderingErrorPage'), i18n.t('errors.errors.caughtProcessingError')); } @@ -334,10 +362,13 @@ errors = { }, error500: function (err, req, res, next) { + var statusCode = getStatusCode(err), + returnErrors = []; + // 500 errors should never be cached res.set({'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0'}); - if (err.status === 404 || err.code === 404) { + if (statusCode === 404) { return this.error404(req, res, next); } @@ -345,11 +376,8 @@ errors = { if (!err || !(err instanceof Error)) { next(); } - errors.renderErrorPage(err.status || err.code || 500, err, req, res, next); + errors.renderErrorPage(statusCode, err, req, res, next); } else { - var statusCode = 500, - returnErrors = []; - if (!_.isArray(err)) { err = [].concat(err); } @@ -357,8 +385,6 @@ errors = { _.each(err, function (errorItem) { var errorContent = {}; - statusCode = errorItem.code || 500; - errorContent.message = _.isString(errorItem) ? errorItem : (_.isObject(errorItem) ? errorItem.message : i18n.t('errors.errors.unknownError')); errorContent.errorType = errorItem.errorType || 'InternalServerError'; diff --git a/core/server/errors/internal-server-error.js b/core/server/errors/internal-server-error.js index 8fc745d0a0..08a18f923d 100644 --- a/core/server/errors/internal-server-error.js +++ b/core/server/errors/internal-server-error.js @@ -4,7 +4,7 @@ function InternalServerError(message) { this.message = message; this.stack = new Error().stack; - this.code = 500; + this.statusCode = 500; this.errorType = this.name; } diff --git a/core/server/errors/method-not-allowed-error.js b/core/server/errors/method-not-allowed-error.js index 946f9baa2f..8efe5e375f 100644 --- a/core/server/errors/method-not-allowed-error.js +++ b/core/server/errors/method-not-allowed-error.js @@ -4,7 +4,7 @@ function MethodNotAllowedError(message) { this.message = message; this.stack = new Error().stack; - this.code = 405; + this.statusCode = 405; this.errorType = this.name; } diff --git a/core/server/errors/no-permission-error.js b/core/server/errors/no-permission-error.js index 90084d68cd..c60dacce1b 100644 --- a/core/server/errors/no-permission-error.js +++ b/core/server/errors/no-permission-error.js @@ -4,7 +4,7 @@ function NoPermissionError(message) { this.message = message; this.stack = new Error().stack; - this.code = 403; + this.statusCode = 403; this.errorType = this.name; } diff --git a/core/server/errors/not-found-error.js b/core/server/errors/not-found-error.js index 30f409b3fe..ee5bd393fe 100644 --- a/core/server/errors/not-found-error.js +++ b/core/server/errors/not-found-error.js @@ -4,7 +4,7 @@ function NotFoundError(message) { this.message = message; this.stack = new Error().stack; - this.code = 404; + this.statusCode = 404; this.errorType = this.name; } diff --git a/core/server/errors/request-too-large-error.js b/core/server/errors/request-too-large-error.js index a1bb8b137e..71c2848a72 100644 --- a/core/server/errors/request-too-large-error.js +++ b/core/server/errors/request-too-large-error.js @@ -4,7 +4,7 @@ function RequestEntityTooLargeError(message) { this.message = message; this.stack = new Error().stack; - this.code = 413; + this.statusCode = 413; this.errorType = this.name; } diff --git a/core/server/errors/too-many-requests-error.js b/core/server/errors/too-many-requests-error.js index 15e5c0f813..183a063142 100644 --- a/core/server/errors/too-many-requests-error.js +++ b/core/server/errors/too-many-requests-error.js @@ -4,7 +4,7 @@ function TooManyRequestsError(message) { this.message = message; this.stack = new Error().stack; - this.code = 429; + this.statusCode = 429; this.errorType = this.name; } diff --git a/core/server/errors/unauthorized-error.js b/core/server/errors/unauthorized-error.js index 52882cb5c7..d03723abea 100644 --- a/core/server/errors/unauthorized-error.js +++ b/core/server/errors/unauthorized-error.js @@ -4,7 +4,7 @@ function UnauthorizedError(message) { this.message = message; this.stack = new Error().stack; - this.code = 401; + this.statusCode = 401; this.errorType = this.name; } diff --git a/core/server/errors/unsupported-media-type-error.js b/core/server/errors/unsupported-media-type-error.js index a5617be92d..1d16691c4f 100644 --- a/core/server/errors/unsupported-media-type-error.js +++ b/core/server/errors/unsupported-media-type-error.js @@ -4,7 +4,7 @@ function UnsupportedMediaTypeError(message) { this.message = message; this.stack = new Error().stack; - this.code = 415; + this.statusCode = 415; this.errorType = this.name; } diff --git a/core/server/errors/validation-error.js b/core/server/errors/validation-error.js index 2526d3f0d6..999f88a44f 100644 --- a/core/server/errors/validation-error.js +++ b/core/server/errors/validation-error.js @@ -4,7 +4,7 @@ function ValidationError(message, offendingProperty) { this.message = message; this.stack = new Error().stack; - this.code = 422; + this.statusCode = 422; if (offendingProperty) { this.property = offendingProperty; } diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js index f73bdc44fc..3f09ebf901 100644 --- a/core/server/helpers/ghost_head.js +++ b/core/server/helpers/ghost_head.js @@ -73,7 +73,7 @@ function getAjaxHelper(clientId, clientSecret) { function ghost_head(options) { // if error page do nothing - if (this.code >= 400) { + if (this.statusCode >= 400) { return; } var metaData = getMetaData(this, options.data.root), diff --git a/core/test/integration/api/api_authentication_spec.js b/core/test/integration/api/api_authentication_spec.js index cbb642466e..c6f9e45ed1 100644 --- a/core/test/integration/api/api_authentication_spec.js +++ b/core/test/integration/api/api_authentication_spec.js @@ -97,7 +97,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); @@ -110,7 +110,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); @@ -123,7 +123,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); @@ -156,7 +156,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); @@ -169,7 +169,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('UnauthorizedError'); - err.code.should.equal(401); + err.statusCode.should.equal(401); err.message.should.equal('Invalid token structure'); done(); }); @@ -191,7 +191,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('UnauthorizedError'); - err.code.should.equal(401); + err.statusCode.should.equal(401); err.message.should.equal('Invalid token structure'); done(); }); @@ -226,7 +226,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); @@ -259,7 +259,7 @@ describe('Authentication API', function () { should.exist(err); err.name.should.equal('NoPermissionError'); - err.code.should.equal(403); + err.statusCode.should.equal(403); done(); }); diff --git a/core/test/integration/api/api_posts_spec.js b/core/test/integration/api/api_posts_spec.js index 2bec6ca873..a94b286787 100644 --- a/core/test/integration/api/api_posts_spec.js +++ b/core/test/integration/api/api_posts_spec.js @@ -263,7 +263,7 @@ describe('Post API', function () { }).catch(function (err) { should.exist(err); err.message.should.eql('Validation (isSlug) failed for tag'); - err.code.should.eql(422); + err.statusCode.should.eql(422); done(); }); }); @@ -274,7 +274,7 @@ describe('Post API', function () { }).catch(function (err) { should.exist(err); err.message.should.eql('Validation (isSlug) failed for author'); - err.code.should.eql(422); + err.statusCode.should.eql(422); done(); }); }); diff --git a/core/test/integration/api/api_upload_spec.js b/core/test/integration/api/api_upload_spec.js index 4bc6ec557e..d1b72d229b 100644 --- a/core/test/integration/api/api_upload_spec.js +++ b/core/test/integration/api/api_upload_spec.js @@ -42,7 +42,7 @@ describe('Upload API', function () { UploadAPI.add({uploadimage: uploadimage}).then(function () { done(new Error('Upload suceeded with invalid file.')); }, function (result) { - result.code.should.equal(415); + result.statusCode.should.equal(415); result.errorType.should.equal('UnsupportedMediaTypeError'); done(); }); @@ -59,7 +59,7 @@ describe('Upload API', function () { UploadAPI.add({uploadimage: uploadimage}).then(function () { done(new Error('Upload suceeded with invalid file.')); }, function (result) { - result.code.should.equal(415); + result.statusCode.should.equal(415); result.errorType.should.equal('UnsupportedMediaTypeError'); done(); }); @@ -88,7 +88,7 @@ describe('Upload API', function () { UploadAPI.add({uploadimage: uploadimage}).then(function () { done(new Error('Upload suceeded with invalid file.')); }, function (result) { - result.code.should.equal(415); + result.statusCode.should.equal(415); result.errorType.should.equal('UnsupportedMediaTypeError'); done(); }); diff --git a/core/test/unit/controllers/frontend/channels_spec.js b/core/test/unit/controllers/frontend/channels_spec.js index b3bd9a3744..420cd32e78 100644 --- a/core/test/unit/controllers/frontend/channels_spec.js +++ b/core/test/unit/controllers/frontend/channels_spec.js @@ -97,7 +97,7 @@ describe('Channels', function () { function testChannel404(props, done) { testChannelError(props, function (error) { error.errorType.should.eql('NotFoundError'); - error.code.should.eql(404); + error.statusCode.should.eql(404); }, done); } diff --git a/core/test/unit/error_handling_spec.js b/core/test/unit/error_handling_spec.js index 6e31bc1f47..0e938eb80f 100644 --- a/core/test/unit/error_handling_spec.js +++ b/core/test/unit/error_handling_spec.js @@ -358,6 +358,7 @@ describe('Error handling', function () { // Test that the message is correct options.message.should.equal('Page not found'); + // Template variable options.code.should.equal(404); this.statusCode.should.equal(404); @@ -389,6 +390,7 @@ describe('Error handling', function () { // Test that the message is correct options.message.should.equal('Page not found'); + // Template variable options.code.should.equal(404); this.statusCode.should.equal(404); @@ -421,6 +423,7 @@ describe('Error handling', function () { // Test that the message is correct options.message.should.equal('I am a big bad error'); + // Template variable options.code.should.equal(500); this.statusCode.should.equal(500); @@ -452,6 +455,7 @@ describe('Error handling', function () { // Test that the message is correct options.message.should.equal('I am a big bad error'); + // Template variable options.code.should.equal(500); this.statusCode.should.equal(500); @@ -469,18 +473,18 @@ describe('Error handling', function () { return res; }); - err.code = 500; + err.statusCode = 500; errors.error500(err, req, res, null); }); it('Renders custom error template if one exists', function (done) { - var code = 404, + var statusCode = 404, error = {message: 'Custom view test'}, req = { session: null }, res = { - status: function (code) { + status: function (statusCode) { /*jshint unused:false*/ return this; }, @@ -493,7 +497,7 @@ describe('Error handling', function () { }, next = null; errors.updateActiveTheme('theme-with-error'); - errors.renderErrorPage(code, error, req, res, next); + errors.renderErrorPage(statusCode, error, req, res, next); }); }); }); diff --git a/core/test/unit/rss_spec.js b/core/test/unit/rss_spec.js index 125f01b081..ecaf73b68f 100644 --- a/core/test/unit/rss_spec.js +++ b/core/test/unit/rss_spec.js @@ -417,7 +417,7 @@ describe('RSS', function () { rss(req, res, function (err) { should.exist(err); - err.code.should.eql(404); + err.statusCode.should.eql(404); res.redirect.called.should.be.false(); res.render.called.should.be.false(); done(); @@ -434,7 +434,7 @@ describe('RSS', function () { rss(req, res, function (err) { should.exist(err); - err.code.should.eql(404); + err.statusCode.should.eql(404); res.redirect.called.should.be.false(); res.render.called.should.be.false(); done();