diff --git a/dev-packages/browser-integration-tests/suites/public-api/setTag/with_non_primitives/test.ts b/dev-packages/browser-integration-tests/suites/public-api/setTag/with_non_primitives/test.ts index 648187404c0e..c3c5494f5b3f 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/setTag/with_non_primitives/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/setTag/with_non_primitives/test.ts @@ -3,19 +3,11 @@ import type { Event } from '@sentry/core'; import { sentryTest } from '../../../../utils/fixtures'; import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; -sentryTest('[bug] accepts non-primitive tags', async ({ getLocalTestUrl, page }) => { - // this is a bug that went unnoticed due to type definitions and a bad assertion - // TODO: We should not accept non-primitive tags. Fix this as a follow-up. +sentryTest("doesn't accept non-primitive tags", async ({ getLocalTestUrl, page }) => { const url = await getLocalTestUrl({ testDir: __dirname }); const eventData = await getFirstSentryEnvelopeRequest(page, url); expect(eventData.message).toBe('non_primitives'); - - // TODO: This should be an empty object but instead, it is: - expect(eventData.tags).toEqual({ - tag_1: {}, - tag_2: [], - tag_3: ['a', {}], - }); + expect(eventData.tags).toEqual({}); }); diff --git a/dev-packages/browser-integration-tests/suites/public-api/setTags/with_non_primitives/test.ts b/dev-packages/browser-integration-tests/suites/public-api/setTags/with_non_primitives/test.ts index 181711650074..29002550b72e 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/setTags/with_non_primitives/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/setTags/with_non_primitives/test.ts @@ -9,5 +9,5 @@ sentryTest('should not accept non-primitive tags', async ({ getLocalTestUrl, pag const eventData = await getFirstSentryEnvelopeRequest(page, url); expect(eventData.message).toBe('non_primitives'); - expect(eventData.tags).toMatchObject({}); + expect(eventData.tags).toBeUndefined(); }); diff --git a/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/scenario.ts index 4e0bc30c0c2b..952c1fb306cb 100644 --- a/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/scenario.ts +++ b/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/scenario.ts @@ -14,4 +14,4 @@ Sentry.setTag('tag_4', null); Sentry.setTag('tag_5', undefined); Sentry.setTag('tag_6', -1); -Sentry.captureMessage('primitive_tags'); +Sentry.captureMessage('primitive_tags-set-tag'); diff --git a/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/test.ts b/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/test.ts index 23e22402c666..b8adcf04b800 100644 --- a/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/setTag/with-primitives/test.ts @@ -1,4 +1,4 @@ -import { afterAll, test } from 'vitest'; +import { afterAll, expect, test } from 'vitest'; import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; afterAll(() => { @@ -8,15 +8,15 @@ afterAll(() => { test('should set primitive tags', async () => { await createRunner(__dirname, 'scenario.ts') .expect({ - event: { - message: 'primitive_tags', - tags: { + event: event => { + expect(event.message).toBe('primitive_tags-set-tag'); + expect(event.tags).toEqual({ tag_1: 'foo', tag_2: 3.141592653589793, tag_3: false, tag_4: null, tag_6: -1, - }, + }); }, }) .start() diff --git a/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/scenario.ts index 4e0bc30c0c2b..0e1350fe74b3 100644 --- a/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/scenario.ts +++ b/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/scenario.ts @@ -7,11 +7,6 @@ Sentry.init({ transport: loggingTransport, }); -Sentry.setTag('tag_1', 'foo'); -Sentry.setTag('tag_2', Math.PI); -Sentry.setTag('tag_3', false); -Sentry.setTag('tag_4', null); -Sentry.setTag('tag_5', undefined); -Sentry.setTag('tag_6', -1); +Sentry.setTags({ tag_1: 'foo', tag_2: Math.PI, tag_3: false, tag_4: null, tag_5: undefined, tag_6: -1 }); -Sentry.captureMessage('primitive_tags'); +Sentry.captureMessage('primitive_tags-set-tags'); diff --git a/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/test.ts b/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/test.ts index 23e22402c666..fcdb94debcee 100644 --- a/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/setTags/with-primitives/test.ts @@ -1,4 +1,4 @@ -import { afterAll, test } from 'vitest'; +import { afterAll, expect, test } from 'vitest'; import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; afterAll(() => { @@ -8,15 +8,15 @@ afterAll(() => { test('should set primitive tags', async () => { await createRunner(__dirname, 'scenario.ts') .expect({ - event: { - message: 'primitive_tags', - tags: { + event: event => { + expect(event.message).toBe('primitive_tags-set-tags'); + expect(event.tags).toEqual({ tag_1: 'foo', tag_2: 3.141592653589793, tag_3: false, tag_4: null, tag_6: -1, - }, + }); }, }) .start() diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 8b1e21acfb4a..40aa38cdf348 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -17,7 +17,7 @@ import type { Span } from './types-hoist/span'; import type { PropagationContext } from './types-hoist/tracing'; import type { User } from './types-hoist/user'; import { debug } from './utils/debug-logger'; -import { isPlainObject } from './utils/is'; +import { isPlainObject, isPrimitive } from './utils/is'; import { merge } from './utils/merge'; import { uuid4 } from './utils/misc'; import { generateTraceId } from './utils/propagationContext'; @@ -279,10 +279,11 @@ export class Scope { * and will be sent as tags data with the event. */ public setTags(tags: { [key: string]: Primitive }): this { - this._tags = { - ...this._tags, - ...tags, - }; + for (const [key, value] of Object.entries(tags)) { + if (isPrimitive(value)) { + this._tags[key] = value; + } + } this._notifyScopeListeners(); return this; } @@ -291,9 +292,7 @@ export class Scope { * Set a single tag that will be sent as tags data with the event. */ public setTag(key: string, value: Primitive): this { - this._tags = { ...this._tags, [key]: value }; - this._notifyScopeListeners(); - return this; + return this.setTags({ [key]: value }); } /** diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts index 280ba4c651ff..5260ad6e0cf5 100644 --- a/packages/core/test/lib/scope.test.ts +++ b/packages/core/test/lib/scope.test.ts @@ -140,16 +140,61 @@ describe('Scope', () => { expect(scope['_extra']).toEqual({ a: undefined }); }); - test('setTag', () => { - const scope = new Scope(); - scope.setTag('a', 'b'); - expect(scope['_tags']).toEqual({ a: 'b' }); + describe('setTag', () => { + it('sets a tag', () => { + const scope = new Scope(); + scope.setTag('a', 'b'); + expect(scope['_tags']).toEqual({ a: 'b' }); + }); + + it('sets a tag with undefined', () => { + const scope = new Scope(); + scope.setTag('a', 'b'); + scope.setTag('a', undefined); + expect(scope['_tags']).toEqual({ a: undefined }); + }); + + it('notifies scope listeners once per call', () => { + const scope = new Scope(); + const listener = vi.fn(); + + scope.addScopeListener(listener); + scope.setTag('a', 'b'); + scope.setTag('a', 'c'); + + expect(listener).toHaveBeenCalledTimes(2); + }); + + it('discards non-primitive values', () => { + const scope = new Scope(); + // @ts-expect-error we want to test with a non-primitive value despite types not allowing it + scope.setTag('a', { b: 'c' }); + expect(scope['_tags']).toEqual({}); + }); }); - test('setTags', () => { - const scope = new Scope(); - scope.setTags({ a: 'b' }); - expect(scope['_tags']).toEqual({ a: 'b' }); + describe('setTags', () => { + it('sets tags', () => { + const scope = new Scope(); + scope.setTags({ a: 'b' }); + expect(scope['_tags']).toEqual({ a: 'b' }); + }); + + it('notifies scope listeners once per call', () => { + const scope = new Scope(); + const listener = vi.fn(); + scope.addScopeListener(listener); + scope.setTags({ a: 'b', c: 'd' }); + scope.setTags({ a: 'e', f: 'g' }); + expect(listener).toHaveBeenCalledTimes(2); + }); + + it('discards non-primitive values', () => { + const scope = new Scope(); + // @ts-expect-error we want to test with a non-primitive value despite types not allowing it + scope.setTags({ a: { b: 'c' }, b: [1, 2, 3], c: new Map(), d: () => {} }); + expect(scope['_tags']).toEqual({}); + }); }); test('setUser', () => {