Skip to content

fix(validation): report schema errors for invalid YAML - #271

Open
mattfaltyn wants to merge 1 commit into
apache:mainfrom
mattfaltyn:fix/validator-non-mapping-yaml
Open

mattfaltyn wants to merge 1 commit into
apache:mainfrom
mattfaltyn:fix/validator-non-mapping-yaml

Conversation

@mattfaltyn

@mattfaltyn mattfaltyn commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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 AttributeError before the existing reporter displayed the schema errors.

This change:

  • reports schema errors for empty, sequence, scalar, and malformed nested YAML;
  • runs semantic checks only after schema validation succeeds;
  • tells users when semantic checks were deferred;
  • adds focused regression coverage to the pytest suite run by Validation CI; and
  • documents the schema-first validation order.

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

uv run validation/test_validate.py
uv run --with pytest --with pyyaml --with jsonschema -m pytest validation/tests/
uv run validation/validate.py examples/tpcds_semantic_model.yaml
python3 -m py_compile validation/validate.py validation/test_validate.py validation/tests/test_validate.py
git diff --check

Results:

  • The upstream duplicate-key unittest suite passes all 35 tests.
  • The validation pytest suite passes all 20 tests, including the four invalid-shape cases and the schema-plus-semantic-error ordering case.
  • The valid TPC-DS example passes.
  • Compilation and diff checks pass.

Checklist

Validation

  • New validation cases are covered by tests

Documentation

  • docs/index.md explains the schema-first validation order

Tests

  • All tests relevant to this change pass locally
  • New functionality is covered by tests

Compliance

  • No new source files or third-party dependencies are added

@kayemkim kayemkim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. It no longer merges. Main gained its own validation/test_validate.py in #337 (a uv script that CI runs directly), so this PR's file is an add/add conflict, and it also points at core-spec/osi-schema.json, which is now ossie-schema.json. The four cases would fit naturally as pytest functions in validation/tests/test_validate.py, which CI already runs.

  2. 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_model is 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 flyrain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Thanks @mattfaltyn. cc @khush-bhatia

@mattfaltyn
mattfaltyn force-pushed the fix/validator-non-mapping-yaml branch from 30221d4 to b5b743b Compare September 15, 2026 15:52
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
@mattfaltyn
mattfaltyn force-pushed the fix/validator-non-mapping-yaml branch from b5b743b to 972e377 Compare September 15, 2026 15:54
@mattfaltyn

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Both points are addressed.

Main gained its own validation/test_validate.py in #337

Rebased onto current main, preserved that suite, and moved the four regression cases into validation/tests/test_validate.py, which Validation CI runs. The tests now use the renamed ossie-schema.json through the validator default.

it is a behaviour change worth stating in the PR

Agreed. The CLI now states when semantic checks are skipped, the behavior is documented in the PR description and docs/index.md, and a regression test covers documents containing both schema and semantic errors.

Thanks again for the clear guidance.

@mattfaltyn
mattfaltyn requested a review from kayemkim September 15, 2026 15:57

@kayemkim kayemkim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validator crashes instead of reporting errors for invalid YAML structures

3 participants