Skip to content

Commit 781485e

Browse files
lobsterkatielobsterkatie
authored
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.
1 parent 5d76ba5 commit 781485e

File tree

2 files changed

+235
-41
lines changed

2 files changed

+235
-41
lines changed

‎packages/core/src/integration.ts

Lines changed: 49 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
22
import { Integration, Options } from '@sentry/types';
3-
import { logger } from '@sentry/utils';
3+
import { arrayify, logger } from '@sentry/utils';
4+
5+
declare module '@sentry/types' {
6+
interface Integration {
7+
isDefaultInstance?: boolean;
8+
}
9+
}
410

511
export const installedIntegrations: string[] = [];
612

@@ -10,46 +16,64 @@ export type IntegrationIndex = {
1016
};
1117

1218
/**
19+
* Remove duplicates from the given array, preferring the last instance of any duplicate. Not guaranteed to
20+
* preseve the order of integrations in the array.
21+
*
1322
* @private
1423
*/
1524
function filterDuplicates(integrations: Integration[]): Integration[] {
16-
return integrations.reduce((acc, integrations) => {
17-
if (acc.every(accIntegration => integrations.name !== accIntegration.name)) {
18-
acc.push(integrations);
25+
const integrationsByName: { [key: string]: Integration } = {};
26+
27+
integrations.forEach(currentInstance => {
28+
const { name } = currentInstance;
29+
30+
const existingInstance = integrationsByName[name];
31+
32+
// We want integrations later in the array to overwrite earlier ones of the same type, except that we never want a
33+
// default instance to overwrite an existing user instance
34+
if (existingInstance && !existingInstance.isDefaultInstance && currentInstance.isDefaultInstance) {
35+
return;
1936
}
20-
return acc;
21-
}, [] as Integration[]);
37+
38+
integrationsByName[name] = currentInstance;
39+
});
40+
41+
return Object.values(integrationsByName);
2242
}
2343

24-
/** Gets integration to install */
44+
/** Gets integrations to install */
2545
export function getIntegrationsToSetup(options: Options): Integration[] {
26-
const defaultIntegrations = (options.defaultIntegrations && [...options.defaultIntegrations]) || [];
46+
const defaultIntegrations = options.defaultIntegrations || [];
2747
const userIntegrations = options.integrations;
2848

29-
let integrations: Integration[] = [...filterDuplicates(defaultIntegrations)];
49+
// We flag default instances, so that later we can tell them apart from any user-created instances of the same class
50+
defaultIntegrations.forEach(integration => {
51+
integration.isDefaultInstance = true;
52+
});
53+
54+
let integrations: Integration[];
3055

3156
if (Array.isArray(userIntegrations)) {
32-
// Filter out integrations that are also included in user options
33-
integrations = [
34-
...integrations.filter(integrations =>
35-
userIntegrations.every(userIntegration => userIntegration.name !== integrations.name),
36-
),
37-
// And filter out duplicated user options integrations
38-
...filterDuplicates(userIntegrations),
39-
];
57+
integrations = [...defaultIntegrations, ...userIntegrations];
4058
} else if (typeof userIntegrations === 'function') {
41-
integrations = userIntegrations(integrations);
42-
integrations = Array.isArray(integrations) ? integrations : [integrations];
59+
integrations = arrayify(userIntegrations(defaultIntegrations));
60+
} else {
61+
integrations = defaultIntegrations;
4362
}
4463

45-
// Make sure that if present, `Debug` integration will always run last
46-
const integrationsNames = integrations.map(i => i.name);
47-
const alwaysLastToRun = 'Debug';
48-
if (integrationsNames.indexOf(alwaysLastToRun) !== -1) {
49-
integrations.push(...integrations.splice(integrationsNames.indexOf(alwaysLastToRun), 1));
64+
const finalIntegrations = filterDuplicates(integrations);
65+
66+
// The `Debug` integration prints copies of the `event` and `hint` which will be passed to `beforeSend`. It therefore
67+
// has to run after all other integrations, so that the changes of all event processors will be reflected in the
68+
// printed values. For lack of a more elegant way to guarantee that, we therefore locate it and, assuming it exists,
69+
// pop it out of its current spot and shove it onto the end of the array.
70+
const debugIndex = finalIntegrations.findIndex(integration => integration.name === 'Debug');
71+
if (debugIndex !== -1) {
72+
const [debugInstance] = finalIntegrations.splice(debugIndex, 1);
73+
finalIntegrations.push(debugInstance);
5074
}
5175

52-
return integrations;
76+
return finalIntegrations;
5377
}
5478

5579
/**

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

Lines changed: 186 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,205 @@
1-
import { Integration } from '@sentry/types';
1+
import { Integration, Options } from '@sentry/types';
22

33
import { getIntegrationsToSetup } from '../../src/integration';
44

55
/** JSDoc */
66
class MockIntegration implements Integration {
77
public name: string;
88

9-
public constructor(name: string) {
9+
// Only for testing - tag to keep separate instances straight when testing deduplication
10+
public tag?: string;
11+
12+
public constructor(name: string, tag?: string) {
1013
this.name = name;
14+
this.tag = tag;
1115
}
1216

1317
public setupOnce(): void {
1418
// noop
1519
}
1620
}
1721

22+
type TestCase = [
23+
string, // test name
24+
Options['defaultIntegrations'], // default integrations
25+
Options['integrations'], // user-provided integrations
26+
Array<string | string[]>, // expected resulst
27+
];
28+
1829
describe('getIntegrationsToSetup', () => {
30+
describe('no duplicate integrations', () => {
31+
const defaultIntegrations = [new MockIntegration('ChaseSquirrels')];
32+
const userIntegrationsArray = [new MockIntegration('CatchTreats')];
33+
const userIntegrationsFunction = (defaults: Integration[]) => [...defaults, ...userIntegrationsArray];
34+
35+
const testCases: TestCase[] = [
36+
// each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
37+
['no default integrations, no user integrations provided', false, undefined, []],
38+
['no default integrations, empty user-provided array', false, [], []],
39+
['no default integrations, user-provided array', false, userIntegrationsArray, ['CatchTreats']],
40+
['no default integrations, user-provided function', false, userIntegrationsFunction, ['CatchTreats']],
41+
['with default integrations, no user integrations provided', defaultIntegrations, undefined, ['ChaseSquirrels']],
42+
['with default integrations, empty user-provided array', defaultIntegrations, [], ['ChaseSquirrels']],
43+
[
44+
'with default integrations, user-provided array',
45+
defaultIntegrations,
46+
userIntegrationsArray,
47+
['ChaseSquirrels', 'CatchTreats'],
48+
],
49+
[
50+
'with default integrations, user-provided function',
51+
defaultIntegrations,
52+
userIntegrationsFunction,
53+
['ChaseSquirrels', 'CatchTreats'],
54+
],
55+
];
56+
57+
test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
58+
const integrations = getIntegrationsToSetup({
59+
defaultIntegrations,
60+
integrations: userIntegrations,
61+
});
62+
expect(integrations.map(i => i.name)).toEqual(expected);
63+
});
64+
});
65+
66+
describe('deduping', () => {
67+
// No duplicates
68+
const defaultIntegrations = [new MockIntegration('ChaseSquirrels', 'defaultA')];
69+
const userIntegrationsArray = [new MockIntegration('CatchTreats', 'userA')];
70+
71+
// Duplicates within either default or user integrations, but no overlap between them (to show that last one wins)
72+
const duplicateDefaultIntegrations = [
73+
new MockIntegration('ChaseSquirrels', 'defaultA'),
74+
new MockIntegration('ChaseSquirrels', 'defaultB'),
75+
];
76+
const duplicateUserIntegrationsArray = [
77+
new MockIntegration('CatchTreats', 'userA'),
78+
new MockIntegration('CatchTreats', 'userB'),
79+
];
80+
const duplicateUserIntegrationsFunctionDefaultsFirst = (defaults: Integration[]) => [
81+
...defaults,
82+
...duplicateUserIntegrationsArray,
83+
];
84+
const duplicateUserIntegrationsFunctionDefaultsSecond = (defaults: Integration[]) => [
85+
...duplicateUserIntegrationsArray,
86+
...defaults,
87+
];
88+
89+
// User integrations containing new instances of default integrations (to show that user integration wins)
90+
const userIntegrationsMatchingDefaultsArray = [
91+
new MockIntegration('ChaseSquirrels', 'userA'),
92+
new MockIntegration('CatchTreats', 'userA'),
93+
];
94+
const userIntegrationsMatchingDefaultsFunctionDefaultsFirst = (defaults: Integration[]) => [
95+
...defaults,
96+
...userIntegrationsMatchingDefaultsArray,
97+
];
98+
const userIntegrationsMatchingDefaultsFunctionDefaultsSecond = (defaults: Integration[]) => [
99+
...userIntegrationsMatchingDefaultsArray,
100+
...defaults,
101+
];
102+
103+
const testCases: TestCase[] = [
104+
// each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
105+
[
106+
'duplicate default integrations',
107+
duplicateDefaultIntegrations,
108+
userIntegrationsArray,
109+
[
110+
['ChaseSquirrels', 'defaultB'],
111+
['CatchTreats', 'userA'],
112+
],
113+
],
114+
[
115+
'duplicate user integrations, user-provided array',
116+
defaultIntegrations,
117+
duplicateUserIntegrationsArray,
118+
[
119+
['ChaseSquirrels', 'defaultA'],
120+
['CatchTreats', 'userB'],
121+
],
122+
],
123+
[
124+
'duplicate user integrations, user-provided function with defaults first',
125+
defaultIntegrations,
126+
duplicateUserIntegrationsFunctionDefaultsFirst,
127+
[
128+
['ChaseSquirrels', 'defaultA'],
129+
['CatchTreats', 'userB'],
130+
],
131+
],
132+
[
133+
'duplicate user integrations, user-provided function with defaults second',
134+
defaultIntegrations,
135+
duplicateUserIntegrationsFunctionDefaultsSecond,
136+
[
137+
['CatchTreats', 'userB'],
138+
['ChaseSquirrels', 'defaultA'],
139+
],
140+
],
141+
[
142+
'same integration in default and user integrations, user-provided array',
143+
defaultIntegrations,
144+
userIntegrationsMatchingDefaultsArray,
145+
[
146+
['ChaseSquirrels', 'userA'],
147+
['CatchTreats', 'userA'],
148+
],
149+
],
150+
[
151+
'same integration in default and user integrations, user-provided function with defaults first',
152+
defaultIntegrations,
153+
userIntegrationsMatchingDefaultsFunctionDefaultsFirst,
154+
[
155+
['ChaseSquirrels', 'userA'],
156+
['CatchTreats', 'userA'],
157+
],
158+
],
159+
[
160+
'same integration in default and user integrations, user-provided function with defaults second',
161+
defaultIntegrations,
162+
userIntegrationsMatchingDefaultsFunctionDefaultsSecond,
163+
[
164+
['ChaseSquirrels', 'userA'],
165+
['CatchTreats', 'userA'],
166+
],
167+
],
168+
];
169+
170+
test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
171+
const integrations = getIntegrationsToSetup({
172+
defaultIntegrations: defaultIntegrations,
173+
integrations: userIntegrations,
174+
}) as MockIntegration[];
175+
176+
expect(integrations.map(i => [i.name, i.tag])).toEqual(expected);
177+
});
178+
});
179+
180+
describe('puts `Debug` integration last', () => {
181+
// No variations here (default vs user, duplicates, user array vs user function, etc) because by the time we're
182+
// dealing with the `Debug` integration, all of the combining and deduping has already been done
183+
const noDebug = [new MockIntegration('ChaseSquirrels')];
184+
const debugNotLast = [new MockIntegration('Debug'), new MockIntegration('CatchTreats')];
185+
const debugAlreadyLast = [new MockIntegration('ChaseSquirrels'), new MockIntegration('Debug')];
186+
187+
const testCases: TestCase[] = [
188+
// each test case is [testName, defaultIntegrations, userIntegrations, expectedResult]
189+
['`Debug` not present', false, noDebug, ['ChaseSquirrels']],
190+
['`Debug` not originally last', false, debugNotLast, ['CatchTreats', 'Debug']],
191+
['`Debug` already last', false, debugAlreadyLast, ['ChaseSquirrels', 'Debug']],
192+
];
193+
194+
test.each(testCases)('%s', (_, defaultIntegrations, userIntegrations, expected) => {
195+
const integrations = getIntegrationsToSetup({
196+
defaultIntegrations,
197+
integrations: userIntegrations,
198+
});
199+
expect(integrations.map(i => i.name)).toEqual(expected);
200+
});
201+
});
202+
19203
it('works with empty array', () => {
20204
const integrations = getIntegrationsToSetup({
21205
integrations: [],
@@ -48,20 +232,6 @@ describe('getIntegrationsToSetup', () => {
48232
expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
49233
});
50234

51-
it('filter duplicated items and always let first win', () => {
52-
const first = new MockIntegration('foo');
53-
(first as any).order = 'first';
54-
const second = new MockIntegration('foo');
55-
(second as any).order = 'second';
56-
57-
const integrations = getIntegrationsToSetup({
58-
integrations: [first, second, new MockIntegration('bar')],
59-
});
60-
61-
expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
62-
expect((integrations[0] as any).order).toEqual('first');
63-
});
64-
65235
it('work with empty defaults', () => {
66236
const integrations = getIntegrationsToSetup({
67237
defaultIntegrations: [],

0 commit comments

Comments
 (0)