Skip to content

Commit

Permalink
fix(nestjs): Exception filters in main app module are not being execu…
Browse files Browse the repository at this point in the history
…ted (#13278)

[ref](#12351)

This PR moves the `SentryGlobalFilter` out of the root module, which led
to the filter overriding user exception filters in certain scenarios.
Now there is two ways to setup sentry error monitoring:
- If users have no catch-all exception filter in their application they
add the `SentryGlobalFilter` as a provider in their main module.
- If users have a catch-all exception filter (annotated with `@Catch()`
they can use the newly introduced `SentryCaptureException()` decorator
to capture alle exceptions caught by this filter.

Testing:
Added a new sample application to test the second setup option and
expanded the test suite in general.

Side note:
- Also removed the `@sentry/microservices` dependency again, since it
does not come out of the box with every nest application so we cannot
rely on it.
  • Loading branch information
nicohrubec authored Aug 13, 2024
1 parent 55220cf commit 51ef02d
Show file tree
Hide file tree
Showing 60 changed files with 1,479 additions and 77 deletions.
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { Controller, Get, Param, ParseIntPipe, UseGuards, UseInterceptors } from '@nestjs/common';
import { Controller, Get, Param, ParseIntPipe, UseFilters, UseGuards, UseInterceptors } from '@nestjs/common';
import { flush } from '@sentry/nestjs';
import { AppService } from './app.service';
import { ExampleExceptionGlobalFilter } from './example-global-filter.exception';
import { ExampleExceptionLocalFilter } from './example-local-filter.exception';
import { ExampleLocalFilter } from './example-local.filter';
import { ExampleGuard } from './example.guard';
import { ExampleInterceptor } from './example.interceptor';

@Controller()
@UseFilters(ExampleLocalFilter)
export class AppController {
constructor(private readonly appService: AppService) {}

Expand Down Expand Up @@ -74,4 +78,14 @@ export class AppController {
async flush() {
await flush();
}

@Get('example-exception-global-filter')
async exampleExceptionGlobalFilter() {
throw new ExampleExceptionGlobalFilter();
}

@Get('example-exception-local-filter')
async exampleExceptionLocalFilter() {
throw new ExampleExceptionLocalFilter();
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,26 @@
import { MiddlewareConsumer, Module } from '@nestjs/common';
import { APP_FILTER } from '@nestjs/core';
import { ScheduleModule } from '@nestjs/schedule';
import { SentryModule } from '@sentry/nestjs/setup';
import { SentryGlobalFilter, SentryModule } from '@sentry/nestjs/setup';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { ExampleGlobalFilter } from './example-global.filter';
import { ExampleMiddleware } from './example.middleware';

@Module({
imports: [SentryModule.forRoot(), ScheduleModule.forRoot()],
controllers: [AppController],
providers: [AppService],
providers: [
AppService,
{
provide: APP_FILTER,
useClass: SentryGlobalFilter,
},
{
provide: APP_FILTER,
useClass: ExampleGlobalFilter,
},
],
})
export class AppModule {
configure(consumer: MiddlewareConsumer): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export class ExampleExceptionGlobalFilter extends Error {
constructor() {
super('Original global example exception!');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { ArgumentsHost, BadRequestException, Catch, ExceptionFilter } from '@nestjs/common';
import { Request, Response } from 'express';
import { ExampleExceptionGlobalFilter } from './example-global-filter.exception';

@Catch(ExampleExceptionGlobalFilter)
export class ExampleGlobalFilter implements ExceptionFilter {
catch(exception: BadRequestException, host: ArgumentsHost): void {
const ctx = host.switchToHttp();
const response = ctx.getResponse<Response>();
const request = ctx.getRequest<Request>();

response.status(400).json({
statusCode: 400,
timestamp: new Date().toISOString(),
path: request.url,
message: 'Example exception was handled by global filter!',
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export class ExampleExceptionLocalFilter extends Error {
constructor() {
super('Original local example exception!');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { ArgumentsHost, BadRequestException, Catch, ExceptionFilter } from '@nestjs/common';
import { Request, Response } from 'express';
import { ExampleExceptionLocalFilter } from './example-local-filter.exception';

@Catch(ExampleExceptionLocalFilter)
export class ExampleLocalFilter implements ExceptionFilter {
catch(exception: BadRequestException, host: ArgumentsHost): void {
const ctx = host.switchToHttp();
const response = ctx.getResponse<Response>();
const request = ctx.getRequest<Request>();

response.status(400).json({
statusCode: 400,
timestamp: new Date().toISOString(),
path: request.url,
message: 'Example exception was handled by local filter!',
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,73 @@ test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => {

expect(errorEventOccurred).toBe(false);
});

test('Global exception filter registered in main module is applied and exception is not sent to Sentry', async ({
baseURL,
}) => {
let errorEventOccurred = false;

waitForError('nestjs-basic', event => {
if (!event.type && event.exception?.values?.[0]?.value === 'Example exception was handled by global filter!') {
errorEventOccurred = true;
}

return event?.transaction === 'GET /example-exception-global-filter';
});

const transactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => {
return transactionEvent?.transaction === 'GET /example-exception-global-filter';
});

const response = await fetch(`${baseURL}/example-exception-global-filter`);
const responseBody = await response.json();

expect(response.status).toBe(400);
expect(responseBody).toEqual({
statusCode: 400,
timestamp: expect.any(String),
path: '/example-exception-global-filter',
message: 'Example exception was handled by global filter!',
});

await transactionEventPromise;

(await fetch(`${baseURL}/flush`)).text();

expect(errorEventOccurred).toBe(false);
});

test('Local exception filter registered in main module is applied and exception is not sent to Sentry', async ({
baseURL,
}) => {
let errorEventOccurred = false;

waitForError('nestjs-basic', event => {
if (!event.type && event.exception?.values?.[0]?.value === 'Example exception was handled by local filter!') {
errorEventOccurred = true;
}

return event?.transaction === 'GET /example-exception-local-filter';
});

const transactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => {
return transactionEvent?.transaction === 'GET /example-exception-local-filter';
});

const response = await fetch(`${baseURL}/example-exception-local-filter`);
const responseBody = await response.json();

expect(response.status).toBe(400);
expect(responseBody).toEqual({
statusCode: 400,
timestamp: expect.any(String),
path: '/example-exception-local-filter',
message: 'Example exception was handled by local filter!',
});

await transactionEventPromise;

(await fetch(`${baseURL}/flush`)).text();

expect(errorEventOccurred).toBe(false);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# compiled output
/dist
/node_modules
/build

# Logs
logs
*.log
npm-debug.log*
pnpm-debug.log*
yarn-debug.log*
yarn-error.log*
lerna-debug.log*

# OS
.DS_Store

# Tests
/coverage
/.nyc_output

# IDEs and editors
/.idea
.project
.classpath
.c9/
*.launch
.settings/
*.sublime-workspace

# IDE - VSCode
.vscode/*
!.vscode/settings.json
!.vscode/tasks.json
!.vscode/launch.json
!.vscode/extensions.json

# dotenv environment variable files
.env
.env.development.local
.env.test.local
.env.production.local
.env.local

# temp directory
.temp
.tmp

# Runtime data
pids
*.pid
*.seed
*.pid.lock

# Diagnostic reports (https://nodejs.org/api/report.html)
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@sentry:registry=http://127.0.0.1:4873
@sentry-internal:registry=http://127.0.0.1:4873
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"$schema": "https://json.schemastore.org/nest-cli",
"collection": "@nestjs/schematics",
"sourceRoot": "src",
"compilerOptions": {
"deleteOutDir": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"name": "nestjs-with-submodules-decorator",
"version": "0.0.1",
"private": true,
"scripts": {
"build": "nest build",
"format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
"start": "nest start",
"start:dev": "nest start --watch",
"start:debug": "nest start --debug --watch",
"start:prod": "node dist/main",
"clean": "npx rimraf node_modules pnpm-lock.yaml",
"test": "playwright test",
"test:build": "pnpm install",
"test:assert": "pnpm test"
},
"dependencies": {
"@nestjs/common": "^10.0.0",
"@nestjs/core": "^10.0.0",
"@nestjs/platform-express": "^10.0.0",
"@sentry/nestjs": "latest || *",
"@sentry/types": "latest || *",
"reflect-metadata": "^0.2.0",
"rxjs": "^7.8.1"
},
"devDependencies": {
"@playwright/test": "^1.44.1",
"@sentry-internal/test-utils": "link:../../../test-utils",
"@nestjs/cli": "^10.0.0",
"@nestjs/schematics": "^10.0.0",
"@nestjs/testing": "^10.0.0",
"@types/express": "^4.17.17",
"@types/node": "18.15.1",
"@types/supertest": "^6.0.0",
"@typescript-eslint/eslint-plugin": "^6.0.0",
"@typescript-eslint/parser": "^6.0.0",
"eslint": "^8.42.0",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-prettier": "^5.0.0",
"prettier": "^3.0.0",
"source-map-support": "^0.5.21",
"supertest": "^6.3.3",
"ts-loader": "^9.4.3",
"tsconfig-paths": "^4.2.0",
"typescript": "^4.9.5"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { getPlaywrightConfig } from '@sentry-internal/test-utils';

const config = getPlaywrightConfig({
startCommand: `pnpm start`,
});

export default config;
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Controller, Get, Param, UseFilters } from '@nestjs/common';
import { flush } from '@sentry/nestjs';
import { AppService } from './app.service';
import { ExampleExceptionLocalFilter } from './example-local.exception';
import { ExampleLocalFilter } from './example-local.filter';
import { ExampleExceptionSpecificFilter } from './example-specific.exception';

@Controller()
@UseFilters(ExampleLocalFilter)
export class AppController {
constructor(private readonly appService: AppService) {}

@Get('test-exception/:id')
async testException(@Param('id') id: string) {
return this.appService.testException(id);
}

@Get('test-expected-exception/:id')
async testExpectedException(@Param('id') id: string) {
return this.appService.testExpectedException(id);
}

@Get('flush')
async flush() {
await flush();
}

@Get('example-exception-specific-filter')
async exampleExceptionGlobalFilter() {
throw new ExampleExceptionSpecificFilter();
}

@Get('example-exception-local-filter')
async exampleExceptionLocalFilter() {
throw new ExampleExceptionLocalFilter();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { Module } from '@nestjs/common';
import { APP_FILTER } from '@nestjs/core';
import { SentryModule } from '@sentry/nestjs/setup';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { ExampleWrappedGlobalFilter } from './example-global.filter';
import { ExampleModuleGlobalFilterRegisteredFirst } from './example-module-global-filter-registered-first/example.module';
import { ExampleModuleGlobalFilter } from './example-module-global-filter/example.module';
import { ExampleModuleLocalFilter } from './example-module-local-filter/example.module';
import { ExampleSpecificFilter } from './example-specific.filter';

@Module({
imports: [
ExampleModuleGlobalFilterRegisteredFirst,
SentryModule.forRoot(),
ExampleModuleGlobalFilter,
ExampleModuleLocalFilter,
],
controllers: [AppController],
providers: [
AppService,
{
provide: APP_FILTER,
useClass: ExampleWrappedGlobalFilter,
},
{
provide: APP_FILTER,
useClass: ExampleSpecificFilter,
},
],
})
export class AppModule {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { HttpException, HttpStatus, Injectable } from '@nestjs/common';

@Injectable()
export class AppService {
constructor() {}

testException(id: string) {
throw new Error(`This is an exception with id ${id}`);
}

testExpectedException(id: string) {
throw new HttpException(`This is an expected exception with id ${id}`, HttpStatus.FORBIDDEN);
}
}
Loading

0 comments on commit 51ef02d

Please sign in to comment.