8000 Fix errors handling in prepareEvent pipe (#2987) · rchl/sentry-javascript@18bce3c · GitHub
[go: up one dir, main page]
Skip to content

Commit 18bce3c

Browse files
kamilogorekkamilogorek
authored
Fix errors handling in prepareEvent pipe (getsentry#2987)
1 parent 2ed3b46 commit 18bce3c

File tree

2 files changed

+90
-27
lines changed

2 files changed

+90
-27
lines changed

‎packages/core/src/baseclient.ts

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
497497

498498
const beforeSendResult = beforeSend(prepared, hint);
499499
if (typeof beforeSendResult === 'undefined') {
500-
logger.error('`beforeSend` method has to return `null` or a valid event.');
500+
throw new SentryError('`beforeSend` method has to return `null` or a valid event.');
501501
} else if (isThenable(beforeSendResult)) {
502502
return (beforeSendResult as PromiseLike<Event | null>).then(
503503
event => event,
@@ -508,31 +508,33 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
508508
}
509509
return beforeSendResult;
510510
})
511-
.then(
512-
processedEvent => {
513-
if (processedEvent === null) {
514-
throw new SentryError('`beforeSend` returned `null`, will not send event.');
515-
}
511+
.then(processedEvent => {
512+
if (processedEvent === null) {
513+
throw new SentryError('`beforeSend` returned `null`, will not send event.');
514+
}
516515

517-
const session = scope && scope.getSession();
518-
if (!isTransaction && session) {
519-
this._updateSessionFromEvent(session, processedEvent);
520-
}
516+
const session = scope && scope.getSession();
517+
if (!isTransaction && session) {
518+
this._updateSessionFromEvent(session, processedEvent);
519+
}
521520

522-
this._sendEvent(processedEvent);
523-
return processedEvent;
524-
},
525-
reason => {
526-
this.captureException(reason, {
527-
data: {
528-
__sentry__: true,
529-
},
530-
originalException: reason as Error,
531-
});
532-
throw new SentryError(
533-
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
534-
);
535-
},
536-
);
521+
this._sendEvent(processedEvent);
522+
return processedEvent;
523+
})
524+
.then(null, reason => {
525+
if (reason instanceof SentryError) {
526+
throw reason;
527+
}
528+
529+
this.captureException(reason, {
530+
data: {
531+
__sentry__: true,
532+
},
533+
originalException: reason as Error,
534+
});
535+
throw new SentryError(
536+
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
537+
);
538+
});
537539
}
538540
}

‎packages/core/test/lib/base.test.ts

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Hub, Scope } from '@sentry/hub';
22
import { Event, Severity, Span } from '@sentry/types';
3-
import { SentryError } from '@sentry/utils';
3+
import { logger, SentryError } from '@sentry/utils';
44

55
import { TestBackend } from '../mocks/backend';
66
import { TestClient } from '../mocks/client';
@@ -55,6 +55,10 @@ describe('BaseClient', () => {
5555
TestBackend.instance = undefined;
5656
});
5757

58+
afterEach(() => {
59+
jest.restoreAllMocks();
60+
});
61+
5862
describe('constructor() / getDsn()', () => {
5963
test('returns the Dsn', () => {
6064
expect.assertions(1);
@@ -639,11 +643,30 @@ describe('BaseClient', () => {
639643
});
640644

641645
test('calls beforeSend and discards the event', () => {
642-
expect.assertions(1);
646+
expect.assertions(3);
643647
const beforeSend = jest.fn(() => null);
644648
const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
649+
const captureExceptionSpy = jest.spyOn(client, 'captureException');
650+
const loggerErrorSpy = jest.spyOn(logger, 'error');
645651
client.captureEvent({ message: 'hello' });
646652
expect(TestBackend.instance!.event).toBeUndefined();
653+
expect(captureExceptionSpy).not.toBeCalled();
654+
expect(loggerErrorSpy).toBeCalledWith(new SentryError('`beforeSend` returned `null`, will not send event.'));
655+
});
656+
657+
test('calls beforeSend and log info about invalid return value', () => {
658+
expect.assertions(3);
659+
const beforeSend = jest.fn(() => undefined);
660+
// @ts-ignore we need to test regular-js behavior
661+
const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
662+
const captureExceptionSpy = jest.spyOn(client, 'captureException');
663+
const loggerErrorSpy = jest.spyOn(logger, 'error');
664+
client.captureEvent({ message: 'hello' });
665+
expect(TestBackend.instance!.event).toBeUndefined();
666+
expect(captureExceptionSpy).not.toBeCalled();
667+
expect(loggerErrorSpy).toBeCalledWith(
668+
new SentryError('`beforeSend` method has to return `null` or a valid event.'),
669+
);
647670
});
648671

649672
test('calls async beforeSend and uses original event without any changes', done => {
@@ -718,6 +741,44 @@ describe('BaseClient', () => {
718741
expect(TestBackend.instance!.event!.message).toBe('hello');
719742
expect((TestBackend.instance!.event! as any).data).toBe('someRandomThing');
720743
});
744+
745+
test('eventProcessor can drop the even when it returns null', () => {
746+
expect.assertions(3);
747+
const client = new TestClient({ dsn: PUBLIC_DSN });
748+
const captureExceptionSpy = jest.spyOn(client, 'captureException');
749+
const loggerErrorSpy = jest.spyOn(logger, 'error');
750+
const scope = new Scope();
751+
scope.addEventProcessor(() => null);
752+
client.captureEvent({ message: 'hello' }, {}, scope);
753+
expect(TestBackend.instance!.event).toBeUndefined();
754+
expect(captureExceptionSpy).not.toBeCalled();
755+
expect(loggerErrorSpy).toBeCalledWith(new SentryError('An event processor returned null, will not send event.'));
756+
});
757+
758+
test('eventProcessor sends an event and logs when it crashes', () => {
759+
expect.assertions(3);
760+
const client = new TestClient({ dsn: PUBLIC_DSN });
761+
const captureExceptionSpy = jest.spyOn(client, 'captureException');
762+
const loggerErrorSpy = jest.spyOn(logger, 'error');
763+
const scope = new Scope();
764+
const exception = new Error('sorry');
765+
scope.addEventProcessor(() => {
766+
throw exception;
767+
});
768+
client.captureEvent({ message: 'hello' }, {}, scope);
769+
expect(TestBackend.instance!.event!.exception!.values![0]).toStrictEqual({ type: 'Error', value: 'sorry' });
770+
expect(captureExceptionSpy).toBeCalledWith(exception, {
771+
data: {
772+
__sentry__: true,
773+
},
774+
originalException: exception,
775+
});
776+
expect(loggerErrorSpy).toBeCalledWith(
777+
new SentryError(
778+
`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`,
779+
),
780+
);
781+
});
721782
});
722783

723784
describe('integrations', () => {

0 commit comments

Comments
 (0)
0