From 178dc258bb092edba363064cd35d45c18bd7dc31 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 15 Oct 2021 00:41:26 -0700 Subject: [PATCH 01/12] add function to track whether an exception has been seen --- packages/utils/src/misc.ts | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index a40ac5d01547..81195deee823 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -2,6 +2,7 @@ import { Event, Mechanism, StackFrame } from '@sentry/types'; import { getGlobalObject } from './global'; +import { logger } from './logger'; import { snipLine } from './string'; /** @@ -237,3 +238,40 @@ export function stripUrlQueryAndFragment(urlPath: string): string { // eslint-disable-next-line no-useless-escape return urlPath.split(/[\?#]/, 1)[0]; } + +/** + * Checks whether or not we've already captured the given exception (note: not an identical exception - the very object + * in question), and marks it captured if not. + * + * Sometimes an error gets captured by more than one mechanism. (This happens, for example, in frameworks where we + * intercept thrown errors, capture them, and then rethrow them so that the framework can handle them however it + * normally would, which may or may not lead to them being caught again by something like the global error handler.) + * This prevents us from actually recording it twice. + * + * Note: It will ignore primitives, as properties can't be set on them. {@link: Object.objectify} can be used on + * exceptions to convert any that are primitives into their equivalent object wrapper forms so that this check will + * always work. However, because we need to flag the exact object which will get rethrown, and because that rethrowing + * happens outside of the event processing pipeline, the objectification must be done before the exception captured. + * + * @param A thrown exception to check or flag as having been seen + * @returns `true` if the exception has already been captured, `false` if not (with the side effect of marking it seen) + */ +export function checkOrSetAlreadyCaught(exception: unknown): boolean { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if ((exception as any)?.__sentry_captured__) { + logger.log("Not capturing exception because it's already been captured."); + return true; + } + + try { + // set it this way rather than by assignment so that it's not ennumerable and therefore isn't recorded by the + // `ExtraErrorData` integration + Object.defineProperty(exception, '__sentry_captured__', { + value: true, + }); + } catch (err) { + // `exception` is a primitive, so we can't mark it seen + } + + return false; +} From 89add5d9f01d0076080dfcacbf9703cf3a13dfd5 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 13 Oct 2021 20:34:46 -0700 Subject: [PATCH 02/12] prevent exceptions being captured more than once --- packages/core/src/baseclient.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 46d5d78eb1ae..fc9594eff129 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -13,6 +13,7 @@ import { Transport, } from '@sentry/types'; import { + checkOrSetAlreadyCaught, dateTimestampInSeconds, Dsn, isPlainObject, @@ -101,6 +102,11 @@ export abstract class BaseClient implement */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined { + // ensure we haven't captured this very object before + if (checkOrSetAlreadyCaught(exception)) { + return; + } + let eventId: string | undefined = hint && hint.event_id; this._process( @@ -140,6 +146,11 @@ export abstract class BaseClient implement * @inheritDoc */ public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined { + // ensure we haven't captured this very object before + if (hint?.originalException && checkOrSetAlreadyCaught(hint.originalException)) { + return; + } + let eventId: string | undefined = hint && hint.event_id; this._process( From 32364c7f7e145bc3cfd10ebe9e1902e0edbc688e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 13 Oct 2021 20:35:34 -0700 Subject: [PATCH 03/12] add test for not recapturing --- packages/core/test/lib/base.test.ts | 53 +++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 64218d8ff97e..13220063ce4e 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -12,6 +12,9 @@ const PUBLIC_DSN = 'https://username@domain/123'; // eslint-disable-next-line no-var declare var global: any; +const backendEventFromException = jest.spyOn(TestBackend.prototype, 'eventFromException'); +const clientProcess = jest.spyOn(TestClient.prototype as any, '_process'); + jest.mock('@sentry/utils', () => { const original = jest.requireActual('@sentry/utils'); return { @@ -57,7 +60,7 @@ describe('BaseClient', () => { }); afterEach(() => { - jest.restoreAllMocks(); + jest.clearAllMocks(); }); describe('constructor() / getDsn()', () => { @@ -249,6 +252,28 @@ describe('BaseClient', () => { }), ); }); + + test.each([ + ['`Error` instance', new Error('Will I get caught twice?')], + ['plain object', { 'Will I': 'get caught twice?' }], + ['primitive wrapper', new String('Will I get caught twice?')], + // primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed + // to `captureException` (which is how we'd end up with a primitive wrapper as tested above) + ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { + const client = new TestClient({ dsn: PUBLIC_DSN }); + + expect(thrown.__sentry_captured__).toBeUndefined(); + + client.captureException(thrown); + + expect(thrown.__sentry_captured__).toBe(true); + expect(backendEventFromException).toHaveBeenCalledTimes(1); + + client.captureException(thrown); + + // `captureException` should bail right away this second time around and not get as far as calling this again + expect(backendEventFromException).toHaveBeenCalledTimes(1); + }); }); describe('captureMessage', () => { @@ -325,6 +350,30 @@ describe('BaseClient', () => { expect(TestBackend.instance!.event).toBeUndefined(); }); + test.each([ + ['`Error` instance', new Error('Will I get caught twice?')], + ['plain object', { 'Will I': 'get caught twice?' }], + ['primitive wrapper', new String('Will I get caught twice?')], + // primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed + // to `captureEvent` (which is how we'd end up with a primitive wrapper as tested above) + ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { + const client = new TestClient({ dsn: PUBLIC_DSN }); + const event = { exception: { values: [{ type: 'Error', message: 'Will I get caught twice?' }] } }; + const hint = { originalException: thrown }; + + expect(thrown.__sentry_captured__).toBeUndefined(); + + client.captureEvent(event, hint); + + expect(thrown.__sentry_captured__).toBe(true); + expect(clientProcess).toHaveBeenCalledTimes(1); + + client.captureEvent(event, hint); + + // `captureEvent` should bail right away this second time around and not get as far as calling this again + expect(clientProcess).toHaveBeenCalledTimes(1); + }); + test('sends an event', () => { expect.assertions(2); const client = new TestClient({ dsn: PUBLIC_DSN }); @@ -798,7 +847,7 @@ describe('BaseClient', () => { expect(TestBackend.instance!.event).toBeUndefined(); }); - test('calls beforeSend gets an access to a hint as a second argument', () => { + test('beforeSend gets access to a hint as a second argument', () => { expect.assertions(2); const beforeSend = jest.fn((event, hint) => ({ ...event, data: hint.data })); const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); From a0d0aa7cad7302c3aa769fe4de91eac72ffa8048 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 15 Oct 2021 00:31:50 -0700 Subject: [PATCH 04/12] add function to objectify input --- packages/utils/src/object.ts | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index fe78d6c2765c..56f89f8891bf 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -398,3 +398,39 @@ export function dropUndefinedKeys(val: T): T { return val; } + +/** + * Ensure that something is an object. + * + * Turns `undefined` and `null` into `String`s and all other primitives into instances of their respective wrapper + * classes (String, Boolean, Number, etc.). Acts as the identity function on non-primitives. + * + * @param wat The subject of the objectification + * @returns A version of "wat` which can safely be used with `Object` class methods + */ +export function objectify(wat: unknown): typeof Object { + let objectified; + switch (true) { + case wat === undefined || wat === null: + objectified = new String(wat); + break; + + // Though symbols and bigints do have wrapper classes (`Symbol` and `BigInt`, respectively), for whatever reason + // those classes don't have constructors which can be used with the `new` keyword. We therefore need to cast each as + // an object in order to wrap it. + case typeof wat === 'symbol' || typeof wat === 'bigint': + objectified = Object(wat); + break; + + // this will catch the remaining primitives: `String`, `Number`, and `Boolean` + case isPrimitive(wat): + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + objectified = new (wat as any).constructor(wat); + break; + + default: + objectified = wat; + break; + } + return objectified; +} From 800675e17ed2baad6ac9a208478dc4f4ce8cdfc7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 15 Oct 2021 02:04:53 -0700 Subject: [PATCH 05/12] objectify caught error in withSentry so we can use seen flag --- packages/nextjs/src/utils/withSentry.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 23b4fab2d65f..f7792070f400 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -1,7 +1,7 @@ import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; +import { addExceptionMechanism, isString, logger, objectify, stripUrlQueryAndFragment } from '@sentry/utils'; import * as domain from 'domain'; import { NextApiHandler, NextApiResponse } from 'next'; @@ -76,6 +76,12 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = try { return await origHandler(req, res); } catch (e) { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced + // to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a + // way to prevent it from actually being reported twice.) + const objectifiedErr = objectify(e); + if (currentScope) { currentScope.addEventProcessor(event => { addExceptionMechanism(event, { @@ -88,9 +94,14 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = }); return event; }); - captureException(e); + + captureException(objectifiedErr); } - throw e; + + // We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it + // would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark + // the error as already having been captured.) + throw objectifiedErr; } }); From 3636ee7c9c7458b32edea6c55d94a1cdc4d15ba6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 15 Oct 2021 22:49:51 -0700 Subject: [PATCH 06/12] add tests for checkOrSetAlreadyCaught --- packages/utils/test/misc.test.ts | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/utils/test/misc.test.ts b/packages/utils/test/misc.test.ts index 440e14ddb1fd..a795a66a9155 100644 --- a/packages/utils/test/misc.test.ts +++ b/packages/utils/test/misc.test.ts @@ -3,6 +3,7 @@ import { Event, Mechanism, StackFrame } from '@sentry/types'; import { addContextToFrame, addExceptionMechanism, + checkOrSetAlreadyCaught, getEventDescription, parseRetryAfterHeader, stripUrlQueryAndFragment, @@ -288,3 +289,32 @@ describe('addExceptionMechanism', () => { }); }); }); + +describe('checkOrSetAlreadyCaught()', () => { + describe('ignores primitives', () => { + it.each([ + ['undefined', undefined], + ['null', null], + ['number', 1231], + ['boolean', true], + ['string', 'Dogs are great!'], + ])('%s', (_case: string, exception: unknown): void => { + // in this case, "ignore" just means reporting them as unseen without actually doing anything to them (which of + // course it can't anyway, because primitives are immutable) + expect(checkOrSetAlreadyCaught(exception)).toBe(false); + }); + }); + + it("recognizes exceptions it's seen before", () => { + const exception = { message: 'Oh, no! Charlie ate the flip-flops! :-(', __sentry_captured__: true }; + + expect(checkOrSetAlreadyCaught(exception)).toBe(true); + }); + + it('recognizes new exceptions as new and marks them as seen', () => { + const exception = { message: 'Oh, no! Charlie ate the flip-flops! :-(' }; + + expect(checkOrSetAlreadyCaught(exception)).toBe(false); + expect((exception as any).__sentry_captured__).toBe(true); + }); +}); From db0569eb909a430b0a8cec90827def6d551a5d83 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 18 Oct 2021 13:37:21 -0700 Subject: [PATCH 07/12] add objectify tests --- packages/utils/test/object.test.ts | 76 +++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 674f665e6349..3a3ffe3237b3 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -3,7 +3,14 @@ */ import * as isModule from '../src/is'; -import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, normalize, urlEncode } from '../src/object'; +import { + dropUndefinedKeys, + extractExceptionKeysForMessage, + fill, + normalize, + objectify, + urlEncode, +} from '../src/object'; import { testOnlyIfNodeVersionAtLeast } from './testutils'; describe('fill()', () => { @@ -638,3 +645,70 @@ describe('dropUndefinedKeys()', () => { }); }); }); + +describe('objectify()', () => { + it('turns undefined and null into `String` objects', () => { + const objectifiedUndefined = objectify(undefined); + const objectifiedNull = objectify(null); + + // not string literals but instances of the class `String` + expect(objectifiedUndefined).toEqual(expect.any(String)); + expect(objectifiedNull).toEqual(expect.any(String)); + + expect(objectifiedUndefined.valueOf()).toEqual('undefined'); + expect(objectifiedNull.valueOf()).toEqual('null'); + }); + + it('wraps other primitives with their respective object wrapper classes', () => { + // Note: BigInts are tested separately in order to be able to restrict the Node version on which the tests run + const numberPrimitive = 1121; + const stringPrimitive = 'Dogs are great!'; + const booleanPrimitive = true; + const symbolPrimitive = Symbol('Maisey'); + + const objectifiedNumber = objectify(numberPrimitive); + const objectifiedString = objectify(stringPrimitive); + const objectifiedBoolean = objectify(booleanPrimitive); + const objectifiedSymbol = objectify(symbolPrimitive); + + // not literals but instances of the respective wrapper classes + expect(objectifiedNumber).toEqual(expect.any(Number)); + expect(objectifiedString).toEqual(expect.any(String)); + + // TODO: There's currently a bug in Jest - if you give it the `Boolean` class, it runs `typeof received === + // 'boolean'` but not `received instanceof Boolean` (the way it correctly does for other primitive wrappers, like + // `Number` and `String). (See https://github.com/facebook/jest/pull/11976.) Once that is fixed and we upgrade jest, + // we can comment the test below back in. (The tests for symbols and bigints are working only because our current + // version of jest is sufficiently old that they're not even considered in the relevant check and just fall to the + // default `instanceof` check jest uses for all unknown classes.) + + // expect(objectifiedBoolean).toEqual(expect.any(Boolean)); + expect(objectifiedSymbol).toEqual(expect.any(Symbol)); + + expect(objectifiedNumber.valueOf()).toEqual(numberPrimitive); + expect(objectifiedString.valueOf()).toEqual(stringPrimitive); + expect(objectifiedBoolean.valueOf()).toEqual(booleanPrimitive); + expect(objectifiedSymbol.valueOf()).toEqual(symbolPrimitive); + }); + + // `BigInt` doesn't exist in Node < 10. + testOnlyIfNodeVersionAtLeast(10)('wraps bigints with the `BigInt` class', () => { + // Hack to get around the fact that literal bigints cause a syntax error in older versions of Node, so the + // assignment needs to not even be parsed as code in those versions + let bigintPrimitive; + eval('bigintPrimitive = 1231n;'); + + const objectifiedBigInt = objectify(bigintPrimitive); + + expect(objectifiedBigInt).toEqual(expect.any(BigInt)); + expect(objectifiedBigInt.valueOf()).toEqual(bigintPrimitive); + }); + + it('leaves objects alone', () => { + const notAPrimitive = new Object(); + const objectifiedNonPrimtive = objectify(notAPrimitive); + + // `.toBe()` tests on identity, so this shows no wrapping has occurred + expect(objectifiedNonPrimtive).toBe(notAPrimitive); + }); +}); From 13c87d324b7b422c45b94ce72c5b138c362a7b6c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 19 Oct 2021 11:23:42 -0700 Subject: [PATCH 08/12] clarify and add comments --- packages/core/test/lib/base.test.ts | 15 +++++++++++---- packages/utils/src/misc.ts | 9 +++++---- packages/utils/src/object.ts | 3 ++- packages/utils/test/misc.test.ts | 1 + 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 13220063ce4e..ea269a5f4819 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -257,8 +257,10 @@ describe('BaseClient', () => { ['`Error` instance', new Error('Will I get caught twice?')], ['plain object', { 'Will I': 'get caught twice?' }], ['primitive wrapper', new String('Will I get caught twice?')], - // primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed - // to `captureException` (which is how we'd end up with a primitive wrapper as tested above) + // Primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed + // to `captureException` (which is how we'd end up with a primitive wrapper as tested above) in order for the + // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it + // is encountered, so this test doesn't apply. ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { const client = new TestClient({ dsn: PUBLIC_DSN }); @@ -354,9 +356,14 @@ describe('BaseClient', () => { ['`Error` instance', new Error('Will I get caught twice?')], ['plain object', { 'Will I': 'get caught twice?' }], ['primitive wrapper', new String('Will I get caught twice?')], - // primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed - // to `captureEvent` (which is how we'd end up with a primitive wrapper as tested above) + // Primitives aren't tested directly here because they need to be wrapped with `objectify` *before* being passed + // to `captureEvent` (which is how we'd end up with a primitive wrapper as tested above) in order for the + // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it + // is encountered, so this test doesn't apply. ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { + // Note: this is the same test as in `describe(captureException)`, except with the exception already wrapped in a + // hint and accompanying an event. Duplicated here because some methods skip `captureException` and go straight to + // `captureEvent`. const client = new TestClient({ dsn: PUBLIC_DSN }); const event = { exception: { values: [{ type: 'Error', message: 'Will I get caught twice?' }] } }; const hint = { originalException: thrown }; diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 81195deee823..5400954b9c24 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -248,10 +248,11 @@ export function stripUrlQueryAndFragment(urlPath: string): string { * normally would, which may or may not lead to them being caught again by something like the global error handler.) * This prevents us from actually recording it twice. * - * Note: It will ignore primitives, as properties can't be set on them. {@link: Object.objectify} can be used on - * exceptions to convert any that are primitives into their equivalent object wrapper forms so that this check will - * always work. However, because we need to flag the exact object which will get rethrown, and because that rethrowing - * happens outside of the event processing pipeline, the objectification must be done before the exception captured. + * Note: It will ignore primitives (always return `false` and not mark them as seen), as properties can't be set on + * them. {@link: Object.objectify} can be used on exceptions to convert any that are primitives into their equivalent + * object wrapper forms so that this check will always work. However, because we need to flag the exact object which + * will get rethrown, and because that rethrowing happens outside of the event processing pipeline, the objectification + * must be done before the exception captured. * * @param A thrown exception to check or flag as having been seen * @returns `true` if the exception has already been captured, `false` if not (with the side effect of marking it seen) diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 56f89f8891bf..9f5161ea9764 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -406,7 +406,7 @@ export function dropUndefinedKeys(val: T): T { * classes (String, Boolean, Number, etc.). Acts as the identity function on non-primitives. * * @param wat The subject of the objectification - * @returns A version of "wat` which can safely be used with `Object` class methods + * @returns A version of `wat` which can safely be used with `Object` class methods */ export function objectify(wat: unknown): typeof Object { let objectified; @@ -428,6 +428,7 @@ export function objectify(wat: unknown): typeof Object { objectified = new (wat as any).constructor(wat); break; + // by process of elimination, at this point we know that `wat` must already be an object default: objectified = wat; break; diff --git a/packages/utils/test/misc.test.ts b/packages/utils/test/misc.test.ts index a795a66a9155..7f013122bcc5 100644 --- a/packages/utils/test/misc.test.ts +++ b/packages/utils/test/misc.test.ts @@ -306,6 +306,7 @@ describe('checkOrSetAlreadyCaught()', () => { }); it("recognizes exceptions it's seen before", () => { + // `exception` can be any object - an `Error`, a class instance, or a plain object const exception = { message: 'Oh, no! Charlie ate the flip-flops! :-(', __sentry_captured__: true }; expect(checkOrSetAlreadyCaught(exception)).toBe(true); From c62a72f3a64906fd1c274630e3c90ea5afc10bd0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 19 Oct 2021 11:24:11 -0700 Subject: [PATCH 09/12] split multi-case tests into one test per case --- packages/utils/test/object.test.ts | 71 +++++++++++++----------------- 1 file changed, 30 insertions(+), 41 deletions(-) diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 3a3ffe3237b3..dffa23e22408 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -647,34 +647,19 @@ describe('dropUndefinedKeys()', () => { }); describe('objectify()', () => { - it('turns undefined and null into `String` objects', () => { - const objectifiedUndefined = objectify(undefined); - const objectifiedNull = objectify(null); + describe('stringifies nullish values', () => { + it.each([ + ['undefined', undefined], + ['null', null], + ])('%s', (stringifiedValue, origValue): void => { + const objectifiedNullish = objectify(origValue); - // not string literals but instances of the class `String` - expect(objectifiedUndefined).toEqual(expect.any(String)); - expect(objectifiedNull).toEqual(expect.any(String)); - - expect(objectifiedUndefined.valueOf()).toEqual('undefined'); - expect(objectifiedNull.valueOf()).toEqual('null'); + expect(objectifiedNullish).toEqual(expect.any(String)); + expect(objectifiedNullish.valueOf()).toEqual(stringifiedValue); + }); }); - it('wraps other primitives with their respective object wrapper classes', () => { - // Note: BigInts are tested separately in order to be able to restrict the Node version on which the tests run - const numberPrimitive = 1121; - const stringPrimitive = 'Dogs are great!'; - const booleanPrimitive = true; - const symbolPrimitive = Symbol('Maisey'); - - const objectifiedNumber = objectify(numberPrimitive); - const objectifiedString = objectify(stringPrimitive); - const objectifiedBoolean = objectify(booleanPrimitive); - const objectifiedSymbol = objectify(symbolPrimitive); - - // not literals but instances of the respective wrapper classes - expect(objectifiedNumber).toEqual(expect.any(Number)); - expect(objectifiedString).toEqual(expect.any(String)); - + describe('wraps other primitives with their respective object wrapper classes', () => { // TODO: There's currently a bug in Jest - if you give it the `Boolean` class, it runs `typeof received === // 'boolean'` but not `received instanceof Boolean` (the way it correctly does for other primitive wrappers, like // `Number` and `String). (See https://github.com/facebook/jest/pull/11976.) Once that is fixed and we upgrade jest, @@ -682,26 +667,30 @@ describe('objectify()', () => { // version of jest is sufficiently old that they're not even considered in the relevant check and just fall to the // default `instanceof` check jest uses for all unknown classes.) - // expect(objectifiedBoolean).toEqual(expect.any(Boolean)); - expect(objectifiedSymbol).toEqual(expect.any(Symbol)); + it.each([ + ['number', Number, 1121], + ['string', String, 'Dogs are great!'], + // ["boolean", Boolean, true], + ['symbol', Symbol, Symbol('Maisey')], + ])('%s', (_caseName, wrapperClass, primitive) => { + const objectifiedPrimitive = objectify(primitive); - expect(objectifiedNumber.valueOf()).toEqual(numberPrimitive); - expect(objectifiedString.valueOf()).toEqual(stringPrimitive); - expect(objectifiedBoolean.valueOf()).toEqual(booleanPrimitive); - expect(objectifiedSymbol.valueOf()).toEqual(symbolPrimitive); - }); + expect(objectifiedPrimitive).toEqual(expect.any(wrapperClass)); + expect(objectifiedPrimitive.valueOf()).toEqual(primitive); + }); - // `BigInt` doesn't exist in Node < 10. - testOnlyIfNodeVersionAtLeast(10)('wraps bigints with the `BigInt` class', () => { - // Hack to get around the fact that literal bigints cause a syntax error in older versions of Node, so the - // assignment needs to not even be parsed as code in those versions - let bigintPrimitive; - eval('bigintPrimitive = 1231n;'); + // `BigInt` doesn't exist in Node < 10, so we test it separately here. + testOnlyIfNodeVersionAtLeast(10)('bigint', () => { + // Hack to get around the fact that literal bigints cause a syntax error in older versions of Node, so the + // assignment needs to not even be parsed as code in those versions + let bigintPrimitive; + eval('bigintPrimitive = 1231n;'); - const objectifiedBigInt = objectify(bigintPrimitive); + const objectifiedBigInt = objectify(bigintPrimitive); - expect(objectifiedBigInt).toEqual(expect.any(BigInt)); - expect(objectifiedBigInt.valueOf()).toEqual(bigintPrimitive); + expect(objectifiedBigInt).toEqual(expect.any(BigInt)); + expect(objectifiedBigInt.valueOf()).toEqual(bigintPrimitive); + }); }); it('leaves objects alone', () => { From c9c298582681a52aa5b4e88dfd2b9d88de501958 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 20 Oct 2021 20:12:33 -0700 Subject: [PATCH 10/12] move log statement to calling functions --- packages/core/src/baseclient.ts | 4 ++++ packages/utils/src/misc.ts | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index fc9594eff129..1b58ad5c318f 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -30,6 +30,8 @@ import { import { Backend, BackendClass } from './basebackend'; import { IntegrationIndex, setupIntegrations } from './integration'; +const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; + /** * Base implementation for all JavaScript SDK clients. * @@ -104,6 +106,7 @@ export abstract class BaseClient implement public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined { // ensure we haven't captured this very object before if (checkOrSetAlreadyCaught(exception)) { + logger.log(ALREADY_SEEN_ERROR); return; } @@ -148,6 +151,7 @@ export abstract class BaseClient implement public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined { // ensure we haven't captured this very object before if (hint?.originalException && checkOrSetAlreadyCaught(hint.originalException)) { + logger.log(ALREADY_SEEN_ERROR); return; } diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 5400954b9c24..0b3513c0a922 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -2,7 +2,6 @@ import { Event, Mechanism, StackFrame } from '@sentry/types'; import { getGlobalObject } from './global'; -import { logger } from './logger'; import { snipLine } from './string'; /** @@ -260,7 +259,6 @@ export function stripUrlQueryAndFragment(urlPath: string): string { export function checkOrSetAlreadyCaught(exception: unknown): boolean { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if ((exception as any)?.__sentry_captured__) { - logger.log("Not capturing exception because it's already been captured."); return true; } From d409c1651dd94bf09da34e554fce6188b6b647a1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 20 Oct 2021 20:13:01 -0700 Subject: [PATCH 11/12] rename test for clarity and differentiation from previous similar test --- packages/core/test/lib/base.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index ea269a5f4819..f29607e5ac94 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -360,7 +360,7 @@ describe('BaseClient', () => { // to `captureEvent` (which is how we'd end up with a primitive wrapper as tested above) in order for the // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it // is encountered, so this test doesn't apply. - ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { + ])("doesn't capture an event wrapping the same exception twice - %s", (_name: string, thrown: any) => { // Note: this is the same test as in `describe(captureException)`, except with the exception already wrapped in a // hint and accompanying an event. Duplicated here because some methods skip `captureException` and go straight to // `captureEvent`. From a11c8863388d254db5110f58d709320eff1ee639 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 20 Oct 2021 20:15:03 -0700 Subject: [PATCH 12/12] reword checkOrSetAlreadySeen docstring --- packages/utils/src/misc.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 0b3513c0a922..164e98950c61 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -242,10 +242,13 @@ export function stripUrlQueryAndFragment(urlPath: string): string { * Checks whether or not we've already captured the given exception (note: not an identical exception - the very object * in question), and marks it captured if not. * - * Sometimes an error gets captured by more than one mechanism. (This happens, for example, in frameworks where we - * intercept thrown errors, capture them, and then rethrow them so that the framework can handle them however it - * normally would, which may or may not lead to them being caught again by something like the global error handler.) - * This prevents us from actually recording it twice. + * This is useful because it's possible for an error to get captured by more than one mechanism. After we intercept and + * record an error, we rethrow it (assuming we've intercepted it before it's reached the top-level global handlers), so + * that we don't interfere with whatever effects the error might have had were the SDK not there. At that point, because + * the error has been rethrown, it's possible for it to bubble up to some other code we've instrumented. If it's not + * caught after that, it will bubble all the way up to the global handlers (which of course we also instrument). This + * function helps us ensure that even if we encounter the same error more than once, we only record it the first time we + * see it. * * Note: It will ignore primitives (always return `false` and not mark them as seen), as properties can't be set on * them. {@link: Object.objectify} can be used on exceptions to convert any that are primitives into their equivalent