feat(core): Write data from setUser, setTags, setExtras, setTag, setExtra, and setContext to isolation scope (#10163)

Luca Forstner · mydea · web-flow · commit b174d53a36dc · 2024-01-15T14:13:51.000+01:00
Co-authored-by: Francesco Novy &lt;francesco.novy@sentry.io&gt;
diff --git 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,44 +403,59 @@ 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);
   }
 
   /**
    * @inheritDoc
    * @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);
   }
 
   /**
    * @inheritDoc
    * @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);
   }
 
   /**
    * @inheritDoc
    * @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);
   }
 
   /**
    * @inheritDoc
    * @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
@@ -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
@@ -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
@@ -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
@@ -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);
   });
 });
