feat(validation): improve user-facing schema validation errors - #940
feat(validation): improve user-facing schema validation errors#940saquibsaifee wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Documentation build overview11 files changed · + 2 added · ± 9 modified + Added ± Modified |
Sorry, something went wrong.
Up to standards ✅🟢 Issues 0 issues🟢 Metrics 5 complexity · 0 duplication
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer |
Sorry, something went wrong.
Implements the planned improvement from PR CycloneDX#836 by the maintainer. - ValidationError gains structured fields: message (str), path (tuple), and data (raw backend object) — replacing the bare data-only shape - JsonValidationError._make_from_jsve recurses into nested jsonschema context errors to surface the most specific leaf failure for oneOf/anyOf - XmlValidationError._make_from_xle maps lxml _LogEntry.message and path - __repr__ and __str__ now emit the human-readable message field Closes CycloneDX#827 Signed-off-by: Saquib Saifee <saquibsaifee2@gmail.com>
PRs already in main that this builds on: - CycloneDX#834 (merged): added all_errors support and JsonValidationError / XmlValidationError subclass stubs - CycloneDX#840 (merged): refined the subclass stubs further - CycloneDX#836 (WIP by maintainer): placeholder for this exact work Changes ------- ValidationError (__init__.py) - Add message (str): human-readable, bounded error message - Add path (tuple[str|int, ...]): location of the offending value - Keep data (Any): raw backend object, unchanged for backward compat - __str__ now returns message; __repr__ is structured JsonValidationError (json.py) - __get_most_relevant_jsve: recurse into nested jsonschema context errors to surface the deepest leaf failure for oneOf/anyOf checks, instead of emitting the generic parent message - _shorten_message: (1) replace a bloated repr(instance) with a head...tail summary for uniqueItems/large-document failures, then (2) hard-cap the whole message at 256 chars + ellipsis XmlValidationError (xml.py) - _shorten_xml_message: hard-cap lxml _LogEntry.message at 256 chars, preventing the full SPDX enumeration set from appearing verbatim - path populated from _LogEntry.path (XPath location string) Before vs after (invalid-license-id test files from issue CycloneDX#827) JSON str(error): 111,672 chars -> 257 chars XML str(error): 13,430 chars -> 257 chars error.message: AttributeError -> bounded str error.path: AttributeError -> structured tuple Tests (test_validation_json.py, test_validation_xml.py) - Assert error is the correct subtype (JsonValidationError / XmlValidationError) - Assert .message is a str and .path is a tuple - Assert len(message) <= 257 across every invalid test file for every schema version Closes CycloneDX#827 Signed-off-by: Saquib Saifee <saquibsaifee2@gmail.com>
|
@jkowalleck is this along the lines of what you had in mind for #827? PR #940 builds directly on the work from #834 and #840 already in main. Here's what it adds:
Before/after on the exact test files from the issue (attaching screenshots):
Raw data is still there for anyone depending on it. Happy to adjust the message cap value, the path shape, or anything else — just let me know! Test code and output: """AFTER — our branch"""
from cyclonedx.schema import SchemaVersion
from cyclonedx.validation.json import JsonStrictValidator
from cyclonedx.validation.xml import XmlValidator
SEP = '=' * 72
# --------------- JSON ---------------
json_file = 'tests/_data/schemaTestData/1.2/invalid-license-id-1.2.json'
with open(json_file) as fh:
json_data = fh.read()
json_validator = JsonStrictValidator(SchemaVersion.V1_2)
error = json_validator.validate_str(json_data)
print(SEP)
print('JSON')
print(SEP)
print(f'type(error) : {type(error).__qualname__}')
print(f'type(error.data) : {type(error.data).__qualname__}')
print(f'len(str(error)) : {len(str(error)):,} chars')
print(f'error.message : {error.message!r}')
print(f'error.path : {error.path!r}')
# --------------- XML ---------------
xml_file = 'tests/_data/schemaTestData/1.1/invalid-license-id-1.1.xml'
with open(xml_file) as fh:
xml_data = fh.read()
xml_validator = XmlValidator(SchemaVersion.V1_1)
error = xml_validator.validate_str(xml_data)
print()
print(SEP)
print('XML')
print(SEP)
print(f'type(error) : {type(error).__qualname__}')
print(f'type(error.data) : {type(error.data).__qualname__}')
print(f'len(str(error)) : {len(str(error)):,} chars')
print(f'error.message : {error.message!r}')
print(f'error.path : {error.path!r}')
|
Sorry, something went wrong.
CAOShurong
left a comment
There was a problem hiding this comment.
I tested the exact head locally and found one correctness issue in the central error-selection heuristic. The implementation and its existing tests otherwise pass (1,620 focused validation tests and 6,959 full tests with UTF-8 mode). I also scanned all 359 repository JSON schema fixtures: 21 top-level errors had nested context, and this heuristic disagreed with jsonschema's tie-aware best_match choice in 10 of them. The concrete repository fixture in the inline comment shows why that matters.
This review was prepared with OpenAI Codex assistance; I independently ran and checked every command and result reported here.
Sorry, something went wrong.
| return e | ||
| # nested `context` errors generally provide more useful details than | ||
| # the generic parent message (e.g. for oneOf/anyOf checks). | ||
| child = max(e.context, key=lambda ce: len(ce.absolute_path)) |
There was a problem hiding this comment.
max(..., key=len(absolute_path)) silently picks the first child when alternative-schema errors have equal depth, so it can surface a false diagnosis from a branch that was never intended to match. On the repository's tests/_data/schemaTestData/1.6/invalid-license-declared-concluded-mix-1.6.json, validate_str(..., all_errors=True) now returns three messages saying 'license' is a required property; those entries already contain valid license or expression forms, and the real violation is mixing declared and concluded choices. The paths produced are components/0/licenses/3, components/1/licenses/0, and components/2/licenses/0.
Please make descent tie-aware (or preserve the parent oneOf/anyOf error when no child is uniquely more relevant) and add this existing fixture as a regression. jsonschema.exceptions.best_match is useful prior art here: it deliberately stops at the parent when the best nested candidates have equal relevance, rather than choosing one arbitrarily.
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 0 approving reviews are required to merge this pull request.
Description
Implements the structured validation error improvement planned in #836 (which remains a WIP draft by the maintainer). PR #840 (already merged) laid the groundwork by introducing JsonValidationError and XmlValidationError subclasses — this PR completes that work.
Changes:
Closes #827
AI Tool Disclosure
Affirmation