From 2484f86d31dc4595f1a4f55ed395cea2917fc229 Mon Sep 17 00:00:00 2001
From: Christopher Van <cvan@mozilla.com>
Date: Wed, 17 Dec 2014 16:33:38 -0800
Subject: [PATCH] ensure DB errors are properly caught and internal errors are
 caught+logged but not shown in responses (fixes #259)

---
 api/controllers/game.js | 12 ++++++------
 api/models/game.js      |  5 ++---
 api/routes.js           | 27 ++++++++++++++-------------
 lib/utils/index.js      | 29 +++++++++++++++++++----------
 4 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/api/controllers/game.js b/api/controllers/game.js
index d63cb3d..1ae29e3 100644
--- a/api/controllers/game.js
+++ b/api/controllers/game.js
@@ -14,12 +14,12 @@ module.exports = {
         'VALUES ($1, $2, $3, $4, NOW()) RETURNING *',
         [game.slug, game.game_url, game.name, game.description],
         function (err, result) {
-          if (err) {
-            return reject(err);
-          }
 
-          // TODO: Throw error if row couldn't be inserted.
-          resolve(err || result.rows[0]);
+        if (err) {
+          return reject(utils.errors.DatabaseError(err));
+        }
+
+        resolve(result.rows[0]);
       });
     });
   },
@@ -28,7 +28,7 @@ module.exports = {
   get: function (request) {
     return Game.objects.get(request.pg.client, {
       idOrSlug: request.params.idOrSlug
-    }).catch(utils.returnError);
+    });
   },
   update: function () {
   }
diff --git a/api/models/game.js b/api/models/game.js
index 0fe03f7..1d7b982 100644
--- a/api/models/game.js
+++ b/api/models/game.js
@@ -30,15 +30,14 @@ Game.objects.get = function (db, data) {
 
     db.query(query, [data.idOrSlug], function (err, result) {
       if (err) {
-        return reject(err);
+        return reject(utils.errors.DatabaseError(err));
       }
 
       if (!result.rows.length) {
         return reject(utils.errors.DoesNotExist());
       }
 
-      // TODO: Throw error when row couldn't get be selected (#259).
-      resolve(err || result.rows[0]);
+      resolve(result.rows[0]);
     });
   });
 };
diff --git a/api/routes.js b/api/routes.js
index 8a799f8..211ee7e 100644
--- a/api/routes.js
+++ b/api/routes.js
@@ -1,8 +1,7 @@
-var hapi = require('hapi');
-
 var joi = require('joi');
 
 var gameController = require('./controllers/game.js');
+var utils = require('../lib/utils');
 
 
 module.exports = function (server) {
@@ -10,11 +9,10 @@ module.exports = function (server) {
     method: 'GET',
     path: '/games',
     handler: function (request, reply) {
-      gameController.get(request).then(reply, function (response) {
-        reply(response).code(response.statusCode);
-      }).catch(function (err) {
-        console.error(err);
-        reply(err);
+      gameController.get(request)
+      .then(reply)
+      .catch(function (err) {
+        reply(utils.returnError(err));
       });
     }
   });
@@ -23,11 +21,10 @@ module.exports = function (server) {
     method: 'POST',
     path: '/games',
     handler: function (request, reply) {
-      gameController.create(request).then(reply, function (response) {
-        reply(response).code(response.statusCode);
-      }).catch(function (err) {
-        console.error(err);
-        reply(err);
+      gameController.create(request)
+      .then(reply)
+      .catch(function (err) {
+        reply(utils.returnError(err));
       });
     },
     config: {
@@ -59,7 +56,11 @@ module.exports = function (server) {
     method: 'GET',
     path: '/games/{idOrSlug}',
     handler: function (request, reply) {
-      gameController.get(request).then(reply).catch(reply);
+      gameController.get(request)
+      .then(reply)
+      .catch(function (err) {
+        reply(utils.returnError(err));
+      });
     },
     // config: {
     //   validate: {
diff --git a/lib/utils/index.js b/lib/utils/index.js
index 293ac71..5a1b810 100644
--- a/lib/utils/index.js
+++ b/lib/utils/index.js
@@ -33,15 +33,24 @@ module.exports.promisify = function (func) {
 
 
 module.exports.returnError = function (err) {
-  console.error('Caught error: %s', stringifyObject(err));
-  switch (err.name) {
-    case 'DatabaseError':
-      return Boom.badImplementation(err.message || 'database_error');
-    case 'DoesNotExist':
-      return Boom.notFound(err.message || 'does_not_exist');
-    case 'ValidationError':
-      return Boom.badRequest(err.message || 'validation_error');
-    default:
-      return Boom.badImplementation();
+  if (err instanceof Object) {
+    if (err instanceof Error) {
+      console.error('Caught stack:\n%s', err.stack);
+    } else if (err.name) {
+      console.error('Caught rejection:\n%s', stringifyObject(err));
+
+      switch (err.name) {
+        case 'DatabaseError':
+          // NOTE: The response will actually contain this generic message:
+          // "An internal server error occurred"
+          return Boom.badImplementation(err.message || 'database_error');
+        case 'DoesNotExist':
+          return Boom.notFound(err.message || 'does_not_exist');
+        case 'ValidationError':
+          return Boom.badRequest(err.message || 'validation_error');
+      }
+    }
   }
+
+  return Boom.badImplementation(err);
 };