Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nestjs): Exception filters in main app module are not being executed #13278

Merged
merged 36 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6e6b32e
Add test that demonstrates wrong behavior
nicohrubec Aug 8, 2024
fd3a30a
Use example exception instead of bad request
nicohrubec Aug 8, 2024
1bcaf37
Rename
nicohrubec Aug 8, 2024
22eadbe
Add local filter test
nicohrubec Aug 8, 2024
61d5fe6
Fix tests
nicohrubec Aug 8, 2024
885fb6b
Start on base exception filter patch
nicohrubec Aug 9, 2024
d4ab597
Basic patch for base exception filter seems to work (we can log somet…
nicohrubec Aug 9, 2024
c9b690f
This works! (I think)
nicohrubec Aug 9, 2024
e73698a
Adding correct logic to patch
nicohrubec Aug 9, 2024
f71297e
Cleanup
nicohrubec Aug 9, 2024
1c87a97
Fix tests to reflect new working state
nicohrubec Aug 12, 2024
83998d2
Remove nest microservice dependency
nicohrubec Aug 12, 2024
5be4440
Add docstrings and improve deprecation message
nicohrubec Aug 12, 2024
daf1156
Pass all arguments
nicohrubec Aug 12, 2024
468fb30
Add test for instrumentation in module registered before Sentry
nicohrubec Aug 12, 2024
1f47da1
Lint2
nicohrubec Aug 12, 2024
75058b4
Remove note about registration order from readme
nicohrubec Aug 12, 2024
20891b3
Add tests to node sdk
nicohrubec Aug 12, 2024
6d1a150
Rename error isntrumentation
nicohrubec Aug 12, 2024
ddf23b2
Default base filter to undefined
nicohrubec Aug 12, 2024
2b5d119
Add comment
nicohrubec Aug 12, 2024
8fa920a
Revert some stuff
nicohrubec Aug 12, 2024
d7a6263
Update to decorator and explicit sentry global filter provider
nicohrubec Aug 13, 2024
47481fa
make stuff not broken
nicohrubec Aug 13, 2024
2d4dc35
Revert node tests
nicohrubec Aug 13, 2024
eb72826
Update tests
nicohrubec Aug 13, 2024
66fa8bb
Add sample application with new decorator setup
nicohrubec Aug 13, 2024
38d0093
Fix tests?
nicohrubec Aug 13, 2024
fbc5a7a
Add test: Unexpected exception not caught by local filter is reported
nicohrubec Aug 13, 2024
b09bb16
Add specific filter test
nicohrubec Aug 13, 2024
878ee0b
Add test local filter
nicohrubec Aug 13, 2024
56a823c
Rename decorator
nicohrubec Aug 13, 2024
45de32e
Make decorator nicer
nicohrubec Aug 13, 2024
0b49d0d
Update readme
nicohrubec Aug 13, 2024
570de0e
Update readme
lforst Aug 13, 2024
242e62e
Remove useless files
nicohrubec Aug 13, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading