From f48651e6300f1958342cc3ed15bbef47c653fd8f Mon Sep 17 00:00:00 2001 From: Sergii Mykyteiek Date: Mon, 18 Oct 2021 13:00:35 +0300 Subject: [PATCH] LT-19: Add throwing exceptions, unify error responses --- src/common/constants.ts | 1 + src/exceptions/http-exception.ts | 35 ++++++++++++++ src/middleware/error.middleware.ts | 16 ++++--- src/modules/stack/stack.controller.ts | 16 ++++++- src/modules/ttl/ttl.controller.ts | 61 ++++++++++++++++++------ src/shared/logger/index.ts | 1 + src/shared/logger/logger.interfaces.ts | 11 +++++ src/shared/logger/logger.service.ts | 10 ++++ src/tests/stack/stack.controller.spec.ts | 9 +++- src/tests/ttl/ttl.controller.spec.ts | 33 +++++++++---- 10 files changed, 160 insertions(+), 33 deletions(-) create mode 100644 src/exceptions/http-exception.ts create mode 100644 src/shared/logger/index.ts create mode 100644 src/shared/logger/logger.interfaces.ts create mode 100644 src/shared/logger/logger.service.ts diff --git a/src/common/constants.ts b/src/common/constants.ts index 5c1b3ab..11f5621 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -5,4 +5,5 @@ export const StatusCodes = { BAD_REQUEST: 400, NOT_FOUND: 404, CONFLICT: 409, + INTERNAL_SERVER: 500, }; diff --git a/src/exceptions/http-exception.ts b/src/exceptions/http-exception.ts new file mode 100644 index 0000000..a4b9a8b --- /dev/null +++ b/src/exceptions/http-exception.ts @@ -0,0 +1,35 @@ +import { StatusCodes } from '../common/constants'; + +export class HttpException extends Error { + status: number; + messages: any[]; + constructor(status, message?, messages?) { + super(message); + this.status = status; + this.messages = messages; + } +} + +export class BadRequestException extends HttpException { + constructor(message = 'Bad request', messages?) { + super(StatusCodes.BAD_REQUEST, message, messages); + } +} + +export class InternalServerErrorException extends HttpException { + constructor(message = 'Internal server error', messages?) { + super(StatusCodes.INTERNAL_SERVER, message, messages); + } +} + +export class NotFoundException extends HttpException { + constructor(message = 'Not found', messages?) { + super(StatusCodes.NOT_FOUND, message, messages); + } +} + +export class ConflictException extends HttpException { + constructor(message = 'Conflict exception', messages?) { + super(StatusCodes.CONFLICT, message, messages); + } +} diff --git a/src/middleware/error.middleware.ts b/src/middleware/error.middleware.ts index cada33f..6eb5e7b 100644 --- a/src/middleware/error.middleware.ts +++ b/src/middleware/error.middleware.ts @@ -1,9 +1,13 @@ -import { Response, Request, NextFunction } from 'express'; +import { Response, Request } from 'express'; +import { HttpException, InternalServerErrorException } from '../exceptions/http-exception'; -export const errorMiddleware = (error: Error, req: Request, res: Response, next: NextFunction) => { - // eslint-disable-next-line no-console - console.log(error); +export const errorMiddleware = (error: Error, req: Request, res: Response) => { + const exception = error instanceof HttpException ? error : new InternalServerErrorException(); - delete error.stack; - next(error); + return res.status(exception.status).json({ + statusCode: exception.status, + timestamp: new Date().toISOString(), + message: exception.message, + ...(exception.messages ? { messages: exception.messages } : {}), + }); }; diff --git a/src/modules/stack/stack.controller.ts b/src/modules/stack/stack.controller.ts index abd24b3..4e28c1b 100644 --- a/src/modules/stack/stack.controller.ts +++ b/src/modules/stack/stack.controller.ts @@ -5,6 +5,8 @@ import { StackService } from './stack.service'; import { StackResponse } from './stack.dto'; import { StackValidationService } from './validation'; import { StatusCodes } from '../../common/constants'; +import { BadRequestException } from '../../exceptions/http-exception'; +import { LoggerService } from '../../shared/logger'; @ApiPath({ name: 'stack', @@ -15,6 +17,7 @@ export class StackController { constructor( private readonly stackService: StackService, private readonly validationService: StackValidationService, + private readonly logger: LoggerService, ) {} @ApiOperationPost({ @@ -41,13 +44,19 @@ export class StackController { try { const errors = await this.validationService.validateBody(req.body); if (errors.length > 0) { - return res.status(StatusCodes.BAD_REQUEST).send({ errors }); + throw new BadRequestException('Bad request', errors); } const result = this.stackService.add(req.body); return res.status(StatusCodes.CREATED).json(result); } catch (e) { + this.logger.error({ + placement: `[${this.constructor.name}].add`, + error: e, + arguments: Array.from([req.body]), + }); + next(e); } } @@ -70,6 +79,11 @@ export class StackController { return res.status(StatusCodes.OK_RESPONSE).json(result); } catch (e) { + this.logger.error({ + placement: `[${this.constructor.name}].get`, + error: e, + }); + next(e); } } diff --git a/src/modules/ttl/ttl.controller.ts b/src/modules/ttl/ttl.controller.ts index 4318024..8df77d0 100644 --- a/src/modules/ttl/ttl.controller.ts +++ b/src/modules/ttl/ttl.controller.ts @@ -4,7 +4,13 @@ import { ApiPath, ApiOperationPost, ApiOperationGet, ApiOperationDelete } from ' import { TtlService } from './ttl.service'; import { TtlResponse } from './ttl.dto'; import { TtlValidationService } from './validation'; +import { LoggerService } from '../../shared/logger'; import { StatusCodes } from '../../common/constants'; +import { + BadRequestException, + ConflictException, + NotFoundException, +} from '../../exceptions/http-exception'; @ApiPath({ name: 'ttl', @@ -15,6 +21,7 @@ export class TtlController { constructor( private readonly ttlService: TtlService, private readonly validationService: TtlValidationService, + private readonly logger: LoggerService, ) {} @ApiOperationPost({ @@ -38,18 +45,24 @@ export class TtlController { try { const errors = await this.validationService.validateBody(req.body); if (errors.length > 0) { - return res.status(StatusCodes.BAD_REQUEST).send({ errors }); + throw new BadRequestException('Bad request', errors); } const data = this.ttlService.get(req.body.key); if (data) { - return res.status(StatusCodes.CONFLICT).json({ message: 'Key already exist' }); + throw new ConflictException(); } const result = this.ttlService.add(req.body); return res.status(StatusCodes.CREATED).json(result); } catch (e) { + this.logger.error({ + placement: `[${this.constructor.name}].add`, + error: e, + arguments: Array.from([req.body]), + }); + next(e); } } @@ -72,13 +85,23 @@ export class TtlController { 404: { description: 'Bad request' }, }, }) - get(req: Request, res: Response): Response { - const result = this.ttlService.get(req.params.key); - if (!result) { - return res.status(StatusCodes.NOT_FOUND).json({ message: 'Not found key' }); - } + get(req: Request, res: Response, next: NextFunction): Response { + try { + const result = this.ttlService.get(req.params.key); + if (!result) { + throw new NotFoundException(); + } - return res.status(StatusCodes.OK_RESPONSE).json(result); + return res.status(StatusCodes.OK_RESPONSE).json(result); + } catch (e) { + this.logger.error({ + placement: `[${this.constructor.name}].get`, + error: e, + arguments: Array.from([req.params]), + }); + + next(e); + } } @ApiOperationDelete({ @@ -99,12 +122,22 @@ export class TtlController { 404: { description: 'Not found key' }, }, }) - remove(req: Request, res: Response): Response { - const result = this.ttlService.remove(req.params.key); - if (!result) { - return res.status(StatusCodes.NOT_FOUND).json({ message: 'Not found key' }); - } + remove(req: Request, res: Response, next: NextFunction): Response { + try { + const result = this.ttlService.remove(req.params.key); + if (!result) { + throw new NotFoundException(); + } - return res.status(StatusCodes.NO_CONTENT).send(); + return res.status(StatusCodes.NO_CONTENT).send(); + } catch (e) { + this.logger.error({ + placement: `[${this.constructor.name}].remove`, + error: e, + arguments: Array.from([req.params]), + }); + + next(e); + } } } diff --git a/src/shared/logger/index.ts b/src/shared/logger/index.ts new file mode 100644 index 0000000..6820e22 --- /dev/null +++ b/src/shared/logger/index.ts @@ -0,0 +1 @@ +export * from './logger.service'; diff --git a/src/shared/logger/logger.interfaces.ts b/src/shared/logger/logger.interfaces.ts new file mode 100644 index 0000000..a850417 --- /dev/null +++ b/src/shared/logger/logger.interfaces.ts @@ -0,0 +1,11 @@ +export type LoggerMessage = + | string + | { + placement: string; + arguments?: any; + error?: Error | string; + }; + +export interface ILoggerError { + error(message: LoggerMessage): void; +} diff --git a/src/shared/logger/logger.service.ts b/src/shared/logger/logger.service.ts new file mode 100644 index 0000000..deac353 --- /dev/null +++ b/src/shared/logger/logger.service.ts @@ -0,0 +1,10 @@ +import { Service } from 'typedi'; +import { ILoggerError, LoggerMessage } from './logger.interfaces'; + +@Service() +export class LoggerService implements ILoggerError { + error(message: LoggerMessage) { + // eslint-disable-next-line no-console + console.log(JSON.stringify(message)); + } +} diff --git a/src/tests/stack/stack.controller.spec.ts b/src/tests/stack/stack.controller.spec.ts index d907765..0fa7587 100644 --- a/src/tests/stack/stack.controller.spec.ts +++ b/src/tests/stack/stack.controller.spec.ts @@ -4,6 +4,8 @@ import { StackController } from '../../modules/stack/stack.controller'; import { StackService } from '../../modules/stack/stack.service'; import { StackValidationService } from '../../modules/stack/validation'; import { StatusCodes } from '../../common/constants'; +import { BadRequestException } from '../../exceptions/http-exception'; +import { LoggerService } from '../../shared/logger'; const mockResponse = { json: jest.fn(), @@ -17,11 +19,13 @@ describe('StackController', () => { let stackController; let validateService; let stackService; + let logger; beforeEach(() => { stackController = Container.get(StackController); validateService = Container.get(StackValidationService); stackService = Container.get(StackService); + logger = Container.get(LoggerService); }); afterEach(() => { @@ -37,13 +41,14 @@ describe('StackController', () => { const errors = [new Error('Wrong data')]; jest.spyOn(stackService, 'add').mockImplementation(() => {}); jest.spyOn(validateService, 'validateBody').mockResolvedValue(errors); + jest.spyOn(logger, 'error').mockImplementation(() => {}); await stackController.add(mockRequest, mockResponse, mockNextFunction); expect(stackService.add).not.toBeCalled(); - expect(mockResponse.status).toBeCalledWith(StatusCodes.BAD_REQUEST); - expect(mockResponse.send).toBeCalledWith({ errors }); expect(mockResponse.json).not.toBeCalled(); + expect(logger.error).toBeCalled(); + expect(mockNextFunction).toBeCalledWith(new BadRequestException()); }); it('When body pass validation -> should call stackService.add, return 200', async () => { diff --git a/src/tests/ttl/ttl.controller.spec.ts b/src/tests/ttl/ttl.controller.spec.ts index 3d6c3b7..f3d6dde 100644 --- a/src/tests/ttl/ttl.controller.spec.ts +++ b/src/tests/ttl/ttl.controller.spec.ts @@ -3,7 +3,9 @@ import { Container } from 'typedi'; import { TtlController } from '../../modules/ttl/ttl.controller'; import { TtlService } from '../../modules/ttl/ttl.service'; import { TtlValidationService } from '../../modules/ttl/validation'; +import { LoggerService } from '../../shared/logger'; import { StatusCodes } from '../../common/constants'; +import { BadRequestException, ConflictException, NotFoundException } from '../../exceptions/http-exception'; const mockResponse = { json: jest.fn(), @@ -17,11 +19,13 @@ describe('TtlController', () => { let ttlController: TtlController; let validateService: TtlValidationService; let ttlService: TtlService; + let logger: LoggerService; beforeEach(() => { ttlController = Container.get(TtlController); validateService = Container.get(TtlValidationService); ttlService = Container.get(TtlService); + logger = Container.get(LoggerService); }); afterEach(() => { @@ -38,11 +42,12 @@ describe('TtlController', () => { jest.spyOn(validateService, 'validateBody').mockResolvedValue(errors); jest.spyOn(ttlService, 'get').mockImplementation(() => ({}) as any); jest.spyOn(ttlService, 'add').mockImplementation(() => ({}) as any); + jest.spyOn(logger, 'error').mockImplementation(() => ({}) as any); await ttlController.add(mockRequest, mockResponse, mockNextFunction); - expect(mockResponse.status).toBeCalledWith(StatusCodes.BAD_REQUEST); - expect(mockResponse.send).toBeCalledWith({ errors }); + expect(logger.error).toBeCalled(); + expect(mockNextFunction).toBeCalledWith(new BadRequestException()); expect(mockResponse.json).not.toBeCalled(); expect(ttlService.get).not.toBeCalled(); expect(ttlService.add).not.toBeCalled(); @@ -58,12 +63,14 @@ describe('TtlController', () => { jest.spyOn(validateService, 'validateBody').mockResolvedValue(errors); jest.spyOn(ttlService, 'get').mockImplementation(() => ({}) as any); jest.spyOn(ttlService, 'add').mockImplementation(() => ({}) as any); + jest.spyOn(logger, 'error').mockImplementation(() => ({}) as any); await ttlController.add(mockRequest, mockResponse, mockNextFunction); - expect(mockResponse.status).toBeCalledWith(StatusCodes.CONFLICT); + expect(logger.error).toBeCalled(); + expect(mockNextFunction).toBeCalledWith(new ConflictException()); expect(mockResponse.send).not.toBeCalled(); - expect(mockResponse.json).toBeCalled(); + expect(mockResponse.json).not.toBeCalled(); expect(ttlService.get).toBeCalled(); expect(ttlService.add).not.toBeCalled(); }); @@ -99,10 +106,13 @@ describe('TtlController', () => { params, }; jest.spyOn(ttlService, 'get').mockImplementation(() => null as any); + jest.spyOn(logger, 'error').mockImplementation(() => ({}) as any); - ttlController.get(mockRequest, mockResponse); + ttlController.get(mockRequest, mockResponse, mockNextFunction); - expect(mockResponse.status).toBeCalledWith(StatusCodes.NOT_FOUND); + expect(logger.error).toBeCalled(); + expect(mockNextFunction).toBeCalledWith(new NotFoundException()); + expect(mockResponse.status).not.toBeCalled(); }); it('When key exists -> should return 200', () => { @@ -115,7 +125,7 @@ describe('TtlController', () => { const response: any = {}; jest.spyOn(ttlService, 'get').mockImplementation(() => response); - ttlController.get(mockRequest, mockResponse); + ttlController.get(mockRequest, mockResponse, mockNextFunction); expect(mockResponse.status).toBeCalledWith(StatusCodes.OK_RESPONSE); expect(mockResponse.json).toBeCalledWith(response); @@ -132,10 +142,13 @@ describe('TtlController', () => { }; const response: any = null; jest.spyOn(ttlService, 'remove').mockImplementation(() => response); + jest.spyOn(logger, 'error').mockImplementation(() => ({}) as any); - ttlController.remove(mockRequest, mockResponse); + ttlController.remove(mockRequest, mockResponse, mockNextFunction); - expect(mockResponse.status).toBeCalledWith(StatusCodes.NOT_FOUND); + expect(logger.error).toBeCalled(); + expect(mockNextFunction).toBeCalledWith(new NotFoundException()); + expect(mockResponse.status).not.toBeCalled(); }); it('When key exists -> should return 204', function () { @@ -148,7 +161,7 @@ describe('TtlController', () => { const response: any = {}; jest.spyOn(ttlService, 'remove').mockImplementation(() => response); - ttlController.remove(mockRequest, mockResponse); + ttlController.remove(mockRequest, mockResponse, mockNextFunction); expect(mockResponse.status).toBeCalledWith(StatusCodes.NO_CONTENT); expect(mockResponse.send).toBeCalled(); -- GitLab