fix(diff): surface parser failures instead of empty ValidationError (fixes #2222)#2223
Open
rokas-reizgys-tg wants to merge 3 commits into
Open
Conversation
…syncapi#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 "<path>".` 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 asyncapi#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 asyncapi#2222
rokas-reizgys-tg
requested review from
AayushSaini101,
Amzani,
Shurtu-gal,
Souvikns and
asyncapi-bot-eve
as code owners
🦋 Changeset detectedLatest commit: d39bb7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
asyncapi diffcurrently exits with code1and prints a literalValidationError:(no diagnostic body) when one of the supplied documents fails to parse. This PR makes the command surface a meaningful, non-empty error message instead.Why
Two bugs in the parser-failure path combined to swallow the error:
src/apps/cli/commands/diff.ts,parseDocumentsconstructednew ValidationError({type: 'invalid-file', err: ...})for parse failures.'invalid-file'is the file-not-found template; it rendersThere is no file or context with name "<path>".and ignores theerr:field. The catch block at the bottom ofrun()already used'parser-error'correctly, so this was an internal inconsistency.src/errors/validation-error.ts,buildErroronly inspectederr.title,err.detail, anderr.validationErrors. The currentValidationService.parseDocumentreturns its error as a plain string ('Failed to parse document'), so none of those checks matched andthis.messagestayed''. Even after fix CLI Specification #1, the outercatchinrun()wraps the innerValidationErrorin another one withtype: 'parser-error', andbuildErroragain saw no recognisable fields, so the message remained empty by the time oclif printed it.See #2222 for full root-cause analysis and reproduction.
How
parseDocumentsfailure branch from'invalid-file'to'parser-error', mirroring the existing pattern in therun()catch block.buildErrorso any error shape (string, genericError,undefined) produces a non-empty message instead of''. Keeps existing ParserError handling intact when those fields are present.buildError's cognitive complexity within the project's lint threshold.Testing
npm test(mocha + c8) -diffintegration suite goes from 26 to 27 passing; full suite is consistent withmaster(the suite has pre-existing flakiness in context/validate tests that's unrelated to this change - reproduces identically onmasterwith no changes).npm run lint- clean (0 warnings, 0 errors).npm run build- clean.New tests added:
test/unit/errors/validation-error.test.ts- coversValidationErrorbehaviour across alltype:branches and the new fallback chain (ParserError shape, plain string, generic Error, undefined, title-only).test/integration/diff.test.ts- asserts thatasyncapi diffagainstspecification.yml+specification-invalid.ymlsurfaces a non-emptyValidationErroron stderr (not justValidationError:).Manual repro before/after on the existing fixtures:
Related
asyncapi diffprints emptyValidationError:on parse failure #2222BUG: Diff command doesn't work when circular reference. Earlier attempts at the circular-ref case in fix: handle circular references in diff command (fixes #2093) #2113 and fix: handle circular references in diff command gracefully (fixes #2093) #2167 were closed without merge.Out of scope
A natural follow-up is to propagate the parser's
diagnosticsarray out ofValidationService.parseDocumentso users see which validation rules failed (line/column/code) instead of the genericFailed to parse documentstring. That requires extendingServiceResultand theValidationErrorinterface, so I left it for a separate PR to keep this one minimal and focused on the empty-message symptom.