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..29deccc04b 100644 --- a/packages/app/src/cli/commands/app/config/validate.test.ts +++ b/packages/app/src/cli/commands/app/config/validate.test.ts @@ -3,9 +3,18 @@ import {linkedAppContext} from '../../../services/app-context.js' import {validateApp} from '../../../services/validate.js' import {testAppLinked} from '../../../models/app/app.test-data.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 +55,163 @@ describe('app config validate command', () => { // Then expect(validateApp).toHaveBeenCalledWith(app, {json: true}) }) + + test('outputs json issues when app loading aborts before validateApp runs', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new AbortError('Validation errors in /tmp/shopify.app.toml:\n\n• [name]: String is required'), + ) + + // When / Then + await Validate.run(['--json', '--path=/tmp/app'], import.meta.url).catch(() => {}) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/shopify.app.toml', + path: [], + pathString: 'name', + message: 'String is required', + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('outputs json issues when app loading aborts with ansi-colored structured text', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new AbortError( + '\u001b[1m\u001b[91mValidation errors\u001b[39m\u001b[22m in /tmp/shopify.app.toml:\n\n• [name]: String is required', + ), + ) + + // When / Then + await Validate.run(['--json', '--path=/tmp/app'], import.meta.url).catch(() => {}) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/shopify.app.toml', + path: [], + pathString: 'name', + message: 'String is required', + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('preserves a root json issue when contextual text precedes structured validation errors', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new AbortError( + 'Could not infer extension handle\n\nValidation errors in /tmp/shopify.app.toml:\n\n• [name]: String is required', + ), + ) + + // When / Then + await Validate.run(['--json', '--path=/tmp/app'], import.meta.url).catch(() => {}) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: '/tmp/shopify.app.toml', + path: [], + pathString: 'name', + message: 'String is required', + }, + { + filePath: '/tmp/shopify.app.toml', + path: [], + pathString: 'root', + message: + 'Could not infer extension handle\n\nValidation errors in /tmp/shopify.app.toml:\n\n• [name]: String is required', + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('parses structured validation errors for windows-style paths', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue( + new AbortError('Validation errors in C:\\tmp\\shopify.app.toml:\n\n• [name]: String is required'), + ) + + // When / Then + await Validate.run(['--json', '--path=/tmp/app'], import.meta.url).catch(() => {}) + expect(outputResult).toHaveBeenCalledWith( + JSON.stringify( + { + valid: false, + issues: [ + { + filePath: 'C:\\tmp\\shopify.app.toml', + path: [], + pathString: 'name', + message: 'String is required', + }, + ], + }, + null, + 2, + ), + ) + expect(validateApp).not.toHaveBeenCalled() + }) + + test('outputs a root json issue when app loading aborts with a non-structured message', async () => { + // Given + vi.mocked(linkedAppContext).mockRejectedValue(new AbortError("Couldn't find an app toml file at /tmp/app")) + + // When / Then + await Validate.run(['--json', '--path=/tmp/app'], import.meta.url).catch(() => {}) + 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('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..5ad27fefda 100644 --- a/packages/app/src/cli/commands/app/config/validate.ts +++ b/packages/app/src/cli/commands/app/config/validate.ts @@ -3,6 +3,76 @@ import {validateApp} from '../../../services/validate.js' import AppLinkedCommand, {AppLinkedCommandOutput} from '../../../utilities/app-linked-command.js' import {linkedAppContext} from '../../../services/app-context.js' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' +import {outputResult, unstyled} from '@shopify/cli-kit/node/output' + +import type {AppValidationIssue} from '../../../models/app/error-parsing.js' + +function toRootIssue(filePath: string, message: string): AppValidationIssue { + return { + filePath, + path: [], + pathString: 'root', + message, + } +} + +function hasMeaningfulPrefix(prefix: string): boolean { + const normalizedPrefix = prefix.trim() + return normalizedPrefix !== '' && normalizedPrefix !== 'App configuration is not valid' +} + +function isJsonValidationAbort(error: AbortError): boolean { + const message = unstyled(error.message).trim() + + return ( + message.includes('Validation errors in ') || + message.startsWith("Couldn't find an app toml file at") || + message.startsWith("Couldn't find directory ") || + /^Couldn't find .* in .+\.?$/.test(message) + ) +} + +function toJsonIssuesFromAbortError(error: AbortError, fallbackFilePath: string): AppValidationIssue[] { + const message = unstyled(error.message).trim() + const marker = 'Validation errors in ' + const markerIndex = message.indexOf(marker) + + if (markerIndex === -1) { + return [toRootIssue(fallbackFilePath, message)] + } + + const bodyStartIndex = message.indexOf('\n\n', markerIndex) + if (bodyStartIndex === -1) { + return [toRootIssue(fallbackFilePath, message)] + } + + const filePathLine = message.slice(markerIndex + marker.length, bodyStartIndex) + if (!filePathLine.endsWith(':')) { + return [toRootIssue(fallbackFilePath, message)] + } + + const filePath = filePathLine.slice(0, -1) + const body = message.slice(bodyStartIndex + 2) + const issues = Array.from(body.matchAll(/^• \[([^\]]+)\]: (.+)$/gm)).map((captures) => { + const pathString = captures[1]! + const issueMessage = captures[2]! + + return { + filePath, + // `pathString` is rendered display text, not a lossless encoding of the + // original structured path, so the early-abort fallback avoids + // reconstructing `path` from it. + path: [], + pathString, + message: issueMessage, + } + }) + + if (issues.length === 0) return [toRootIssue(filePath, message)] + if (hasMeaningfulPrefix(message.slice(0, markerIndex))) return [...issues, toRootIssue(filePath, message)] + return issues +} export default class Validate extends AppLinkedCommand { static summary = 'Validate your app configuration and extensions.' @@ -20,16 +90,34 @@ 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, - }) + 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}) - await validateApp(app, {json: flags.json}) + return {app} + } catch (error) { + if (flags.json && error instanceof AbortError && isJsonValidationAbort(error)) { + outputResult( + JSON.stringify( + { + valid: false, + issues: toJsonIssuesFromAbortError(error, flags.path), + }, + null, + 2, + ), + ) + throw new AbortSilentError() + } - return {app} + throw error + } } } diff --git a/packages/app/src/cli/services/validate.test.ts b/packages/app/src/cli/services/validate.test.ts index c475c4399d..22bc95600f 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() }) @@ -54,16 +54,28 @@ describe('validateApp', () => { await expect(validateApp(app)).rejects.toThrow(AbortSilentError) expect(renderError).toHaveBeenCalledWith({ headline: 'Validation errors found.', - body: expect.stringContaining('client_id is required'), + body: 'client_id is required\n\ninvalid type "unknown"', }) expect(renderSuccess).not.toHaveBeenCalled() expect(outputResult).not.toHaveBeenCalled() }) - test('outputs json errors and throws when --json is enabled and there are validation errors', async () => { + test('outputs structured json issues and throws when --json is enabled and there are validation errors', async () => { // Given const errors = new AppErrors() - errors.addError('/path/to/shopify.app.toml', 'client_id is required') + errors.addError( + '/path/to/shopify.app.toml', + 'App configuration is not valid\nValidation 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', + }, + ], + ) errors.addError('/path/to/extensions/my-ext/shopify.extension.toml', 'invalid type "unknown"') const app = testAppLinked() app.errors = errors @@ -74,7 +86,21 @@ describe('validateApp', () => { JSON.stringify( { valid: false, - errors: ['client_id is required', 'invalid type "unknown"'], + issues: [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: '/path/to/extensions/my-ext/shopify.extension.toml', + path: [], + pathString: 'root', + message: 'invalid type "unknown"', + }, + ], }, null, 2, @@ -84,6 +110,321 @@ describe('validateApp', () => { expect(renderSuccess).not.toHaveBeenCalled() }) + test('flattens multiple structured issues across files in json mode', 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• [name]: Must be set', + [ + { + filePath: '/path/to/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: '/path/to/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ], + ) + errors.addError( + '/path/to/extensions/my-ext/shopify.extension.toml', + 'Validation errors in /path/to/extensions/my-ext/shopify.extension.toml:\n\n• [targeting.0.target]: Invalid target', + [ + { + filePath: '/path/to/extensions/my-ext/shopify.extension.toml', + path: ['targeting', 0, 'target'], + pathString: 'targeting.0.target', + message: 'Invalid target', + code: 'custom', + }, + ], + ) + 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: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + { + filePath: '/path/to/extensions/my-ext/shopify.extension.toml', + path: ['targeting', 0, 'target'], + pathString: 'targeting.0.target', + message: 'Invalid target', + code: 'custom', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('preserves structured issues and appends a root issue for same-file unstructured follow-up errors', async () => { + // Given + const errors = new AppErrors() + errors.addError('tmp/shopify.app.toml', 'Validation errors in tmp/shopify.app.toml:\n\n• [client_id]: Required', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('tmp/shopify.app.toml', 'Could not infer extension handle from configuration') + 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: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: 'tmp/shopify.app.toml', + path: [], + pathString: 'root', + message: 'Could not infer extension handle from configuration', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('outputs json success when errors object exists but is empty', async () => { + // Given + const errors = new AppErrors() + const app = testAppLinked() + app.errors = errors + + // When + await validateApp(app, {json: true}) + + // Then + expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: true, issues: []}, null, 2)) + expect(renderSuccess).not.toHaveBeenCalled() + expect(renderError).not.toHaveBeenCalled() + }) + + test('uses structured issues without a synthetic root issue when the rendered message is only bullet lines', async () => { + // Given + const errors = new AppErrors() + errors.addError('tmp/shopify.app.toml', '• [client_id]: Required', [ + { + filePath: 'tmp/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: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('does not append a synthetic root issue for repeated same-file structured errors', async () => { + // Given + const errors = new AppErrors() + errors.addError('tmp/shopify.app.toml', 'Validation errors in tmp/shopify.app.toml:\n\n• [client_id]: Required', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + ]) + errors.addError('tmp/shopify.app.toml', 'Validation errors in tmp/shopify.app.toml:\n\n• [name]: Must be set', [ + { + filePath: 'tmp/shopify.app.toml', + path: ['name'], + pathString: 'name', + message: 'Must be set', + code: 'custom', + }, + ]) + 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: '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', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('preserves a root issue when structured bullets are prefixed by meaningful context', async () => { + // Given + const errors = new AppErrors() + errors.addError('tmp/shopify.app.toml', 'Could not infer extension handle\n\n• [client_id]: Required', [ + { + filePath: 'tmp/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: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: 'tmp/shopify.app.toml', + path: [], + pathString: 'root', + message: 'Could not infer extension handle\n\n• [client_id]: Required', + }, + ], + }, + null, + 2, + ), + ) + }) + + test('preserves a root issue when contextual text precedes a validation wrapper', async () => { + // Given + const errors = new AppErrors() + errors.addError( + 'tmp/shopify.app.toml', + 'Could not infer extension handle\n\nValidation errors in tmp/shopify.app.toml:\n\n• [client_id]: Required', + [ + { + filePath: 'tmp/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: 'tmp/shopify.app.toml', + path: ['client_id'], + pathString: 'client_id', + message: 'Required', + code: 'invalid_type', + }, + { + filePath: 'tmp/shopify.app.toml', + path: [], + pathString: 'root', + message: + 'Could not infer extension handle\n\nValidation errors in tmp/shopify.app.toml:\n\n• [client_id]: Required', + }, + ], + }, + 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..2b4efd379a 100644 --- a/packages/app/src/cli/services/validate.ts +++ b/packages/app/src/cli/services/validate.ts @@ -1,18 +1,63 @@ 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, + } +} + +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(JSON.stringify({valid: true, issues: []}, null, 2)) return } @@ -23,7 +68,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(JSON.stringify({valid: false, issues: toPublicIssues(app)}, null, 2)) throw new AbortSilentError() }