Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,48 @@ describe('MultiStatus', () => {
errorreporter: 'INTEGRATIONS'
})
})

it('should reject events where external_id exceeds 255 characters', async () => {
nock(API_URL).post('/event-bulk-create-jobs/').reply(202, {})

const longExternalId = 'a'.repeat(256)
const events: SegmentEvent[] = [
// Event with external_id exceeding 255 characters
createTestEvent({
type: 'track',
timestamp,
properties: {
external_id: longExternalId
}
}),
// Valid Event
createTestEvent({
type: 'track',
timestamp,
properties: {
email: 'valid@gmail.com'
}
})
]

const response = await testDestination.executeBatch('trackEvent', {
events,
settings,
mapping
})

expect(response[0]).toMatchObject({
status: 400,
errortype: 'PAYLOAD_VALIDATION_FAILED',
errormessage: 'Length of external_id must be no more than 255 characters.',
errorreporter: 'INTEGRATIONS'
})

expect(response[1]).toMatchObject({
status: 200,
body: '{}'
})
})
})
describe('addProfileToList', () => {
beforeEach(() => {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { KLAVIYO_REVISION_DATE } from './versioning-info'

export const API_URL = 'https://a.klaviyo.com/api'
export const REVISION_DATE = KLAVIYO_REVISION_DATE
export const MAX_EXTERNAL_ID_LENGTH = 255
export const COUNTRY_CODES = [
{ label: 'AD - Andorra', value: 'AD' },
{ label: 'AE - United Arab Emirates', value: 'AE' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
ErrorCodes,
StatsContext
} from '@segment/actions-core'
import { API_URL, REVISION_DATE } from './config'
import { API_URL, REVISION_DATE, MAX_EXTERNAL_ID_LENGTH } from './config'
import { Settings } from './generated-types'
import {
KlaviyoAPIError,
Expand Down Expand Up @@ -40,6 +40,18 @@ import { ActionDestinationErrorResponseType } from '@segment/actions-core/destin

const phoneUtil = PhoneNumberUtil.getInstance()

const EXTERNAL_ID_LENGTH_ERROR: ActionDestinationErrorResponseType = {
status: 400,
errortype: 'PAYLOAD_VALIDATION_FAILED',
errormessage: `Length of external_id must be no more than ${MAX_EXTERNAL_ID_LENGTH} characters.`
}

export function validateExternalId(externalId: string | undefined): void {
if (externalId && externalId.length > MAX_EXTERNAL_ID_LENGTH) {
throw new PayloadValidationError(EXTERNAL_ID_LENGTH_ERROR.errormessage)
}
Comment thread
AnkitSegment marked this conversation as resolved.
}

export async function getListIdDynamicData(request: RequestClient): Promise<DynamicFieldResponse> {
try {
const result: ListIdResponse = await request(`${API_URL}/lists/`, {
Expand Down Expand Up @@ -99,6 +111,7 @@ export async function createProfile(
phone_number: string | undefined,
additionalAttributes: AdditionalAttributes
) {
validateExternalId(external_id)
try {
const profileData: ProfileData = {
data: {
Comment thread
AnkitSegment marked this conversation as resolved.
Expand Down Expand Up @@ -586,6 +599,11 @@ function validateAndConstructRemoveProfilePayloads(payload: RemoveProfilePayload
return response
}

if (external_id && external_id.length > MAX_EXTERNAL_ID_LENGTH) {
response.error = EXTERNAL_ID_LENGTH_ERROR
return response
}
Comment thread
AnkitSegment marked this conversation as resolved.

if (phone_number) {
const validPhoneNumber = validateAndConvertPhoneNumber(phone_number, payload.country_code as string)
if (!validPhoneNumber) {
Expand Down Expand Up @@ -616,6 +634,11 @@ function validateAndConstructProfilePayload(payload: AddProfileToListPayload): {
return response
}

if (external_id && external_id.length > MAX_EXTERNAL_ID_LENGTH) {
response.error = EXTERNAL_ID_LENGTH_ERROR
return response
}

if (phone_number) {
const validPhoneNumber = validateAndConvertPhoneNumber(phone_number, payload.country_code as string)
if (!validPhoneNumber) {
Expand Down Expand Up @@ -756,20 +779,22 @@ function validateAndPreparePayloads(payloads: TrackEventPayload[], multiStatusRe
return
}

if (external_id && external_id.length > MAX_EXTERNAL_ID_LENGTH) {
multiStatusResponse.setErrorResponseAtIndex(originalBatchIndex, EXTERNAL_ID_LENGTH_ERROR)
return
}

if (phone_number) {
// Validate and convert the phone number if present
const validPhoneNumber = validateAndConvertPhoneNumber(phone_number, country_code as string)
// If the phone number is not valid, skip this payload
if (!validPhoneNumber) {
multiStatusResponse.setErrorResponseAtIndex(originalBatchIndex, {
status: 400,
errortype: 'PAYLOAD_VALIDATION_FAILED',
errormessage: 'Phone number could not be converted to E.164 format.'
})
return // Skip this payload
return
}

// Update the payload's phone number with the validated format
payload.profile.phone_number = validPhoneNumber
delete payload?.profile?.country_code
}
Expand Down Expand Up @@ -920,6 +945,11 @@ export function validateProfilePayload(payload: Payload): validateProfilePayload
return response
}

if (payload.external_id && payload.external_id.length > MAX_EXTERNAL_ID_LENGTH) {
response.error = EXTERNAL_ID_LENGTH_ERROR
return response
}

if (payload.phone_number) {
const validPhoneNumber = validateAndConvertPhoneNumber(payload.phone_number, payload.country_code as string)
if (!validPhoneNumber) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,23 @@ describe('Order Completed', () => {
)
})

it('should throw error if external_id exceeds 255 characters', async () => {
const event = createTestEvent({
type: 'track',
timestamp: '2022-01-01T00:00:00.000Z'
})

const mapping = {
profile: { external_id: 'a'.repeat(256) },
metric_name: 'Order Completed',
properties: { key: 'value' }
}

await expect(testDestination.testAction('orderCompleted', { event, mapping, settings })).rejects.toThrowError(
'Length of external_id must be no more than 255 characters.'
)
})

it('should throw an error for invalid phone number format', async () => {
const profile = { email: 'test@example.com', phone_number: 'invalid-phone-number', country_code: 'US' }
const properties = { key: 'value' }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { PayloadValidationError, RequestClient } from '@segment/actions-core'
import { API_URL } from '../config'
import { EventData } from '../types'
import { v4 as uuidv4 } from '@lukeed/uuid'
import { processPhoneNumber } from '../functions'
import { processPhoneNumber, validateExternalId } from '../functions'
import { country_code } from '../properties'
import dayjs from 'dayjs'

Expand Down Expand Up @@ -176,6 +176,7 @@ const action: ActionDefinition<Settings, Payload> = {
if (!email && !phone_number && !external_id && !anonymous_id) {
throw new PayloadValidationError('One of External ID, Anonymous ID, Phone Number or Email is required.')
}
validateExternalId(external_id)

Comment thread
AnkitSegment marked this conversation as resolved.
const eventData = createEventData(payload)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ export const email: InputField = {

export const external_id: InputField = {
label: 'External ID',
description: `A unique identifier used by customers to associate Klaviyo profiles with profiles in an external system. One of External ID and Email required.`,
type: 'string'
description: `A unique identifier used by customers to associate Klaviyo profiles with profiles in an external system. One of External ID, Email or Phone Number is required. Must not exceed 255 characters.`,
type: 'string',
minimum: 0,
maximum: 255

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Bug: minimum/maximum are numeric JSON Schema constraints — wrong type for a string field

minimum and maximum apply to numbers. For strings, use minLength / maxLength.

The framework's fields-to-jsonschema.ts checks if (minimum) (truthiness). Since minimum: 0 is falsy, minLength is never set — it silently does nothing. The maximum: 255 applied to a string field causes AJV to throw an opaque AggregateAjvError instead of the intended PayloadValidationError. This is confirmed by the removeProfileFromList test which expects AggregateAjvError — while every other test in this PR expects the friendly message string.

Fix: Remove minimum and maximum entirely and rely on the runtime validateExternalId checks that are already in place:

export const external_id: InputField = {
  label: 'External ID',
  description: `A unique identifier used by customers to associate Klaviyo profiles with profiles in an external system. One of External ID and Email required. Must not exceed 255 characters.`,
  type: 'string'
  // 255-char limit enforced at runtime by validateExternalId()
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The framework's fields-to-jsonschema.ts actually handles this correctly for string types — it converts minimumminLength and maximummaxLength when field.type === 'string'. So maximum: 255 does enforce a 255-character limit at the schema level.

The minimum: 0 being falsy is harmless since minLength: 0 is meaningless anyway.

Keeping minimum/maximum in place since it provides schema-level validation before perform is ever called. The runtime validateExternalId serves as a safety net for batch paths.

Updated the description to mention Phone Number as a valid identifier alternative.

}

export const enable_batching: InputField = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@ describe('Remove Profile', () => {
)
})

it('should throw error if external_id exceeds 255 characters', async () => {
const event = createTestEvent({
type: 'track',
properties: {}
})

const mapping = {
list_id: listId,
external_id: 'a'.repeat(256)
}

await expect(testDestination.testAction('removeProfile', { event, mapping, settings })).rejects.toThrowError(
'Length of external_id must be no more than 255 characters.'
)
})

it('should throw an error for invalid phone number format', async () => {
const event = createTestEvent({
type: 'track',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getListIdDynamicData,
getProfiles,
processPhoneNumber,
validateExternalId,
removeBulkProfilesFromList,
removeProfileFromList
} from '../functions'
Expand Down Expand Up @@ -67,6 +68,7 @@ const action: ActionDefinition<Settings, Payload> = {
if (!email && !external_id && !phone_number) {
throw new PayloadValidationError('One of External ID, Phone Number and Email is required.')
}
validateExternalId(external_id)
const profileIds = await getProfiles(
request,
email ? [email] : undefined,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import nock from 'nock'
import { createTestEvent, createTestIntegration } from '@segment/actions-core'
import { AggregateAjvError } from '@segment/ajv-human-errors'
import Definition from '../../index'
import { API_URL } from '../../config'
import { AggregateAjvError } from '@segment/ajv-human-errors'

const testDestination = createTestIntegration(Definition)

Expand All @@ -25,6 +25,22 @@ describe('Remove List from Profile', () => {
)
})

it('should throw error if external_id exceeds 255 characters', async () => {
const event = createTestEvent({
type: 'track',
properties: {}
})

const mapping = {
list_id: listId,
external_id: 'a'.repeat(256)
}

await expect(
testDestination.testAction('removeProfileFromList', { event, mapping, settings })
).rejects.toThrowError(AggregateAjvError)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Inconsistency: this test expects AggregateAjvError but all other equivalent tests in this PR expect a specific message

Every other new external_id test added in this PR (removeProfile, trackEvent, orderCompleted, upsertProfile) expects:

.rejects.toThrowError('Length of external_id must be no more than 255 characters.')

This test expects AggregateAjvError — which is the AJV schema-level validator firing before the custom validateExternalId runtime check is ever reached. This is a symptom of the bug in properties.ts where maximum: 255 is applied to a type: 'string' field using the wrong constraint type.

The user sees an opaque schema error rather than the intended descriptive message.

Fix: After removing minimum/maximum from properties.ts, change this to match the other tests:

await expect(
  testDestination.testAction('removeProfileFromList', { event, mapping, settings })
).rejects.toThrowError('Length of external_id must be no more than 255 characters.')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AggregateAjvError expectation here is intentional and correct. The framework's fields-to-jsonschema.ts converts maximum: 255 on string-type fields to maxLength: 255 in the JSON Schema. This means schema validation catches the >255 char external_id before the perform method is called, resulting in AggregateAjvError.

This is consistent with how removeProfileFromList uses the shared external_id field from properties.ts with maximum: 255 — schema validation fires first. The other tests (trackEvent, orderCompleted, etc.) use validateExternalId in their perform path because their field definitions don't all carry the schema constraint directly.

Both approaches reject invalid input — schema-level is just earlier in the pipeline.

})

it('should throw an error for invalid phone number format', async () => {
const event = createTestEvent({
type: 'track',
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import { ActionDefinition, PayloadValidationError } from '@segment/actions-core'
import type { Settings } from '../generated-types'
import { Payload } from './generated-types'

import { getProfiles, processPhoneNumber, removeBulkProfilesFromList, removeProfileFromList } from '../functions'
import {
getProfiles,
processPhoneNumber,
validateExternalId,
removeBulkProfilesFromList,
removeProfileFromList
} from '../functions'
import { email, list_id, external_id, enable_batching, phone_number, country_code, batch_size } from '../properties'

const action: ActionDefinition<Settings, Payload> = {
Expand Down Expand Up @@ -33,6 +39,7 @@ const action: ActionDefinition<Settings, Payload> = {
if (!email && !external_id && !phone_number) {
throw new PayloadValidationError('One of External ID, Phone Number and Email is required.')
}
validateExternalId(external_id)
const profileIds = await getProfiles(
Comment thread
AnkitSegment marked this conversation as resolved.
request,
email ? [email] : undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ describe('Track Event', () => {
)
})

it('should throw error if external_id exceeds 255 characters', async () => {
const event = createTestEvent({
type: 'track',
timestamp: '2022-01-01T00:00:00.000Z'
})

const mapping = {
profile: { external_id: 'a'.repeat(256) },
metric_name: 'Test Event',
properties: {}
}

await expect(testDestination.testAction('trackEvent', { event, mapping, settings })).rejects.toThrowError(
'Length of external_id must be no more than 255 characters.'
)
})

it('should throw an error for invalid phone number format', async () => {
const profile = { email: 'test@example.com', phone_number: 'invalid-phone-number', country_code: 'US' }
const properties = { key: 'value' }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { Payload } from './generated-types'
import { PayloadValidationError } from '@segment/actions-core'
import { API_URL } from '../config'
import { batch_size, enable_batching, country_code } from '../properties'
import { processPhoneNumber, sendBatchedTrackEvent } from '../functions'
import { processPhoneNumber, sendBatchedTrackEvent, validateExternalId } from '../functions'
import dayjs from '../../../lib/dayjs'

const action: ActionDefinition<Settings, Payload> = {
Expand Down Expand Up @@ -106,6 +106,7 @@ const action: ActionDefinition<Settings, Payload> = {
if (!email && !phone_number && !external_id && !anonymous_id) {
throw new PayloadValidationError('One of External ID, Anonymous ID, Phone Number or Email is required.')
}
validateExternalId(external_id)
const eventData = {
Comment thread
AnkitSegment marked this conversation as resolved.
data: {
type: 'event',
Expand Down
Loading
Loading