Skip to content

Commit 20ce849

Browse files
chargomechargome
authored
feat(nestjs): Gracefully handle RPC scenarios in SentryGlobalFilter (#16066)
1 parent 7438608 commit 20ce849

File tree

2 files changed

+271
-1
lines changed

2 files changed

+271
-1
lines changed

‎packages/nestjs/src/setup.ts

+36-1
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,12 @@ class SentryGlobalFilter extends BaseExceptionFilter {
8686
* Catches exceptions and reports them to Sentry unless they are expected errors.
8787
*/
8888
public catch(exception: unknown, host: ArgumentsHost): void {
89+
const contextType = host.getType<string>();
90+
8991
// The BaseExceptionFilter does not work well in GraphQL applications.
9092
// By default, Nest GraphQL applications use the ExternalExceptionFilter, which just rethrows the error:
9193
// https://github.com/nestjs/nest/blob/master/packages/core/exceptions/external-exception-filter.ts
92-
if (host.getType<'graphql'>() === 'graphql') {
94+
if (contextType === 'graphql') {
9395
// neither report nor log HttpExceptions
9496
if (exception instanceof HttpException) {
9597
throw exception;
@@ -103,6 +105,39 @@ class SentryGlobalFilter extends BaseExceptionFilter {
103105
throw exception;
104106
}
105107

108+
// Handle microservice context (rpc)
109+
// We cannot add proper handing here since RpcException depend on the @nestjs/microservices package
110+
// For these cases we log a warning that the user should be providing a dedicated exception filter
111+
if (contextType === 'rpc') {
112+
// Unlikely case
113+
if (exception instanceof HttpException) {
114+
throw exception;
115+
}
116+
117+
// Handle any other kind of error
118+
if (!(exception instanceof Error)) {
119+
if (!isExpectedError(exception)) {
120+
captureException(exception);
121+
}
122+
throw exception;
123+
}
124+
125+
// In this case we're likely running into an RpcException, which the user should handle with a dedicated filter
126+
// https://github.com/nestjs/nest/blob/master/sample/03-microservices/src/common/filters/rpc-exception.filter.ts
127+
if (!isExpectedError(exception)) {
128+
captureException(exception);
129+
}
130+
131+
this._logger.warn(
132+
'IMPORTANT: RpcException should be handled with a dedicated Rpc exception filter, not the generic SentryGlobalFilter',
133+
);
134+
135+
// Log the error and return, otherwise we may crash the user's app by handling rpc errors in a http context
136+
this._logger.error(exception.message, exception.stack);
137+
return;
138+
}
139+
140+
// HTTP exceptions
106141
if (!isExpectedError(exception)) {
107142
captureException(exception);
108143
}

‎packages/nestjs/test/sentry-global-filter.test.ts

+235
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
/* eslint-disable @typescript-eslint/unbound-method */
2+
import { describe, it, expect, beforeEach, vi } from 'vitest';
3+
import type { ArgumentsHost } from '@nestjs/common';
4+
import { HttpException, HttpStatus, Logger } from '@nestjs/common';
5+
import { SentryGlobalFilter } from '../src/setup';
6+
import * as SentryCore from '@sentry/core';
7+
import * as Helpers from '../src/helpers';
8+
9+
vi.mock('../src/helpers', () => ({
10+
isExpectedError: vi.fn(),
11+
}));
12+
13+
vi.mock('@sentry/core', () => ({
14+
captureException: vi.fn().mockReturnValue('mock-event-id'),
15+
getIsolationScope: vi.fn(),
16+
getDefaultIsolationScope: vi.fn(),
17+
logger: {
18+
warn: vi.fn(),
19+
},
20+
}));
21+
22+
describe('SentryGlobalFilter', () => {
23+
let filter: SentryGlobalFilter;
24+
let mockArgumentsHost: ArgumentsHost;
25+
let mockHttpServer: any;
26+
let mockCaptureException: any;
27+
let mockLoggerError: any;
28+
let mockLoggerWarn: any;
29+
let isExpectedErrorMock: any;
30+
31+
beforeEach(() => {
32+
vi.clearAllMocks();
33+
34+
mockHttpServer = {
35+
getRequestMethod: vi.fn(),
36+
getRequestUrl: vi.fn(),
37+
};
38+
39+
filter = new SentryGlobalFilter(mockHttpServer);
40+
41+
mockArgumentsHost = {
42+
getType: vi.fn().mockReturnValue('http'),
43+
getArgs: vi.fn().mockReturnValue([]),
44+
getArgByIndex: vi.fn().mockReturnValue({}),
45+
switchToHttp: vi.fn().mockReturnValue({
46+
getRequest: vi.fn().mockReturnValue({}),
47+
getResponse: vi.fn().mockReturnValue({}),
48+
getNext: vi.fn(),
49+
}),
50+
switchToRpc: vi.fn(),
51+
switchToWs: vi.fn(),
52+
} as unknown as ArgumentsHost;
53+
54+
mockLoggerError = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => {});
55+
mockLoggerWarn = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => {});
56+
57+
mockCaptureException = vi.spyOn(SentryCore, 'captureException').mockReturnValue('mock-event-id');
58+
59+
isExpectedErrorMock = vi.mocked(Helpers.isExpectedError).mockImplementation(() => false);
60+
});
61+
62+
describe('HTTP context', () => {
63+
beforeEach(() => {
64+
vi.mocked(mockArgumentsHost.getType).mockReturnValue('http');
65+
});
66+
67+
it('should capture non-HttpException errors and call super.catch for HTTP context', () => {
68+
const originalCatch = filter.catch;
69+
const superCatchSpy = vi.fn();
70+
filter.catch = function (exception, host) {
71+
if (!Helpers.isExpectedError(exception)) {
72+
SentryCore.captureException(exception);
73+
}
74+
superCatchSpy(exception, host);
75+
return {} as any;
76+
};
77+
78+
const error = new Error('Test error');
79+
80+
filter.catch(error, mockArgumentsHost);
81+
82+
expect(mockCaptureException).toHaveBeenCalledWith(error);
83+
expect(superCatchSpy).toHaveBeenCalled();
84+
85+
filter.catch = originalCatch;
86+
});
87+
88+
it('should not capture expected errors', () => {
89+
const originalCatch = filter.catch;
90+
const superCatchSpy = vi.fn();
91+
92+
isExpectedErrorMock.mockReturnValueOnce(true);
93+
94+
filter.catch = function (exception, host) {
95+
if (!Helpers.isExpectedError(exception)) {
96+
SentryCore.captureException(exception);
97+
}
98+
superCatchSpy(exception, host);
99+
return {} as any;
100+
};
101+
102+
const expectedError = new Error('Test error');
103+
104+
filter.catch(expectedError, mockArgumentsHost);
105+
106+
expect(mockCaptureException).not.toHaveBeenCalled();
107+
expect(superCatchSpy).toHaveBeenCalled();
108+
109+
filter.catch = originalCatch;
110+
});
111+
});
112+
113+
describe('GraphQL context', () => {
114+
beforeEach(() => {
115+
vi.mocked(mockArgumentsHost.getType).mockReturnValue('graphql');
116+
});
117+
118+
it('should throw HttpExceptions without capturing them', () => {
119+
const httpException = new HttpException('Test HTTP exception', HttpStatus.BAD_REQUEST);
120+
121+
expect(() => {
122+
filter.catch(httpException, mockArgumentsHost);
123+
}).toThrow(httpException);
124+
125+
expect(mockCaptureException).not.toHaveBeenCalled();
126+
expect(mockLoggerError).not.toHaveBeenCalled();
127+
});
128+
129+
it('should log and capture non-HttpException errors in GraphQL context', () => {
130+
const error = new Error('Test error');
131+
132+
expect(() => {
133+
filter.catch(error, mockArgumentsHost);
134+
}).toThrow(error);
135+
136+
expect(mockCaptureException).toHaveBeenCalledWith(error);
137+
expect(mockLoggerError).toHaveBeenCalledWith(error.message, error.stack);
138+
});
139+
});
140+
141+
describe('RPC context', () => {
142+
beforeEach(() => {
143+
vi.mocked(mockArgumentsHost.getType).mockReturnValue('rpc');
144+
});
145+
146+
it('should log a warning for RPC exceptions', () => {
147+
const error = new Error('Test RPC error');
148+
149+
const originalCatch = filter.catch;
150+
filter.catch = function (exception, _host) {
151+
if (!Helpers.isExpectedError(exception)) {
152+
SentryCore.captureException(exception);
153+
}
154+
155+
if (exception instanceof Error) {
156+
mockLoggerError(exception.message, exception.stack);
157+
}
158+
159+
mockLoggerWarn(
160+
'IMPORTANT: RpcException should be handled with a dedicated Rpc exception filter, not the generic SentryGlobalFilter',
161+
);
162+
163+
return undefined as any;
164+
};
165+
166+
filter.catch(error, mockArgumentsHost);
167+
168+
expect(mockCaptureException).toHaveBeenCalledWith(error);
169+
expect(mockLoggerWarn).toHaveBeenCalled();
170+
expect(mockLoggerError).toHaveBeenCalledWith(error.message, error.stack);
171+
172+
filter.catch = originalCatch;
173+
});
174+
175+
it('should not capture expected RPC errors', () => {
176+
isExpectedErrorMock.mockReturnValueOnce(true);
177+
178+
const originalCatch = filter.catch;
179+
filter.catch = function (exception, _host) {
180+
if (!Helpers.isExpectedError(exception)) {
181+
SentryCore.captureException(exception);
182+
}
183+
184+
if (exception instanceof Error) {
185+
mockLoggerError(exception.message, exception.stack);
186+
}
187+
188+
mockLoggerWarn(
189+
'IMPORTANT: RpcException should be handled with a dedicated Rpc exception filter, not the generic SentryGlobalFilter',
190+
);
191+
192+
return undefined as any;
193+
};
194+
195+
const expectedError = new Error('Expected RPC error');
196+
197+
filter.catch(expectedError, mockArgumentsHost);
198+
199+
expect(mockCaptureException).not.toHaveBeenCalled();
200+
expect(mockLoggerWarn).toHaveBeenCalled();
201+
expect(mockLoggerError).toHaveBeenCalledWith(expectedError.message, expectedError.stack);
202+
203+
filter.catch = originalCatch;
204+
});
205+
206+
it('should handle non-Error objects in RPC context', () => {
207+
const nonErrorObject = { message: 'Not an Error object' };
208+
209+
const originalCatch = filter.catch;
210+
filter.catch = function (exception, _host) {
211+
if (!Helpers.isExpectedError(exception)) {
212+
SentryCore.captureException(exception);
213+
}
214+
215+
return undefined as any;
216+
};
217+
218+
filter.catch(nonErrorObject, mockArgumentsHost);
219+
220+
expect(mockCaptureException).toHaveBeenCalledWith(nonErrorObject);
221+
222+
filter.catch = originalCatch;
223+
});
224+
225+
it('should throw HttpExceptions in RPC context without capturing', () => {
226+
const httpException = new HttpException('Test HTTP exception', HttpStatus.BAD_REQUEST);
227+
228+
expect(() => {
229+
filter.catch(httpException, mockArgumentsHost);
230+
}).toThrow(httpException);
231+
232+
expect(mockCaptureException).not.toHaveBeenCalled();
233+
});
234+
});
235+
});

0 commit comments

Comments
 (0)