From 653d350fdebe06ad269642fd0df9a51eae5af332 Mon Sep 17 00:00:00 2001 From: Rokas Reizgys Date: Tue, 2 Jun 2026 13:26:57 +0300 Subject: [PATCH] fix(diff): surface parser failures instead of empty ValidationError (#2222) When one of the documents passed to `asyncapi diff` fails to parse, the command exited with code 1 and printed a literal `ValidationError:` followed by no diagnostic body, making the failure unactionable for both humans and CI logs. Two bugs combined to produce that symptom: 1. `parseDocuments` in `src/apps/cli/commands/diff.ts` constructed a `ValidationError` with `type: 'invalid-file'` for parse failures. `invalid-file` is the file-not-found template; it renders `There is no file or context with name "".` and ignores the `err:` field entirely. The catch block in `run()` already used `'parser-error'` correctly, so this was an internal inconsistency. 2. `buildError` in `src/errors/validation-error.ts` only inspected `err.title`, `err.detail`, and `err.validationErrors`. The current `ValidationService.parseDocument` returns the error as a plain string (`'Failed to parse document'`), so none of those checks matched and `this.message` stayed `''`. Even after fix #1, the outer `catch` in `run()` wraps the inner `ValidationError` again, and `buildError` once more sees no recognisable fields, so the message was still empty by the time oclif printed it. This change: - Switches the `parseDocuments` failure branch to `'parser-error'` so the `err:` field is honoured and the catch wrap behaves consistently. - Adds a fallback in `buildError` so any error shape (string, generic `Error`, or `undefined`) produces a non-empty message instead of `''`. - Extracts the ParserError-shape walking into a helper to keep cognitive complexity within the project's lint threshold. Tests: - New unit test covering `ValidationError` behaviour for ParserError, string, generic `Error`, undefined, and other-typed inputs. - New integration test asserting `asyncapi diff` against a valid + parse-failing document pair surfaces a non-empty `ValidationError` on stderr. Fixes #2222 --- .changeset/diff-parser-error-empty-message.md | 10 +++ src/apps/cli/commands/diff.ts | 2 +- src/errors/validation-error.ts | 87 ++++++++++++------ test/integration/diff.test.ts | 15 ++++ test/unit/errors/validation-error.test.ts | 88 +++++++++++++++++++ 5 files changed, 172 insertions(+), 30 deletions(-) create mode 100644 .changeset/diff-parser-error-empty-message.md create mode 100644 test/unit/errors/validation-error.test.ts diff --git a/.changeset/diff-parser-error-empty-message.md b/.changeset/diff-parser-error-empty-message.md new file mode 100644 index 000000000..81e2d45bd --- /dev/null +++ b/.changeset/diff-parser-error-empty-message.md @@ -0,0 +1,10 @@ +--- +'@asyncapi/cli': patch +--- + +Fix `asyncapi diff` printing a bare `ValidationError:` with no diagnostic body +when one of the supplied documents fails to parse. The parse-failure branch in +`parseDocuments` used the `'invalid-file'` ValidationError type (the file-not- +found template) instead of `'parser-error'`, and `buildError` produced an +empty string for non-ParserError error shapes such as plain strings or generic +`Error` objects. Both paths now surface a meaningful, non-empty message. diff --git a/src/apps/cli/commands/diff.ts b/src/apps/cli/commands/diff.ts index c76687c8a..9e18bb983 100644 --- a/src/apps/cli/commands/diff.ts +++ b/src/apps/cli/commands/diff.ts @@ -247,7 +247,7 @@ export default class Diff extends Command { if (!firstResult.success || !secondResult.success) { this.error( new ValidationError({ - type: 'invalid-file', + type: 'parser-error', filepath: firstDocument.getFilePath() || secondDocument.getFilePath(), err: firstResult.error || secondResult.error, }), diff --git a/src/errors/validation-error.ts b/src/errors/validation-error.ts index 71dc7b7e1..2582a44d6 100644 --- a/src/errors/validation-error.ts +++ b/src/errors/validation-error.ts @@ -30,40 +30,69 @@ export class ValidationError extends Error { } private buildError(err: any) { - const errorsInfo: Array = []; + const errorsInfo = collectParserErrorInfo(err); - if (err.title) { - errorsInfo.push(err.title); + if (errorsInfo.length > 0) { + this.message = errorsInfo.join('\n'); + return; } - if (err.detail) { - errorsInfo.push(err.details); - } + // Fallback for error shapes that don't follow the @asyncapi/parser + // ParserError contract (plain strings, generic Errors, undefined): surface + // the most informative non-empty value we can find so callers never see a + // bare "ValidationError:" with no diagnostic body. + this.message = + (typeof err === 'string' && err) || + err?.message || + err?.title || + 'Unknown parser error'; + } +} - if (err.validationErrors) { - for (const e of err.validationErrors) { - const errorHasTitle = !!e.title; - const errorHasLocation = !!e.location; - /* - * All the conditions below are needed since validationErrors (from ParserError) come from Parser JS library, - * so we cannot assure that all the fields or properties are always provided in the error. There might be cases - * that even title is not provided. - */ - if (errorHasTitle && errorHasLocation) { - errorsInfo.push( - `${e.title} ${e.location.startLine}:${e.location.startColumn}`, - ); - continue; - } - if (errorHasTitle) { - errorsInfo.push(`${e.title}`); - continue; - } - if (errorHasLocation) { - errorsInfo.push(`${e.location.startLine}:${e.location.startColumn}`); - } +function collectParserErrorInfo(err: any): string[] { + const errorsInfo: string[] = []; + + if (!err) { + return errorsInfo; + } + + if (err.title) { + errorsInfo.push(err.title); + } + + if (err.detail) { + errorsInfo.push(err.details); + } + + if (err.validationErrors) { + for (const e of err.validationErrors) { + const formatted = formatValidationError(e); + if (formatted) { + errorsInfo.push(formatted); } } - this.message = errorsInfo.join('\n'); } + + return errorsInfo; +} + +/* + * Format a single ParserError validationErrors entry. All fields are + * defensive: validationErrors come from the @asyncapi/parser JS library and + * are not guaranteed to provide a title or a location. + */ +function formatValidationError(e: any): string | undefined { + const errorHasTitle = !!e.title; + const errorHasLocation = !!e.location; + + if (errorHasTitle && errorHasLocation) { + return `${e.title} ${e.location.startLine}:${e.location.startColumn}`; + } + if (errorHasTitle) { + return `${e.title}`; + } + if (errorHasLocation) { + return `${e.location.startLine}:${e.location.startColumn}`; + } + return undefined; } diff --git a/test/integration/diff.test.ts b/test/integration/diff.test.ts index 5befd309c..1a6c38a7b 100644 --- a/test/integration/diff.test.ts +++ b/test/integration/diff.test.ts @@ -577,4 +577,19 @@ describe('diff', () => { }); }); }); + + describe('when one of the documents cannot be parsed', () => { + test + .stderr() + .stdout() + .command(['diff', './test/fixtures/specification.yml', './test/fixtures/specification-invalid.yml']) + .it('surfaces a non-empty ValidationError message instead of a bare "ValidationError:"', (ctx, done) => { + expect(ctx.stderr).to.contain('ValidationError'); + // The historical bug emitted a literal "ValidationError:" with no + // diagnostic body; ensure at least one informative token follows. + expect(ctx.stderr).to.not.match(/ValidationError:\s*\n/); + expect(ctx.stderr).to.contain('parse'); + done(); + }); + }); }); diff --git a/test/unit/errors/validation-error.test.ts b/test/unit/errors/validation-error.test.ts new file mode 100644 index 000000000..e7203bd13 --- /dev/null +++ b/test/unit/errors/validation-error.test.ts @@ -0,0 +1,88 @@ +import { expect } from 'chai'; +import { ValidationError } from '../../../src/errors/validation-error'; + +describe('ValidationError', () => { + describe('type: parser-error', () => { + it('formats validationErrors with title and location', () => { + const error = new ValidationError({ + type: 'parser-error', + err: { + title: 'Validation failed', + validationErrors: [ + { title: 'Missing required field', location: { startLine: 3, startColumn: 5 } }, + { title: 'Invalid type' }, + ], + }, + }); + + expect(error.message).to.contain('Validation failed'); + expect(error.message).to.contain('Missing required field 3:5'); + expect(error.message).to.contain('Invalid type'); + }); + + it('falls back to the string err itself when no ParserError fields are present', () => { + // Validation service returns errors as plain strings (e.g. "Failed to + // parse document"). Without the fallback, buildError produced an empty + // message and the user saw a bare "ValidationError:" with no body. + const error = new ValidationError({ + type: 'parser-error', + err: 'Failed to parse document', + }); + + expect(error.message).to.equal('Failed to parse document'); + }); + + it('falls back to err.message for generic Error objects', () => { + const inner = new Error('Something went wrong'); + const error = new ValidationError({ + type: 'parser-error', + err: inner, + }); + + expect(error.message).to.equal('Something went wrong'); + }); + + it('falls back to a sentinel string when err is undefined', () => { + const error = new ValidationError({ + type: 'parser-error', + err: undefined, + }); + + expect(error.message).to.equal('Unknown parser error'); + }); + + it('falls back to err.title for objects without validationErrors', () => { + const error = new ValidationError({ + type: 'parser-error', + err: { title: 'A short title' }, + }); + + expect(error.message).to.equal('A short title'); + }); + }); + + describe('type: invalid-file', () => { + it('renders the file-not-found message and ignores err', () => { + const error = new ValidationError({ + type: 'invalid-file', + filepath: '/tmp/missing.yaml', + err: { title: 'should not appear' }, + }); + + expect(error.message).to.equal( + 'There is no file or context with name "/tmp/missing.yaml".', + ); + }); + }); + + describe('type: invalid-syntax-file', () => { + it('renders the syntax error template', () => { + const error = new ValidationError({ + type: 'invalid-syntax-file', + filepath: '/tmp/bad.yaml', + }); + + expect(error.message).to.equal('Syntax Error in "/tmp/bad.yaml".'); + }); + }); +});