From 2904d3e93eef38f28ad9fdf6dbb87e82a74630ac Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Jan 2024 10:53:33 +0000 Subject: [PATCH 1/8] feat(core): Write data from `setUser`, `setTags`, `setExtras`, `setTag`, `setExtra`, and `setContext` to isolation scope --- packages/core/src/hub.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 34d71883a6ed..f62c71578385 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -387,6 +387,14 @@ export class Hub implements HubInterface { client.emit('beforeAddBreadcrumb', finalBreadcrumb, hint); } + // TODO(v8): I know this comment doesn't make much sense because the hub will be deprecated but I still wanted to + // write it down. In theory, we would have to add the breadcrumbs to the isolation scope here, however, that would + // duplicate all of the breadcrumbs. There was the possibility of adding breadcrumbs to both, the isolation scope + // and the normal scope, and deduplicating it down the line in the event processing pipeline. However, that would + // have been very fragile, because the breadcrumb objects would have needed to keep their identity all throughout + // the event processing pipeline. + // In the new implementation, the top level `Sentry.addBreadcrumb()` should ONLY write to the isolation scope. + scope.addBreadcrumb(finalBreadcrumb, maxBreadcrumbs); } @@ -395,8 +403,11 @@ export class Hub implements HubInterface { * @deprecated Use `Sentry.setUser()` instead. */ public setUser(user: User | null): void { + // TODO(v8): The top level `Sentry.setUser()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setUser(user); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setUser(user); } /** @@ -404,8 +415,11 @@ export class Hub implements HubInterface { * @deprecated Use `Sentry.setTags()` instead. */ public setTags(tags: { [key: string]: Primitive }): void { + // TODO(v8): The top level `Sentry.setTags()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setTags(tags); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setTags(tags); } /** @@ -413,8 +427,11 @@ export class Hub implements HubInterface { * @deprecated Use `Sentry.setExtras()` instead. */ public setExtras(extras: Extras): void { + // TODO(v8): The top level `Sentry.setExtras()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setExtras(extras); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setExtras(extras); } /** @@ -422,8 +439,11 @@ export class Hub implements HubInterface { * @deprecated Use `Sentry.setTag()` instead. */ public setTag(key: string, value: Primitive): void { + // TODO(v8): The top level `Sentry.setTag()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setTag(key, value); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setTag(key, value); } /** @@ -431,8 +451,11 @@ export class Hub implements HubInterface { * @deprecated Use `Sentry.setExtra()` instead. */ public setExtra(key: string, extra: Extra): void { + // TODO(v8): The top level `Sentry.setExtra()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setExtra(key, extra); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setExtra(key, extra); } /** @@ -441,8 +464,11 @@ export class Hub implements HubInterface { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any public setContext(name: string, context: { [key: string]: any } | null): void { + // TODO(v8): The top level `Sentry.setContext()` function should write ONLY to the isolation scope. // eslint-disable-next-line deprecation/deprecation this.getScope().setContext(name, context); + // eslint-disable-next-line deprecation/deprecation + this.getIsolationScope().setContext(name, context); } /** From 9c70c8fa042af214a502fbe3337e0da7ea4c00ee Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 09:43:27 +0000 Subject: [PATCH 2/8] Override scope data even with undefined and null values --- .../core/src/utils/applyScopeDataToEvent.ts | 35 ++++------- .../lib/utils/applyScopeDataToEvent.test.ts | 59 +++---------------- 2 files changed, 21 insertions(+), 73 deletions(-) diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index 426f2295fbb0..3f598a92e673 100644 --- a/packages/core/src/utils/applyScopeDataToEvent.ts +++ b/packages/core/src/utils/applyScopeDataToEvent.ts @@ -44,11 +44,11 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void { span, } = mergeData; - mergePropOverwrite(data, 'extra', extra); - mergePropOverwrite(data, 'tags', tags); - mergePropOverwrite(data, 'user', user); - mergePropOverwrite(data, 'contexts', contexts); - mergePropOverwrite(data, 'sdkProcessingMetadata', sdkProcessingMetadata); + mergeAndOverwriteScopeData(data, 'extra', extra); + mergeAndOverwriteScopeData(data, 'tags', tags); + mergeAndOverwriteScopeData(data, 'user', user); + mergeAndOverwriteScopeData(data, 'contexts', contexts); + mergeAndOverwriteScopeData(data, 'sdkProcessingMetadata', sdkProcessingMetadata); if (level) { data.level = level; @@ -83,28 +83,17 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void { } /** - * Merge properties, overwriting existing keys. + * Merges certain scope data. Undefined values will overwrite any existing values. * Exported only for tests. */ -export function mergePropOverwrite< +export function mergeAndOverwriteScopeData< Prop extends 'extra' | 'tags' | 'user' | 'contexts' | 'sdkProcessingMetadata', - Data extends ScopeData | Event, + Data extends ScopeData, >(data: Data, prop: Prop, mergeVal: Data[Prop]): void { - if (mergeVal && Object.keys(mergeVal).length) { - data[prop] = { ...data[prop], ...mergeVal }; - } -} - -/** - * Merge properties, keeping existing keys. - * Exported only for tests. - */ -export function mergePropKeep< - Prop extends 'extra' | 'tags' | 'user' | 'contexts' | 'sdkProcessingMetadata', - Data extends ScopeData | Event, ->(data: Data, prop: Prop, mergeVal: Data[Prop]): void { - if (mergeVal && Object.keys(mergeVal).length) { - data[prop] = { ...mergeVal, ...data[prop] }; + for (const key in mergeVal) { + if (Object.prototype.hasOwnProperty.call(mergeVal, key)) { + data[prop][key] = mergeVal[key]; + } } } diff --git a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts index 897524cf05e2..08be9769cedf 100644 --- a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts +++ b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts @@ -1,10 +1,5 @@ import type { Attachment, Breadcrumb, EventProcessor, ScopeData } from '@sentry/types'; -import { - mergeArray, - mergePropKeep, - mergePropOverwrite, - mergeScopeData, -} from '../../../src/utils/applyScopeDataToEvent'; +import { mergeAndOverwriteScopeData, mergeArray, mergeScopeData } from '../../../src/utils/applyScopeDataToEvent'; describe('mergeArray', () => { it.each([ @@ -28,45 +23,6 @@ describe('mergeArray', () => { }); }); -describe('mergePropKeep', () => { - it.each([ - [{}, {}, {}], - [{ a: 'aa' }, {}, { a: 'aa' }], - [{ a: 'aa' }, { b: 'bb' }, { a: 'aa', b: 'bb' }], - // Does not overwrite existing keys - [{ a: 'aa' }, { b: 'bb', a: 'cc' }, { a: 'aa', b: 'bb' }], - ])('works with %s and %s', (a, b, expected) => { - const data = { tags: a } as unknown as ScopeData; - mergePropKeep(data, 'tags', b); - expect(data.tags).toEqual(expected); - }); - - it('does not deep merge', () => { - const data = { - contexts: { - app: { app_version: 'v1' }, - culture: { display_name: 'name1' }, - }, - } as unknown as ScopeData; - mergePropKeep(data, 'contexts', { - os: { name: 'os1' }, - app: { app_name: 'name1' }, - }); - expect(data.contexts).toEqual({ - os: { name: 'os1' }, - culture: { display_name: 'name1' }, - app: { app_version: 'v1' }, - }); - }); - - it('does not mutate the original object if no changes are made', () => { - const tags = { a: 'aa' }; - const data = { tags } as unknown as ScopeData; - mergePropKeep(data, 'tags', {}); - expect(data.tags).toBe(tags); - }); -}); - describe('mergePropOverwrite', () => { it.each([ [{}, {}, {}], @@ -74,9 +30,12 @@ describe('mergePropOverwrite', () => { [{ a: 'aa' }, { b: 'bb' }, { a: 'aa', b: 'bb' }], // overwrites existing keys [{ a: 'aa' }, { b: 'bb', a: 'cc' }, { a: 'cc', b: 'bb' }], - ])('works with %s and %s', (a, b, expected) => { - const data = { tags: a } as unknown as ScopeData; - mergePropOverwrite(data, 'tags', b); + // undefined values overwrite existing values + [{ a: 'defined' }, { a: undefined, b: 'defined' }, { a: undefined, b: 'defined' }], + [{ a: 'defined' }, { a: null, b: 'defined' }, { a: null, b: 'defined' }], + ])('works with %s and %s', (oldData, newData, expected) => { + const data = { tags: oldData } as unknown as ScopeData; + mergeAndOverwriteScopeData(data, 'tags', newData); expect(data.tags).toEqual(expected); }); @@ -87,7 +46,7 @@ describe('mergePropOverwrite', () => { culture: { display_name: 'name1' }, }, } as unknown as ScopeData; - mergePropOverwrite(data, 'contexts', { + mergeAndOverwriteScopeData(data, 'contexts', { os: { name: 'os1' }, app: { app_name: 'name1' }, }); @@ -101,7 +60,7 @@ describe('mergePropOverwrite', () => { it('does not mutate the original object if no changes are made', () => { const tags = { a: 'aa' }; const data = { tags } as unknown as ScopeData; - mergePropOverwrite(data, 'tags', {}); + mergeAndOverwriteScopeData(data, 'tags', {}); expect(data.tags).toBe(tags); }); }); From c4becdb145f47e2953a0d83f465467323d524afc Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 10:09:53 +0000 Subject: [PATCH 3/8] clear user data when passing null to setUser --- packages/core/src/scope.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index fe682bf02661..140fda42199a 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -189,10 +189,18 @@ export class Scope implements ScopeInterface { * @inheritDoc */ public setUser(user: User | null): this { - this._user = user || {}; + this._user = user || { + email: undefined, + id: undefined, + ip_address: undefined, + segment: undefined, + username: undefined, + }; + if (this._session) { updateSession(this._session, { user }); } + this._notifyScopeListeners(); return this; } From 81e84876ff3c0c07afff75dcd3cf3e0b66a2981a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 11:21:06 +0100 Subject: [PATCH 4/8] Update packages/core/test/lib/utils/applyScopeDataToEvent.test.ts Co-authored-by: Francesco Novy --- packages/core/test/lib/utils/applyScopeDataToEvent.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts index 08be9769cedf..7543d9ed39e3 100644 --- a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts +++ b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts @@ -23,7 +23,7 @@ describe('mergeArray', () => { }); }); -describe('mergePropOverwrite', () => { +describe('mergeAndOverwriteScopeData', () => { it.each([ [{}, {}, {}], [{ a: 'aa' }, {}, { a: 'aa' }], From 5b8d8c8b735031e8f632d09eeb8b1d77a9ca15df Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 11:24:47 +0000 Subject: [PATCH 5/8] Very weird but avoids leakage --- packages/core/src/utils/applyScopeDataToEvent.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index 3f598a92e673..2a1b1c7efa7a 100644 --- a/packages/core/src/utils/applyScopeDataToEvent.ts +++ b/packages/core/src/utils/applyScopeDataToEvent.ts @@ -90,9 +90,14 @@ export function mergeAndOverwriteScopeData< Prop extends 'extra' | 'tags' | 'user' | 'contexts' | 'sdkProcessingMetadata', Data extends ScopeData, >(data: Data, prop: Prop, mergeVal: Data[Prop]): void { - for (const key in mergeVal) { - if (Object.prototype.hasOwnProperty.call(mergeVal, key)) { - data[prop][key] = mergeVal[key]; + // Clone object + data[prop] = { ...data[prop] }; + + if (mergeVal) { + for (const key in mergeVal) { + if (Object.prototype.hasOwnProperty.call(mergeVal, key)) { + data[prop][key] = mergeVal[key]; + } } } } From 5f47eb486e8cca584a869a2aba0698aa767e97b9 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 11:42:29 +0000 Subject: [PATCH 6/8] short circuit --- packages/core/src/utils/applyScopeDataToEvent.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index 2a1b1c7efa7a..500310ab779d 100644 --- a/packages/core/src/utils/applyScopeDataToEvent.ts +++ b/packages/core/src/utils/applyScopeDataToEvent.ts @@ -90,10 +90,9 @@ export function mergeAndOverwriteScopeData< Prop extends 'extra' | 'tags' | 'user' | 'contexts' | 'sdkProcessingMetadata', Data extends ScopeData, >(data: Data, prop: Prop, mergeVal: Data[Prop]): void { - // Clone object - data[prop] = { ...data[prop] }; - - if (mergeVal) { + if (mergeVal && Object.keys(mergeVal).length) { + // Clone object + data[prop] = { ...data[prop] }; for (const key in mergeVal) { if (Object.prototype.hasOwnProperty.call(mergeVal, key)) { data[prop][key] = mergeVal[key]; From a2c253b837e449ad2001ff502e1341d4f19b58c6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 12:24:18 +0000 Subject: [PATCH 7/8] send minimized data --- .../core/src/utils/applyScopeDataToEvent.ts | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index 500310ab779d..bdf60497cff1 100644 --- a/packages/core/src/utils/applyScopeDataToEvent.ts +++ b/packages/core/src/utils/applyScopeDataToEvent.ts @@ -1,5 +1,5 @@ import type { Breadcrumb, Event, PropagationContext, ScopeData, Span } from '@sentry/types'; -import { arrayify } from '@sentry/utils'; +import { arrayify, dropUndefinedKeys } from '@sentry/utils'; import { getDynamicSamplingContextFromSpan } from '../tracing/dynamicSamplingContext'; import { getRootSpan } from './getRootSpan'; import { spanToJSON, spanToTraceContext } from './spanUtils'; @@ -129,21 +129,30 @@ function applyDataToEvent(event: Event, data: ScopeData): void { transactionName, } = data; - if (extra && Object.keys(extra).length) { - event.extra = { ...extra, ...event.extra }; + const cleanedExtra = dropUndefinedKeys(extra); + if (cleanedExtra && Object.keys(cleanedExtra).length) { + event.extra = { ...cleanedExtra, ...event.extra }; } - if (tags && Object.keys(tags).length) { - event.tags = { ...tags, ...event.tags }; + + const cleanedTags = dropUndefinedKeys(tags); + if (cleanedTags && Object.keys(cleanedTags).length) { + event.tags = { ...cleanedTags, ...event.tags }; } - if (user && Object.keys(user).length) { - event.user = { ...user, ...event.user }; + + const cleanedUser = dropUndefinedKeys(user); + if (cleanedUser && Object.keys(cleanedUser).length) { + event.user = { ...cleanedUser, ...event.user }; } - if (contexts && Object.keys(contexts).length) { - event.contexts = { ...contexts, ...event.contexts }; + + const cleanedContexts = dropUndefinedKeys(contexts); + if (cleanedContexts && Object.keys(cleanedContexts).length) { + event.contexts = { ...cleanedContexts, ...event.contexts }; } + if (level) { event.level = level; } + if (transactionName) { event.transaction = transactionName; } From b84cc30adca01d159054cf58d1c806495593212a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 15 Jan 2024 12:53:10 +0000 Subject: [PATCH 8/8] Add tests --- packages/core/src/scope.ts | 2 + packages/core/test/lib/exports.test.ts | 63 ++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 140fda42199a..01546a84be25 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -189,6 +189,8 @@ export class Scope implements ScopeInterface { * @inheritDoc */ public setUser(user: User | null): this { + // If null is passed we want to unset everything, but still define keys, + // so that later down in the pipeline any existing values are cleared. this._user = user || { email: undefined, id: undefined, diff --git a/packages/core/test/lib/exports.test.ts b/packages/core/test/lib/exports.test.ts index a02673d15a1f..e4512b7f6732 100644 --- a/packages/core/test/lib/exports.test.ts +++ b/packages/core/test/lib/exports.test.ts @@ -6,7 +6,14 @@ import { getCurrentScope, getIsolationScope, makeMain, + setContext, + setExtra, + setExtras, + setTag, + setTags, + setUser, startSession, + withIsolationScope, withScope, } from '../../src'; import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; @@ -249,4 +256,60 @@ describe('session APIs', () => { expect(getIsolationScope().getSession()).toBe(undefined); }); }); + + describe('setUser', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setUser({ id: 'foo' }); + expect(isolationScope.getScopeData().user.id).toBe('foo'); + }); + }); + }); + + describe('setTags', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setTags({ wee: true, woo: false }); + expect(isolationScope.getScopeData().tags['wee']).toBe(true); + expect(isolationScope.getScopeData().tags['woo']).toBe(false); + }); + }); + }); + + describe('setTag', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setTag('foo', true); + expect(isolationScope.getScopeData().tags['foo']).toBe(true); + }); + }); + }); + + describe('setExtras', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setExtras({ wee: { foo: 'bar' }, woo: { foo: 'bar' } }); + expect(isolationScope.getScopeData().extra?.wee).toEqual({ foo: 'bar' }); + expect(isolationScope.getScopeData().extra?.woo).toEqual({ foo: 'bar' }); + }); + }); + }); + + describe('setExtra', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setExtra('foo', { bar: 'baz' }); + expect(isolationScope.getScopeData().extra?.foo).toEqual({ bar: 'baz' }); + }); + }); + }); + + describe('setContext', () => { + it('should write to the isolation scope', () => { + withIsolationScope(isolationScope => { + setContext('foo', { bar: 'baz' }); + expect(isolationScope.getScopeData().contexts?.foo?.bar).toBe('baz'); + }); + }); + }); });