Skip to content

Commit 8fe4e6f

Browse files
author
Arthur Cinader
committed
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! :)
1 parent 43c3d8e commit 8fe4e6f

File tree

7 files changed

+93
-30
lines changed

7 files changed

+93
-30
lines changed

spec/CloudCodeLogger.spec.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ describe("Cloud Code Logger", () => {
194194
Parse.Cloud.run('aFunction', { foo: 'bar' })
195195
.then(null, () => logController.getLogs({ from: Date.now() - 500, size: 1000 }))
196196
.then(logs => {
197-
const log = logs[1];
197+
const log = logs[2];
198198
expect(log.level).toEqual('error');
199199
expect(log.message).toMatch(
200200
/Failed running cloud function aFunction for user [^ ]* with:\n {2}Input: {"foo":"bar"}\n {2}Error: {"code":141,"message":"it failed!"}/);

spec/FilesController.spec.js

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,17 @@
1-
var GridStoreAdapter = require("../src/Adapters/Files/GridStoreAdapter").GridStoreAdapter;
2-
var Config = require("../src/Config");
3-
var FilesController = require('../src/Controllers/FilesController').default;
1+
const LoggerController = require('../src/Controllers/LoggerController').LoggerController;
2+
const WinstonLoggerAdapter = require('../src/Adapters/Logger/WinstonLoggerAdapter').WinstonLoggerAdapter;
3+
const GridStoreAdapter = require("../src/Adapters/Files/GridStoreAdapter").GridStoreAdapter;
4+
const Config = require("../src/Config");
5+
const FilesController = require('../src/Controllers/FilesController').default;
46

7+
const mockAdapter = {
8+
createFile: () => {
9+
return Parse.Promise.reject(new Error('it failed'));
10+
},
11+
deleteFile: () => { },
12+
getFileData: () => { },
13+
getFileLocation: () => 'xyz'
14+
}
515

616
// Small additional tests to improve overall coverage
717
describe("FilesController",() =>{
@@ -26,5 +36,25 @@ describe("FilesController",() =>{
2636
expect(anObject.aFile.url).toEqual("http://an.url");
2737

2838
done();
29-
})
39+
});
40+
41+
it('should create a server log on failure', done => {
42+
const logController = new LoggerController(new WinstonLoggerAdapter());
43+
44+
reconfigureServer({ filesAdapter: mockAdapter })
45+
.then(() => new Parse.File("yolo.txt", [1,2,3], "text/plain").save())
46+
.then(
47+
() => done.fail('should not succeed'),
48+
() => logController.getLogs({ from: Date.now() - 500, size: 1000 })
49+
)
50+
.then((logs) => {
51+
const log = logs.pop();
52+
expect(log.level).toBe('error');
53+
expect(log.code).toBe(130);
54+
expect(log.message).toBe('Could not store file.');
55+
expect(log.serverLogError).toBe('it failed');
56+
expect(log.stack).not.toBe(undefined);
57+
done();
58+
});
59+
});
3060
});

src/Controllers/FilesController.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ export class FilesController extends AdaptableController {
1414
}
1515

1616
createFile(config, filename, data, contentType) {
17-
1817
const extname = path.extname(filename);
1918

2019
const hasExtension = extname.length > 0;
@@ -26,7 +25,6 @@ export class FilesController extends AdaptableController {
2625
}
2726

2827
filename = randomHexString(32) + '_' + filename;
29-
3028
var location = this.adapter.getFileLocation(config, filename);
3129
return this.adapter.createFile(filename, data, contentType).then(() => {
3230
return Promise.resolve({

src/ParseServer.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,4 +394,6 @@ function addParseCloud() {
394394
global.Parse = Parse;
395395
}
396396

397+
ParseServer.Error = require('./ServerError').default;
398+
397399
export default ParseServer;

src/Routers/FilesRouter.js

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import express from 'express';
2-
import BodyParser from 'body-parser';
3-
import * as Middlewares from '../middlewares';
4-
import Parse from 'parse/node';
5-
import Config from '../Config';
6-
import mime from 'mime';
1+
import express from 'express';
2+
import BodyParser from 'body-parser';
3+
import * as Middlewares from '../middlewares';
4+
import Parse from 'parse/node';
5+
import Config from '../Config';
6+
import mime from 'mime';
7+
import ParseServer from '../ParseServer';
78

89
export class FilesRouter {
910

@@ -87,8 +88,9 @@ export class FilesRouter {
8788
res.status(201);
8889
res.set('Location', result.url);
8990
res.json(result);
90-
}).catch(() => {
91-
next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file.'));
91+
}).catch((e) => {
92+
const serverError = new ParseServer.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file.', e.message, e.stack);
93+
next(serverError);
9294
});
9395
}
9496

src/ServerError.js

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import Parse from 'parse';
2+
3+
/**
4+
* Constructs a new ServerError object with the given message and source.
5+
*
6+
* The message is an error that is intended for the server logs
7+
* while the source message is intended for the server logs to aid a developer
8+
*
9+
* @class ServerError
10+
* @constructor
11+
* @param {ErrorCode} code the Parse.Error code
12+
* @param {String} message will be sent to clients
13+
* @param {String} serverLogError additional information
14+
* about the error for logging on the server
15+
* @param {String} stack the stack trace
16+
*/
17+
export default class ServerError extends Parse.Error {
18+
serverLogError;
19+
stack;
20+
21+
constructor(code, message, serverLogError, stack) {
22+
super(code, message);
23+
this.serverLogError = serverLogError;
24+
this.stack = stack;
25+
}
26+
}

src/middlewares.js

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import AppCache from './cache';
2-
import log from './logger';
3-
import Parse from 'parse/node';
4-
import auth from './Auth';
5-
import Config from './Config';
6-
import ClientSDK from './ClientSDK';
1+
import AppCache from './cache';
2+
import log from './logger';
3+
import Parse from 'parse/node';
4+
import auth from './Auth';
5+
import Config from './Config';
6+
import ClientSDK from './ClientSDK';
7+
import ParseServer from './ParseServer';
78

89
// Checks that the request is authorized for this app and checks user
910
// auth too.
@@ -233,10 +234,8 @@ export function allowMethodOverride(req, res, next) {
233234
}
234235

235236
export function handleParseErrors(err, req, res, next) {
236-
// TODO: Add logging as those errors won't make it to the PromiseRouter
237-
if (err instanceof Parse.Error) {
238-
var httpStatus;
239-
237+
if (err instanceof Parse.Error || err instanceof ParseServer.Error) {
238+
let httpStatus;
240239
// TODO: fill out this mapping
241240
switch (err.code) {
242241
case Parse.Error.INTERNAL_SERVER_ERROR:
@@ -250,17 +249,23 @@ export function handleParseErrors(err, req, res, next) {
250249
}
251250

252251
res.status(httpStatus);
253-
res.json({code: err.code, error: err.message});
252+
res.json({ code: err.code, error: err.message });
253+
log.error(err.message, err);
254254
} else if (err.status && err.message) {
255+
log.error(err.message);
255256
res.status(err.status);
256-
res.json({error: err.message});
257+
res.json({ error: err.message });
258+
next(err);
257259
} else {
258260
log.error('Uncaught internal server error.', err, err.stack);
259261
res.status(500);
260-
res.json({code: Parse.Error.INTERNAL_SERVER_ERROR,
261-
message: 'Internal server error.'});
262+
res.json({
263+
code: Parse.Error.INTERNAL_SERVER_ERROR,
264+
message: 'Internal server error.'
265+
});
266+
next(err);
262267
}
263-
next(err);
268+
264269
}
265270

266271
export function enforceMasterKeyAccess(req, res, next) {

0 commit comments

Comments
 (0)