Skip to content

Commit

Permalink
Log Parse Errors so they are intelligible.
Browse files Browse the repository at this point in the history
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: parse-community#661
  • Loading branch information
Arthur Cinader committed Jan 28, 2017
1 parent 43c3d8e commit 145a1fe
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 19 deletions.
2 changes: 1 addition & 1 deletion spec/CloudCodeLogger.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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!"}/);
Expand Down
38 changes: 34 additions & 4 deletions spec/FilesController.spec.js
Original file line number Diff line number Diff line change
@@ -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",() =>{
Expand All @@ -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();
});
});
});
31 changes: 17 additions & 14 deletions src/middlewares.js
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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:
Expand All @@ -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) {
Expand Down

0 comments on commit 145a1fe

Please sign in to comment.