From 2d0a9d1cf4a29aefdcbc9ad6fc978301d62367a8 Mon Sep 17 00:00:00 2001 From: Ptroger <44851272+Ptroger@users.noreply.github.com> Date: Thu, 21 Mar 2024 07:53:32 -0400 Subject: [PATCH] add error management for failed Validation Pipes (#169) * add error management for failed Validation Pipes * format * test exception on ping controller * registered exception filters to policy-engine * format * Update apps/armory/src/shared/filter/application-exception.filter.ts Co-authored-by: William Calderipe * ported filter test file --------- Co-authored-by: William Calderipe --- apps/armory/src/main.ts | 7 +- .../shared/filter/http-exception.filter.ts | 36 ++++++++ .../src/engine/app.controller.ts | 1 - apps/policy-engine/src/main.ts | 19 +++++ .../unit/application-exception.filter.spec.ts | 82 +++++++++++++++++++ .../filter/application-exception.filter.ts | 52 ++++++++++++ .../shared/filter/http-exception.filter.ts | 41 ++++++++++ 7 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 apps/armory/src/shared/filter/http-exception.filter.ts create mode 100644 apps/policy-engine/src/shared/filter/__test__/unit/application-exception.filter.spec.ts create mode 100644 apps/policy-engine/src/shared/filter/application-exception.filter.ts create mode 100644 apps/policy-engine/src/shared/filter/http-exception.filter.ts diff --git a/apps/armory/src/main.ts b/apps/armory/src/main.ts index 1fa31cfa1..f8bf31edb 100644 --- a/apps/armory/src/main.ts +++ b/apps/armory/src/main.ts @@ -6,6 +6,7 @@ import { lastValueFrom, map, of, switchMap } from 'rxjs' import { Config } from './armory.config' import { ArmoryModule } from './armory.module' import { ApplicationExceptionFilter } from './shared/filter/application-exception.filter' +import { HttpExceptionFilter } from './shared/filter/http-exception.filter' import { ZodExceptionFilter } from './shared/filter/zod-exception.filter' /** @@ -64,7 +65,11 @@ const withGlobalInterceptors = (app: INestApplication): INestApplication => { const withGlobalFilters = (configService: ConfigService) => (app: INestApplication): INestApplication => { - app.useGlobalFilters(new ApplicationExceptionFilter(configService), new ZodExceptionFilter(configService)) + app.useGlobalFilters( + new ApplicationExceptionFilter(configService), + new ZodExceptionFilter(configService), + new HttpExceptionFilter(configService) + ) return app } diff --git a/apps/armory/src/shared/filter/http-exception.filter.ts b/apps/armory/src/shared/filter/http-exception.filter.ts new file mode 100644 index 000000000..ef32f857d --- /dev/null +++ b/apps/armory/src/shared/filter/http-exception.filter.ts @@ -0,0 +1,36 @@ +import { ArgumentsHost, Catch, ExceptionFilter, HttpException, Logger } from '@nestjs/common' +import { ConfigService } from '@nestjs/config' +import { Response } from 'express' +import { Config, Env } from '../../armory.config' + +@Catch(HttpException) +export class HttpExceptionFilter implements ExceptionFilter { + private logger = new Logger(HttpExceptionFilter.name) + + constructor(private configService: ConfigService) {} + + catch(exception: HttpException, host: ArgumentsHost) { + const ctx = host.switchToHttp() + const response = ctx.getResponse() + const status = exception.getStatus() + + const isProduction = this.configService.get('env') === Env.PRODUCTION + + this.logger.error(exception) + + response.status(status).json( + isProduction + ? { + statusCode: status, + message: exception.message, + response: exception.getResponse() + } + : { + statusCode: status, + message: exception.message, + response: exception.getResponse(), + stack: exception.stack + } + ) + } +} diff --git a/apps/policy-engine/src/engine/app.controller.ts b/apps/policy-engine/src/engine/app.controller.ts index b663b0adc..562973e4a 100644 --- a/apps/policy-engine/src/engine/app.controller.ts +++ b/apps/policy-engine/src/engine/app.controller.ts @@ -20,7 +20,6 @@ export class AppController { this.logger.log({ message: 'Received ping' }) - return 'pong' } diff --git a/apps/policy-engine/src/main.ts b/apps/policy-engine/src/main.ts index 724cf85c6..c368c635b 100644 --- a/apps/policy-engine/src/main.ts +++ b/apps/policy-engine/src/main.ts @@ -3,7 +3,10 @@ import { ConfigService } from '@nestjs/config' import { NestFactory } from '@nestjs/core' import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger' import { lastValueFrom, map, of, switchMap } from 'rxjs' +import { Config } from './policy-engine.config' import { PolicyEngineModule } from './policy-engine.module' +import { ApplicationExceptionFilter } from './shared/filter/application-exception.filter' +import { HttpExceptionFilter } from './shared/filter/http-exception.filter' /** * Adds Swagger documentation to the application. @@ -37,6 +40,21 @@ const withGlobalPipes = (app: INestApplication): INestApplication => { return app } +/** + * Adds a global exception filter to the application. + * + * @param app - The Nest application instance. + * @param configService - The configuration service instance. + * @returns The modified Nest application instance. + */ +const withGlobalFilters = + (configService: ConfigService) => + (app: INestApplication): INestApplication => { + app.useGlobalFilters(new HttpExceptionFilter(configService), new ApplicationExceptionFilter(configService)) + + return app + } + async function bootstrap() { const logger = new Logger('PolicyEngineBootstrap') const application = await NestFactory.create(PolicyEngineModule, { bodyParser: true }) @@ -51,6 +69,7 @@ async function bootstrap() { of(application).pipe( map(withSwagger), map(withGlobalPipes), + map(withGlobalFilters(configService)), switchMap((app) => app.listen(port)) ) ) diff --git a/apps/policy-engine/src/shared/filter/__test__/unit/application-exception.filter.spec.ts b/apps/policy-engine/src/shared/filter/__test__/unit/application-exception.filter.spec.ts new file mode 100644 index 000000000..f440e4e79 --- /dev/null +++ b/apps/policy-engine/src/shared/filter/__test__/unit/application-exception.filter.spec.ts @@ -0,0 +1,82 @@ +import { ArgumentsHost, HttpStatus } from '@nestjs/common' +import { HttpArgumentsHost } from '@nestjs/common/interfaces' +import { ConfigService } from '@nestjs/config' +import { Response } from 'express' +import { mock } from 'jest-mock-extended' +import { Config, Env } from '../../../../policy-engine.config' +import { ApplicationException } from '../../../exception/application.exception' +import { ApplicationExceptionFilter } from '../../application-exception.filter' + +describe(ApplicationExceptionFilter.name, () => { + const exception = new ApplicationException({ + message: 'Test application exception filter', + suggestedHttpStatusCode: HttpStatus.INTERNAL_SERVER_ERROR, + context: { + additional: 'information', + to: 'debug' + } + }) + + const buildArgumentsHostMock = (): [ArgumentsHost, jest.Mock, jest.Mock] => { + const jsonMock = jest.fn() + const statusMock = jest.fn().mockReturnValue( + mock({ + json: jsonMock + }) + ) + + const host = mock({ + switchToHttp: jest.fn().mockReturnValue( + mock({ + getResponse: jest.fn().mockReturnValue( + mock({ + status: statusMock + }) + ) + }) + ) + }) + + return [host, statusMock, jsonMock] + } + + const buildConfigServiceMock = (env: Env) => + mock>({ + get: jest.fn().mockReturnValue(env) + }) + + describe('catch', () => { + describe('when environment is production', () => { + it('responds with exception status and short message', () => { + const filter = new ApplicationExceptionFilter(buildConfigServiceMock(Env.PRODUCTION)) + const [host, statusMock, jsonMock] = buildArgumentsHostMock() + + filter.catch(exception, host) + + expect(statusMock).toHaveBeenCalledWith(exception.getStatus()) + expect(jsonMock).toHaveBeenCalledWith({ + statusCode: exception.getStatus(), + message: exception.message, + context: exception.context + }) + }) + }) + + describe('when environment is not production', () => { + it('responds with exception status and complete message', () => { + const filter = new ApplicationExceptionFilter(buildConfigServiceMock(Env.DEVELOPMENT)) + const [host, statusMock, jsonMock] = buildArgumentsHostMock() + + filter.catch(exception, host) + + expect(statusMock).toHaveBeenCalledWith(exception.getStatus()) + expect(jsonMock).toHaveBeenCalledWith({ + statusCode: exception.getStatus(), + message: exception.message, + context: exception.context, + stack: exception.stack + }) + }) + }) + }) +}) diff --git a/apps/policy-engine/src/shared/filter/application-exception.filter.ts b/apps/policy-engine/src/shared/filter/application-exception.filter.ts new file mode 100644 index 000000000..f94f56b2d --- /dev/null +++ b/apps/policy-engine/src/shared/filter/application-exception.filter.ts @@ -0,0 +1,52 @@ +import { ArgumentsHost, Catch, ExceptionFilter, LogLevel, Logger } from '@nestjs/common' +import { ConfigService } from '@nestjs/config' +import { Response } from 'express' +import { Config, Env } from '../../policy-engine.config' +import { ApplicationException } from '../../shared/exception/application.exception' + +@Catch(ApplicationException) +export class ApplicationExceptionFilter implements ExceptionFilter { + private logger = new Logger(ApplicationExceptionFilter.name) + + constructor(private configService: ConfigService) {} + + catch(exception: ApplicationException, host: ArgumentsHost) { + const ctx = host.switchToHttp() + const response = ctx.getResponse() + const status = exception.getStatus() + const isProduction = this.configService.get('env') === Env.PRODUCTION + + this.log(exception) + + response.status(status).json( + isProduction + ? { + statusCode: status, + message: exception.message, + context: exception.context + } + : { + statusCode: status, + message: exception.message, + context: exception.context, + stack: exception.stack, + ...(exception.origin && { origin: exception.origin }) + } + ) + } + + // TODO (@wcalderipe, 16/01/24): Unit test the logging logic. For that, we + // must inject the logger in the constructor via dependency injection. + private log(exception: ApplicationException) { + const level: LogLevel = exception.getStatus() >= 500 ? 'error' : 'warn' + + if (this.logger[level]) { + this.logger[level](exception.message, { + status: exception.getStatus(), + context: exception.context, + stacktrace: exception.stack, + origin: exception.origin + }) + } + } +} diff --git a/apps/policy-engine/src/shared/filter/http-exception.filter.ts b/apps/policy-engine/src/shared/filter/http-exception.filter.ts new file mode 100644 index 000000000..6fe3ee39a --- /dev/null +++ b/apps/policy-engine/src/shared/filter/http-exception.filter.ts @@ -0,0 +1,41 @@ +import { ArgumentsHost, Catch, ExceptionFilter, HttpException, Logger } from '@nestjs/common' +import { ConfigService } from '@nestjs/config' +import { Response } from 'express' +import { Config, Env } from '../../policy-engine.config' + +@Catch(HttpException) +export class HttpExceptionFilter implements ExceptionFilter { + private logger = new Logger(HttpExceptionFilter.name) + + constructor(private configService: ConfigService) {} + + catch(exception: HttpException, host: ArgumentsHost) { + const ctx = host.switchToHttp() + const response = ctx.getResponse() + const status = exception.getStatus() + + const isProduction = this.configService.get('env') === Env.PRODUCTION + + this.logger.error({ + message: exception.message, + stack: exception.stack, + response: exception.getResponse(), + statusCode: status + }) + + response.status(status).json( + isProduction + ? { + statusCode: status, + message: exception.message, + response: exception.getResponse() + } + : { + statusCode: status, + message: exception.message, + response: exception.getResponse(), + stack: exception.stack + } + ) + } +}