fix(core): Fix integration deduping (#5696)

When the SDK is initialized, we set up a combination of default and user-provided integrations, controlled by two `Sentry.init()` options:

- `defaultIntegrations` - can be set to `false` to disable defaults
- `integrations` - can be an array of integrations (to be merged with the existing array of defaults) or a function `(defaults: Integration[]) => Integration[]` (to replace the entire array of defaults with a new array)

The first option works correctly, but the second has long had two bugs, both relating to how duplicates are handled. Specifically, when two instances of the same integration are present, the following two principles should (but don't) always hold: 1) a user-provided instance should always beat a default instance, and within that, 2) a more recent instance should always beat a less recent one.

To understand why this happens, it helps to know the basics of the integration set-up flow, which goes like this:
- Default integrations are set by the SDK
- The user either does or (much more often) doesn't disable them
- The user either does or doesn't provide either an array or a function to specify their own integrations
- The SDK combines the sets of default and user integrations using the `getIntegrationsToSetup` function, which returns an array of integrations to install
- As each integration is installed, it is added to a registry (which lives in a closure)
- If multiple instances of the same integration are installed, every installation after the first is a no-op (because we check the registry, see that that integration has already been installed, and bail)

Because of the last point, if `getIntegrationsToSetup` returns a list containing duplicates, unexpected things can happen. For example, consider the following `getIntegrationsToSetup` return values under the current implementation:

- `[UserProvidedCopyofIntegration, DefaultCopyOfSameIntegration]` - the user's copy should win, and does
- `[DefaultCopyofIntegration, UserProvidedCopyOfSameIntegration]` - the user's copy should win, but doesn't
- `[DefaultCopyAofIntegration, DefaultCopyBofIntegration]` - copy B should win, but doesn't
- `[UserCopyAofIntegration, UserCopyBofIntegration]` - copy B should win, but doesn't

The most straightforward way to fix this would be to make it so that installing an existing integration would overwrite the existing copy with the new copy, but that would change the end result not just in the above situations (which all involve a single `Sentry.init()` call) but also in situations involving competing `Sentry.init()` calls and in situations where a second (third, fourth, etc) client or hub is being initialized directly. In those latter cases, we _do_ want the first copy installed to take precedence, because it presumably corresponds to the "main" `Sentry` instance.

In order not to cause that larger behavior shift, it's therefore better to fix the aforementinoed bugs by making sure that a) `getIntegrationsToSetup` never returns duplicates, and b) its deduping process preserves the correct copy of each integration.

This both makes that change and introduces a new (hopefully more comprehensive) test suite.

The roots of the problem in the current code are:
- When the user provides a function rather than an array, no deduping is done, so neither duplicate user integrations nor user integrations which duplicate default integrations are caught and dealt with.
- The deduping prefers first instance over last instance.

To solve this, the following changes have been made:

- The entire combining-default-with-user-integrations algorithm has been reworked, so that deduping now happens only once, at the end, after all default and user-provided integrations have been collected. This guarantees that the deduping applies equally to default and user-provided integrations, and applies whether the user has provided an array or a function. 
- The deduping now prefers last over first, by using an object keyed by integration name to store integration instances. It also now takes into account whether an integration instance is the default one or a user-provided one, so that a user-provided function can return defaults before or after custom instances without changing the eventual outcome.

Note that in order to show that this change doesn't break existing behavior, for the moment the original tests (other than the one explicitly testing wrong behavior) have been retained for now. Since the new tests cover everything the existing tests cover, however, the existing tests can (and will) be removed in the future.

packages/core/src/integration.ts

@@ -1,6 +1,12 @@
 import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
 import { Integration, Options } from '@sentry/types';
-import { logger } from '@sentry/utils';
+import { arrayify, logger } from '@sentry/utils';
+
+declare module '@sentry/types' {
+  interface Integration {
+    isDefaultInstance?: boolean;
+  }
+}
 
 export const installedIntegrations: string[] = [];
 
@@ -10,46 +16,64 @@ export type IntegrationIndex = {
 };
 
 /**
+ * Remove duplicates from the given array, preferring the last instance of any duplicate. Not guaranteed to
+ * preseve the order of integrations in the array.
+ *
  * @private
  */
 function filterDuplicates(integrations: Integration[]): Integration[] {
-  return integrations.reduce((acc, integrations) => {
-    if (acc.every(accIntegration => integrations.name !== accIntegration.name)) {
-      acc.push(integrations);
+  const integrationsByName: { [key: string]: Integration } = {};
+
+  integrations.forEach(currentInstance => {
+    const { name } = currentInstance;
+
+    const existingInstance = integrationsByName[name];
+
+    // We want integrations later in the array to overwrite earlier ones of the same type, except that we never want a
+    // default instance to overwrite an existing user instance
+    if (existingInstance && !existingInstance.isDefaultInstance && currentInstance.isDefaultInstance) {
+      return;
     }
-    return acc;
-  }, [] as Integration[]);
+
+    integrationsByName[name] = currentInstance;
+  });
+
+  return Object.values(integrationsByName);
 }
 
-/** Gets integration to install */
+/** Gets integrations to install */
 export function getIntegrationsToSetup(options: Options): Integration[] {
-  const defaultIntegrations = (options.defaultIntegrations && [...options.defaultIntegrations]) || [];
+  const defaultIntegrations = options.defaultIntegrations || [];
   const userIntegrations = options.integrations;
 
-  let integrations: Integration[] = [...filterDuplicates(defaultIntegrations)];
+  // We flag default instances, so that later we can tell them apart from any user-created instances of the same class
+  defaultIntegrations.forEach(integration => {
+    integration.isDefaultInstance = true;
+  });
+
+  let integrations: Integration[];
 
   if (Array.isArray(userIntegrations)) {
-    // Filter out integrations that are also included in user options
-    integrations = [
-      ...integrations.filter(integrations =>
-        userIntegrations.every(userIntegration => userIntegration.name !== integrations.name),
-      ),
-      // And filter out duplicated user options integrations
-      ...filterDuplicates(userIntegrations),
-    ];
+    integrations = [...defaultIntegrations, ...userIntegrations];
   } else if (typeof userIntegrations === 'function') {
-    integrations = userIntegrations(integrations);
-    integrations = Array.isArray(integrations) ? integrations : [integrations];
+    integrations = arrayify(userIntegrations(defaultIntegrations));
+  } else {
+    integrations = defaultIntegrations;
   }
 
-  // Make sure that if present, `Debug` integration will always run last
-  const integrationsNames = integrations.map(i => i.name);
-  const alwaysLastToRun = 'Debug';
-  if (integrationsNames.indexOf(alwaysLastToRun) !== -1) {
-    integrations.push(...integrations.splice(integrationsNames.indexOf(alwaysLastToRun), 1));
+  const finalIntegrations = filterDuplicates(integrations);
+
+  // The `Debug` integration prints copies of the `event` and `hint` which will be passed to `beforeSend`. It therefore
+  // has to run after all other integrations, so that the changes of all event processors will be reflected in the
+  // printed values. For lack of a more elegant way to guarantee that, we therefore locate it and, assuming it exists,
+  // pop it out of its current spot and shove it onto the end of the array.
+  const debugIndex = finalIntegrations.findIndex(integration => integration.name === 'Debug');
+  if (debugIndex !== -1) {
+    const [debugInstance] = finalIntegrations.splice(debugIndex, 1);
+    finalIntegrations.push(debugInstance);
   }
 
-  return integrations;
+  return finalIntegrations;
 }
 
 /**

packages/core/test/lib/integration.test.ts

@@ -1,21 +1,205 @@
-import { Integration } from '@sentry/types';
+import { Integration, Options } from '@sentry/types';
 
 import { getIntegrationsToSetup } from '../../src/integration';
 
 /** JSDoc */
 class MockIntegration implements Integration {
   public name: string;
 
-  public constructor(name: string) {
+  // Only for testing - tag to keep separate instances straight when testing deduplication
+  public tag?: string;
+
+  public constructor(name: string, tag?: string) {
     this.name = name;
+    this.tag = tag;
   }
 
   public setupOnce(): void {
     // noop
   }
 }
 
+type TestCase = [
+  string, // test name
+  Options['defaultIntegrations'], // default integrations
+  Options['integrations'], // user-provided integrations
+  Array<string | string[]>, // expected resulst
+];
+
 describe('getIntegrationsToSetup', () => {
+  describe('no duplicate integrations', () => {
+    const defaultIntegrations = [new MockIntegration('ChaseSquirrels')];
+    const userIntegrationsArray = [new MockIntegration('CatchTreats')];
+    const userIntegrationsFunction = (defaults: Integration[]) => [...defaults, ...userIntegrationsArray];
+
+    const testCases: TestCase[] = [
+      // each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
+      ['no default integrations, no user integrations provided', false, undefined, []],
+      ['no default integrations, empty user-provided array', false, [], []],
+      ['no default integrations, user-provided array', false, userIntegrationsArray, ['CatchTreats']],
+      ['no default integrations, user-provided function', false, userIntegrationsFunction, ['CatchTreats']],
+      ['with default integrations, no user integrations provided', defaultIntegrations, undefined, ['ChaseSquirrels']],
+      ['with default integrations, empty user-provided array', defaultIntegrations, [], ['ChaseSquirrels']],
+      [
+        'with default integrations, user-provided array',
+        defaultIntegrations,
+        userIntegrationsArray,
+        ['ChaseSquirrels', 'CatchTreats'],
+      ],
+      [
+        'with default integrations, user-provided function',
+        defaultIntegrations,
+        userIntegrationsFunction,
+        ['ChaseSquirrels', 'CatchTreats'],
+      ],
+    ];
+
+    test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
+      const integrations = getIntegrationsToSetup({
+        defaultIntegrations,
+        integrations: userIntegrations,
+      });
+      expect(integrations.map(i => i.name)).toEqual(expected);
+    });
+  });
+
+  describe('deduping', () => {
+    // No duplicates
+    const defaultIntegrations = [new MockIntegration('ChaseSquirrels', 'defaultA')];
+    const userIntegrationsArray = [new MockIntegration('CatchTreats', 'userA')];
+
+    // Duplicates within either default or user integrations, but no overlap between them (to show that last one wins)
+    const duplicateDefaultIntegrations = [
+      new MockIntegration('ChaseSquirrels', 'defaultA'),
+      new MockIntegration('ChaseSquirrels', 'defaultB'),
+    ];
+    const duplicateUserIntegrationsArray = [
+      new MockIntegration('CatchTreats', 'userA'),
+      new MockIntegration('CatchTreats', 'userB'),
+    ];
+    const duplicateUserIntegrationsFunctionDefaultsFirst = (defaults: Integration[]) => [
+      ...defaults,
+      ...duplicateUserIntegrationsArray,
+    ];
+    const duplicateUserIntegrationsFunctionDefaultsSecond = (defaults: Integration[]) => [
+      ...duplicateUserIntegrationsArray,
+      ...defaults,
+    ];
+
+    // User integrations containing new instances of default integrations (to show that user integration wins)
+    const userIntegrationsMatchingDefaultsArray = [
+      new MockIntegration('ChaseSquirrels', 'userA'),
+      new MockIntegration('CatchTreats', 'userA'),
+    ];
+    const userIntegrationsMatchingDefaultsFunctionDefaultsFirst = (defaults: Integration[]) => [
+      ...defaults,
+      ...userIntegrationsMatchingDefaultsArray,
+    ];
+    const userIntegrationsMatchingDefaultsFunctionDefaultsSecond = (defaults: Integration[]) => [
+      ...userIntegrationsMatchingDefaultsArray,
+      ...defaults,
+    ];
+
+    const testCases: TestCase[] = [
+      // each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
+      [
+        'duplicate default integrations',
+        duplicateDefaultIntegrations,
+        userIntegrationsArray,
+        [
+          ['ChaseSquirrels', 'defaultB'],
+          ['CatchTreats', 'userA'],
+        ],
+      ],
+      [
+        'duplicate user integrations, user-provided array',
+        defaultIntegrations,
+        duplicateUserIntegrationsArray,
+        [
+          ['ChaseSquirrels', 'defaultA'],
+          ['CatchTreats', 'userB'],
+        ],
+      ],
+      [
+        'duplicate user integrations, user-provided function with defaults first',
+        defaultIntegrations,
+        duplicateUserIntegrationsFunctionDefaultsFirst,
+        [
+          ['ChaseSquirrels', 'defaultA'],
+          ['CatchTreats', 'userB'],
+        ],
+      ],
+      [
+        'duplicate user integrations, user-provided function with defaults second',
+        defaultIntegrations,
+        duplicateUserIntegrationsFunctionDefaultsSecond,
+        [
+          ['CatchTreats', 'userB'],
+          ['ChaseSquirrels', 'defaultA'],
+        ],
+      ],
+      [
+        'same integration in default and user integrations, user-provided array',
+        defaultIntegrations,
+        userIntegrationsMatchingDefaultsArray,
+        [
+          ['ChaseSquirrels', 'userA'],
+          ['CatchTreats', 'userA'],
+        ],
+      ],
+      [
+        'same integration in default and user integrations, user-provided function with defaults first',
+        defaultIntegrations,
+        userIntegrationsMatchingDefaultsFunctionDefaultsFirst,
+        [
+          ['ChaseSquirrels', 'userA'],
+          ['CatchTreats', 'userA'],
+        ],
+      ],
+      [
+        'same integration in default and user integrations, user-provided function with defaults second',
+        defaultIntegrations,
+        userIntegrationsMatchingDefaultsFunctionDefaultsSecond,
+        [
+          ['ChaseSquirrels', 'userA'],
+          ['CatchTreats', 'userA'],
+        ],
+      ],
+    ];
+
+    test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
+      const integrations = getIntegrationsToSetup({
+        defaultIntegrations: defaultIntegrations,
+        integrations: userIntegrations,
+      }) as MockIntegration[];
+
+      expect(integrations.map(i => [i.name, i.tag])).toEqual(expected);
+    });
+  });
+
+  describe('puts `Debug` integration last', () => {
+    // No variations here (default vs user, duplicates, user array vs user function, etc) because by the time we're
+    // dealing with the `Debug` integration, all of the combining and deduping has already been done
+    const noDebug = [new MockIntegration('ChaseSquirrels')];
+    const debugNotLast = [new MockIntegration('Debug'), new MockIntegration('CatchTreats')];
+    const debugAlreadyLast = [new MockIntegration('ChaseSquirrels'), new MockIntegration('Debug')];
+
+    const testCases: TestCase[] = [
+      // each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
+      ['`Debug` not present', false, noDebug, ['ChaseSquirrels']],
+      ['`Debug` not originally last', false, debugNotLast, ['CatchTreats', 'Debug']],
+      ['`Debug` already last', false, debugAlreadyLast, ['ChaseSquirrels', 'Debug']],
+    ];
+
+    test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
+      const integrations = getIntegrationsToSetup({
+        defaultIntegrations,
+        integrations: userIntegrations,
+      });
+      expect(integrations.map(i => i.name)).toEqual(expected);
+    });
+  });
+
   it('works with empty array', () => {
     const integrations = getIntegrationsToSetup({
       integrations: [],
@@ -48,20 +232,6 @@ describe('getIntegrationsToSetup', () => {
     expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
   });
 
-  it('filter duplicated items and always let first win', () => {
-    const first = new MockIntegration('foo');
-    (first as any).order = 'first';
-    const second = new MockIntegration('foo');
-    (second as any).order = 'second';
-
-    const integrations = getIntegrationsToSetup({
-      integrations: [first, second, new MockIntegration('bar')],
-    });
-
-    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
-    expect((integrations[0] as any).order).toEqual('first');
-  });
-
   it('work with empty defaults', () => {
     const integrations = getIntegrationsToSetup({
       defaultIntegrations: [],