Conversation
validate.py silently skipped or crashed on inputs the schema accepts:
- Duplicate empty-string ("") dataset, field, metric, and relationship
names bypassed uniqueness detection via a truthiness filter; guard on
"is not None" so "" is treated as a real name.
- An empty-string relationship from/to was never reported as an unknown
dataset for the same reason; guard on "is not None".
- Deeply nested SQL raised an uncaught RecursionError from sqlglot;
catch it (and other unexpected errors) and return a diagnostic.
- Deeply nested YAML raised an uncaught RecursionError with a raw
traceback from main(); catch it and exit cleanly.
Add regression tests for each case in the unittest and pytest suites.
Closes apache#408
kayemkim
left a comment
There was a problem hiding this comment.
Thanks for picking this up the day after the issue went in, and welcome. Good call adding the cases to both suites, since validation-ci runs the unittest file and the pytest directory separately.
Ran the branch merged into current main (bc7b2c0) through the validation-ci steps on Python 3.11 to 3.14: all green. I reproduced the four #408 cases on main first: two datasets named "" and a relationship with to: "" print Validation PASSED with exit 0, and 5000 nested parentheses in an expression or 3000 nested [ in the file end in a raw RecursionError. On this branch the first two fail with [Unique] and [Reference] lines and the other two exit 1 with the new diagnostics. The next layer down, a custom_extensions dict nested 150 and 300 levels (valid YAML, deep jsonschema recursion), already fails cleanly with a [Schema] error on both trees, so I don't see a remaining gap of the same kind. The 11 Ossie documents under examples/ and converters/** keep their exit codes.
Two small things, neither blocking:
- In
validate_sql_expressionthe first attempt now hasexcept (ParseError, TokenError): passfollowed byexcept Exception: pass; the second covers the first. Thenoqa: BLE001markers are inert too, sincevalidation/has no ruff configuration. - The new comments are denser than the rest of the file and mostly restate the PR description and test comments. The one-line style used nearby would read more evenly.
Ordering note: #271 (approved, conflicting with main) edits the same block in main(), and merging both conflicts in validate.py and the pytest file. Whichever lands second has a small rebase.
LGTM.
Summary
Fixes #408.
validation/validate.pysilently skipped validation or crashed with a raw traceback on several inputs that the JSON Schema itself accepts —name,from, andtoare strings with nominLength, so""is schema-valid and reaches the semantic checks.Bugs fixed
validate_unique_namescollected names with a truthiness filter (if d.get("name")), so duplicate""dataset/field/metric/relationship names were dropped before duplicate detection. Now filtered withis not None.validate_referencesusedif from_ds/if to_ds, so a relationship withfrom: ""orto: ""silently passed instead of being reported as an unknown dataset. Nowis not None.validate_sql_expressiononly caughtParseError/TokenError; pathologically nested expressions raised an uncaughtRecursionError. Now caught and reported as a[SQL]diagnostic.main()only caughtyaml.YAMLError; deeply nested flow collections raiseRecursionErrorduring composition. Now caught, exits cleanly.Tests
Added regression tests in both suites:
validation/tests/test_validate.py(pytest): empty-name duplicates, missing-name skip, emptyfrom/toreported, missing-endpoint skip, deeply nested SQL diagnostic.validation/test_validate.py(unittest): deeply nested YAML exits cleanly without a traceback, empty endpoint reported end-to-end.All Validation CI steps pass locally on Python 3.11 and 3.12:
uv run validation/test_validate.py— 41 passedvalidation/tests/— 55 passeduv run validation/validate.py examples/tpcds_semantic_model.yaml— PASSED