Fix ID Validation Error Message / refactor (#835)

.changeset/mighty-ads-fly.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-next': patch
+'@segment/analytics-core': patch
+---
+
+Refactor shared validation logic. Create granular error message if user ID does not match string type.

packages/browser/src/core/arguments-resolver/index.ts

@@ -3,7 +3,7 @@ import {
   isPlainObject,
   isString,
   isNumber,
-} from '../../plugins/validation'
+} from '@segment/analytics-core'
 import { Context } from '../context'
 import {
   Callback,

packages/browser/src/plugins/validation/__tests__/index.test.ts

@@ -12,70 +12,66 @@ const validEvent: SegmentEvent = {
 
 describe('validation', () => {
   ;['track', 'identify', 'group', 'page', 'alias'].forEach((method) => {
+    // @ts-ignore
+    const validationFn: Function = validation[method] as any
     describe(method, () => {
       it('validates that the `event` exists', async () => {
-        const val = async () =>
+        try {
           // @ts-ignore
-          validation[method](
-            // @ts-ignore
-            new Context()
-          )
-
-        await expect(val()).rejects.toMatchInlineSnapshot(
-          `[Error: Event is missing]`
-        )
+          await validationFn(new Context())
+        } catch (err) {
+          expect(err).toBeTruthy()
+        }
+        expect.assertions(1)
       })
 
       it('validates that `event.event` exists', async () => {
-        const val = async () =>
-          // @ts-ignore
-          validation[method](
-            new Context({
-              ...validEvent,
-              event: undefined,
-            })
-          )
-
         if (method === 'track') {
-          await expect(val()).rejects.toMatchInlineSnapshot(
-            `[Error: Event is not a string]`
-          )
+          try {
+            await validationFn(
+              new Context({
+                ...validEvent,
+                event: undefined,
+              })
+            )
+          } catch (err) {
+            expect(err).toBeTruthy()
+          }
+          expect.assertions(1)
         }
       })
 
       it('validates that `properties` or `traits` are objects', async () => {
         if (method === 'alias') {
           return
         }
-        const val = async () =>
-          // @ts-ignore
-          validation[method](
+        try {
+          await validationFn(
             new Context({
               ...validEvent,
               properties: undefined,
               traits: undefined,
             })
           )
-
-        await expect(val()).rejects.toMatchInlineSnapshot(
-          `[Error: properties is not an object]`
-        )
+        } catch (err) {
+          expect(err).toBeTruthy()
+        }
+        expect.assertions(1)
       })
 
       it('validates that it contains an user', async () => {
-        const val = async () =>
-          // @ts-ignore
-          validation[method](
+        try {
+          await validationFn(
             new Context({
               ...validEvent,
               userId: undefined,
               anonymousId: undefined,
             })
           )
-
-        await expect(val()).rejects.toMatchInlineSnapshot(
-          `[Error: Missing userId or anonymousId]`
-        )
+        } catch (err) {
+          expect(err).toBeTruthy()
+        }
+        expect.assertions(1)
       })
     })
   })

packages/browser/src/plugins/validation/index.ts

@@ -1,65 +1,29 @@
 import type { Plugin } from '../../core/plugin'
-import type { SegmentEvent } from '../../core/events'
 import type { Context } from '../../core/context'
-
-export function isString(obj: unknown): obj is string {
-  return typeof obj === 'string'
-}
-
-export function isNumber(obj: unknown): obj is number {
-  return typeof obj === 'number'
-}
-
-export function isFunction(obj: unknown): obj is Function {
-  return typeof obj === 'function'
-}
-
-export function isPlainObject(obj: unknown): obj is Record<string, any> {
-  return (
-    Object.prototype.toString.call(obj).slice(8, -1).toLowerCase() === 'object'
-  )
-}
-
-function hasUser(event: SegmentEvent): boolean {
-  const id =
-    event.userId ?? event.anonymousId ?? event.groupId ?? event.previousId
-  return isString(id)
-}
-
-class ValidationError extends Error {
-  field: string
-
-  constructor(field: string, message: string) {
-    super(message)
-    this.field = field
-  }
-}
+import {
+  assertUserIdentity,
+  isPlainObject,
+  ValidationError,
+  assertEventExists,
+  assertEventType,
+  assertTrackEventName,
+} from '@segment/analytics-core'
 
 function validate(ctx: Context): Context {
-  const eventType: unknown = ctx && ctx.event && ctx.event.type
   const event = ctx.event
+  assertEventExists(event)
+  assertEventType(event)
 
-  if (event === undefined) {
-    throw new ValidationError('event', 'Event is missing')
-  }
-
-  if (!isString(eventType)) {
-    throw new ValidationError('event', 'Event is not a string')
-  }
-
-  if (eventType === 'track' && !isString(event.event)) {
-    throw new ValidationError('event', 'Event is not a string')
+  if (event.type === 'track') {
+    assertTrackEventName(event)
   }
 
   const props = event.properties ?? event.traits
-  if (eventType !== 'alias' && !isPlainObject(props)) {
-    throw new ValidationError('properties', 'properties is not an object')
-  }
-
-  if (!hasUser(event)) {
-    throw new ValidationError('userId', 'Missing userId or anonymousId')
+  if (event.type !== 'alias' && !isPlainObject(props)) {
+    throw new ValidationError('.properties', 'is not an object')
   }
 
+  assertUserIdentity(event)
   return ctx
 }
 

packages/core/src/index.ts

@@ -11,6 +11,7 @@ export * from './queue/event-queue'
 export * from './analytics'
 export * from './analytics/dispatch'
 export * from './validation/helpers'
+export * from './validation/errors'
 export * from './validation/assertions'
 export * from './utils/bind-all'
 export * from './stats'

packages/core/src/validation/__tests__/assertions.test.ts

@@ -5,7 +5,7 @@ const baseEvent: Partial<CoreSegmentEvent> = {
   userId: 'foo',
   event: 'Test Event',
 }
-describe('validateEvent', () => {
+describe(validateEvent, () => {
   test('should be capable of working with empty properties and traits', () => {
     expect(() => validateEvent(undefined)).toThrowError()
     expect(() => validateEvent(null)).toThrowError()
@@ -76,54 +76,80 @@ describe('validateEvent', () => {
     ).not.toThrow()
   })
 
-  test('should require either a user ID or anonymous ID or previousID or Group ID for all events', () => {
-    expect(() =>
-      validateEvent({
-        ...baseEvent,
-        type: 'track',
-        properties: {},
-        userId: undefined,
-        anonymousId: 'foo',
-      })
-    ).not.toThrow()
+  describe('User Validation', () => {
+    test('should pass if either a userID/anonymousID/previousID/groupID are defined', () => {
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'track',
+          properties: {},
+          userId: undefined,
+          anonymousId: 'foo',
+        })
+      ).not.toThrow()
 
-    expect(() =>
-      validateEvent({
-        ...baseEvent,
-        type: 'identify',
-        traits: {},
-        userId: 'foo',
-        anonymousId: undefined,
-      })
-    ).not.toThrow()
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'identify',
+          traits: {},
+          userId: 'foo',
+          anonymousId: undefined,
+        })
+      ).not.toThrow()
 
-    expect(() =>
-      validateEvent({
-        ...baseEvent,
-        type: 'alias',
-        previousId: 'foo',
-        userId: undefined,
-        anonymousId: undefined,
-      })
-    ).not.toThrow()
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'alias',
+          previousId: 'foo',
+          userId: undefined,
+          anonymousId: undefined,
+        })
+      ).not.toThrow()
 
-    expect(() =>
-      validateEvent({
-        ...baseEvent,
-        type: 'group',
-        traits: {},
-        groupId: 'foo',
-      })
-    ).not.toThrow()
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'group',
+          traits: {},
+          groupId: 'foo',
+        })
+      ).not.toThrow()
+    })
 
-    expect(() =>
-      validateEvent({
-        ...baseEvent,
-        type: 'alias',
-        previousId: undefined,
-        userId: undefined,
-        anonymousId: undefined,
-      })
-    ).toThrow()
+    test('should fail if ID is _not_ string', () => {
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'track',
+          properties: {},
+          userId: undefined,
+          anonymousId: 123 as any,
+        })
+      ).toThrowError(/string/)
+
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'track',
+          properties: {},
+          userId: undefined,
+          anonymousId: 123 as any,
+        })
+      ).toThrowError(/string/)
+    })
+
+    test('should handle null as well as undefined', () => {
+      expect(() =>
+        validateEvent({
+          ...baseEvent,
+          type: 'track',
+          properties: {},
+          userId: undefined,
+          anonymousId: null,
+        })
+      ).toThrowError(/nil/i)
+    })
   })
 })