fix(core): Do not run setup for integration on client multiple times (getsentry#10116)

mydea · web-flow · commit 15ab8eeb2e84 · 2024-01-09T15:54:03.000+01:00
Currently, if you do:

```js
const myIntegration = new InboundFilters();
const myIntegration2 = new InboundFilters();
const myIntegration3 = new InboundFilters();

client.addIntegration(myIntegration);
client.addIntegration(myIntegration2);
client.addIntegration(myIntegration3);
```

All of these will have their `setup` hooks called, and thus they will be
initialized multiple times. However, all but the last will be discarded
from the client and not be accessible anymore via e.g.
`getIntegration()` or similar.

This used to not matter because everything was guarded through
`setupOnce()` anyhow, but i would say this is more of a bug and not
really expected.

With this change, an integration can only be added to a client once, if
you try to add it again it will noop.
diff --git a/packages/core/src/integration.ts b/packages/core/src/integration.ts
@@ -101,6 +101,10 @@ export function setupIntegrations(client: Client, integrations: Integration[]):
 
 /** Setup a single integration.  */
 export function setupIntegration(client: Client, integration: Integration, integrationIndex: IntegrationIndex): void {
+  if (integrationIndex[integration.name]) {
+    DEBUG_BUILD && logger.log(`Integration skipped because it was already installed: ${integration.name}`);
+    return;
+  }
   integrationIndex[integration.name] = integration;
 
   // `setupOnce` is only called the first time
diff --git a/packages/core/test/lib/integration.test.ts b/packages/core/test/lib/integration.test.ts
@@ -377,7 +377,7 @@ describe('setupIntegration', () => {
     setupIntegration(client2, integration3, integrationIndex);
     setupIntegration(client2, integration4, integrationIndex);
 
-    expect(integrationIndex).toEqual({ test: integration4 });
+    expect(integrationIndex).toEqual({ test: integration1 });
     expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
     expect(integration2.setupOnce).not.toHaveBeenCalled();
     expect(integration3.setupOnce).not.toHaveBeenCalled();
@@ -394,32 +394,32 @@ describe('setupIntegration', () => {
     const client1 = getTestClient();
     const client2 = getTestClient();
 
-    const integrationIndex = {};
+    const integrationIndex1 = {};
+    const integrationIndex2 = {};
     const integration1 = new CustomIntegration();
     const integration2 = new CustomIntegration();
     const integration3 = new CustomIntegration();
     const integration4 = new CustomIntegration();
 
-    setupIntegration(client1, integration1, integrationIndex);
-    setupIntegration(client1, integration2, integrationIndex);
-    setupIntegration(client2, integration3, integrationIndex);
-    setupIntegration(client2, integration4, integrationIndex);
+    setupIntegration(client1, integration1, integrationIndex1);
+    setupIntegration(client1, integration2, integrationIndex1);
+    setupIntegration(client2, integration3, integrationIndex2);
+    setupIntegration(client2, integration4, integrationIndex2);
 
-    expect(integrationIndex).toEqual({ test: integration4 });
+    expect(integrationIndex1).toEqual({ test: integration1 });
+    expect(integrationIndex2).toEqual({ test: integration3 });
     expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
     expect(integration2.setupOnce).not.toHaveBeenCalled();
     expect(integration3.setupOnce).not.toHaveBeenCalled();
     expect(integration4.setupOnce).not.toHaveBeenCalled();
 
     expect(integration1.setup).toHaveBeenCalledTimes(1);
-    expect(integration2.setup).toHaveBeenCalledTimes(1);
+    expect(integration2.setup).toHaveBeenCalledTimes(0);
     expect(integration3.setup).toHaveBeenCalledTimes(1);
-    expect(integration4.setup).toHaveBeenCalledTimes(1);
+    expect(integration4.setup).toHaveBeenCalledTimes(0);
 
     expect(integration1.setup).toHaveBeenCalledWith(client1);
-    expect(integration2.setup).toHaveBeenCalledWith(client1);
     expect(integration3.setup).toHaveBeenCalledWith(client2);
-    expect(integration4.setup).toHaveBeenCalledWith(client2);
   });
 
   it('binds preprocessEvent for each client', () => {
@@ -432,18 +432,20 @@ describe('setupIntegration', () => {
     const client1 = getTestClient();
     const client2 = getTestClient();
 
-    const integrationIndex = {};
+    const integrationIndex1 = {};
+    const integrationIndex2 = {};
     const integration1 = new CustomIntegration();
     const integration2 = new CustomIntegration();
     const integration3 = new CustomIntegration();
     const integration4 = new CustomIntegration();
 
-    setupIntegration(client1, integration1, integrationIndex);
-    setupIntegration(client1, integration2, integrationIndex);
-    setupIntegration(client2, integration3, integrationIndex);
-    setupIntegration(client2, integration4, integrationIndex);
+    setupIntegration(client1, integration1, integrationIndex1);
+    setupIntegration(client1, integration2, integrationIndex1);
+    setupIntegration(client2, integration3, integrationIndex2);
+    setupIntegration(client2, integration4, integrationIndex2);
 
-    expect(integrationIndex).toEqual({ test: integration4 });
+    expect(integrationIndex1).toEqual({ test: integration1 });
+    expect(integrationIndex2).toEqual({ test: integration3 });
     expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
     expect(integration2.setupOnce).not.toHaveBeenCalled();
     expect(integration3.setupOnce).not.toHaveBeenCalled();
@@ -456,14 +458,12 @@ describe('setupIntegration', () => {
     client2.captureEvent({ event_id: '2c' });
 
     expect(integration1.preprocessEvent).toHaveBeenCalledTimes(2);
-    expect(integration2.preprocessEvent).toHaveBeenCalledTimes(2);
+    expect(integration2.preprocessEvent).toHaveBeenCalledTimes(0);
     expect(integration3.preprocessEvent).toHaveBeenCalledTimes(3);
-    expect(integration4.preprocessEvent).toHaveBeenCalledTimes(3);
+    expect(integration4.preprocessEvent).toHaveBeenCalledTimes(0);
 
     expect(integration1.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '1b' }, {}, client1);
-    expect(integration2.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '1b' }, {}, client1);
     expect(integration3.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '2c' }, {}, client2);
-    expect(integration4.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '2c' }, {}, client2);
   });
 
   it('allows to mutate events in preprocessEvent', async () => {
@@ -504,18 +504,20 @@ describe('setupIntegration', () => {
     const client1 = getTestClient();
     const client2 = getTestClient();
 
-    const integrationIndex = {};
+    const integrationIndex1 = {};
+    const integrationIndex2 = {};
     const integration1 = new CustomIntegration();
     const integration2 = new CustomIntegration();
     const integration3 = new CustomIntegration();
     const integration4 = new CustomIntegration();
 
-    setupIntegration(client1, integration1, integrationIndex);
-    setupIntegration(client1, integration2, integrationIndex);
-    setupIntegration(client2, integration3, integrationIndex);
-    setupIntegration(client2, integration4, integrationIndex);
+    setupIntegration(client1, integration1, integrationIndex1);
+    setupIntegration(client1, integration2, integrationIndex1);
+    setupIntegration(client2, integration3, integrationIndex2);
+    setupIntegration(client2, integration4, integrationIndex2);
 
-    expect(integrationIndex).toEqual({ test: integration4 });
+    expect(integrationIndex1).toEqual({ test: integration1 });
+    expect(integrationIndex2).toEqual({ test: integration3 });
     expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
     expect(integration2.setupOnce).not.toHaveBeenCalled();
     expect(integration3.setupOnce).not.toHaveBeenCalled();
@@ -528,30 +530,20 @@ describe('setupIntegration', () => {
     client2.captureEvent({ event_id: '2c' });
 
     expect(integration1.processEvent).toHaveBeenCalledTimes(2);
-    expect(integration2.processEvent).toHaveBeenCalledTimes(2);
+    expect(integration2.processEvent).toHaveBeenCalledTimes(0);
     expect(integration3.processEvent).toHaveBeenCalledTimes(3);
-    expect(integration4.processEvent).toHaveBeenCalledTimes(3);
+    expect(integration4.processEvent).toHaveBeenCalledTimes(0);
 
     expect(integration1.processEvent).toHaveBeenLastCalledWith(
       expect.objectContaining({ event_id: '1b' }),
       {},
       client1,
     );
-    expect(integration2.processEvent).toHaveBeenLastCalledWith(
-      expect.objectContaining({ event_id: '1b' }),
-      {},
-      client1,
-    );
     expect(integration3.processEvent).toHaveBeenLastCalledWith(
       expect.objectContaining({ event_id: '2c' }),
       {},
       client2,
     );
-    expect(integration4.processEvent).toHaveBeenLastCalledWith(
-      expect.objectContaining({ event_id: '2c' }),
-      {},
-      client2,
-    );
   });
 
   it('allows to mutate events in processEvent', async () => {
