Skip to content

return original zod schema after app module validation #6889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ryancbahan wants to merge 1 commit into from
Closed

return original zod schema after app module validation #6889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
121 changes: 100 additions & 21 deletions packages/app/src/cli/utilities/json-schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,15 @@ describe('unifiedConfigurationParserFactory', () => {
}

test('falls back to zod parser when no JSON schema is provided', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
validationSchema: undefined,
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

// Then
expect(result).toEqual({
state: 'ok',
data: {type: 'product_subscription'},
Expand All @@ -39,7 +36,6 @@ describe('unifiedConfigurationParserFactory', () => {
})

test('falls back to zod parser when JSON schema is empty', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -48,11 +44,9 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

// Then
expect(result).toEqual({
state: 'ok',
data: {type: 'product_subscription'},
Expand All @@ -61,7 +55,6 @@ describe('unifiedConfigurationParserFactory', () => {
})

test('validates with both zod and JSON schema when both succeed', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -70,11 +63,9 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

// Then
expect(result).toEqual({
state: 'ok',
data: {type: 'product_subscription'},
Expand All @@ -83,7 +74,6 @@ describe('unifiedConfigurationParserFactory', () => {
})

test('returns errors when zod validation fails', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -92,19 +82,16 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'invalid'})

// Then
expect(result.state).toBe('error')
expect(result.data).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors?.[0]).toEqual({path: ['type'], message: 'Invalid type'})
})

test('returns errors when JSON schema validation fails', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -113,11 +100,9 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
Copy link
Copy Markdown
Contributor Author

@ryancbahan ryancbahan Feb 23, 2026

Choose a reason for hiding this comment

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

deleting some unneeded llm output as i pass by

Copy link
Copy Markdown
Contributor

@isaacroldan isaacroldan Feb 24, 2026

Choose a reason for hiding this comment

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

I don't think this is llm 😅, is something we used to do as a team to separate the different parts of a test. And llm's have picked it up when adding new tests

Copy link
Copy Markdown
Contributor

@gonzaloriestra gonzaloriestra Feb 24, 2026

Choose a reason for hiding this comment

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

Yeah, it's actually part our testing conventions: https://shopify.github.io/cli/cli/testing-strategy.html

const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

// Then
expect(result.state).toBe('error')
expect(result.data).toBeUndefined()
expect(result.errors).toBeDefined()
Expand All @@ -126,7 +111,6 @@ describe('unifiedConfigurationParserFactory', () => {
})

test('combines errors from both validations', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -135,11 +119,9 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'invalid'})

// Then
expect(result.state).toBe('error')
expect(result.data).toBeUndefined()
expect(result.errors).toBeDefined()
Expand All @@ -152,8 +134,107 @@ describe('unifiedConfigurationParserFactory', () => {
expect(priceError).toBeDefined()
})

test('transforms data to server format before contract validation', async () => {
// Given: Zod outputs TOML-shaped data, but the contract expects server-shaped data
const merged = {
identifier: randomUUID(),
parseConfigurationObject: (config: any) => ({
state: 'ok' as const,
data: {access_scopes: {scopes: config.access_scopes?.scopes}},
errors: undefined,
}),
transformLocalToRemote: (local: any) => ({
scopes: local.access_scopes?.scopes,
}),
validationSchema: {
jsonSchema: JSON.stringify({
type: 'object',
properties: {scopes: {type: 'string'}},
required: ['scopes'],
}),
},
}

const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({access_scopes: {scopes: 'read_products'}})

expect(result.state).toBe('ok')
})

test('returns Zod-validated data, not contract-stripped data', async () => {
// Given: Zod outputs TOML-shaped data, transform converts to server-shaped for validation
const merged = {
identifier: randomUUID(),
parseConfigurationObject: (config: any) => ({
state: 'ok' as const,
data: {access_scopes: {scopes: config.access_scopes?.scopes}},
errors: undefined,
}),
transformLocalToRemote: (local: any) => ({
scopes: local.access_scopes?.scopes,
}),
validationSchema: {
jsonSchema: JSON.stringify({
type: 'object',
properties: {scopes: {type: 'string'}},
}),
},
}

const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({access_scopes: {scopes: 'read_products'}})

expect(result.state).toBe('ok')
expect(result.data).toEqual({access_scopes: {scopes: 'read_products'}})
})

test('falls back to raw data when no transform is available', async () => {
// Given: no transformLocalToRemote, contract matches raw shape
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
validationSchema: {
jsonSchema: JSON.stringify({
type: 'object',
properties: {type: {type: 'string'}},
}),
},
}

const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

expect(result.state).toBe('ok')
expect(result.data).toEqual({type: 'product_subscription'})
})

test('contract cannot strip fields from returned data', async () => {
// Given: Zod output has extra_field, contract doesn't define it, strip mode is on
const merged = {
identifier: randomUUID(),
parseConfigurationObject: (config: any) => ({
state: 'ok' as const,
data: {...config, extra_field: 'preserved'},
errors: undefined,
}),
validationSchema: {
jsonSchema: JSON.stringify({
type: 'object',
properties: {type: {type: 'string'}},
}),
},
}

// When: strip mode (default)
const parser = await unifiedConfigurationParserFactory(merged as any)
const result = parser({type: 'product_subscription'})

// Then: extra_field survives because we return Zod data, not contract-processed data
expect(result.state).toBe('ok')
expect(result.data).toEqual({type: 'product_subscription', extra_field: 'preserved'})
})

test('adds base properties to the JSON schema', async () => {
// Given
const merged = {
identifier: randomUUID(),
parseConfigurationObject: mockParseConfigurationObject,
Expand All @@ -162,10 +243,8 @@ describe('unifiedConfigurationParserFactory', () => {
},
}

// When
const parser = await unifiedConfigurationParserFactory(merged as any)

// Then - base properties should be accepted
const result = parser({
type: 'product_subscription',
handle: 'test-handle',
Expand Down
12 changes: 10 additions & 2 deletions packages/app/src/cli/utilities/json-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {FlattenedRemoteSpecification} from '../api/graphql/extension_specifications.js'
import {AppConfigurationWithoutPath} from '../models/app/app.js'
import {BaseConfigType} from '../models/extensions/schemas.js'
import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js'
import {ParseConfigurationResult} from '@shopify/cli-kit/node/schema'
Expand Down Expand Up @@ -49,7 +50,14 @@ export async function unifiedConfigurationParserFactory(

// Then, even if this failed, we try to validate against the contract.
const zodValidatedData = zodParse.state === 'ok' ? zodParse.data : undefined
const subjectForAjv = zodValidatedData ?? (config as JsonMapType)
let subjectForAjv: object
// If the zod validation succeeded and a transform function is provided, use the transform function to convert the data to the format expected by the contract.
// Otherwise, use the zod validated data as is.
if (zodValidatedData && merged.transformLocalToRemote) {
subjectForAjv = merged.transformLocalToRemote(zodValidatedData, {} as AppConfigurationWithoutPath)
} else {
subjectForAjv = zodValidatedData ?? (config as JsonMapType)
}

const jsonSchemaParse = jsonSchemaValidate(
subjectForAjv,
Expand Down Expand Up @@ -82,7 +90,7 @@ export async function unifiedConfigurationParserFactory(

return {
state: 'ok',
data: jsonSchemaParse.data as BaseConfigType,
data: zodValidatedData as BaseConfigType,
errors: undefined,
}
}
Expand Down
Loading