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
Documentation build overview
11 files changed ·
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
eb0350d to
40d2d1b
Compare
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>
40d2d1b to
c66c077
Compare
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}')
|
CAOShurong
left a comment
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.
| 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.
2 participants
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
JsonValidationErrorandXmlValidationErrorsubclasses — this PR completes that work.Changes:
ValidationErrorgains three structured fields:message(human-readable str),path(tuple of path segments to the offending value), anddata(raw backend error object).__str__and__repr__now usemessage.JsonValidationError._make_from_jsverecurses into nestedjsonschemacontext errors to surface the most specific leaf failure foroneOf/anyOfchecks, instead of emitting the generic parent message.XmlValidationError._make_from_xleextractse.messageande.pathfromlxml's_LogEntry, normalising XML errors to the samemessage/pathshape.Closes #827
AI Tool Disclosure
GTP CodexGPT-5.3-Codex[Summarize the key prompts or instructions given to the AI tools]Affirmation