Repository navigation
fix(validation): report unreadable input or schema, not a traceback - #534
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Parsing YAML from a string removes the filename from parser diagnostics, contradicting the promised message preservation.
1 open finding
What changed in this PR
Improves validator diagnostics for unreadable, non-UTF-8, and malformed schema inputs.
Changes:
- Adds guarded UTF-8 file reading and JSON parsing.
- Adds integration tests for invalid input and schema paths.
| File | Description |
|---|---|
validation/validate.py |
Handles file-read, decoding, and schema parsing errors. |
validation/test_validate.py |
Tests new failure diagnostics and exit statuses. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ec257c9 to
c0ddacf
Compare
The exists() checks pass for a path that cannot be read as text, so these inputs reached a traceback while every neighbouring bad input got a clean line — a directory, a file without read permission, a binary file, and a --schema that is any of those or not JSON. Read each file first and parse the text after, so one guard covers both files and neither read nests inside a parse: Error: Could not read validation: [Errno 21] Is a directory: 'validation' Error: Invalid JSON in schema junk.json: Expecting value: line 1 column 1 Error: model.yaml is not valid UTF-8 text: 'utf-8' codec can't decode ... The model is parsed through load_yaml_named, which names the loader so the path stays in every parser mark: PyYAML names a str source "<unicode string>", and a named stream would carry the path but drop the offending line, because Reader.get_mark() attaches its buffer only for a str source. Exit status was already 1 in each case and is unchanged, as are the messages for a missing file, a missing schema and malformed YAML.
c0ddacf to
96f90c0
Compare
kayemkim
left a comment
kayemkim
left a comment
There was a problem hiding this comment.
Ran this merged onto current main (c460302) through the validation workflow on 3.11 to 3.14, all green. I also fed the eight inputs to main: a directory, a file without read permission and a binary file, each as the model and as the schema, plus a schema that is not JSON and a model with a NUL byte. Seven end in a traceback on main, the NUL case was already a clean line, and on this branch each prints the single Error line and exits 1, as the description says. The 26 model, ontology and mapping documents in the repository give byte-identical output on both trees, and nothing outside validation/ imports the loader or matches on these messages, so the wording changes stay inside the CLI. Merges cleanly with #271 as well. Good to go from my side.
|
Thanks for the PR @ayushtkn . I will take a look later this weekend. |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
LGTM!
Turning unreadable input, bad encoding, malformed YAML and invalid schema JSON into clear messages with exit code 1 is a good improvement. I checked the following:
OSErrorandUnicodeDecodeErrorare handled separately, so neither handler hides the other.- Setting
loader.namemeans YAML errors name the file and keep the offending line. dispose()runs in afinally.- The new tests match the actual messages and exit codes.
Nit: a schema that is valid JSON but not an object would still fail later. That was already the case before this PR, so it can be a follow-up 😄
Summary
The exists() checks pass for a path that cannot be read as text, so these inputs reached a traceback while every neighbouring bad input got a clean line — a directory, a file without read permission, a binary file, and a --schema that is any of those or not JSON.
Read each file first and parse the text after, so one guard covers both files and neither read nests inside a parse:
Error: Could not read validation: [Errno 21] Is a directory: 'validation'
Error: Invalid JSON in schema junk.json: Expecting value: line 1 column 1
Error: model.yaml is not valid UTF-8 text: 'utf-8' codec can't decode ...
Exit status was already 1 in each case and is unchanged, as are the messages for a missing file, a missing schema and malformed YAML.
Related Issues
Checklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changes.github/labeler.ymlis updated with the new pathValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance
NOTICEand/orLICENSEhave been updated per ASF policy