From 95fd2fd801da26505ddcead96ffaa83aa4364994 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Tue, 27 Feb 2024 11:44:18 -0600 Subject: [PATCH] Retain messageId if exists (allow overriding) (#1043) Co-authored-by: H.Drake --- .../message-id-backward-compatibility.md | 8 +++ .../core/src/events/__tests__/index.test.ts | 32 +++++++---- packages/core/src/events/index.ts | 7 ++- packages/core/src/events/interfaces.ts | 6 ++ .../validation/__tests__/assertions.test.ts | 27 +++++++++ packages/core/src/validation/assertions.ts | 7 +++ .../node/src/__tests__/integration.test.ts | 57 +++++++++++++++++++ packages/node/src/app/analytics-node.ts | 22 ++++++- packages/node/src/app/types/params.ts | 25 ++++++++ 9 files changed, 176 insertions(+), 15 deletions(-) create mode 100644 .changeset/message-id-backward-compatibility.md diff --git a/.changeset/message-id-backward-compatibility.md b/.changeset/message-id-backward-compatibility.md new file mode 100644 index 000000000..c7d0b144a --- /dev/null +++ b/.changeset/message-id-backward-compatibility.md @@ -0,0 +1,8 @@ +--- +'@segment/analytics-core': patch +'@segment/analytics-node': minor +--- + +This ensures backward compatibility with analytics-node by modifying '@segment/analytics-core'. Specifically, the changes prevent the generation of a messageId if it is already set. This adjustment aligns with the behavior outlined in analytics-node's source code [here](https://github.com/segmentio/analytics-node/blob/master/index.js#L195-L201). + +While this is a core release, only the node library is affected, as the browser has its own EventFactory atm. \ No newline at end of file diff --git a/packages/core/src/events/__tests__/index.test.ts b/packages/core/src/events/__tests__/index.test.ts index eba8961b7..27c482a5c 100644 --- a/packages/core/src/events/__tests__/index.test.ts +++ b/packages/core/src/events/__tests__/index.test.ts @@ -350,6 +350,27 @@ describe('Event Factory', () => { innerProp: '👻', }) }) + + test('accepts a messageId', () => { + const messageId = 'business-id-123' + const track = factory.track('Order Completed', shoes, { + messageId, + }) + + expect(track.context).toEqual({}) + expect(track.messageId).toEqual(messageId) + }) + + it('should ignore undefined options', () => { + const event = factory.track( + 'Order Completed', + { ...shoes }, + { timestamp: undefined, traits: { foo: 123 } } + ) + + expect(typeof event.timestamp).toBe('object') + expect(isDate(event.timestamp)).toBeTruthy() + }) }) describe('normalize', () => { @@ -380,15 +401,4 @@ describe('Event Factory', () => { }) }) }) - - it('should ignore undefined options', () => { - const event = factory.track( - 'Order Completed', - { ...shoes }, - { timestamp: undefined, traits: { foo: 123 } } - ) - - expect(typeof event.timestamp).toBe('object') - expect(isDate(event.timestamp)).toBeTruthy() - }) }) diff --git a/packages/core/src/events/index.ts b/packages/core/src/events/index.ts index 366d942a9..2c7d80ee0 100644 --- a/packages/core/src/events/index.ts +++ b/packages/core/src/events/index.ts @@ -19,6 +19,10 @@ interface EventFactorySettings { user?: User } +/** + * This is currently only used by node.js, but the original idea was to have something that could be shared between browser and node. + * Unfortunately, there are some differences in the way the two environments handle events, so this is not currently shared. + */ export class EventFactory { createMessageId: EventFactorySettings['createMessageId'] user?: User @@ -201,6 +205,7 @@ export class EventFactory { 'userId', 'anonymousId', 'timestamp', + 'messageId', ] delete options['integrations'] @@ -271,7 +276,7 @@ export class EventFactory { const evt: CoreSegmentEvent = { ...body, - messageId: this.createMessageId(), + messageId: options.messageId || this.createMessageId(), } validateEvent(evt) diff --git a/packages/core/src/events/interfaces.ts b/packages/core/src/events/interfaces.ts index 0bc59bf5c..0db091874 100644 --- a/packages/core/src/events/interfaces.ts +++ b/packages/core/src/events/interfaces.ts @@ -33,6 +33,12 @@ export interface CoreOptions { anonymousId?: string userId?: string traits?: Traits + /** + * Override the messageId. Under normal circumstances, this is not recommended -- but neccessary for deduping events. + * + * **Currently, This option only works in `@segment/analytics-node`.** + */ + messageId?: string // ugh, this is ugly, but we allow literally any property to be passed to options (which get spread onto the event) [key: string]: any } diff --git a/packages/core/src/validation/__tests__/assertions.test.ts b/packages/core/src/validation/__tests__/assertions.test.ts index c6f9b81c3..df4735c14 100644 --- a/packages/core/src/validation/__tests__/assertions.test.ts +++ b/packages/core/src/validation/__tests__/assertions.test.ts @@ -4,7 +4,9 @@ import { validateEvent } from '../assertions' const baseEvent: Partial = { userId: 'foo', event: 'Test Event', + messageId: 'foo', } + describe(validateEvent, () => { test('should be capable of working with empty properties and traits', () => { expect(() => validateEvent(undefined)).toThrowError() @@ -40,6 +42,7 @@ describe(validateEvent, () => { test('identify: traits should be an object', () => { expect(() => validateEvent({ + ...baseEvent, type: 'identify', traits: undefined, }) @@ -151,5 +154,29 @@ describe(validateEvent, () => { }) ).toThrowError(/nil/i) }) + + test('should fail if messageId is _not_ string', () => { + expect(() => + validateEvent({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 'foo', + messageId: 'bar', + }) + ).not.toThrow() + + expect(() => + validateEvent({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 'foo', + messageId: 123 as any, + }) + ).toThrow(/messageId/) + }) }) }) diff --git a/packages/core/src/validation/assertions.ts b/packages/core/src/validation/assertions.ts index 8717f4800..e563a6a0e 100644 --- a/packages/core/src/validation/assertions.ts +++ b/packages/core/src/validation/assertions.ts @@ -55,9 +55,16 @@ export function assertTraits(event: CoreSegmentEvent): void { } } +export function assertMessageId(event: CoreSegmentEvent): void { + if (!isString(event.messageId)) { + throw new ValidationError('.messageId', stringError) + } +} + export function validateEvent(event?: CoreSegmentEvent | null) { assertEventExists(event) assertEventType(event) + assertMessageId(event) if (event.type === 'track') { assertTrackEventName(event) diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index f16bbb4ca..2554c115d 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -88,6 +88,16 @@ describe('alias', () => { expect(ctx.event.userId).toEqual('chris radek') expect(ctx.event.previousId).toEqual('chris') expect(ctx.event.timestamp).toEqual(timestamp) + expect(ctx.event.messageId).toEqual(expect.any(String)) + }) + it('allows messageId to be overridden', async () => { + const analytics = createTestAnalytics({ + httpClient: testClient, + }) + const messageId = 'overridden' + analytics.alias({ userId: 'foo', previousId: 'bar', messageId }) + const ctx = await resolveCtx(analytics, 'alias') + expect(ctx.event.messageId).toBe(messageId) }) }) @@ -107,6 +117,7 @@ describe('group', () => { expect(ctx.event.userId).toEqual('foo') expect(ctx.event.anonymousId).toBe('bar') expect(ctx.event.timestamp).toEqual(timestamp) + expect(ctx.event.messageId).toEqual(expect.any(String)) }) it('invocations are isolated', async () => { @@ -134,6 +145,16 @@ describe('group', () => { expect(ctx2.event.anonymousId).toBeUndefined() expect(ctx2.event.userId).toEqual('me') }) + + it('allows messageId to be overridden', async () => { + const analytics = createTestAnalytics({ + httpClient: testClient, + }) + const messageId = 'overridden' + analytics.group({ groupId: 'foo', userId: 'sup', messageId }) + const ctx = await resolveCtx(analytics, 'group') + expect(ctx.event.messageId).toBe(messageId) + }) }) describe('identify', () => { @@ -181,6 +202,7 @@ describe('page', () => { expect(ctx1.event.userId).toBeUndefined() expect(ctx1.event.properties).toEqual({ category }) expect(ctx1.event.timestamp).toEqual(timestamp) + expect(ctx1.event.messageId).toEqual(expect.any(String)) analytics.page({ name, properties: { title: 'wip' }, userId: 'user-id' }) @@ -192,6 +214,7 @@ describe('page', () => { expect(ctx2.event.userId).toEqual('user-id') expect(ctx2.event.properties).toEqual({ title: 'wip' }) expect(ctx2.event.timestamp).toEqual(expect.any(Date)) + expect(ctx2.event.messageId).toEqual(expect.any(String)) analytics.page({ properties: { title: 'invisible' }, userId: 'user-id' }) const ctx3 = await resolveCtx(analytics, 'page') @@ -201,6 +224,17 @@ describe('page', () => { expect(ctx3.event.anonymousId).toBeUndefined() expect(ctx3.event.userId).toEqual('user-id') expect(ctx3.event.properties).toEqual({ title: 'invisible' }) + expect(ctx3.event.messageId).toEqual(expect.any(String)) + }) + + it('allows messageId to be overridden', async () => { + const analytics = createTestAnalytics({ + httpClient: testClient, + }) + const messageId = 'overridden' + analytics.page({ name: 'foo', userId: 'sup', messageId }) + const ctx = await resolveCtx(analytics, 'page') + expect(ctx.event.messageId).toBe(messageId) }) }) @@ -224,6 +258,7 @@ describe('screen', () => { expect(ctx1.event.userId).toEqual('user-id') expect(ctx1.event.properties).toEqual({ title: 'wip' }) expect(ctx1.event.timestamp).toEqual(timestamp) + expect(ctx1.event.messageId).toEqual(expect.any(String)) analytics.screen({ properties: { title: 'invisible' }, @@ -238,6 +273,16 @@ describe('screen', () => { expect(ctx2.event.userId).toEqual('user-id') expect(ctx2.event.properties).toEqual({ title: 'invisible' }) expect(ctx2.event.timestamp).toEqual(expect.any(Date)) + expect(ctx2.event.messageId).toEqual(expect.any(String)) + }) + it('allows messageId to be overridden', async () => { + const analytics = createTestAnalytics({ + httpClient: testClient, + }) + const messageId = 'overridden' + analytics.screen({ name: 'foo', userId: 'sup', messageId }) + const ctx = await resolveCtx(analytics, 'screen') + expect(ctx.event.messageId).toBe(messageId) }) }) @@ -272,6 +317,7 @@ describe('track', () => { expect(ctx1.event.anonymousId).toEqual('unknown') expect(ctx1.event.userId).toEqual('known') expect(ctx1.event.timestamp).toEqual(timestamp) + expect(ctx1.event.messageId).toEqual(expect.any(String)) analytics.track({ event: eventName, @@ -286,6 +332,17 @@ describe('track', () => { expect(ctx2.event.anonymousId).toBeUndefined() expect(ctx2.event.userId).toEqual('known') expect(ctx2.event.timestamp).toEqual(expect.any(Date)) + expect(ctx2.event.messageId).toEqual(expect.any(String)) + }) + + it('allows messageId to be overridden', async () => { + const analytics = createTestAnalytics({ + httpClient: testClient, + }) + const messageId = 'overridden' + analytics.track({ event: 'foo', userId: 'sup', messageId }) + const ctx = await resolveCtx(analytics, 'track') + expect(ctx.event.messageId).toBe(messageId) }) }) diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index f956ca3a3..a5ceb8ab0 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -148,13 +148,21 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { * @link https://segment.com/docs/connections/sources/catalog/libraries/server/node/#alias */ alias( - { userId, previousId, context, timestamp, integrations }: AliasParams, + { + userId, + previousId, + context, + timestamp, + integrations, + messageId, + }: AliasParams, callback?: Callback ): void { const segmentEvent = this._eventFactory.alias(userId, previousId, { context, integrations, timestamp, + messageId, }) this._dispatch(segmentEvent, callback) } @@ -172,6 +180,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { traits = {}, context, integrations, + messageId, }: GroupParams, callback?: Callback ): void { @@ -181,6 +190,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { userId, timestamp, integrations, + messageId, }) this._dispatch(segmentEvent, callback) @@ -198,6 +208,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { context, timestamp, integrations, + messageId, }: IdentifyParams, callback?: Callback ): void { @@ -207,6 +218,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { userId, timestamp, integrations, + messageId, }) this._dispatch(segmentEvent, callback) } @@ -225,6 +237,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { context, timestamp, integrations, + messageId, }: PageParams, callback?: Callback ): void { @@ -232,7 +245,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { category ?? null, name ?? null, properties, - { context, anonymousId, userId, timestamp, integrations } + { context, anonymousId, userId, timestamp, integrations, messageId } ) this._dispatch(segmentEvent, callback) } @@ -253,6 +266,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { context, timestamp, integrations, + messageId, }: PageParams, callback?: Callback ): void { @@ -260,7 +274,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { category ?? null, name ?? null, properties, - { context, anonymousId, userId, timestamp, integrations } + { context, anonymousId, userId, timestamp, integrations, messageId } ) this._dispatch(segmentEvent, callback) @@ -279,6 +293,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { context, timestamp, integrations, + messageId, }: TrackParams, callback?: Callback ): void { @@ -288,6 +303,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { anonymousId, timestamp, integrations, + messageId, }) this._dispatch(segmentEvent, callback) diff --git a/packages/node/src/app/types/params.ts b/packages/node/src/app/types/params.ts index c8974bd8e..983be3b2b 100644 --- a/packages/node/src/app/types/params.ts +++ b/packages/node/src/app/types/params.ts @@ -30,6 +30,11 @@ export type AliasParams = { context?: ExtraContext timestamp?: Timestamp integrations?: Integrations + /** + * Override the default messageId for the purposes of deduping events. Using a uuid library is strongly encouraged. + * @link https://segment.com/docs/partners/faqs/#does-segment-de-dupe-messages + */ + messageId?: string } export type GroupParams = { @@ -43,6 +48,11 @@ export type GroupParams = { context?: ExtraContext timestamp?: Timestamp integrations?: Integrations + /** + * Override the default messageId for the purposes of deduping events. Using a uuid library is strongly encouraged. + * @link https://segment.com/docs/partners/faqs/#does-segment-de-dupe-messages + */ + messageId?: string } & IdentityOptions export type IdentifyParams = { @@ -55,6 +65,11 @@ export type IdentifyParams = { context?: ExtraContext timestamp?: Timestamp integrations?: Integrations + /** + * Override the default messageId for the purposes of deduping events. Using a uuid library is strongly encouraged. + * @link https://segment.com/docs/partners/faqs/#does-segment-de-dupe-messages + */ + messageId?: string } & IdentityOptions export type PageParams = { @@ -67,6 +82,11 @@ export type PageParams = { timestamp?: Timestamp context?: ExtraContext integrations?: Integrations + /** + * Override the default messageId for the purposes of deduping events. Using a uuid library is strongly encouraged. + * @link https://segment.com/docs/partners/faqs/#does-segment-de-dupe-messages + */ + messageId?: string } & IdentityOptions export type TrackParams = { @@ -75,6 +95,11 @@ export type TrackParams = { context?: ExtraContext timestamp?: Timestamp integrations?: Integrations + /** + * Override the default messageId for the purposes of deduping events. Using a uuid library is strongly encouraged. + * @link https://segment.com/docs/partners/faqs/#does-segment-de-dupe-messages + */ + messageId?: string } & IdentityOptions export type FlushParams = {