From 3c313f88789a5e7c76bff7a748fadfc153b6cd50 Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Fri, 20 Mar 2026 14:10:53 -0400 Subject: [PATCH] Preserve structured app validation issues internally --- .../src/cli/models/app/error-parsing.test.ts | 149 +++++++++++++- .../app/src/cli/models/app/error-parsing.ts | 84 +++++++- .../app/src/cli/models/app/loader.test.ts | 190 ++++++++++++++++-- packages/app/src/cli/models/app/loader.ts | 61 +++++- 4 files changed, 457 insertions(+), 27 deletions(-) diff --git a/packages/app/src/cli/models/app/error-parsing.test.ts b/packages/app/src/cli/models/app/error-parsing.test.ts index 85986f50887..3023346bdc2 100644 --- a/packages/app/src/cli/models/app/error-parsing.test.ts +++ b/packages/app/src/cli/models/app/error-parsing.test.ts @@ -1,4 +1,4 @@ -import {parseHumanReadableError} from './error-parsing.js' +import {parseHumanReadableError, parseStructuredErrors} from './error-parsing.js' import {describe, expect, test} from 'vitest' describe('parseHumanReadableError', () => { @@ -235,3 +235,150 @@ describe('parseHumanReadableError', () => { expect(result).not.toContain('Union validation failed') }) }) + +describe('parseStructuredErrors', () => { + test('preserves regular issues with file path and path string', () => { + const issues = [ + { + path: ['name'], + message: 'Required field is missing', + code: 'invalid_type', + }, + { + path: ['version'], + message: 'Must be a valid semver string', + code: 'custom', + }, + ] + + const result = parseStructuredErrors(issues, '/tmp/shopify.app.toml') + + expect(result).toEqual([ + { + filePath: '/tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'Required field is missing', + code: 'invalid_type', + }, + { + filePath: '/tmp/shopify.app.toml', + path: ['version'], + pathString: 'version', + message: 'Must be a valid semver string', + code: 'custom', + }, + ]) + }) + + test('uses the best matching union variant for structured issues', () => { + const issues = [ + { + code: 'invalid_union', + unionErrors: [ + { + issues: [ + { + code: 'invalid_type', + path: ['web', 'roles'], + message: 'Expected array, received number', + }, + { + code: 'invalid_type', + path: ['web', 'commands', 'build'], + message: 'Required', + }, + ], + name: 'ZodError', + }, + { + issues: [ + { + code: 'invalid_literal', + path: ['web', 'type'], + message: "Invalid literal value, expected 'frontend'", + }, + ], + name: 'ZodError', + }, + ], + path: ['web'], + message: 'Invalid input', + }, + ] + + const result = parseStructuredErrors(issues, '/tmp/shopify.web.toml') + + expect(result).toEqual([ + { + filePath: '/tmp/shopify.web.toml', + path: ['web', 'roles'], + pathString: 'web.roles', + message: 'Expected array, received number', + code: 'invalid_type', + }, + { + filePath: '/tmp/shopify.web.toml', + path: ['web', 'commands', 'build'], + pathString: 'web.commands.build', + message: 'Required', + code: 'invalid_type', + }, + ]) + }) + + test('falls back to recovered nested union issues before returning a synthetic root issue', () => { + const unionIssues = Array.from({length: 51}, (_, index) => ({ + code: 'custom', + path: ['variants', index], + message: `Invalid variant ${index + 1}`, + })) + + const issues = [ + { + code: 'invalid_union', + unionErrors: [{issues: unionIssues, name: 'ZodError'}], + path: ['root'], + message: 'Invalid input', + }, + ] + + const result = parseStructuredErrors(issues, '/tmp/shopify.app.toml') + + expect(result).toEqual( + unionIssues.map((issue, index) => ({ + filePath: '/tmp/shopify.app.toml', + path: ['variants', index], + pathString: `variants.${index}`, + message: issue.message, + code: 'custom', + })), + ) + }) + + test('falls back to a synthetic root issue when union variants expose no nested issues', () => { + const issues = [ + { + code: 'invalid_union', + unionErrors: [ + {issues: [], name: 'ZodError'}, + {issues: [], name: 'ZodError'}, + ], + path: ['root'], + message: 'Invalid input', + }, + ] + + const result = parseStructuredErrors(issues, '/tmp/shopify.app.toml') + + expect(result).toEqual([ + { + filePath: '/tmp/shopify.app.toml', + path: ['root'], + pathString: 'root', + message: "Configuration doesn't match any expected format", + code: 'invalid_union', + }, + ]) + }) +}) diff --git a/packages/app/src/cli/models/app/error-parsing.ts b/packages/app/src/cli/models/app/error-parsing.ts index 96b3c81e283..a5a1b11a58d 100644 --- a/packages/app/src/cli/models/app/error-parsing.ts +++ b/packages/app/src/cli/models/app/error-parsing.ts @@ -1,5 +1,13 @@ +import type {OutputMessage} from '@shopify/cli-kit/node/output' + +interface UnionErrorIssue { + path?: (string | number)[] + message: string + code?: string +} + interface UnionError { - issues?: {path?: (string | number)[]; message: string}[] + issues?: UnionErrorIssue[] name: string } @@ -10,6 +18,20 @@ interface ExtendedZodIssue { unionErrors?: UnionError[] } +export interface AppValidationIssue { + filePath: string + path: (string | number)[] + pathString: string + message: string + code?: string +} + +export interface AppValidationFileIssues { + filePath: string + message: OutputMessage + issues: AppValidationIssue[] +} + /** * Finds the best matching variant from a union error by scoring each variant * based on how close it is to the user's likely intent. @@ -56,13 +78,69 @@ function findBestMatchingVariant(unionErrors: UnionError[]): UnionError | null { return bestVariant } +function getIssuePath(path?: (string | number)[]) { + return path ?? [] +} + +function getIssuePathString(path?: (string | number)[]) { + const resolvedPath = getIssuePath(path) + return resolvedPath.length > 0 ? resolvedPath.map(String).join('.') : 'root' +} + /** * Formats an issue into a human-readable error line */ function formatErrorLine(issue: {path?: (string | number)[]; message?: string}, indent = '') { - const path = issue.path && issue.path.length > 0 ? issue.path.map(String).join('.') : 'root' + const pathString = getIssuePathString(issue.path) const message = issue.message ?? 'Unknown error' - return `${indent}• [${path}]: ${message}\n` + return `${indent}• [${pathString}]: ${message}\n` +} + +function toStructuredIssue( + filePath: string, + issue: Pick, +) { + return { + filePath, + path: getIssuePath(issue.path), + pathString: getIssuePathString(issue.path), + message: issue.message ?? 'Unknown error', + code: issue.code, + } +} + +export function parseStructuredErrors(issues: ExtendedZodIssue[], filePath: string): AppValidationIssue[] { + return issues.flatMap((issue) => { + if (issue.code === 'invalid_union' && issue.unionErrors) { + // Intentionally mirror the current human-readable union selection heuristic + // so structured/internal issues stay aligned with existing CLI behavior. + // If we change this heuristic later, text and structured output should move together. + const bestVariant = findBestMatchingVariant(issue.unionErrors) + + if (bestVariant?.issues?.length) { + return bestVariant.issues.map((nestedIssue) => toStructuredIssue(filePath, nestedIssue)) + } + + const fallbackIssues = issue.unionErrors.flatMap((unionError) => unionError.issues ?? []) + if (fallbackIssues.length > 0) { + // Preserve any concrete nested issues we were able to recover before + // falling back to a synthetic root issue. This structured path is still + // internal, and retaining leaf issues is more actionable than erasing + // them behind a generic union failure. + return fallbackIssues.map((nestedIssue) => toStructuredIssue(filePath, nestedIssue)) + } + + return [ + toStructuredIssue(filePath, { + path: issue.path, + message: "Configuration doesn't match any expected format", + code: issue.code, + }), + ] + } + + return [toStructuredIssue(filePath, issue)] + }) } export function parseHumanReadableError(issues: ExtendedZodIssue[]) { diff --git a/packages/app/src/cli/models/app/loader.test.ts b/packages/app/src/cli/models/app/loader.test.ts index 7b44163ed3f..ffe6d839674 100644 --- a/packages/app/src/cli/models/app/loader.test.ts +++ b/packages/app/src/cli/models/app/loader.test.ts @@ -6,6 +6,7 @@ import { loadDotEnv, parseConfigurationObject, checkFolderIsValidApp, + AppErrors, AppLoaderMode, getAppConfigurationState, loadConfigForAppCreation, @@ -2910,7 +2911,23 @@ describe('parseConfigurationObject', () => { const {schema} = await buildVersionedAppSchema() await parseConfigurationObject(schema, 'tmp', configurationObject, abortOrReport) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) + expect(abortOrReport.mock.calls[0]![3]).toEqual([ + { + filePath: 'tmp', + path: ['auth'], + pathString: 'auth', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: 'tmp', + path: ['embedded'], + pathString: 'embedded', + message: 'Boolean is required', + code: 'invalid_type', + }, + ]) }) test('throws an error when client_id is missing in app schema TOML file', async () => { @@ -2924,6 +2941,15 @@ describe('parseConfigurationObject', () => { expect(abortOrReport).toHaveBeenCalledOnce() const errorString = abortOrReport.mock.calls[0]![0].value expect(errorString).toContain('[client_id]: Required') + expect(abortOrReport.mock.calls[0]![3]).toEqual([ + { + filePath: 'tmp', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) }) test('throws an error if fields are missing in a frontend config web TOML file', async () => { @@ -2952,6 +2978,142 @@ describe('parseConfigurationObject', () => { expect(errorString).not.toContain('Union validation failed') expect(errorString).not.toContain('Option 1:') expect(errorString).not.toContain('Option 2:') + expect(callArgs[3]).toEqual([ + { + filePath: 'tmp', + path: ['roles'], + pathString: 'roles', + message: 'Expected array, received number', + code: 'invalid_type', + }, + ]) + }) +}) + +describe('AppErrors', () => { + test('preserves structured issues while keeping existing rendered message accessors', () => { + const errors = new AppErrors() + errors.addError('tmp/shopify.app.toml', 'rendered error', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('tmp/shopify.app.toml', 'replacement rendered error') + + expect(errors.getError('tmp/shopify.app.toml')).toBe('replacement rendered error') + expect(errors.getIssues('tmp/shopify.app.toml')).toEqual([ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + expect(errors.toJSON()).toEqual(['replacement rendered error']) + expect(errors.toStructuredJSON()).toEqual([ + { + filePath: 'tmp/shopify.app.toml', + message: 'replacement rendered error', + issues: [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + }, + ]) + }) + + test('accumulates issues across multiple addError calls for the same path', () => { + const errors = new AppErrors() + + errors.addError('tmp/shopify.app.toml', 'first rendered error', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('tmp/shopify.app.toml', 'second rendered error', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ]) + + expect(errors.getError('tmp/shopify.app.toml')).toBe('second rendered error') + expect(errors.getIssues('tmp/shopify.app.toml')).toEqual([ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: 'tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ]) + }) + + test('keeps issues isolated per file path and returns an empty array for unknown paths', () => { + const errors = new AppErrors() + + errors.addError('tmp/shopify.app.toml', 'app error', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('tmp/extensions/foo/shopify.extension.toml', 'extension error', [ + { + filePath: 'tmp/extensions/foo/shopify.extension.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ]) + + expect(errors.getIssues('tmp/shopify.app.toml')).toEqual([ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + expect(errors.getIssues('tmp/extensions/foo/shopify.extension.toml')).toEqual([ + { + filePath: 'tmp/extensions/foo/shopify.extension.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ]) + expect(errors.getIssues('tmp/missing.toml')).toEqual([]) }) }) @@ -2974,7 +3136,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('removes trailing slashes on uri', async () => { @@ -3002,7 +3164,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('accepts an https uri', async () => { @@ -3111,7 +3273,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('throws an error if we have duplicate subscriptions in different topics array', async () => { @@ -3131,7 +3293,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('removes trailing forward slash', async () => { @@ -3169,7 +3331,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('accepts a pub sub config with both project and topic', async () => { @@ -3211,7 +3373,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('throws an error if we have duplicate pub sub subscriptions', async () => { @@ -3237,7 +3399,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('throws an error if we have duplicate arn subscriptions', async () => { @@ -3265,7 +3427,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('does not allow identical topic and uri and filter in different subscriptions', async () => { @@ -3293,7 +3455,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('shows multiple duplicate subscriptions in error message', async () => { @@ -3331,7 +3493,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('allows identical topic and uri if filter is different', async () => { @@ -3376,7 +3538,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('throws an error if neither topics nor compliance_topics are added', async () => { @@ -3395,7 +3557,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('throws an error when there are duplicated compliance topics', async () => { @@ -3420,7 +3582,7 @@ describe('WebhooksSchema', () => { } const {abortOrReport, expectedFormatted} = await setupParsing(errorObj, webhookConfig) - expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp') + expect(abortOrReport).toHaveBeenCalledWith(expectedFormatted, {}, 'tmp', expect.any(Array)) }) test('accepts webhook subscription with payload_query', async () => { diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 2521101367c..fca06a82a5a 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -15,7 +15,12 @@ import { AppLinkedInterface, AppHiddenConfig, } from './app.js' -import {parseHumanReadableError} from './error-parsing.js' +import { + AppValidationFileIssues, + AppValidationIssue, + parseHumanReadableError, + parseStructuredErrors, +} from './error-parsing.js' import {configurationFileNames, dotEnvFileNames} from '../../constants.js' import metadata from '../../metadata.js' import {ExtensionInstance} from '../extensions/extension-instance.js' @@ -67,7 +72,12 @@ const defaultExtensionDirectory = 'extensions/*' */ export type AppLoaderMode = 'strict' | 'report' | 'local' -type AbortOrReport = (errorMessage: OutputMessage, fallback: T, configurationPath: string) => T +type AbortOrReport = ( + errorMessage: OutputMessage, + fallback: T, + configurationPath: string, + issues?: AppValidationIssue[], +) => T const abort: AbortOrReport = (errorMessage) => { throw new AbortError(errorMessage) @@ -136,6 +146,7 @@ export function parseConfigurationObject( )}:\n\n${parseHumanReadableError(parseResult.error.issues)}`, fallbackOutput, filepath, + parseStructuredErrors(parseResult.error.issues, filepath), ) } return parseResult.data @@ -163,6 +174,7 @@ export function parseConfigurationObjectAgainstSpecification 0 ? [...existingIssues, ...issues] : existingIssues} } getError(path: string) { - return this.errors[path] + return this.errors[path]?.message + } + + getIssues(path: string): AppValidationIssue[] { + return this.errors[path]?.issues ?? [] } isEmpty() { @@ -186,7 +206,11 @@ export class AppErrors { } toJSON(): OutputMessage[] { - return Object.values(this.errors) + return Object.values(this.errors).map(({message}) => message) + } + + toStructuredJSON(): AppValidationFileIssues[] { + return Object.entries(this.errors).map(([filePath, {message, issues}]) => ({filePath, message, issues})) } } @@ -811,13 +835,32 @@ class AppLoader(errorMessage: OutputMessage, fallback: T, configurationPath: string): T { + private abortOrReport( + errorMessage: OutputMessage, + fallback: undefined, + configurationPath: string, + issues?: AppValidationIssue[], + ): undefined + + private abortOrReport( + errorMessage: OutputMessage, + fallback: T, + configurationPath: string, + issues?: AppValidationIssue[], + ): T + + private abortOrReport( + errorMessage: OutputMessage, + fallback: T, + configurationPath: string, + issues: AppValidationIssue[] = [], + ): T { switch (this.mode) { case 'strict': case 'local': throw new AbortError(errorMessage) case 'report': - this.errors.addError(configurationPath, errorMessage) + this.errors.addError(configurationPath, errorMessage, issues) return fallback } }