fix(core): Fix integration deduping

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: [],