From 145a1fe23b6f20567580e54101a20bcbc7ecf446 Mon Sep 17 00:00:00 2001
From: Arthur Cinader <arthur@sugarinc.com>
Date: Tue, 24 Jan 2017 11:30:37 -0800
Subject: [PATCH] Log Parse Errors so they are intelligible.

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, on the server, the log is just [object, object]

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! :)

Instead, we'll log the error ourselves using the ParseServer's logger.

fixes: #661
---
 spec/CloudCodeLogger.spec.js |  2 +-
 spec/FilesController.spec.js | 38 ++++++++++++++++++++++++++++++++----
 src/middlewares.js           | 31 ++++++++++++++++-------------
 3 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/spec/CloudCodeLogger.spec.js b/spec/CloudCodeLogger.spec.js
index 0ded5e04ea..8a0cd92fb5 100644
--- a/spec/CloudCodeLogger.spec.js
+++ b/spec/CloudCodeLogger.spec.js
@@ -194,7 +194,7 @@ describe("Cloud Code Logger", () => {
     Parse.Cloud.run('aFunction', { foo: 'bar' })
             .then(null, () => logController.getLogs({ from: Date.now() - 500, size: 1000 }))
             .then(logs => {
-              const log = logs[1];
+              const log = logs[2];
               expect(log.level).toEqual('error');
               expect(log.message).toMatch(
                     /Failed running cloud function aFunction for user [^ ]* with:\n {2}Input: {"foo":"bar"}\n {2}Error: {"code":141,"message":"it failed!"}/);
diff --git a/spec/FilesController.spec.js b/spec/FilesController.spec.js
index d6e9e4cf43..66897ce781 100644
--- a/spec/FilesController.spec.js
+++ b/spec/FilesController.spec.js
@@ -1,7 +1,17 @@
-var GridStoreAdapter = require("../src/Adapters/Files/GridStoreAdapter").GridStoreAdapter;
-var Config = require("../src/Config");
-var FilesController = require('../src/Controllers/FilesController').default;
+const LoggerController = require('../src/Controllers/LoggerController').LoggerController;
+const WinstonLoggerAdapter = require('../src/Adapters/Logger/WinstonLoggerAdapter').WinstonLoggerAdapter;
+const GridStoreAdapter = require("../src/Adapters/Files/GridStoreAdapter").GridStoreAdapter;
+const Config = require("../src/Config");
+const FilesController = require('../src/Controllers/FilesController').default;
 
+const mockAdapter = {
+  createFile: () => {
+    return Parse.Promise.reject(new Error('it failed'));
+  },
+  deleteFile: () => { },
+  getFileData: () => { },
+  getFileLocation: () => 'xyz'
+}
 
 // Small additional tests to improve overall coverage
 describe("FilesController",() =>{
@@ -26,5 +36,25 @@ describe("FilesController",() =>{
     expect(anObject.aFile.url).toEqual("http://an.url");
 
     done();
-  })
+  });
+
+  it('should create a server log on failure', done => {
+    const logController = new LoggerController(new WinstonLoggerAdapter());
+
+    reconfigureServer({ filesAdapter: mockAdapter })
+      .then(() => new Promise(resolve => setTimeout(resolve, 1000)))
+      .then(() => new Parse.File("yolo.txt", [1,2,3], "text/plain").save())
+      .then(
+        () => done.fail('should not succeed'),
+        () => setImmediate(() => Parse.Promise.as('done'))
+      )
+      .then(() => logController.getLogs({ from: Date.now() - 500, size: 1000 }))
+      .then((logs) => {
+        const log = logs.pop();
+        expect(log.level).toBe('error');
+        expect(log.code).toBe(130);
+        expect(log.message).toBe('Could not store file.');
+        done();
+      });
+  });
 });
diff --git a/src/middlewares.js b/src/middlewares.js
index 482c773ba6..2edbb3b1c2 100644
--- a/src/middlewares.js
+++ b/src/middlewares.js
@@ -1,9 +1,9 @@
-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';
 
 // Checks that the request is authorized for this app and checks user
 // auth too.
@@ -233,10 +233,8 @@ export function allowMethodOverride(req, res, 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;
-
+    let httpStatus;
     // TODO: fill out this mapping
     switch (err.code) {
     case Parse.Error.INTERNAL_SERVER_ERROR:
@@ -250,17 +248,22 @@ export function handleParseErrors(err, req, res, next) {
     }
 
     res.status(httpStatus);
-    res.json({code: err.code, error: err.message});
+    res.json({ code: err.code, error: err.message });
+    log.error(err.message, err);
   } else if (err.status && err.message) {
     res.status(err.status);
-    res.json({error: err.message});
+    res.json({ error: err.message });
+    next(err);
   } else {
     log.error('Uncaught internal server error.', err, err.stack);
     res.status(500);
-    res.json({code: Parse.Error.INTERNAL_SERVER_ERROR,
-      message: 'Internal server error.'});
+    res.json({
+      code: Parse.Error.INTERNAL_SERVER_ERROR,
+      message: 'Internal server error.'
+    });
+    next(err);
   }
-  next(err);
+
 }
 
 export function enforceMasterKeyAccess(req, res, next) {