From c39577eb7a92a8024281f3b6a40f8c449a1afb09 Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Wed, 13 Mar 2024 18:54:10 -0400 Subject: [PATCH 1/7] add error management for failed Validation Pipes --- apps/policy-engine/src/app/app.module.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/apps/policy-engine/src/app/app.module.ts b/apps/policy-engine/src/app/app.module.ts index 6c4a6101f..8aff823dd 100644 --- a/apps/policy-engine/src/app/app.module.ts +++ b/apps/policy-engine/src/app/app.module.ts @@ -1,6 +1,6 @@ import { EncryptionModule } from '@narval/encryption-module' import { HttpModule } from '@nestjs/axios' -import { Module, OnApplicationBootstrap, ValidationPipe } from '@nestjs/common' +import { BadRequestException, Logger, Module, OnApplicationBootstrap, ValidationPipe } from '@nestjs/common' import { ConfigModule, ConfigService } from '@nestjs/config' import { APP_PIPE } from '@nestjs/core' import { load } from '../policy-engine.config' @@ -55,11 +55,19 @@ import { TenantRepository } from './persistence/repository/tenant.repository' TenantService, { provide: APP_PIPE, - useClass: ValidationPipe - } + useValue: new ValidationPipe({ + exceptionFactory: (errors) => { + const logger = new Logger('ValidationPipe'); + const errorMessages = errors.map(error => ({ + property: error.property, + constraints: error.constraints + })); + logger.error(JSON.stringify(errorMessages)); + return new BadRequestException('Validation failed'); + }, + }), + }, ], - // - The EngineService is required by the EncryptionModule async registration. - // - The ProvisionService is required by the CliModule. exports: [EngineService, ProvisionService] }) export class AppModule implements OnApplicationBootstrap { @@ -68,4 +76,4 @@ export class AppModule implements OnApplicationBootstrap { async onApplicationBootstrap() { await this.bootstrapService.boot() } -} +} \ No newline at end of file From 34e5ee9ea04b1e8762b6627901bf8c69b24bd5e6 Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Wed, 13 Mar 2024 18:58:21 -0400 Subject: [PATCH 2/7] format --- apps/policy-engine/src/app/app.module.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/apps/policy-engine/src/app/app.module.ts b/apps/policy-engine/src/app/app.module.ts index 8aff823dd..0afc58107 100644 --- a/apps/policy-engine/src/app/app.module.ts +++ b/apps/policy-engine/src/app/app.module.ts @@ -57,16 +57,16 @@ import { TenantRepository } from './persistence/repository/tenant.repository' provide: APP_PIPE, useValue: new ValidationPipe({ exceptionFactory: (errors) => { - const logger = new Logger('ValidationPipe'); - const errorMessages = errors.map(error => ({ + const logger = new Logger('ValidationPipe') + const errorMessages = errors.map((error) => ({ property: error.property, constraints: error.constraints - })); - logger.error(JSON.stringify(errorMessages)); - return new BadRequestException('Validation failed'); - }, - }), - }, + })) + logger.error(JSON.stringify(errorMessages)) + return new BadRequestException('Validation failed') + } + }) + } ], exports: [EngineService, ProvisionService] }) @@ -76,4 +76,4 @@ export class AppModule implements OnApplicationBootstrap { async onApplicationBootstrap() { await this.bootstrapService.boot() } -} \ No newline at end of file +} From 7b870b6466ec7d2f2a311dd1fec6603c712bf6bc Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Thu, 14 Mar 2024 08:56:55 -0400 Subject: [PATCH 3/7] test exception on ping controller --- apps/armory/src/main.ts | 7 +++- .../filter/application-exception.filter.ts | 1 + .../shared/filter/http-exception.filter.ts | 36 +++++++++++++++++++ apps/policy-engine/src/app/app.controller.ts | 9 +++++ apps/policy-engine/src/app/app.module.ts | 14 ++------ 5 files changed, 54 insertions(+), 13 deletions(-) create mode 100644 apps/armory/src/shared/filter/http-exception.filter.ts diff --git a/apps/armory/src/main.ts b/apps/armory/src/main.ts index a5d7b49dd..70fa39d74 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/application-exception.filter.ts b/apps/armory/src/shared/filter/application-exception.filter.ts index a570a168f..10c9cdf7e 100644 --- a/apps/armory/src/shared/filter/application-exception.filter.ts +++ b/apps/armory/src/shared/filter/application-exception.filter.ts @@ -16,6 +16,7 @@ export class ApplicationExceptionFilter implements ExceptionFilter { const status = exception.getStatus() const isProduction = this.configService.get('env') === Env.PRODUCTION + console.log('### ApplicationExceptionFilter.catch() ###') this.log(exception) response.status(status).json( 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/app/app.controller.ts b/apps/policy-engine/src/app/app.controller.ts index ea1b1723b..d755e39fd 100644 --- a/apps/policy-engine/src/app/app.controller.ts +++ b/apps/policy-engine/src/app/app.controller.ts @@ -1,6 +1,7 @@ import { EvaluationRequest } from '@narval/policy-engine-shared' import { Body, Controller, Get, Logger, Post } from '@nestjs/common' import { generateInboundRequest } from '../app/persistence/repository/mock_data' +import { ApplicationException } from '../shared/exception/application.exception' import { AppService } from './app.service' import { EvaluationRequestDto } from './evaluation-request.dto' @@ -21,6 +22,14 @@ export class AppController { message: 'Received ping' }) + throw new ApplicationException({ + message: 'Test error message', + context: { + foo: 'bar' + }, + suggestedHttpStatusCode: 400 + }) + // throw new HttpException('THIS SHOULD SHOW SOMEHWERE', 500) return 'pong' } diff --git a/apps/policy-engine/src/app/app.module.ts b/apps/policy-engine/src/app/app.module.ts index 0afc58107..7c765ddba 100644 --- a/apps/policy-engine/src/app/app.module.ts +++ b/apps/policy-engine/src/app/app.module.ts @@ -1,6 +1,6 @@ import { EncryptionModule } from '@narval/encryption-module' import { HttpModule } from '@nestjs/axios' -import { BadRequestException, Logger, Module, OnApplicationBootstrap, ValidationPipe } from '@nestjs/common' +import { Module, OnApplicationBootstrap, ValidationPipe } from '@nestjs/common' import { ConfigModule, ConfigService } from '@nestjs/config' import { APP_PIPE } from '@nestjs/core' import { load } from '../policy-engine.config' @@ -55,17 +55,7 @@ import { TenantRepository } from './persistence/repository/tenant.repository' TenantService, { provide: APP_PIPE, - useValue: new ValidationPipe({ - exceptionFactory: (errors) => { - const logger = new Logger('ValidationPipe') - const errorMessages = errors.map((error) => ({ - property: error.property, - constraints: error.constraints - })) - logger.error(JSON.stringify(errorMessages)) - return new BadRequestException('Validation failed') - } - }) + useClass: ValidationPipe } ], exports: [EngineService, ProvisionService] From c5217c81e81dde2fcb698ebd07f21c439e8ef27b Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Thu, 14 Mar 2024 09:24:03 -0400 Subject: [PATCH 4/7] registered exception filters to policy-engine --- apps/policy-engine/src/app/app.controller.ts | 10 ---- apps/policy-engine/src/main.ts | 19 +++++++ .../filter/application-exception.filter.ts | 52 +++++++++++++++++++ .../shared/filter/http-exception.filter.ts | 41 +++++++++++++++ 4 files changed, 112 insertions(+), 10 deletions(-) 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/policy-engine/src/app/app.controller.ts b/apps/policy-engine/src/app/app.controller.ts index d755e39fd..765a2ed86 100644 --- a/apps/policy-engine/src/app/app.controller.ts +++ b/apps/policy-engine/src/app/app.controller.ts @@ -1,7 +1,6 @@ import { EvaluationRequest } from '@narval/policy-engine-shared' import { Body, Controller, Get, Logger, Post } from '@nestjs/common' import { generateInboundRequest } from '../app/persistence/repository/mock_data' -import { ApplicationException } from '../shared/exception/application.exception' import { AppService } from './app.service' import { EvaluationRequestDto } from './evaluation-request.dto' @@ -21,15 +20,6 @@ export class AppController { this.logger.log({ message: 'Received ping' }) - - throw new ApplicationException({ - message: 'Test error message', - context: { - foo: 'bar' - }, - suggestedHttpStatusCode: 400 - }) - // throw new HttpException('THIS SHOULD SHOW SOMEHWERE', 500) return 'pong' } diff --git a/apps/policy-engine/src/main.ts b/apps/policy-engine/src/main.ts index b17a3c481..cf9e2479d 100644 --- a/apps/policy-engine/src/main.ts +++ b/apps/policy-engine/src/main.ts @@ -4,6 +4,9 @@ import { NestFactory } from '@nestjs/core' import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger' import { lastValueFrom, map, of, switchMap } from 'rxjs' import { AppModule } from './app/app.module' +import { Config } from './policy-engine.config' +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('AuthorizationNodeBootstrap') const application = await NestFactory.create(AppModule, { 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/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 + } + ) + } +} From 5b3698e72aeae7525e961dedfe311ddf58cb1744 Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Thu, 14 Mar 2024 09:41:08 -0400 Subject: [PATCH 5/7] format --- apps/policy-engine/src/main.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/policy-engine/src/main.ts b/apps/policy-engine/src/main.ts index 24f5c9fa2..d2235a00c 100644 --- a/apps/policy-engine/src/main.ts +++ b/apps/policy-engine/src/main.ts @@ -4,9 +4,9 @@ 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' -import { PolicyEngineModule } from './policy-engine.module' /** * Adds Swagger documentation to the application. From 7b34c21d420beea25c84a4ab369137694a688fe0 Mon Sep 17 00:00:00 2001 From: Ptroger <44851272+Ptroger@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:16:38 -0400 Subject: [PATCH 6/7] Update apps/armory/src/shared/filter/application-exception.filter.ts Co-authored-by: William Calderipe --- apps/armory/src/shared/filter/application-exception.filter.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/armory/src/shared/filter/application-exception.filter.ts b/apps/armory/src/shared/filter/application-exception.filter.ts index 10c9cdf7e..a570a168f 100644 --- a/apps/armory/src/shared/filter/application-exception.filter.ts +++ b/apps/armory/src/shared/filter/application-exception.filter.ts @@ -16,7 +16,6 @@ export class ApplicationExceptionFilter implements ExceptionFilter { const status = exception.getStatus() const isProduction = this.configService.get('env') === Env.PRODUCTION - console.log('### ApplicationExceptionFilter.catch() ###') this.log(exception) response.status(status).json( From d0d3c00bb36a4cf7c8ff2553d0f6cedcdad9e7e7 Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Wed, 20 Mar 2024 15:50:06 -0400 Subject: [PATCH 7/7] ported filter test file --- .../unit/application-exception.filter.spec.ts | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 apps/policy-engine/src/shared/filter/__test__/unit/application-exception.filter.spec.ts 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 + }) + }) + }) + }) +})