Fix errors handling in prepareEvent pipe

packages/core/src/baseclient.ts

@@ -497,7 +497,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
 
         const beforeSendResult = beforeSend(prepared, hint);
         if (typeof beforeSendResult === 'undefined') {
-          logger.error('`beforeSend` method has to return `null` or a valid event.');
+          throw new SentryError('`beforeSend` method has to return `null` or a valid event.');
         } else if (isThenable(beforeSendResult)) {
           return (beforeSendResult as PromiseLike<Event | null>).then(
             event => event,
@@ -508,31 +508,33 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
         }
         return beforeSendResult;
       })
-      .then(
-        processedEvent => {
-          if (processedEvent === null) {
-            throw new SentryError('`beforeSend` returned `null`, will not send event.');
-          }
+      .then(processedEvent => {
+        if (processedEvent === null) {
+          throw new SentryError('`beforeSend` returned `null`, will not send event.');
+        }
 
-          const session = scope && scope.getSession();
-          if (!isTransaction && session) {
-            this._updateSessionFromEvent(session, processedEvent);
-          }
+        const session = scope && scope.getSession();
+        if (!isTransaction && session) {
+          this._updateSessionFromEvent(session, processedEvent);
+        }
 
-          this._sendEvent(processedEvent);
-          return processedEvent;
-        },
-        reason => {
-          this.captureException(reason, {
-            data: {
-              __sentry__: true,
-            },
-            originalException: reason as Error,
-          });
-          throw new SentryError(
-            `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
-          );
-        },
-      );
+        this._sendEvent(processedEvent);
+        return processedEvent;
+      })
+      .then(null, reason => {
+        if (reason instanceof SentryError) {
+          throw reason;
+        }
+
+        this.captureException(reason, {
+          data: {
+            __sentry__: true,
+          },
+          originalException: reason as Error,
+        });
+        throw new SentryError(
+          `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
+        );
+      });
   }
 }

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

@@ -1,6 +1,6 @@
 import { Hub, Scope } from '@sentry/hub';
 import { Event, Severity, Span } from '@sentry/types';
-import { SentryError } from '@sentry/utils';
+import { logger, SentryError } from '@sentry/utils';
 
 import { TestBackend } from '../mocks/backend';
 import { TestClient } from '../mocks/client';
@@ -55,6 +55,10 @@ describe('BaseClient', () => {
     TestBackend.instance = undefined;
   });
 
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
   describe('constructor() / getDsn()', () => {
     test('returns the Dsn', () => {
       expect.assertions(1);
@@ -639,11 +643,30 @@ describe('BaseClient', () => {
     });
 
     test('calls beforeSend and discards the event', () => {
-      expect.assertions(1);
+      expect.assertions(3);
       const beforeSend = jest.fn(() => null);
       const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
+      const captureExceptionSpy = jest.spyOn(client, 'captureException');
+      const loggerErrorSpy = jest.spyOn(logger, 'error');
       client.captureEvent({ message: 'hello' });
       expect(TestBackend.instance!.event).toBeUndefined();
+      expect(captureExceptionSpy).not.toBeCalled();
+      expect(loggerErrorSpy).toBeCalledWith(new SentryError('`beforeSend` returned `null`, will not send event.'));
+    });
+
+    test('calls beforeSend and log info about invalid return value', () => {
+      expect.assertions(3);
+      const beforeSend = jest.fn(() => undefined);
+      // @ts-ignore we need to test regular-js behavior
+      const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });
+      const captureExceptionSpy = jest.spyOn(client, 'captureException');
+      const loggerErrorSpy = jest.spyOn(logger, 'error');
+      client.captureEvent({ message: 'hello' });
+      expect(TestBackend.instance!.event).toBeUndefined();
+      expect(captureExceptionSpy).not.toBeCalled();
+      expect(loggerErrorSpy).toBeCalledWith(
+        new SentryError('`beforeSend` method has to return `null` or a valid event.'),
+      );
     });
 
     test('calls async beforeSend and uses original event without any changes', done => {
@@ -718,6 +741,44 @@ describe('BaseClient', () => {
       expect(TestBackend.instance!.event!.message).toBe('hello');
       expect((TestBackend.instance!.event! as any).data).toBe('someRandomThing');
     });
+
+    test('eventProcessor can drop the even when it returns null', () => {
+      expect.assertions(3);
+      const client = new TestClient({ dsn: PUBLIC_DSN });
+      const captureExceptionSpy = jest.spyOn(client, 'captureException');
+      const loggerErrorSpy = jest.spyOn(logger, 'error');
+      const scope = new Scope();
+      scope.addEventProcessor(() => null);
+      client.captureEvent({ message: 'hello' }, {}, scope);
+      expect(TestBackend.instance!.event).toBeUndefined();
+      expect(captureExceptionSpy).not.toBeCalled();
+      expect(loggerErrorSpy).toBeCalledWith(new SentryError('An event processor returned null, will not send event.'));
+    });
+
+    test('eventProcessor sends an event and logs when it crashes', () => {
+      expect.assertions(3);
+      const client = new TestClient({ dsn: PUBLIC_DSN });
+      const captureExceptionSpy = jest.spyOn(client, 'captureException');
+      const loggerErrorSpy = jest.spyOn(logger, 'error');
+      const scope = new Scope();
+      const exception = new Error('sorry');
+      scope.addEventProcessor(() => {
+        throw exception;
+      });
+      client.captureEvent({ message: 'hello' }, {}, scope);
+      expect(TestBackend.instance!.event!.exception!.values![0]).toStrictEqual({ type: 'Error', value: 'sorry' });
+      expect(captureExceptionSpy).toBeCalledWith(exception, {
+        data: {
+          __sentry__: true,
+        },
+        originalException: exception,
+      });
+      expect(loggerErrorSpy).toBeCalledWith(
+        new SentryError(
+          `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`,
+        ),
+      );
+    });
   });
 
   describe('integrations', () => {