diff --git a/packages/app/src/cli/commands/app/config/validate.test.ts b/packages/app/src/cli/commands/app/config/validate.test.ts index 5ae0a8203e..16fc36676e 100644 --- a/packages/app/src/cli/commands/app/config/validate.test.ts +++ b/packages/app/src/cli/commands/app/config/validate.test.ts @@ -2,10 +2,20 @@ import Validate from './validate.js' import {linkedAppContext} from '../../../services/app-context.js' import {validateApp} from '../../../services/validate.js' import {testAppLinked} from '../../../models/app/app.test-data.js' +import {LocalConfigError} from '../../../models/app/local-config-error.js' import {describe, expect, test, vi} from 'vitest' +import {AbortError} from '@shopify/cli-kit/node/error' +import {outputResult} from '@shopify/cli-kit/node/output' vi.mock('../../../services/app-context.js') vi.mock('../../../services/validate.js') +vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + outputResult: vi.fn(), + } +}) describe('app config validate command', () => { test('calls validateApp with json: false by default', async () => { @@ -46,4 +56,151 @@ describe('app config validate command', () => { // Then expect(validateApp).toHaveBeenCalledWith(app, {json: true}) }) + + test('rethrows LocalConfigError in non-json mode without emitting json', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new LocalConfigError('Validation errors in /tmp/shopify.app.toml', '/tmp/shopify.app.toml'), + ) + + // When / Then + await expect(Validate.run(['--path=/tmp/app'], import.meta.url)).rejects.toThrow() + expect(outputResult).not.toHaveBeenCalled() + expect(validateApp).not.toHaveBeenCalled() + }) + + test('outputs structured configuration issues from app loading before validateApp runs', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new LocalConfigError( + 'Validation errors in /tmp/shopify.app.toml:\n\n• [name]: String is required', + '/tmp/shopify.app.toml', + [ + { + filePath: '/tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'String is required', + }, + ], + ), + ) + + // When / Then + await expect(Validate.run(['--json', '--path=/tmp/app'], import.meta.url)).rejects.toThrow( + 'process.exit unexpectedly called with "1"', + ) + expect(outputResult).toHaveBeenCalledTimes(1) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'String is required', + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('outputs a root json issue when app loading fails without structured issues', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new LocalConfigError("Couldn't find an app toml file at /tmp/app", '/tmp/app'), + ) + + // When / Then + await expect(Validate.run(['--json', '--path=/tmp/app'], import.meta.url)).rejects.toThrow( + 'process.exit unexpectedly called with "1"', + ) + expect(outputResult).toHaveBeenCalledTimes(1) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/app', + path: [], + pathString: 'root', + message: "Couldn't find an app toml file at /tmp/app", + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('outputs json when validateApp throws a structured configuration abort', async () => { + // Given + const app = testAppLinked() + vi.mocked(linkedAppContext).mockResolvedValue({app} as Awaited>) + vi.mocked(validateApp).mockRejectedValue( + new LocalConfigError( + 'Validation errors in /tmp/shopify.app.toml:\n\n• [name]: String is required', + '/tmp/shopify.app.toml', + [ + { + filePath: '/tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'String is required', + }, + ], + ), + ) + + // When / Then + await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow('process.exit unexpectedly called with "1"') + expect(outputResult).toHaveBeenCalledTimes(1) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'String is required', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('rethrows non-configuration errors from validateApp in json mode without converting them to validation json', async () => { + // Given + const app = testAppLinked() + vi.mocked(linkedAppContext).mockResolvedValue({app} as Awaited>) + vi.mocked(validateApp).mockRejectedValue(new AbortError('network problem')) + + // When / Then + await expect(Validate.run(['--json'], import.meta.url)).rejects.toThrow() + expect(outputResult).not.toHaveBeenCalled() + }) + + test('rethrows unrelated abort errors in json mode without converting them to validation json', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue(new AbortError('Could not find store for domain shop.example.com')) + + // When / Then + await expect(Validate.run(['--json', '--path=/tmp/app'], import.meta.url)).rejects.toThrow() + expect(outputResult).not.toHaveBeenCalled() + expect(validateApp).not.toHaveBeenCalled() + }) }) diff --git a/packages/app/src/cli/commands/app/config/validate.ts b/packages/app/src/cli/commands/app/config/validate.ts index c597c5228e..4ad584bff4 100644 --- a/packages/app/src/cli/commands/app/config/validate.ts +++ b/packages/app/src/cli/commands/app/config/validate.ts @@ -2,7 +2,12 @@ import {appFlags} from '../../../flags.js' import {validateApp} from '../../../services/validate.js' import AppLinkedCommand, {AppLinkedCommandOutput} from '../../../utilities/app-linked-command.js' import {linkedAppContext} from '../../../services/app-context.js' +import {toRootValidationIssue} from '../../../models/app/error-parsing.js' +import {LocalConfigError} from '../../../models/app/local-config-error.js' +import {invalidAppValidationResult, stringifyAppValidationResult} from '../../../services/validation-result.js' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' +import {AbortSilentError} from '@shopify/cli-kit/node/error' +import {outputResult, stringifyMessage} from '@shopify/cli-kit/node/output' export default class Validate extends AppLinkedCommand { static summary = 'Validate your app configuration and extensions.' @@ -20,16 +25,29 @@ export default class Validate extends AppLinkedCommand { public async run(): Promise { const {flags} = await this.parse(Validate) - const {app} = await linkedAppContext({ - directory: flags.path, - clientId: flags['client-id'], - forceRelink: flags.reset, - userProvidedConfigName: flags.config, - unsafeTolerateErrors: true, - }) - - await validateApp(app, {json: flags.json}) - - return {app} + try { + const {app} = await linkedAppContext({ + directory: flags.path, + clientId: flags['client-id'], + forceRelink: flags.reset, + userProvidedConfigName: flags.config, + unsafeTolerateErrors: true, + }) + + await validateApp(app, {json: flags.json}) + + return {app} + } catch (error) { + if (flags.json && error instanceof LocalConfigError) { + const issues = + error.issues.length > 0 + ? error.issues + : [toRootValidationIssue(error.configurationPath, stringifyMessage(error.message).trim())] + outputResult(stringifyAppValidationResult(invalidAppValidationResult(issues))) + throw new AbortSilentError() + } + + throw error + } } } 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 3023346bdc..c1d42012a2 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, parseStructuredErrors} from './error-parsing.js' +import {parseHumanReadableError, parseStructuredErrors, toRootValidationIssue} from './error-parsing.js' import {describe, expect, test} from 'vitest' describe('parseHumanReadableError', () => { @@ -236,6 +236,17 @@ describe('parseHumanReadableError', () => { }) }) +describe('toRootValidationIssue', () => { + test('creates a root issue for a file path and message', () => { + expect(toRootValidationIssue('/tmp/app', "Couldn't find an app toml file at /tmp/app")).toEqual({ + filePath: '/tmp/app', + path: [], + pathString: 'root', + message: "Couldn't find an app toml file at /tmp/app", + }) + }) +}) + describe('parseStructuredErrors', () => { test('preserves regular issues with file path and path string', () => { const issues = [ diff --git a/packages/app/src/cli/models/app/error-parsing.ts b/packages/app/src/cli/models/app/error-parsing.ts index 41132346b8..421d6ad7ec 100644 --- a/packages/app/src/cli/models/app/error-parsing.ts +++ b/packages/app/src/cli/models/app/error-parsing.ts @@ -32,6 +32,15 @@ export interface AppValidationFileIssues { issues: AppValidationIssue[] } +export function toRootValidationIssue(filePath: string, message: string): AppValidationIssue { + return { + filePath, + path: [], + pathString: 'root', + message, + } +} + /** * 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. diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 0788275d7c..c0b2ce3521 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -970,7 +970,10 @@ async function getConfigurationPath(appDirectory: string, configName: string | u if (await fileExists(configurationPath)) { return {configurationPath, configurationFileName} } else { - throw new AbortError(outputContent`Couldn't find ${configurationFileName} in ${outputToken.path(appDirectory)}.`) + throw new LocalConfigError( + outputContent`Couldn't find ${configurationFileName} in ${outputToken.path(appDirectory)}.`, + configurationPath, + ) } } diff --git a/packages/app/src/cli/services/validate.test.ts b/packages/app/src/cli/services/validate.test.ts index c475c4399d..7ba5d2b2ac 100644 --- a/packages/app/src/cli/services/validate.test.ts +++ b/packages/app/src/cli/services/validate.test.ts @@ -37,7 +37,7 @@ describe('validateApp', () => { await validateApp(app, {json: true}) // Then - expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: true, errors: []}, null, 2)) + expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: true, issues: []}, null, 2)) expect(renderSuccess).not.toHaveBeenCalled() expect(renderError).not.toHaveBeenCalled() }) @@ -74,7 +74,20 @@ describe('validateApp', () => { JSON.stringify( { valid: false, - errors: ['client_id is required', 'invalid type "unknown"'], + issues: [ + { + filePath: '/path/to/shopify.app.toml', + path: [], + pathString: 'root', + message: 'client_id is required', + }, + { + filePath: '/path/to/extensions/my-ext/shopify.extension.toml', + path: [], + pathString: 'root', + message: 'invalid type "unknown"', + }, + ], }, null, 2, @@ -84,6 +97,132 @@ describe('validateApp', () => { expect(renderSuccess).not.toHaveBeenCalled() }) + test('outputs only structured issues when the rendered message matches them exactly', async () => { + // Given + const errors = new AppErrors() + errors.addError('/path/to/shopify.app.toml', '• [client_id]: Required', [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + const app = testAppLinked() + app.errors = errors + + // When / Then + await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('outputs only structured issues when the rendered message is a validation wrapper around them', async () => { + // Given + const errors = new AppErrors() + errors.addError( + '/path/to/shopify.app.toml', + 'Validation errors in /path/to/shopify.app.toml:\n\n• [client_id]: Required', + [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + ) + const app = testAppLinked() + app.errors = errors + + // When / Then + await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('adds a root issue when the rendered message includes extra context beyond the structured issues', async () => { + // Given + const errors = new AppErrors() + errors.addError( + '/path/to/shopify.app.toml', + 'Validation errors in /path/to/shopify.app.toml:\n\n• [client_id]: Required\n\nFix the app config before continuing.', + [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + ) + const app = testAppLinked() + app.errors = errors + + // When / Then + await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: '/path/to/shopify.app.toml', + path: [], + pathString: 'root', + message: + 'Validation errors in /path/to/shopify.app.toml:\n\n• [client_id]: Required\n\nFix the app config before continuing.', + }, + ], + }, + null, + 2, + ), + ) + }) + test('renders success when errors object exists but is empty', async () => { // Given const errors = new AppErrors() diff --git a/packages/app/src/cli/services/validate.ts b/packages/app/src/cli/services/validate.ts index d27efebeea..3377983b33 100644 --- a/packages/app/src/cli/services/validate.ts +++ b/packages/app/src/cli/services/validate.ts @@ -1,18 +1,70 @@ +import { + invalidAppValidationResult, + stringifyAppValidationResult, + validAppValidationResult, +} from './validation-result.js' import {AppLinkedInterface} from '../models/app/app.js' +import {AbortSilentError} from '@shopify/cli-kit/node/error' import {outputResult, stringifyMessage} from '@shopify/cli-kit/node/output' import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui' -import {AbortSilentError} from '@shopify/cli-kit/node/error' +import type {AppValidationIssue} from '../models/app/error-parsing.js' interface ValidateAppOptions { json: boolean } +function toRootIssue(filePath: string, message: string): AppValidationIssue { + return { + filePath, + path: [], + pathString: 'root', + message, + } +} + +// Some loader/config messages are just a rendered wrapper around the same +// structured issues. Detect that case so JSON mode doesn't duplicate them. +function isStructuredMessageEquivalent(message: string, issues: AppValidationIssue[]): boolean { + if (issues.length === 0) return false + + const normalizedMessage = message.trim().replace(/^App configuration is not valid\n/, '') + + for (let startIndex = 0; startIndex < issues.length; startIndex++) { + const issueLines = issues + .slice(startIndex) + .map((issue) => `• [${issue.pathString}]: ${issue.message}`) + .join('\n') + + if (normalizedMessage === issueLines) { + return true + } + + const isValidationWrapper = normalizedMessage.startsWith('Validation errors in ') + if (isValidationWrapper && normalizedMessage.endsWith(`\n\n${issueLines}`)) { + return true + } + } + + return false +} + +function toPublicIssues(app: AppLinkedInterface): AppValidationIssue[] { + const structuredErrors = app.errors?.toStructuredJSON() ?? [] + + return structuredErrors.flatMap(({filePath, message, issues}) => { + const renderedMessage = stringifyMessage(message).trim() + if (issues.length === 0) return [toRootIssue(filePath, renderedMessage)] + if (isStructuredMessageEquivalent(renderedMessage, issues)) return issues + return [...issues, toRootIssue(filePath, renderedMessage)] + }) +} + export async function validateApp(app: AppLinkedInterface, options: ValidateAppOptions = {json: false}): Promise { const errors = app.errors if (!errors || errors.isEmpty()) { if (options.json) { - outputResult(JSON.stringify({valid: true, errors: []}, null, 2)) + outputResult(stringifyAppValidationResult(validAppValidationResult())) return } @@ -23,7 +75,7 @@ export async function validateApp(app: AppLinkedInterface, options: ValidateAppO const errorMessages = errors.toJSON().map((error) => stringifyMessage(error).trim()) if (options.json) { - outputResult(JSON.stringify({valid: false, errors: errorMessages}, null, 2)) + outputResult(stringifyAppValidationResult(invalidAppValidationResult(toPublicIssues(app)))) throw new AbortSilentError() } diff --git a/packages/app/src/cli/services/validation-result.ts b/packages/app/src/cli/services/validation-result.ts new file mode 100644 index 0000000000..23dc56bc2f --- /dev/null +++ b/packages/app/src/cli/services/validation-result.ts @@ -0,0 +1,25 @@ +import type {AppValidationIssue} from '../models/app/error-parsing.js' + +interface AppValidationResult { + valid: boolean + issues: AppValidationIssue[] +} + +/** + * Public machine-readable result contract for `shopify app validate --json`. + * + * This is limited to local app configuration discovery, parsing, and + * validation failures. Unrelated operational failures should continue through + * the normal CLI error path instead of being serialized as validation JSON. + */ +export function validAppValidationResult(): AppValidationResult { + return {valid: true, issues: []} +} + +export function invalidAppValidationResult(issues: AppValidationIssue[]): AppValidationResult { + return {valid: false, issues} +} + +export function stringifyAppValidationResult(result: AppValidationResult): string { + return JSON.stringify(result, null, 2) +}