From fac4c8b35b7d2be7322564678ff7a663b5413e02 Mon Sep 17 00:00:00 2001 From: Maciej Ziarkowski Date: Fri, 31 Jan 2020 18:02:24 +0000 Subject: [PATCH] Split errors into api and general --- app/src/api/api.ts | 9 +- .../api/controllers/editHistoryController.ts | 13 ++- app/src/api/dataAccess/editHistory.ts | 91 +++++++++++-------- app/src/api/{errors.ts => errors/api.ts} | 18 ++-- app/src/api/errors/general.ts | 17 ++++ app/src/api/parameters.ts | 10 +- .../services/__tests__/editHistory.test.ts | 8 +- app/src/api/services/editHistory.ts | 4 +- 8 files changed, 110 insertions(+), 60 deletions(-) rename app/src/api/{errors.ts => errors/api.ts} (51%) create mode 100644 app/src/api/errors/general.ts diff --git a/app/src/api/api.ts b/app/src/api/api.ts index 79458ee5..efa2bc14 100644 --- a/app/src/api/api.ts +++ b/app/src/api/api.ts @@ -2,7 +2,8 @@ import bodyParser from 'body-parser'; import express from 'express'; import * as editHistoryController from './controllers/editHistoryController'; -import { RequestParameterError, UserInputError } from './errors'; +import { ApiParamError, ApiUserError } from './errors/api'; +import { DatabaseError } from './errors/general'; import buildingsRouter from './routes/buildingsRouter'; import extractsRouter from './routes/extractsRouter'; import usersRouter from './routes/usersRouter'; @@ -102,16 +103,18 @@ server.use((err: any, req: express.Request, res: express.Response, next: express if (err != undefined) { console.log('Global error handler: ', err); - if (err instanceof UserInputError) { + if (err instanceof ApiUserError) { let errorMessage: string; - if(err instanceof RequestParameterError) { + if(err instanceof ApiParamError) { errorMessage = `Problem with parameter ${err.paramName}: ${err.message}`; } else { errorMessage = err.message; } res.status(400).send({ error: errorMessage }); + } else if(err instanceof DatabaseError){ + res.status(500).send({ error: 'Database error' }); } else { res.status(500).send({ error: 'Server error' }); } diff --git a/app/src/api/controllers/editHistoryController.ts b/app/src/api/controllers/editHistoryController.ts index b6897c28..2c8dcc8d 100644 --- a/app/src/api/controllers/editHistoryController.ts +++ b/app/src/api/controllers/editHistoryController.ts @@ -1,6 +1,7 @@ import express from 'express'; -import { UserInputError } from '../errors'; +import { ApiParamError, ApiUserError } from '../errors/api'; +import { ArgumentError } from '../errors/general'; import { checkRegexParam, parsePositiveIntParam, processParam } from '../parameters'; import asyncController from "../routes/asyncController"; import * as editHistoryService from '../services/editHistory'; @@ -13,15 +14,19 @@ const getGlobalEditHistory = asyncController(async (req: express.Request, res: e const count: number = processParam(req.query, 'count', parsePositiveIntParam); if(afterId != undefined && beforeId != undefined) { - throw new UserInputError('Cannot specify both after_id and before_id parameters'); + throw new ApiUserError('Cannot specify both after_id and before_id parameters'); } try { const result = await editHistoryService.getGlobalEditHistory(beforeId, afterId, count); res.send(result); } catch(error) { - console.error(error); - res.send({ error: 'Database error' }); + if(error instanceof ArgumentError && error.argumentName === 'count') { + const apiErr = new ApiParamError(error.message, 'count'); + throw apiErr; + } + + throw error; } }); diff --git a/app/src/api/dataAccess/editHistory.ts b/app/src/api/dataAccess/editHistory.ts index b16c097c..c500f293 100644 --- a/app/src/api/dataAccess/editHistory.ts +++ b/app/src/api/dataAccess/editHistory.ts @@ -1,5 +1,6 @@ import db from '../../db'; import { EditHistoryEntry } from '../../frontend/models/edit-history-entry'; +import { DatabaseError } from '../errors/general'; const baseQuery = ` SELECT @@ -19,53 +20,69 @@ export function getHistoryAfterId(id: string, count: number): Promise $1 - ORDER BY revision_id ASC - LIMIT $2 - ) AS result_asc ORDER BY revision_id DESC`, - [id, count] - ); + try { + return db.any(` + SELECT * FROM ( + ${baseQuery} + WHERE log_id > $1 + ORDER BY revision_id ASC + LIMIT $2 + ) AS result_asc ORDER BY revision_id DESC`, + [id, count] + ); + } catch(err) { + throw new DatabaseError(err); + } } export function getHistoryBeforeId(id: string, count: number): Promise { - if(id == undefined) { - - return db.any(` - ${baseQuery} - ORDER BY revision_id DESC - LIMIT $1 - `, [count]); - - } else { - - return db.any(` - ${baseQuery} - WHERE log_id < $1 - ORDER BY revision_id DESC - LIMIT $2 - `, [id, count]); + try { + if(id == undefined) { + + return db.any(` + ${baseQuery} + ORDER BY revision_id DESC + LIMIT $1 + `, [count]); + + } else { + + return db.any(` + ${baseQuery} + WHERE log_id < $1 + ORDER BY revision_id DESC + LIMIT $2 + `, [id, count]); + } + } catch(err) { + throw new DatabaseError(err); } } export async function getIdOlderThan(id: string): Promise { - const result = await db.oneOrNone<{revision_id:string}>(` - SELECT MAX(log_id) as revision_id - FROM logs - WHERE log_id < $1 - `, [id]); + try { + const result = await db.oneOrNone<{revision_id:string}>(` + SELECT MAX(log_id) as revision_id + FROM logs + WHERE log_id < $1 + `, [id]); - return result?.revision_id; + return result?.revision_id; + } catch(err) { + throw new DatabaseError(err); + } } export async function getIdNewerThan(id: string): Promise { - const result = await db.oneOrNone<{revision_id:string}>(` - SELECT MIN(log_id) as revision_id - FROM logs - WHERE log_id > $1 - `, [id]); + try { + const result = await db.oneOrNone<{revision_id:string}>(` + SELECT MIN(log_id) as revision_id + FROM logs + WHERE log_id > $1 + `, [id]); - return result?.revision_id; + return result?.revision_id; + } catch(err) { + throw new DatabaseError(err); + } } diff --git a/app/src/api/errors.ts b/app/src/api/errors/api.ts similarity index 51% rename from app/src/api/errors.ts rename to app/src/api/errors/api.ts index e1d1bd35..cbc4ba2f 100644 --- a/app/src/api/errors.ts +++ b/app/src/api/errors/api.ts @@ -5,34 +5,40 @@ * https://stackoverflow.com/questions/41102060/typescript-extending-error-class */ -export class UserInputError extends Error { +export class ApiUserError extends Error { constructor(message?: string) { super(message); + this.name = 'ApiUserError'; } } -export class RequestParameterError extends UserInputError { +export class ApiParamError extends ApiUserError { public paramName: string; - constructor(message?: string) { + constructor(message?: string, paramName?: string) { super(message); + this.name = 'ApiParamError'; + this.paramName = paramName; } } -export class ParamRequiredError extends RequestParameterError { +export class ApiParamRequiredError extends ApiParamError { constructor(message?: string) { super(message); + this.name = 'ApiParamRequiredError'; } } -export class ParamOutOfBoundsError extends RequestParameterError { +export class ApiParamOutOfBoundsError extends ApiParamError { constructor(message?: string) { super(message); + this.name = 'ApiParamOutOfBoundsError'; } } -export class ParamInvalidFormatError extends RequestParameterError { +export class ApiParamInvalidFormatError extends ApiParamError { constructor(message?: string) { super(message); + this.name = 'ApiParamInvalidFormatError'; } } diff --git a/app/src/api/errors/general.ts b/app/src/api/errors/general.ts new file mode 100644 index 00000000..faacdb41 --- /dev/null +++ b/app/src/api/errors/general.ts @@ -0,0 +1,17 @@ +export class ArgumentError extends Error { + public argumentName: string; + constructor(message?: string, argumentName?: string) { + super(message); + this.name = 'ArgumentError'; + this.argumentName = argumentName; + } +} + +export class DatabaseError extends Error { + public detail: any; + constructor(detail?: string) { + super(); + this.name = 'DatabaseError'; + this.detail = detail; + } +} diff --git a/app/src/api/parameters.ts b/app/src/api/parameters.ts index 19c84fcb..865c0947 100644 --- a/app/src/api/parameters.ts +++ b/app/src/api/parameters.ts @@ -1,13 +1,13 @@ import { strictParseInt } from '../parse'; -import { ParamInvalidFormatError, ParamRequiredError, RequestParameterError } from './errors'; +import { ApiParamError, ApiParamInvalidFormatError, ApiParamRequiredError } from './errors/api'; export function processParam(params: object, paramName: string, processingFn: (x: string) => T, required: boolean = false) { const stringValue = params[paramName]; if(stringValue == undefined && required) { - const err = new ParamRequiredError('Parameter required but not supplied'); + const err = new ApiParamRequiredError('Parameter required but not supplied'); err.paramName = paramName; throw err; } @@ -15,7 +15,7 @@ export function processParam(params: object, paramName: string, processingFn: try { return processingFn(stringValue); } catch(error) { - if(error instanceof RequestParameterError) { + if(error instanceof ApiParamError) { error.paramName = paramName; } @@ -28,7 +28,7 @@ export function parsePositiveIntParam(param: string) { const result = strictParseInt(param); if (isNaN(result)) { - throw new ParamInvalidFormatError('Invalid format: not a positive integer'); + throw new ApiParamInvalidFormatError('Invalid format: not a positive integer'); } return result; } @@ -37,7 +37,7 @@ export function checkRegexParam(param: string, regex: RegExp): string { if(param == undefined) return undefined; if(param.match(regex) == undefined) { - throw new ParamInvalidFormatError(`Invalid format: does not match regular expression ${regex}`); + throw new ApiParamInvalidFormatError(`Invalid format: does not match regular expression ${regex}`); } return param; diff --git a/app/src/api/services/__tests__/editHistory.test.ts b/app/src/api/services/__tests__/editHistory.test.ts index 9876d5ac..e8fd2c6e 100644 --- a/app/src/api/services/__tests__/editHistory.test.ts +++ b/app/src/api/services/__tests__/editHistory.test.ts @@ -1,6 +1,6 @@ import { EditHistoryEntry } from '../../../frontend/models/edit-history-entry'; import * as editHistoryData from '../../dataAccess/editHistory'; // manually mocked -import { UserInputError } from '../../errors'; +import { ArgumentError } from '../../errors/general'; import { getGlobalEditHistory } from '../editHistory'; jest.mock('../../dataAccess/editHistory'); @@ -28,8 +28,10 @@ describe('getGlobalEditHistory()', () => { [null, null], ['100', null], [null, '100'] - ])('Should error when requesting non-positive number of records', (beforeId: string, afterId: string) => { - expect(getGlobalEditHistory(beforeId, afterId, 0)).rejects.toBeInstanceOf(UserInputError); + ])('Should error when requesting non-positive number of records', async (beforeId: string, afterId: string) => { + let resultPromise = getGlobalEditHistory(beforeId, afterId, 0); + await expect(resultPromise).rejects.toBeInstanceOf(ArgumentError); + await expect(resultPromise).rejects.toHaveProperty('argumentName', 'count'); }); describe('getting history before a point', () => { diff --git a/app/src/api/services/editHistory.ts b/app/src/api/services/editHistory.ts index 40299fe1..81b4c899 100644 --- a/app/src/api/services/editHistory.ts +++ b/app/src/api/services/editHistory.ts @@ -1,10 +1,10 @@ import { EditHistoryEntry } from '../../frontend/models/edit-history-entry'; import { decBigInt, incBigInt } from '../../helpers'; import { getHistoryAfterId, getHistoryBeforeId, getIdNewerThan, getIdOlderThan } from '../dataAccess/editHistory'; -import { UserInputError } from '../errors'; +import { ArgumentError } from '../errors/general'; async function getGlobalEditHistory(beforeId?: string, afterId?: string, count: number = 100) { - if(count <= 0) throw new UserInputError('cannot request less than 1 history record'); + if(count <= 0) throw new ArgumentError('cannot request less than 1 history record', 'count'); if(count > 100) count = 100; // limited set of records. Expected to be already ordered from newest to oldest