Fix a trio of schema validation bugs - #421
Open
dmarcotte wants to merge 10 commits into
Open
Conversation
Validating a document against a schema with problems was resulting in the schema being ignored entirely and the validation reporting an inaccurate "OK!" We now add a diagnostic to the validation's results noting the invalid schema so the reader knows the validation could not succeed (it is impossible to be valid against an invalid schema) with a pointer to what the real problem is. Note that "schema with problems" here is broader than "schema won't parse": a schema can parse cleanly and still be invalid (`type: 5`, for instance).
A caller who asked the parser to `ignoreErrors` should not really have any business then asking the resulting (possibly error-stitched) AST for a KsonValue (KsonValue is explicitly designed to represent fully valid KSON values), but nothing in the type design prevented it, so we've got a couple of callers who are taking advantage of being able to get a KsonValue when possible (and null otherwise). This makes for a somewhat inconsistent abstraction: we _want_ for `messages` to have errors logged any time an AST is invalid, and hence cannot be turned into a KsonValue, but `ignoreErrors` currently creates a hole where it's possible for an `AstParseResult` to have a null `ksonValue` with no messages to explain why. This is currently not a problem in the code, so we've queued up the intention to refactor this and in the meantime this commit simply clarifies the AstParseResult contract for `ignoreErrors` and improves a bit of the code around it: `parseSchema` can be less defensive around null, and `AstParseResult.ksonValue` can be improved to _only_ swallow errors when `ignoreErrors` is used, rather than risk masking real bugs (neither the test suite nor manual testing revealed any already-masked bugs, so as far as we know, the time we were inappropriately swallowing these has not allowed any bugs to creep in)
Our schema loader was inappropriately "helpful", swallowing schemas with any parse message (even warnings!) and making it appear to callers in that case that NO schema was loaded. In the face of errors in an attached schema, the LSP now tells users that the schema they asked to validate against is _impossible_ to validate against. For schemas whose only problems are warnings in their KSON source, the LSP now correctly uses the schema to validate the document it is attached to, rather than possibly invalidly declaring the document problem-free. Testing this wanted a `SchemaProvider` stub identical to what was implemented for `CompositeSchemaProvider`, so that stub graduates to `TestHelpers` and is shared from there. We also introduce `KsonDocumentsManager` tests covering the link that carries a loaded schema to the document that asked for it.
`assertCommand` had the classic argument order mistake in its `assertEquals`. Fix the order so we don't trip again on the incorrect expected/actual output when a test fails.
The LargeClass lint does not apply to tests since tests are a collection of individual little "apps" that verify the class the test is linked to by name, so a large test class never implies an incoherent or sprawling class interface, rather it means good coverage for some other hopefully coherent class. We had already ad-hoc been declining to apply the LargeClass lint to test classes (as evidenced by the baseline entries we were able to remove as part of this commit), so this change just makes more ergonomic a policy we are already applying.
Seems the CLI's `--schema` switch somehow dodged being tested. Fill that gap now that we've spotted it.
Using the "trivial" JSON Schema to represent "no schema" worked, but erased the downstream code's ability to easily/robustly differentiate between calls that asked for schema validation and those which did not. Using `null` here is semantically more accurate and robust. As it stands today, this change is a pure refactor with no behavior change and hence no test changes. This change was motivated by upcoming work in schema handling that likely needs/wants this improved ability to distinguish when no schema was passed.
NOTE: this is a breaking change to the public API, adding a parameter to the `analyze` endpoint. Our root analysis entry point did not previously have the ability to incorporate schema validation (SchemaValidator lives as a standalone object so a schema can be parsed once and used to validate many KSON documents), but of course in the case of a schema'ed document, that validation should definitely be part of "analyzing" a KSON document. So: `Kson.analyze` now takes optionally a SchemaValidator object to use as part of its analysis. This lets callers choose how they'd like to use the SchemaValidator: - as a pure schema validator that ONLY does schema validation (in particular, it does not concern itself with KSON language-level warnings when asked to validate a chunk of KSON source) - or does the caller want a _full_ analysis of the KSON source, with all messages/errors/warnings returned at once? This fixes a bug in `validate` code paths (including the KSON Cli entry point) where KSON warnings were always swallowed if a schema was also given to validate against.
`validateWithSchema` was echo'ing "Document is valid according to the schema" to stdout, where `json`, `yaml` and `format` write their results by default, corrupting output that likely often wants to be piped elsewhere as pure data. Also update the cli test suite to catch this type of stdout pollution in the future by adding an assertion in `assertCommand` that ensures nothing extraneous gets written to stdout on success of any of our commands, letting the commands' results and their exit code speak for themselves.
The `validate` command exits 1 on errors _and_ warnings, intentionally, so fix up the help text to properly reflect that.
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.
Fix up a trio of gaps in how we serve parse requests with schema validation, plus some clean-ups adjacent to this work that either supported the fixes or came up naturally during this work. The bugs were all of the same flavor (incorrect or incomplete parse messages when a schema is provided), so I tackled them together here even though it makes this PR quite large. The individual commits should be read rather than reading this PR as a whole.
NOTE: bb3cbfb is a breaking change to the public API, adding a parameter to the
analyzeendpoint.Fixes
SchemaValidatorparameter to the publicKson.analyzewith itClean-ups
ignoreErrorsparse contract--schemaswitchnullfor "no schema", not the trivial schema