fix(nextjs): Allow onUncaughtException integration to remain excluded (#6148)

lobsterkatie · web-flow · commit b428d14d7345 · 2022-11-07T23:48:13.000-08:00
In the nextjs SDK, the `addOrUpdateIntegration` function exists to force the inclusion of certain integrations with certain options set. If such an integration is included in the underlying SDK's default integrations, however, (in other words, if it's included in the defaults set by `@sentry/browser` or `@sentry/node`), it's possible for the user to have actively chosen to exclude it, which we would then be overriding. This PR adds to the `addOrUpdateIntegration` logic to provide the ability to respect that choice.

The only way for a user to choose to filter out a default integration is by providing a function as their `integrations` option in `Sentry.init()`. Therefore, when handling the function case,  we can check if a given integration is included in the return value, and if it's not, not add the default instance we otherwise would. This is controlled by a flag on that default instance named `allowExclusionByUser`. If it's set to `true`, we'll perform the check and respect the user's choice. If it's set to `false` or not set at all, we'll continue to behave as we have, forcing the inclusion of the given integration.

The inspiration for this change is our recent inclusion of the `onUncaughtException` integration in the nextjs defaults. This PR therefore also applies the above change to that default instance.

Finally, the test suite for `addOrUpdateIntegration` has been entirely reworked, to ensure that it covers all possible cases.
diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts
@@ -10,7 +10,7 @@ import * as path from 'path';
 import { isBuild } from './utils/isBuild';
 import { buildMetadata } from './utils/metadata';
 import { NextjsOptions } from './utils/nextjsOptions';
-import { addOrUpdateIntegration } from './utils/userIntegrations';
+import { addOrUpdateIntegration, IntegrationWithExclusionOption } from './utils/userIntegrations';
 
 export * from '@sentry/node';
 export { captureUnderscoreErrorException } from './utils/_error';
@@ -118,8 +118,11 @@ function addServerIntegrations(options: NextjsOptions): void {
   });
   integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations);
 
-  const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException();
-  integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, {
+  const defaultOnUncaughtExceptionIntegration: IntegrationWithExclusionOption = new Integrations.OnUncaughtException({
+    exitEvenIfOtherHandlersAreRegistered: false,
+  });
+  defaultOnUncaughtExceptionIntegration.allowExclusionByUser = true;
+  integrations = addOrUpdateIntegration(defaultOnUncaughtExceptionIntegration, integrations, {
     _options: { exitEvenIfOtherHandlersAreRegistered: false },
   });
 
diff --git a/packages/nextjs/src/utils/userIntegrations.ts b/packages/nextjs/src/utils/userIntegrations.ts
@@ -7,6 +7,15 @@ type ForcedIntegrationOptions = {
   [keyPath: string]: unknown;
 };
 
+export type IntegrationWithExclusionOption = Integration & {
+  /**
+   * Allow the user to exclude this integration by not returning it from a function provided as the `integrations` option
+   * in `Sentry.init()`. Meant to be used with default integrations, the idea being that if a user has actively filtered
+   * an integration out, we should be able to respect that choice if we wish.
+   */
+  allowExclusionByUser?: boolean;
+};
+
 /**
  * Recursively traverses an object to update an existing nested key.
  * Note: The provided key path must include existing properties,
@@ -43,14 +52,21 @@ function setNestedKey(obj: Record<string, any>, keyPath: string, value: unknown)
  * @param forcedOptions Options with which to patch an existing user-derived instance on the integration.
  * @returns A final integrations array.
  */
-export function addOrUpdateIntegration(
+export function addOrUpdateIntegration<T extends UserIntegrations>(
   defaultIntegrationInstance: Integration,
-  userIntegrations: UserIntegrations,
+  userIntegrations: T,
   forcedOptions: ForcedIntegrationOptions = {},
-): UserIntegrations {
-  return Array.isArray(userIntegrations)
-    ? addOrUpdateIntegrationInArray(defaultIntegrationInstance, userIntegrations, forcedOptions)
-    : addOrUpdateIntegrationInFunction(defaultIntegrationInstance, userIntegrations, forcedOptions);
+): T {
+  return (
+    Array.isArray(userIntegrations)
+      ? addOrUpdateIntegrationInArray(defaultIntegrationInstance, userIntegrations, forcedOptions)
+      : addOrUpdateIntegrationInFunction(
+          defaultIntegrationInstance,
+          // Somehow TS can't figure out that not being an array makes this necessarily a function
+          userIntegrations as UserIntegrationsFunction,
+          forcedOptions,
+        )
+  ) as T;
 }
 
 function addOrUpdateIntegrationInArray(
@@ -72,13 +88,27 @@ function addOrUpdateIntegrationInArray(
 }
 
 function addOrUpdateIntegrationInFunction(
-  defaultIntegrationInstance: Integration,
+  defaultIntegrationInstance: IntegrationWithExclusionOption,
   userIntegrationsFunc: UserIntegrationsFunction,
   forcedOptions: ForcedIntegrationOptions,
 ): UserIntegrationsFunction {
   const wrapper: UserIntegrationsFunction = defaultIntegrations => {
     const userFinalIntegrations = userIntegrationsFunc(defaultIntegrations);
+
+    // There are instances where we want the user to be able to prevent an integration from appearing at all, which they
+    // would do by providing a function which filters out the integration in question. If that's happened in one of
+    // those cases, don't add our default back in.
+    if (defaultIntegrationInstance.allowExclusionByUser) {
+      const userFinalInstance = userFinalIntegrations.find(
+        integration => integration.name === defaultIntegrationInstance.name,
+      );
+      if (!userFinalInstance) {
+        return userFinalIntegrations;
+      }
+    }
+
     return addOrUpdateIntegrationInArray(defaultIntegrationInstance, userFinalIntegrations, forcedOptions);
   };
+
   return wrapper;
 }
diff --git a/packages/nextjs/test/utils/userIntegrations.test.ts b/packages/nextjs/test/utils/userIntegrations.test.ts
@@ -1,50 +1,215 @@
-import { RewriteFrames } from '@sentry/integrations';
-import { Integration } from '@sentry/types';
-
-import { addOrUpdateIntegration, UserIntegrationsFunction } from '../../src/utils/userIntegrations';
-
-const testIntegration = new RewriteFrames();
-
-describe('user integrations without any integrations', () => {
-  test('as an array', () => {
-    const userIntegrations: Integration[] = [];
-    // Should get a single integration
-    let finalIntegrations = addOrUpdateIntegration(testIntegration, userIntegrations);
-    expect(finalIntegrations).toBeInstanceOf(Array);
-    finalIntegrations = finalIntegrations as Integration[];
-    expect(finalIntegrations).toHaveLength(1);
-    expect(finalIntegrations[0]).toMatchObject(testIntegration);
-  });
+import type { IntegrationWithExclusionOption as Integration } from '../../src/utils/userIntegrations';
+import { addOrUpdateIntegration, UserIntegrations } from '../../src/utils/userIntegrations';
+
+type MockIntegrationOptions = {
+  name: string;
+  descriptor: string;
+  age?: number;
+};
+
+class DogIntegration implements Integration {
+  public static id: string = 'Dog';
+  public name: string = DogIntegration.id;
+
+  public dogName: string;
+  public descriptor: string;
+  public age?: number;
+
+  public allowExclusionByUser?: boolean;
+
+  constructor(options: MockIntegrationOptions) {
+    this.dogName = options.name;
+    this.descriptor = options.descriptor;
+    this.age = options.age;
+  }
+
+  setupOnce() {
+    return undefined;
+  }
+}
+
+class CatIntegration implements Integration {
+  public static id: string = 'Cat';
+  public name: string = CatIntegration.id;
+
+  public catName: string;
+  public descriptor: string;
+  public age?: number;
+
+  constructor(options: MockIntegrationOptions) {
+    this.catName = options.name;
+    this.descriptor = options.descriptor;
+    this.age = options.age;
+  }
+
+  setupOnce() {
+    return undefined;
+  }
+}
+
+const defaultDogIntegration = new DogIntegration({ name: 'Maisey', descriptor: 'silly' });
+const defaultCatIntegration = new CatIntegration({ name: 'Piper', descriptor: 'fluffy' });
+const forcedDogIntegration = new DogIntegration({ name: 'Charlie', descriptor: 'goofy' });
+const forcedDogIntegrationProperties = { dogName: 'Charlie', descriptor: 'goofy' };
+
+// Note: This is essentially the implementation of a `test.each()` test. Making it a separate function called in
+// individual tests instead allows the various `describe` clauses to be nested, which is helpful here given how many
+// different combinations of factors come into play.
+function runTest(testOptions: {
+  userIntegrations: UserIntegrations;
+  forcedDogIntegrationInstance: DogIntegration;
+  underlyingDefaultIntegrations?: Integration[];
+  allowIntegrationExclusion?: boolean;
+  expectedDogIntegrationProperties: Partial<DogIntegration> | undefined;
+}): void {
+  const {
+    userIntegrations,
+    forcedDogIntegrationInstance,
+    underlyingDefaultIntegrations = [],
+    allowIntegrationExclusion = false,
+    expectedDogIntegrationProperties,
+  } = testOptions;
+
+  if (allowIntegrationExclusion) {
+    forcedDogIntegrationInstance.allowExclusionByUser = true;
+  }
+
+  let integrations;
+  if (typeof userIntegrations === 'function') {
+    const wrappedUserIntegrationsFunction = addOrUpdateIntegration(forcedDogIntegrationInstance, userIntegrations, {
+      dogName: 'Charlie',
+      descriptor: 'goofy',
+    });
+    integrations = wrappedUserIntegrationsFunction(underlyingDefaultIntegrations);
+  } else {
+    integrations = addOrUpdateIntegration(
+      forcedDogIntegrationInstance,
+      userIntegrations,
+      forcedDogIntegrationProperties,
+    );
+  }
+
+  const finalDogIntegrationInstance = integrations.find(integration => integration.name === 'Dog') as DogIntegration;
+
+  if (expectedDogIntegrationProperties) {
+    expect(finalDogIntegrationInstance).toMatchObject(expectedDogIntegrationProperties);
+  } else {
+    expect(finalDogIntegrationInstance).toBeUndefined();
+  }
 
-  test('as a function', () => {
-    const userIntegrationFnc: UserIntegrationsFunction = (): Integration[] => [];
-    // Should get a single integration
-    const integrationWrapper = addOrUpdateIntegration(testIntegration, userIntegrationFnc);
-    expect(integrationWrapper).toBeInstanceOf(Function);
-    const finalIntegrations = (integrationWrapper as UserIntegrationsFunction)([]);
-    expect(finalIntegrations).toHaveLength(1);
-    expect(finalIntegrations[0]).toMatchObject(testIntegration);
+  delete forcedDogIntegrationInstance.allowExclusionByUser;
+}
+
+describe('addOrUpdateIntegration()', () => {
+  describe('user provides no `integrations` option', () => {
+    it('adds forced integration instance', () => {
+      expect.assertions(1);
+
+      runTest({
+        userIntegrations: [], // default if no option is provided
+        forcedDogIntegrationInstance: forcedDogIntegration,
+        expectedDogIntegrationProperties: forcedDogIntegrationProperties,
+      });
+    });
   });
-});
 
-describe('user integrations with integrations', () => {
-  test('as an array', () => {
-    const userIntegrations = [new RewriteFrames()];
-    // Should get the same array (with no patches)
-    const finalIntegrations = addOrUpdateIntegration(testIntegration, userIntegrations);
-    expect(finalIntegrations).toMatchObject(userIntegrations);
+  describe('user provides `integrations` array', () => {
+    describe('array contains forced integration type', () => {
+      it('updates user instance with forced options', () => {
+        expect.assertions(1);
+
+        runTest({
+          userIntegrations: [{ ...defaultDogIntegration, age: 9 } as unknown as Integration],
+          forcedDogIntegrationInstance: forcedDogIntegration,
+          expectedDogIntegrationProperties: { ...forcedDogIntegrationProperties, age: 9 },
+        });
+      });
+    });
+
+    describe('array does not contain forced integration type', () => {
+      it('adds forced integration instance', () => {
+        expect.assertions(1);
+
+        runTest({
+          userIntegrations: [defaultCatIntegration],
+          forcedDogIntegrationInstance: forcedDogIntegration,
+          expectedDogIntegrationProperties: forcedDogIntegrationProperties,
+        });
+      });
+    });
   });
 
-  test('as a function', () => {
-    const userIntegrations = [new RewriteFrames()];
-    const integrationsFnc: UserIntegrationsFunction = (_integrations: Integration[]): Integration[] => {
-      return userIntegrations;
-    };
-    // Should get a function that returns the test integration
-    let finalIntegrations = addOrUpdateIntegration(testIntegration, integrationsFnc);
-    expect(typeof finalIntegrations === 'function').toBe(true);
-    expect(finalIntegrations).toBeInstanceOf(Function);
-    finalIntegrations = finalIntegrations as UserIntegrationsFunction;
-    expect(finalIntegrations([])).toMatchObject(userIntegrations);
+  describe('user provides `integrations` function', () => {
+    describe('forced integration in `defaultIntegrations`', () => {
+      const underlyingDefaultIntegrations = [defaultDogIntegration, defaultCatIntegration];
+
+      describe('function filters out forced integration type', () => {
+        it('adds forced integration instance by default', () => {
+          expect.assertions(1);
+
+          runTest({
+            userIntegrations: _defaults => [defaultCatIntegration],
+            forcedDogIntegrationInstance: forcedDogIntegration,
+            underlyingDefaultIntegrations,
+            expectedDogIntegrationProperties: forcedDogIntegrationProperties,
+          });
+        });
+
+        it('does not add forced integration instance if integration exclusion is allowed', () => {
+          expect.assertions(1);
+
+          runTest({
+            userIntegrations: _defaults => [defaultCatIntegration],
+            forcedDogIntegrationInstance: forcedDogIntegration,
+            underlyingDefaultIntegrations,
+            allowIntegrationExclusion: true,
+            expectedDogIntegrationProperties: undefined, // this means no instance was found
+          });
+        });
+      });
+
+      describe("function doesn't filter out forced integration type", () => {
+        it('updates user instance with forced options', () => {
+          expect.assertions(1);
+
+          runTest({
+            userIntegrations: _defaults => [{ ...defaultDogIntegration, age: 9 } as unknown as Integration],
+            forcedDogIntegrationInstance: forcedDogIntegration,
+            underlyingDefaultIntegrations,
+            expectedDogIntegrationProperties: { ...forcedDogIntegrationProperties, age: 9 },
+          });
+        });
+      });
+    });
+
+    describe('forced integration not in `defaultIntegrations`', () => {
+      const underlyingDefaultIntegrations = [defaultCatIntegration];
+
+      describe('function returns forced integration type', () => {
+        it('updates user instance with forced options', () => {
+          expect.assertions(1);
+
+          runTest({
+            userIntegrations: _defaults => [{ ...defaultDogIntegration, age: 9 } as unknown as Integration],
+            forcedDogIntegrationInstance: forcedDogIntegration,
+            underlyingDefaultIntegrations,
+            expectedDogIntegrationProperties: { ...forcedDogIntegrationProperties, age: 9 },
+          });
+        });
+      });
+
+      describe("function doesn't return forced integration type", () => {
+        it('adds forced integration instance', () => {
+          expect.assertions(1);
+
+          runTest({
+            userIntegrations: _defaults => [{ ...defaultCatIntegration, age: 1 } as unknown as Integration],
+            forcedDogIntegrationInstance: forcedDogIntegration,
+            underlyingDefaultIntegrations,
+            expectedDogIntegrationProperties: forcedDogIntegrationProperties,
+          });
+        });
+      });
+    });
   });
 });
