-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(nestjs): Improve control flow exception filtering #19524
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,34 @@ | ||
| /** | ||
| * Determines if the exception is an expected control flow error. | ||
| * - HttpException errors will have a status property | ||
| * - RpcException errors will have an error property | ||
| * Determines if the exception is an expected NestJS control flow error. | ||
| * - HttpException have getStatus and getResponse methods: https://github.com/nestjs/nest/blob/master/packages/microservices/exceptions/rpc-exception.ts | ||
| * - RpcException have getError and initMessage methods: https://github.com/nestjs/nest/blob/master/packages/common/exceptions/http.exception.ts | ||
| * | ||
| * We cannot use `instanceof HttpException` here because this file is imported | ||
| * from the main entry point (via decorators.ts), and importing @nestjs/common at that | ||
| * point would load it before OpenTelemetry instrumentation can patch it, breaking instrumentations. | ||
| * | ||
| * @returns `true` if the exception is expected and should not be reported to Sentry, otherwise `false`. | ||
| */ | ||
| export function isExpectedError(exception: unknown): boolean { | ||
| if (typeof exception === 'object' && exception !== null) { | ||
| return 'status' in exception || 'error' in exception; | ||
| if (typeof exception !== 'object' || exception === null) { | ||
| return false; | ||
| } | ||
|
|
||
| const ex = exception as Record<string, unknown>; | ||
|
|
||
| // HttpException | ||
| if ( | ||
| typeof ex.getStatus === 'function' && | ||
| typeof ex.getResponse === 'function' && | ||
| typeof ex.initMessage === 'function' | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| // RpcException | ||
| if (typeof ex.getError === 'function' && typeof ex.initMessage === 'function') { | ||
| return true; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duck-typing check may break for older NestJS versionsMedium Severity The |
||
| } | ||
|
|
||
| return false; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { HttpException, HttpStatus } from '@nestjs/common'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { isExpectedError } from '../src/helpers'; | ||
|
|
||
| describe('isExpectedError', () => { | ||
| it('should return true for HttpException', () => { | ||
| expect(isExpectedError(new HttpException('Bad Request', HttpStatus.BAD_REQUEST))).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true for RpcException-like objects', () => { | ||
| const rpcLike = { | ||
| getError: () => 'some error', | ||
| initMessage: () => {}, | ||
| }; | ||
| expect(isExpectedError(rpcLike)).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false for plain Error', () => { | ||
| expect(isExpectedError(new Error('test'))).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false for object with status property', () => { | ||
| expect(isExpectedError({ status: 502, message: 'Bad Gateway' })).toBe(false); | ||
| }); | ||
| }); |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: Are we dropping the status check on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add it as an additional condition but don't think it's necessary