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); } /** diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index fe682bf02661..01546a84be25 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -189,10 +189,20 @@ export class Scope implements ScopeInterface { * @inheritDoc */ public setUser(user: User | null): this { - this._user = user || {}; + // 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, + ip_address: undefined, + segment: undefined, + username: undefined, + }; + if (this._session) { updateSession(this._session, { user }); } + this._notifyScopeListeners(); return this; } diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index 426f2295fbb0..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'; @@ -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,21 @@ 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] }; + // Clone object + data[prop] = { ...data[prop] }; + for (const key in mergeVal) { + if (Object.prototype.hasOwnProperty.call(mergeVal, key)) { + data[prop][key] = mergeVal[key]; + } + } } } @@ -136,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; } 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'); + }); + }); + }); }); diff --git a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts index 897524cf05e2..7543d9ed39e3 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,55 +23,19 @@ 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', () => { +describe('mergeAndOverwriteScopeData', () => { it.each([ [{}, {}, {}], [{ a: 'aa' }, {}, { a: 'aa' }], [{ 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); }); });