Skip to content

Commit 9353e09

Browse files
sileskysilesky
authored
Fix ID Validation Error Message / refactor (#835)
1 parent 243b0eb commit 9353e09

File tree

9 files changed

+192
-166
lines changed

9 files changed

+192
-166
lines changed

‎.changeset/mighty-ads-fly.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@segment/analytics-next': patch
3+
'@segment/analytics-core': patch
4+
---
5+
6+
Refactor shared validation logic. Create granular error message if user ID does not match string type.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
isPlainObject,
44
isString,
55
isNumber,
6-
} from '../../plugins/validation'
6+
} from '@segment/analytics-core'
77
import { Context } from '../context'
88
import {
99
Callback,

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

Lines changed: 31 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -12,70 +12,66 @@ const validEvent: SegmentEvent = {
1212

1313
describe('validation', () => {
1414
;['track', 'identify', 'group', 'page', 'alias'].forEach((method) => {
15+
// @ts-ignore
16+
const validationFn: Function = validation[method] as any
1517
describe(method, () => {
1618
it('validates that the `event` exists', async () => {
17-
const val = async () =>
19+
try {
1820
// @ts-ignore
19-
validation[method](
20-
// @ts-ignore
21-
new Context()
22-
)
23-
24-
await expect(val()).rejects.toMatchInlineSnapshot(
25-
`[Error: Event is missing]`
26-
)
21+
await validationFn(new Context())
22+
} catch (err) {
23+
expect(err).toBeTruthy()
24+
}
25+
expect.assertions(1)
2726
})
2827

2928
it('validates that `event.event` exists', async () => {
30-
const val = async () =>
31-
// @ts-ignore
32-
validation[method](
33-
new Context({
34-
...validEvent,
35-
event: undefined,
36-
})
37-
)
38-
3929
if (method === 'track') {
40-
await expect(val()).rejects.toMatchInlineSnapshot(
41-
`[Error: Event is not a string]`
42-
)
30+
try {
31+
await validationFn(
32+
new Context({
33+
...validEvent,
34+
event: undefined,
35+
})
36+
)
37+
} catch (err) {
38+
expect(err).toBeTruthy()
39+
}
40+
expect.assertions(1)
4341
}
4442
})
4543

4644
it('validates that `properties` or `traits` are objects', async () => {
4745
if (method === 'alias') {
4846
return
4947
}
50-
const val = async () =>
51-
// @ts-ignore
52-
validation[method](
48+
try {
49+
await validationFn(
5350
new Context({
5451
...validEvent,
5552
properties: undefined,
5653
traits: undefined,
5754
})
5855
)
59-
60-
await expect(val()).rejects.toMatchInlineSnapshot(
61-
`[Error: properties is not an object]`
62-
)
56+
} catch (err) {
57+
expect(err).toBeTruthy()
58+
}
59+
expect.assertions(1)
6360
})
6461

6562
it('validates that it contains an user', async () => {
66-
const val = async () =>
67-
// @ts-ignore
68-
validation[method](
63+
try {
64+
await validationFn(
6965
new Context({
7066
...validEvent,
7167
userId: undefined,
7268
anonymousId: undefined,
7369
})
7470
)
75-
76-
await expect(val()).rejects.toMatchInlineSnapshot(
77-
`[Error: Missing userId or anonymousId]`
78-
)
71+
} catch (err) {
72+
expect(err).toBeTruthy()
73+
}
74+
expect.assertions(1)
7975
})
8076
})
8177
})

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

Lines changed: 15 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,65 +1,29 @@
11
import type { Plugin } from '../../core/plugin'
2-
import type { SegmentEvent } from '../../core/events'
32
import type { Context } from '../../core/context'
4-
5-
export function isString(obj: unknown): obj is string {
6-
return typeof obj === 'string'
7-
}
8-
9-
export function isNumber(obj: unknown): obj is number {
10-
return typeof obj === 'number'
11-
}
12-
13-
export function isFunction(obj: unknown): obj is Function {
14-
return typeof obj === 'function'
15-
}
16-
17-
export function isPlainObject(obj: unknown): obj is Record<string, any> {
18-
return (
19-
Object.prototype.toString.call(obj).slice(8, -1).toLowerCase() === 'object'
20-
)
21-
}
22-
23-
function hasUser(event: SegmentEvent): boolean {
24-
const id =
25-
event.userId ?? event.anonymousId ?? event.groupId ?? event.previousId
26-
return isString(id)
27-
}
28-
29-
class ValidationError extends Error {
30-
field: string
31-
32-
constructor(field: string, message: string) {
33-
super(message)
34-
this.field = field
35-
}
36-
}
3+
import {
4+
assertUserIdentity,
5+
isPlainObject,
6+
ValidationError,
7+
assertEventExists,
8+
assertEventType,
9+
assertTrackEventName,
10+
} from '@segment/analytics-core'
3711

3812
function validate(ctx: Context): Context {
39-
const eventType: unknown = ctx && ctx.event && ctx.event.type
4013
const event = ctx.event
14+
assertEventExists(event)
15+
assertEventType(event)
4116

42-
if (event === undefined) {
43-
throw new ValidationError('event', 'Event is missing')
44-
}
45-
46-
if (!isString(eventType)) {
47-
throw new ValidationError('event', 'Event is not a string')
48-
}
49-
50-
if (eventType === 'track' && !isString(event.event)) {
51-
throw new ValidationError('event', 'Event is not a string')
17+
if (event.type === 'track') {
18+
assertTrackEventName(event)
5219
}
5320

5421
const props = event.properties ?? event.traits
55-
if (eventType !== 'alias' && !isPlainObject(props)) {
56-
throw new ValidationError('properties', 'properties is not an object')
57-
}
58-
59-
if (!hasUser(event)) {
60-
throw new ValidationError('userId', 'Missing userId or anonymousId')
22+
if (event.type !== 'alias' && !isPlainObject(props)) {
23+
throw new ValidationError('.properties', 'is not an object')
6124
}
6225

26+
assertUserIdentity(event)
6327
return ctx
6428
}
6529

‎packages/core/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export * from './queue/event-queue'
1111
export * from './analytics'
1212
export * from './analytics/dispatch'
1313
export * from './validation/helpers'
14+
export * from './validation/errors'
1415
export * from './validation/assertions'
1516
export * from './utils/bind-all'
1617
export * from './stats'

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

Lines changed: 72 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ const baseEvent: Partial<CoreSegmentEvent> = {
55
userId: 'foo',
66
event: 'Test Event',
77
}
8-
describe('validateEvent', () => {
8+
describe(validateEvent, () => {
99
test('should be capable of working with empty properties and traits', () => {
1010
expect(() => validateEvent(undefined)).toThrowError()
1111
expect(() => validateEvent(null)).toThrowError()
@@ -76,54 +76,80 @@ describe('validateEvent', () => {
7676
).not.toThrow()
7777
})
7878

79-
test('should require either a user ID or anonymous ID or previousID or Group ID for all events', () => {
80-
expect(() =>
81-
validateEvent({
82-
...baseEvent,
83-
type: 'track',
84-
properties: {},
85-
userId: undefined,
86-
anonymousId: 'foo',
87-
})
88-
).not.toThrow()
79+
describe('User Validation', () => {
80+
test('should pass if either a userID/anonymousID/previousID/groupID are defined', () => {
81+
expect(() =>
82+
validateEvent({
83+
...baseEvent,
84+
type: 'track',
85+
properties: {},
86+
userId: undefined,
87+
anonymousId: 'foo',
88+
})
89+
).not.toThrow()
8990

90-
expect(() =>
91-
validateEvent({
92-
...baseEvent,
93-
type: 'identify',
94-
traits: {},
95-
userId: 'foo',
96-
anonymousId: undefined,
97-
})
98-
).not.toThrow()
91+
expect(() =>
92+
validateEvent({
93+
...baseEvent,
94+
type: 'identify',
95+
traits: {},
96+
userId: 'foo',
97+
anonymousId: undefined,
98+
})
99+
).not.toThrow()
99100

100-
expect(() =>
101-
validateEvent({
102-
...baseEvent,
103-
type: 'alias',
104-
previousId: 'foo',
105-
userId: undefined,
106-
anonymousId: undefined,
107-
})
108-
).not.toThrow()
101+
expect(() =>
102+
validateEvent({
103+
...baseEvent,
104+
type: 'alias',
105+
previousId: 'foo',
106+
userId: undefined,
107+
anonymousId: undefined,
108+
})
109+
).not.toThrow()
109110

110-
expect(() =>
111-
validateEvent({
112-
...baseEvent,
113-
type: 'group',
114-
traits: {},
115-
groupId: 'foo',
116-
})
117-
).not.toThrow()
111+
expect(() =>
112+
validateEvent({
113+
...baseEvent,
114+
type: 'group',
115+
traits: {},
116+
groupId: 'foo',
117+
})
118+
).not.toThrow()
119+
})
118120

119-
expect(() =>
120-
validateEvent({
121-
...baseEvent,
122-
type: 'alias',
123-
previousId: undefined,
124-
userId: undefined,
125-
anonymousId: undefined,
126-
})
127-
).toThrow()
121+
test('should fail if ID is _not_ string', () => {
122+
expect(() =>
123+
validateEvent({
124+
...baseEvent,
125+
type: 'track',
126+
properties: {},
127+
userId: undefined,
128+
anonymousId: 123 as any,
129+
})
130+
).toThrowError(/string/)
131+
132+
expect(() =>
133+
validateEvent({
134+
...baseEvent,
135+
type: 'track',
136+
properties: {},
137+
userId: undefined,
138+
anonymousId: 123 as any,
139+
})
140+
).toThrowError(/string/)
141+
})
142+
143+
test('should handle null as well as undefined', () => {
144+
expect(() =>
145+
validateEvent({
146+
...baseEvent,
147+
type: 'track',
148+
properties: {},
149+
userId: undefined,
150+
anonymousId: null,
151+
})
152+
).toThrowError(/nil/i)
153+
})
128154
})
129155
})

0 commit comments

Comments
 (0)