Client friendly error handling

4 files changed, 179 insertions, 150 deletions

diff --git a/app/index.js b/app/index.js
index 719c424..419adfc 100644
--- a/app/index.js
+++ b/app/index.js
@@ -36,6 +36,7 @@ function OKCMS(options) {
   var templateRoot = options.templateRoot || 'templates';
   var adminTemplateRoot = options.templateRoot ||
     path.join(__dirname, '../themes/okadmin/templates');
+  var debug = options.debug || false;
 
   // Set metadata defaults
   // TODO Abstract this out somewhere else
@@ -84,13 +85,17 @@ function OKCMS(options) {
   });
   var resourceCache = this._resourceCache =
       this._createResources(resourceConfig, db, schemas);
+  var errorHandler = createErrorHandlerProducer(
+      templateProvider, adminTemplateProvider, debug);
 
   // Create view instances from config
   var views = this._views =
-      this._createViews(viewConfig, db, meta, resourceCache, templateProvider);
+      this._createViews(viewConfig, db, meta, resourceCache, templateProvider,
+          errorHandler);
   var adminViews = this._adminViews =
       this._createAdminViews(adminPath, app, express, resourceConfig,
-          resourceCache, adminTemplateProvider, adminMeta);
+          resourceCache, adminTemplateProvider, adminMeta,
+          errorHandler);
 
   // Create services
   var imageService = OKImageService({
@@ -109,7 +114,8 @@ function OKCMS(options) {
     adminPath: adminPath,
     services: {
       image: imageService
-    }
+    },
+    errorHandler: errorHandler
   });
 }
 
@@ -152,7 +158,7 @@ OKCMS.prototype._createResources = function(resourceConfig, db, schemaCache) {
 };
 
 OKCMS.prototype._createViews = function(viewConfig, db,
-    meta, resourceCache, templateProvider) {
+    meta, resourceCache, templateProvider, errorHandler) {
   viewConfig = viewConfig || {};
   var self = this;
   var createQueries = this._createQueries.bind(this);
@@ -171,7 +177,8 @@ OKCMS.prototype._createViews = function(viewConfig, db,
       route: route,
       template: template,
       queries: queries,
-      meta: meta
+      meta: meta,
+      errorHandler: errorHandler
     });
     return cache;
   }, {});
@@ -193,7 +200,8 @@ OKCMS.prototype._createViews = function(viewConfig, db,
 };
 
 OKCMS.prototype._createAdminViews = function(path, app, express,
-  resourceConfig, resourceCache, templateProvider, meta) {
+  resourceConfig, resourceCache, templateProvider, meta,
+  errorHandler) {
   var views = {};
   var withTrail = withTrailingSlash(path);
   var withoutTrail = withoutTrailingSlash(path);
@@ -217,7 +225,8 @@ OKCMS.prototype._createAdminViews = function(path, app, express,
     resourceConfig: resourceConfig,
     resourceCache: resourceCache,
     templateProvider: templateProvider,
-    meta: meta
+    meta: meta,
+    errorHandler: errorHandler
   });
   return views;
 };
@@ -269,6 +278,47 @@ ResourceCache.prototype.get = function(type, id) {
   }
 };
 
+/**
+ * Higher order function implementing customizable error handling
+ */
+function createErrorHandlerProducer(templateProvider, adminTemplateProvider, debugMode) {
+
+  var template404 = templateProvider.getTemplate('404') ||
+      adminTemplateProvider.getTemplate('404');
+  var template5xx = templateProvider.getTemplate('5xx') ||
+      adminTemplateProvider.getTemplate('5xx');
+
+  if (!template404 || !template5xx)
+    throw new Error('No error templates provided by admin theme or user')
+
+  return debugMode ? createDebugHandler : createErrorHandler;
+
+  function createErrorHandler(req, res, status) {
+    return function handleError(err) {
+      switch (status) {
+        case 404:
+          template404.render().then(function(rendered) {
+            res.status(status);
+            res.send(rendered);
+          });
+          break;
+        default:
+          template5xx.render().then(function(rendered) {
+            res.status(status);
+            res.send(rendered);
+          });
+      }
+
+    };
+  }
+
+  function createDebugHandler(req, res, status) {
+    return function handleError(err) {
+      res.send(err.stack);
+    };
+  }
+}
+
 module.exports = {
 
   createApp: function(options) {
diff --git a/app/node_modules/okadminview/index.js b/app/node_modules/okadminview/index.js
index 2f31e1e..648487c 100644
--- a/app/node_modules/okadminview/index.js
+++ b/app/node_modules/okadminview/index.js
@@ -37,6 +37,8 @@ function OKAdminView(options) {
     throw new Error('No templateProvider provided to OKAdminView');
   if (!options.meta)
     throw new Error('No meta query provided to OKAdminView');
+  if (!options.errorHandler)
+    throw new Error('No error handler provided to OKAdminView');
 
   var app = options.app;
   var express = options.express;
@@ -44,6 +46,7 @@ function OKAdminView(options) {
   var resourceCache = this._resourceCache = options.resourceCache;
   var resourceConfig = this._resourceConfig = cloneDeep(options.resourceConfig);
   var provider = options.templateProvider;
+  var error = this._error = options.errorHandler;
 
   // Load templates
   var templates = this._templates =
@@ -112,12 +115,10 @@ function OKAdminView(options) {
       }
     }));
 
-    var auth = passport.authenticate('digest', {session: false});
-
     // This should really be mounted on the router, but can't be due to
     // https://github.com/jaredhanson/passport-http/pull/16
     app.use('/admin/', passport.initialize());
-    app.all('/admin/:path*', auth);
+    app.all('/admin/:path*', passport.authenticate('digest', {session: false}));
 
     router.get('/', function readIndex(req, res, next) {
       fetchIndexTemplateData(meta, indexQueries).then(function(data) {
@@ -125,22 +126,22 @@ function OKAdminView(options) {
           success: req.flash('success'),
           errors: req.flash('errors')
         }));
-      }).fail(errorHandler(req, res));
+      }).fail(error(req, res, 500));
     });
 
     router.get('/:type/new/', function createResourceView(req, res, next) {
       var type = req.params.type || '';
       var resource = resourceCache.get(type);
       if (!resource) {
-        errorHandler(req, res)(new Error('No such resource ' + type));
+        error(req, res, 404)(new Error('No such resource ' + type));
       } else {
         meta.get().then(function(metadata) {
-          var templateData = getResourceTemplateData(metadata, resource, {});
+          var templateData = transformData(metadata, resource, {});
           view.renderResourceNew(req, res, assign(templateData, {
             success: req.flash('success'),
             errors: req.flash('errors'),
           }));
-        }).fail(errorHandler(req, res));
+        }).fail(error(req, res, 500));
       }
     });
 
@@ -148,26 +149,27 @@ function OKAdminView(options) {
       var type = req.params.type || '';
       var id = req.params.id || '';
       var resource = resourceCache.get(type, id);
-      if (!resource) {
-        errorHandler(req, res)(new Error('No such resource'));
-      } else {
-        var query = OKQuery({
-          resource: resource,
-          query: id
-        });
-        fetchResourceTemplateData(meta, query, getResourceTemplateData)
-            .then(function(data) {
-              if (!data) {
-                resourceMissingHandler(req, res)()
-              } else {
-                view.renderResource(req, res, assign(data, {
-                  success: req.flash('success'),
-                  errors: req.flash('errors')
-                }));
-              }
-            }).fail(errorHandler(req, res));
-      }
-
+      var query = OKQuery({
+        resource: resource,
+        query: id
+      });
+      fetchResourceTemplateData(meta, query, transformData)
+          .then(function(data) {
+            if (!data) {
+              resourceMissingHandler(req, res)()
+            } else {
+              view.renderResource(req, res, assign(data, {
+                success: req.flash('success'),
+                errors: req.flash('errors')
+              }));
+            }
+          }).fail(function(err) {
+            if (err.message === 'No resource data') {
+              error(req, res, 404)(new Error('No such resource'));
+            } else {
+              error(req, res, 500)(err);
+            }
+          });
     });
 
     router.post('/:type/', function createResource(req, res, next) {
@@ -175,7 +177,7 @@ function OKAdminView(options) {
       var resource = resourceCache.get(type);
       var data = req.body;
       if (!resource) {
-        errorHandler(req, res)(new Error('No such resource ' + type));
+        error(req, res, 400)(new Error('No such resource ' + type));
       } else {
         meta.get().then(function(metadata) {
           try {
@@ -183,12 +185,12 @@ function OKAdminView(options) {
             resource.create(data).then(function(created) {
               req.flash('success', {action: 'create'});
               res.redirect(303, resource.getID(data));
-            }).fail(errorHandler(req, res));
+            }).fail(error(req, res, 500));
           } catch (errors) {
-            var templateData = getResourceTemplateData(metadata, resource, data);
+            var templateData = transformData(metadata, resource, data);
             view.renderResource(req, res, assign(templateData, {errors: errors}));
           }
-        }).fail(errorHandler(req, res));;
+        }).fail(error(req, res, 500));;
       }
     });
 
@@ -198,11 +200,9 @@ function OKAdminView(options) {
       var resourcesJSON = body[type];
       var resource = resourceCache.get(type);
       if (!resourcesJSON || !resourcesJSON.length) {
-        res.status(400);
-        errorHandler(req, res)(new Error('Bad request'));
+        error(req, res, 400)(new Error('Bad request'));
       } else if (!resource) {
-        res.status(404);
-        errorHandler(req, res)(new Error('No such resource'));
+        error(req, res, 404)(new Error('No such resource'));
       } else {
         try {
           var ids = [];
@@ -212,7 +212,7 @@ function OKAdminView(options) {
             return data;
           });
         } catch (e) {
-           errorHandler(req, res)(new Error('Resource batch contains invalid JSON'));
+           error(req, res, 500)(new Error('Resource batch contains invalid JSON'));
            return;
         }
         Q.all([
@@ -222,7 +222,7 @@ function OKAdminView(options) {
           var metadata = results.shift();
           req.flash('success', {action: 'batch_update'});
           res.redirect(303, '../..');
-        }).fail(errorHandler(req, res));
+        }).fail(error(req, res, 500));
       }
     });
 
@@ -232,7 +232,7 @@ function OKAdminView(options) {
       var data = req.body;
       var resource = resourceCache.get(type, id);
       if (!resource) {
-        errorHandler(req, res)(new Error('No such resource ' + type));
+        error(req, res, 400)(new Error('No such resource ' + type));
       } else {
         // TODO Prob should make metadata synchronous...
         meta.get().then(function(metadata) {
@@ -241,12 +241,12 @@ function OKAdminView(options) {
             resource.update(id, data).then(function(updated) {
               req.flash('success', {action: 'update'});
               res.redirect(303, '../' + resource.getID(updated));
-            }).fail(errorHandler(req, res));
+            }).fail(error(req, res, 500));
           } catch (errors) {
-            var templateData = getResourceTemplateData(metadata, resource, data);
+            var templateData = transformData(metadata, resource, data);
             view.renderResource(req, res, assign(templateData, {errors: errors}));
           }
-        }).fail(errorHandler(req, res));
+        }).fail(error(req, res, 500));
       }
     });
 
@@ -255,14 +255,14 @@ function OKAdminView(options) {
       var id = req.params.id;
       var resource = resourceCache.get(type, id);
       if (!resource) {
-        errorHandler(req, res)(new Error('No such resource ' + type));
+        error(req, res, 500)(new Error('No such resource ' + type));
       } else {
         meta.get().then(function(metadata) {
           resource.destroy(id).then(function() {
             req.flash('success', {action: 'delete'});
             res.redirect(303, '../..');
-          }).fail(errorHandler(req, res));
-        }).fail(errorHandler(req, res));
+          }).fail(error(req, res, 500));
+        }).fail(error(req, res, 500));
       }
     });
 
@@ -271,9 +271,9 @@ function OKAdminView(options) {
 }
 
 /**
- * Get template data for a single resource
+ * Yields formatted template data for a single resource
  */
-function getResourceTemplateData(meta, resource, data) {
+function transformData(meta, resource, data) {
   meta = meta || {};
   resource = resource || {};
   data = data || {};
@@ -306,21 +306,21 @@ OKAdminView.prototype.renderIndex = function(req, res, data) {
   data = data || {};
   this._templates['index'].render(data).then(function(rendered) {
     res.send(rendered);
-  }).fail(errorHandler(req, res));
+  }).fail(this._error(req, res, 500));
 };
 
 OKAdminView.prototype.renderResource = function(req, res, data) {
   data = data || {};
   this._templates['resource'].render(data).then(function(rendered) {
     res.send(rendered);
-  }).fail(errorHandler(req, res));
+  }).fail(this._error(req, res, 500));
 };
 
 OKAdminView.prototype.renderResourceNew = function(req, res, data) {
   data = data || {meta: {}, resource: {}};
   this._templates['resource_new'].render(data).then(function(rendered) {
     res.send(rendered);
-  }).fail(errorHandler(req, res));
+  }).fail(this._error(req, res, 500));
 };
 
 /**
@@ -378,30 +378,15 @@ function fetchResourceTemplateData(meta, query, fn) {
   return Q.promise(function(resolve, reject) {
     meta.get().then(function(metadata) {
       query.get().then(function(data) {
-        var resource = query.resource;
-        resolve(fn(metadata, resource, data));
+        if (!data) {
+          reject(new Error('No resource data'));
+        } else {
+          var resource = query.resource;
+          resolve(fn(metadata, resource, data));
+        }
       }).fail(reject);
     }).fail(reject)
   });
 }
 
-/**
- * TODO Real error handling
- */
-function errorHandler(req, res) {
-  return function(err) {
-    res.send(err.stack);
-  };
-}
-
-/**
- * TODO Real 404 handling
- */
-function resourceMissingHandler(req, res) {
-  return function() {
-    res.status(404);
-    res.send('404');
-  }
-}
-
 module.exports = OKAdminView;
diff --git a/app/node_modules/okserver/index.js b/app/node_modules/okserver/index.js
index cf06b3c..abca8b5 100644
--- a/app/node_modules/okserver/index.js
+++ b/app/node_modules/okserver/index.js
@@ -16,6 +16,8 @@ function OKServer(options) {
     throw new Error('No admin root directory provided to OKServer');
   if (!options.adminPath)
     throw new Error('No admin path provided to OKServer');
+  if (!options.errorHandler)
+    throw new Error('No error handler provided to OKServer');
   var root = options.root;
   var adminRoot = options.adminRoot;
   var adminPath = options.adminPath;
@@ -25,6 +27,7 @@ function OKServer(options) {
   var router = express.Router({
     strict: app.get('strict routing')
   });
+  var error = options.errorHandler;
   var services = options.services || {};
   Object.keys(views)
     // Sort such that more general routes are matched last
@@ -67,6 +70,10 @@ function OKServer(options) {
   // Make sure this lady is last. Checks whether the desired
   // route has a trailing-slash counterpart and redirects there
   app.use(slash());
+  // Otherwise it's a 404
+  app.use(function(req, res) {
+    error(req, res, 404)(new Error('No matching route'));
+  });
 }
 
 OKServer.prototype.listen = function listen(port) {
diff --git a/app/node_modules/okview/index.js b/app/node_modules/okview/index.js
index 2d1c0a0..63f22b5 100644
--- a/app/node_modules/okview/index.js
+++ b/app/node_modules/okview/index.js
@@ -24,8 +24,11 @@ function OKView(options) {
     throw new Error('No meta resource provided to OKView');
   if (!options.route)
     throw new Error('No route provided to OKView');
+  if (!options.errorHandler)
+    throw new Error('No error handler provided to OKView');
   var route = options.route;
   var mount = options.mount || 'get';
+  var error = this._error = options.errorHandler;
   this._template = options.template;
   var meta = this._meta = options.meta;
   var queries = this._queries = options.queries || [];
@@ -47,16 +50,9 @@ function OKView(options) {
     enumerable: true
   });
 
-  this._middleware = createMiddleware(this);
-  this._fetchTemplateData = unbound ? fetchUnbound : fetchBound;
-
-  function fetchUnbound(id) {
-    return fetchTemplateData(meta, queries, id)
-  }
-
-  function fetchBound() {
-    return fetchTemplateData(meta, queries);
-  }
+  this._middleware = unbound
+      ? unboundMiddleware(this, meta, queries, error)
+      : boundMiddleware(this, meta, queries, error);
 }
 
 OKView.prototype.middleware = function() {
@@ -66,39 +62,20 @@ OKView.prototype.middleware = function() {
 OKView.prototype.render = function(req, res, data) {
   this._template.render(data).then(function(html) {
     res.send(html);
-  }).fail(errorHandler(req, res, data));
-};
-
-OKView.prototype.fetchTemplateData = function() {
-  return this._fetchTemplateData.apply(this, arguments);
+  }).fail(this._error(req, res, 500));
 };
 
 /**
- * Unbound views need different middleware to resolve requests
- */
-function createMiddleware(view) {
-  if (view.unbound) {
-    return unboundMiddleware(view);
-  } else {
-    return boundMiddleware(view);
-  }
-}
-
-// Note that these middleware do not call next
-// and should thus always be added at the end of the
-// middleware chain.
-
-/**
  * Creates middleware for a view which does not
  * yet have a resource id associated with it
  */
-function unboundMiddleware(view) {
+function unboundMiddleware(view, meta, queries, error) {
   var paramName = getParamName(view.route);
   return function(req, res, next) {
     var id = req.params[paramName];
-    view.fetchTemplateData(id).then(function(data) {
+    fetchTemplateData(meta, queries, id).then(function(data) {
       view.render(req, res, data);
-    }).fail(errorHandler(req, res, next));
+    }).fail(failHandler(req, res, error));
   };
 }
 
@@ -106,20 +83,22 @@ function unboundMiddleware(view) {
  * Creates middleware for a view which already
  * has a resource id associated with it
  */
-function boundMiddleware(view) {
+function boundMiddleware(view, meta, queries, error) {
   return function(req, res, next) {
-    view.fetchTemplateData().then(function(data) {
+   fetchTemplateData(meta, queries).then(function(data) {
       view.render(req, res, data);
-    }).fail(errorHandler(req, res, next));
+    }).fail(failHandler(req, res, error));
   };
 }
 
-/**
- * TODO BS error handling for now
- */
-function errorHandler(req, res, next) {
-  return function(err) {
-    res.send(err.stack);
+function failHandler(req, res, error) {
+  return function (err) {
+    // TODO Use custom exception type
+    if (err.message === 'No resource found') {
+      error(req, res, 404)(err);
+    } else {
+      error(req, res, 500)(err);
+    }
   }
 }
 
@@ -138,9 +117,12 @@ function getParamName(route) {
  * and returns a promise for an object merging all queried
  * data, pluralizing keys where necessary.
  *
- * Lil bit convoluted, sorry.
+ * Pretty convoluted, sorry.
  */
 function fetchTemplateData(meta, queries, id) {
+  // If there's only one query, we assume it is for a single
+  // resource and will resolve errors if no data is found
+  var single = queries && queries.length === 1;
   return Q.promise(function(resolve, reject) {
     return Q.all(
       [meta.get()].concat(queries.map(function(query) {
@@ -148,39 +130,44 @@ function fetchTemplateData(meta, queries, id) {
       })))
     .then(function(results) {
       var metadata = results.shift();
-      var normalized = results.reduce(function(cache, result, i) {
-        // Could be just some rogue request
-        if (!result) {
-          return cache;
-        }
-        var resource = queries[i].resource;
-        var type = queries[i].type;
-        var manyResult = isarray(result);
-        // Inform template of ID in generic field
-        if (manyResult) {
-          result = result.map(function(data) {
-            return assign({}, data, {id: resource.getID(data)})
-          });
-        } else {
-          result = assign({}, result, {id: resource.getID(result)});
-        }
-        // If we have a lot of results for a certain type,
-        // we pluralize the key and yield an array of results
-        if (cache[type] || manyResult) {
-          var plural = pluralize(type);
-          delete cache[type];
-          cache[plural] = [];
+      if (single && !results[0]) {
+        reject(new Error('No resource found'));
+      } else {
+        var normalized = results.reduce(function(cache, result, i) {
+          // Could be just some rogue request
+          if (!result) {
+            return cache;
+          }
+          var resource = queries[i].resource;
+          var type = queries[i].type;
+          var manyResult = isarray(result);
+          // Inform template of ID in generic field
           if (manyResult) {
-            cache[plural] = cache[plural].concat(result);
+            result = result.map(function(data) {
+              return assign({}, data, {id: resource.getID(data)})
+            });
+          } else {
+            result = assign({}, result, {id: resource.getID(result)});
+          }
+          // If we have a lot of results for a certain type,
+          // we pluralize the key and yield an array of results
+          if (cache[type] || manyResult) {
+            var plural = pluralize(type);
+            delete cache[type];
+            cache[plural] = [];
+            if (manyResult) {
+              cache[plural] = cache[plural].concat(result);
+            } else {
+              cache[plural].push(result);
+            }
           } else {
-            cache[plural].push(result);
+            cache[type] = result;
           }
-        } else {
-          cache[type] = result;
-        }
-        return cache;
-      }, {meta: metadata});
-      resolve(normalized);
+          return cache;
+        }, {meta: metadata});
+
+        resolve(normalized);
+      }
     }).fail(reject);
   });
 }