From 0feb3429763327810d03571bbbacae0c852a6a7e Mon Sep 17 00:00:00 2001
From: Arthur Cinader <arthur@sugarinc.com>
Date: Tue, 24 Jan 2017 11:30:37 -0800
Subject: [PATCH] Create a ServerError class.

The problem this pr is trying to solve:

When an error occurs on the server, a message should
be returned to the client, and a message should be logged.

Currently, noting is often logged.

Byt adding a ServerError class, we can embed a ParseError into
the ServerError.  The ServerError contains the message for the log
and the ParseError contains the error for the client.

In addition, this pr will stop calling the default express error handler
which causes two problems: 1. it writes to console instead of log file
2. the output is completely useless! :)
---
 src/ParseServer.js         |  2 ++
 src/Routers/FilesRouter.js | 19 +++++++++++--------
 src/ServerError.js         | 19 +++++++++++++++++++
 src/middlewares.js         | 33 ++++++++++++++++++++-------------
 4 files changed, 52 insertions(+), 21 deletions(-)
 create mode 100644 src/ServerError.js

diff --git a/src/ParseServer.js b/src/ParseServer.js
index 11cb38f56e..06e38e1756 100644
--- a/src/ParseServer.js
+++ b/src/ParseServer.js
@@ -394,4 +394,6 @@ function addParseCloud() {
   global.Parse = Parse;
 }
 
+ParseServer.Error = require('./ServerError').default;
+
 export default ParseServer;
diff --git a/src/Routers/FilesRouter.js b/src/Routers/FilesRouter.js
index f58971dfd4..0b5d73beb9 100644
--- a/src/Routers/FilesRouter.js
+++ b/src/Routers/FilesRouter.js
@@ -1,9 +1,10 @@
-import express             from 'express';
-import BodyParser          from 'body-parser';
-import * as Middlewares    from '../middlewares';
-import Parse               from 'parse/node';
-import Config              from '../Config';
-import mime                from 'mime';
+import express from 'express';
+import BodyParser from 'body-parser';
+import * as Middlewares from '../middlewares';
+import Parse from 'parse/node';
+import Config from '../Config';
+import mime from 'mime';
+import ParseServer from '../ParseServer';
 
 export class FilesRouter {
 
@@ -87,8 +88,10 @@ export class FilesRouter {
       res.status(201);
       res.set('Location', result.url);
       res.json(result);
-    }).catch(() => {
-      next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file.'));
+    }).catch((e) => {
+      const parseError = new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file.');
+      const parseServerError = new ParseServer.Error(e.message, parseError);
+      next(parseServerError);
     });
   }
 
diff --git a/src/ServerError.js b/src/ServerError.js
new file mode 100644
index 0000000000..254e0f31ac
--- /dev/null
+++ b/src/ServerError.js
@@ -0,0 +1,19 @@
+/**
+ * Constructs a new ServerError object with the given message and source.
+ *
+ * The message is an error that the consumer of the server API will see,
+ * while the source message is intended for the server logs to aid a developer
+ *
+ * @class ServerError
+ * @constructor
+ * @param {String} message A detailed description of the error.
+ * @param {ParseError} source The
+ */
+export default class ServerError extends Error {
+  source;
+
+  constructor(message, source) {
+    super(message);
+    this.source = source;
+  }
+}
diff --git a/src/middlewares.js b/src/middlewares.js
index 482c773ba6..fb7cf2b51f 100644
--- a/src/middlewares.js
+++ b/src/middlewares.js
@@ -1,9 +1,10 @@
-import AppCache   from './cache';
-import log        from './logger';
-import Parse      from 'parse/node';
-import auth       from './Auth';
-import Config     from './Config';
-import ClientSDK  from './ClientSDK';
+import AppCache from './cache';
+import log from './logger';
+import Parse from 'parse/node';
+import auth from './Auth';
+import Config from './Config';
+import ClientSDK from './ClientSDK';
+import ParseServer from './ParseServer';
 
 // Checks that the request is authorized for this app and checks user
 // auth too.
@@ -232,13 +233,13 @@ export function allowMethodOverride(req, res, next) {
   next();
 }
 
-export function handleParseErrors(err, req, res, next) {
-  // TODO: Add logging as those errors won't make it to the PromiseRouter
-  if (err instanceof Parse.Error) {
-    var httpStatus;
+export function handleParseErrors(err, req, res) {
+  if (err instanceof Parse.Error || err instanceof ParseServer.Error) {
+    const parseError = err instanceof Parse.Error ? err : err.source;
+    let httpStatus;
 
     // TODO: fill out this mapping
-    switch (err.code) {
+    switch (parseError.code) {
     case Parse.Error.INTERNAL_SERVER_ERROR:
       httpStatus = 500;
       break;
@@ -249,9 +250,11 @@ export function handleParseErrors(err, req, res, next) {
       httpStatus = 400;
     }
 
+    log.error(err.message);
     res.status(httpStatus);
-    res.json({code: err.code, error: err.message});
+    res.json({code: parseError.code, error: parseError.message});
   } else if (err.status && err.message) {
+    log.error(err.message);
     res.status(err.status);
     res.json({error: err.message});
   } else {
@@ -260,7 +263,11 @@ export function handleParseErrors(err, req, res, next) {
     res.json({code: Parse.Error.INTERNAL_SERVER_ERROR,
       message: 'Internal server error.'});
   }
-  next(err);
+
+  // this is the end of the road.  Don't call next()
+  // or else the error will go to the default express
+  // error handler.
+
 }
 
 export function enforceMasterKeyAccess(req, res, next) {