fix(validation): report schema errors for invalid YAML - #271
mattfaltyn wants to merge 1 commit into
Conversation
kayemkim
left a comment
There was a problem hiding this comment.
Picked this up since #270 still reproduces on main (831f48e): /dev/null, [], a bare scalar, and semantic_model: invalid all end in an AttributeError traceback. With the validate.py hunk from this PR applied on top of main, all four exit 1 with the [Schema] line and no traceback, the existing suites pass (uv run validation/test_validate.py and pytest on validation/tests/), and the 14 Ossie documents under examples/ and converters/**/tests produce byte-identical output before and after.
Two things before this can go in:
-
It no longer merges. Main gained its own
validation/test_validate.pyin #337 (a uv script that CI runs directly), so this PR's file is an add/add conflict, and it also points atcore-spec/osi-schema.json, which is nowossie-schema.json. The four cases would fit naturally as pytest functions invalidation/tests/test_validate.py, which CI already runs. -
if not errors and ...changes what a schema-invalid document reports. On main a document with one unknown property plus a duplicate dataset name and a dangling relationship target gets all three errors; with this change it gets only the schema one, and the other two show up on the next run. That is a defensible trade for never crashing, but it is a behaviour change worth stating in the PR, and maybe worth one line in the output ("semantic checks skipped until schema errors are fixed") so nobody is surprised. Gating on shape instead (root is a mapping,semantic_modelis a list of mappings) would keep today's behaviour for the common case, but the semantic checks would still need guards for deeper malformed values, so I can see why the simpler gate was chosen.
For ordering, #373 and #383 both edit the same part of main() (#383 changes the semantic_model gate itself), so whichever lands first, the others will need a small rebase.
flyrain
left a comment
There was a problem hiding this comment.
+1 Thanks @mattfaltyn. cc @khush-bhatia
30221d4 to
b5b743b
Compare
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
b5b743b to
972e377
Compare
|
Thanks for the thorough review. Both points are addressed.
Rebased onto current
Agreed. The CLI now states when semantic checks are skipped, the behavior is documented in the PR description and Thanks again for the clear guidance. |
kayemkim
left a comment
There was a problem hiding this comment.
Re-ran the rebased head (972e377) merged into current main (fc6c9df): the validation-ci matrix (3.11 to 3.14) passes, the four #270 inputs (empty file, [], a bare scalar, semantic_model: invalid) exit 1 with a [Schema] line and the new notice instead of a traceback, and the 14 Ossie documents under examples/ and converters/** keep their exit codes, the only output change being the notice on the three ontology files that already fail the core schema. The tests in validation/tests/, the notice, and the docs/index.md line cover both points I raised. LGTM.
One ordering note: #383 (approved yesterday) adds the same not errors and isinstance(data, dict) gate on its own, and with that branch alone the four inputs also stop crashing. If it lands first, this conflicts in both files and reduces to the notice, the docs line, and the tests, which #383 does not have for malformed shapes. The test_schema_errors_defer_semantic_errors fixture would then need the flat shape (datasets: at the root); otherwise the wrapper itself trips the schema and the duplicate-name and dangling-target parts of the case are no longer exercised.
Summary
JSON Schema validation already detects invalid YAML shapes, but the CLI continued into semantic checks that assume schema-valid mappings and lists. Those checks could raise
AttributeErrorbefore the existing reporter displayed the schema errors.This change:
Behavior note
When a document has both schema and semantic errors, the first run reports the schema errors and states that semantic checks were skipped. After the schema errors are fixed, a subsequent run reports any remaining semantic errors. This avoids tracebacks from malformed nested values while making the validation order explicit.
The specification, CLI arguments, and dependency set are unchanged.
Related Issues
Closes #270
Testing
Results:
Checklist
Validation
Documentation
docs/index.mdexplains the schema-first validation orderTests
Compliance