Improving error handling

TryGhost · Jul 28, 2014 · e7dc51d · e7dc51d
1 parent 4e3b21b
commit e7dc51d
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 13 deletions.
diff --git a/core/server/api/utils.js b/core/server/api/utils.js
@@ -16,7 +16,7 @@ utils = {
      */
     checkObject: function (object, docName) {
         if (_.isEmpty(object) || _.isEmpty(object[docName]) || _.isEmpty(object[docName][0])) {
-            return when.reject(new errors.BadRequestError('No root key (\'' + docName + '\') provided.'));
+            return errors.logAndRejectError(new errors.BadRequestError('No root key (\'' + docName + '\') provided.'));
         }
 
         // convert author property to author_id to match the name in the database
@@ -27,7 +27,7 @@ utils = {
                 delete object.posts[0].author;
             }
         }
-        return when.resolve(object);
+        return when(object);
     }
 };
 

diff --git a/core/server/errors/index.js b/core/server/errors/index.js
@@ -33,7 +33,7 @@ errors = {
 
     throwError: function (err) {
         if (!err) {
-            err = new Error("An error occurred");
+            err = new Error('An error occurred');
         }
 
         if (_.isString(err)) {
@@ -137,7 +137,7 @@ errors = {
     logAndRejectError: function (err, context, help) {
         this.logError(err, context, help);
 
-        this.rejectError(err, context, help);
+        return this.rejectError(err, context, help);
     },
 
     logErrorWithRedirect: function (msg, context, help, redirectTo, req, res) {
@@ -153,6 +153,22 @@ errors = {
         };
     },
 
+    handleAPIError: function (error) {
+        if (!error) {
+            return this.rejectError(new this.NoPermissionError('You do not have permission to perform this action'));
+        }
+
+        if (_.isString(error)) {
+            return this.rejectError(new this.NoPermissionError(error));
+        }
+
+        if (error.type) {
+            return this.rejectError(error);
+        }
+
+        return this.rejectError(new this.InternalServerError(error));
+    },
+
     renderErrorPage: function (code, err, req, res, next) {
         /*jshint unused:false*/
         var self = this;
@@ -207,17 +223,18 @@ errors = {
 
                 // And then try to explain things to the user...
                 // Cheat and output the error using handlebars escapeExpression
-                return res.send(500, "<h1>Oops, seems there is an an error in the error template.</h1>"
-                    + "<p>Encountered the error: </p>"
-                    + "<pre>" + hbs.handlebars.Utils.escapeExpression(templateErr.message || templateErr) + "</pre>"
-                    + "<br ><p>whilst trying to render an error page for the error: </p>"
-                    + code + " " + "<pre>"  + hbs.handlebars.Utils.escapeExpression(err.message || err) + "</pre>"
-                    );
+                return res.send(500,
+                    '<h1>Oops, seems there is an an error in the error template.</h1>' +
+                    '<p>Encountered the error: </p>' +
+                    '<pre>' + hbs.handlebars.Utils.escapeExpression(templateErr.message || templateErr) + '</pre>' +
+                    '<br ><p>whilst trying to render an error page for the error: </p>' +
+                    code + ' ' + '<pre>'  + hbs.handlebars.Utils.escapeExpression(err.message || err) + '</pre>'
+                );
             });
         }
 
         if (code >= 500) {
-            this.logError(err, "Rendering Error Page", "Ghost caught a processing error in the middleware layer.");
+            this.logError(err, 'Rendering Error Page', 'Ghost caught a processing error in the middleware layer.');
         }
 
         // Are we admin? If so, don't worry about the user template
@@ -230,7 +247,7 @@ errors = {
     },
 
     error404: function (req, res, next) {
-        var message = res.isAdmin && req.user ? "No Ghost Found" : "Page Not Found";
+        var message = res.isAdmin && req.user ? 'No Ghost Found' : 'Page Not Found';
 
         // do not cache 404 error
         res.set({'Cache-Control': 'no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0'});
@@ -271,8 +288,16 @@ errors = {
 // Ensure our 'this' context for methods and preserve method arity by
 // using Function#bind for expressjs
 _.each([
+    'logWarn',
+    'logInfo',
+    'rejectError',
+    'throwError',
+    'logError',
     'logAndThrowError',
+    'logAndRejectError',
+    'logErrorAndExit',
     'logErrorWithRedirect',
+    'handleAPIError',
     'renderErrorPage',
     'error404',
     'error500'

diff --git a/core/test/utils/api.js b/core/test/utils/api.js
@@ -45,14 +45,14 @@ function getAdminURL() {
 
 // make sure the API only returns expected properties only
 function checkResponseValue(jsonResponse, properties) {
-    Object.keys(jsonResponse).length.should.eql(properties.length);
     for (var i = 0; i < properties.length; i = i + 1) {
         // For some reason, settings response objects do not have the 'hasOwnProperty' method
         if (Object.prototype.hasOwnProperty.call(jsonResponse, properties[i])) {
             continue;
         }
         jsonResponse.should.have.property(properties[i]);
     }
+    Object.keys(jsonResponse).length.should.eql(properties.length);
 }
 
 function checkResponse(jsonResponse, objectType, additionalProperties) {