Skip to content

fix(validation): report empty names/refs and survive nested input - #414

Open
OGsiji wants to merge 1 commit into
apache:mainfrom
OGsiji:fix/validate-truthiness-exception-408
Open

OGsiji wants to merge 1 commit into
apache:mainfrom
OGsiji:fix/validate-truthiness-exception-408

Conversation

@OGsiji

@OGsiji OGsiji commented Sep 17, 2026

Copy link
Copy Markdown

Summary

Fixes #408. validation/validate.py silently skipped validation or crashed with a raw traceback on several inputs that the JSON Schema itself accepts — name, from, and to are strings with no minLength, so "" is schema-valid and reaches the semantic checks.

Bugs fixed

  1. Empty-string names bypassed uniqueness checks. validate_unique_names collected names with a truthiness filter (if d.get("name")), so duplicate "" dataset/field/metric/relationship names were dropped before duplicate detection. Now filtered with is not None.
  2. Empty-string relationship endpoints were never flagged. validate_references used if from_ds / if to_ds, so a relationship with from: "" or to: "" silently passed instead of being reported as an unknown dataset. Now is not None.
  3. Deeply nested SQL crashed the validator. validate_sql_expression only caught ParseError/TokenError; pathologically nested expressions raised an uncaught RecursionError. Now caught and reported as a [SQL] diagnostic.
  4. Deeply nested YAML crashed with a traceback. main() only caught yaml.YAMLError; deeply nested flow collections raise RecursionError during composition. Now caught, exits cleanly.

Tests

Added regression tests in both suites:

  • validation/tests/test_validate.py (pytest): empty-name duplicates, missing-name skip, empty from/to reported, 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 passed
  • pytest validation/tests/ — 55 passed
  • uv run validation/validate.py examples/tpcds_semantic_model.yaml — PASSED

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 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.

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_expression the first attempt now has except (ParseError, TokenError): pass followed by except Exception: pass; the second covers the first. The noqa: BLE001 markers are inert too, since validation/ 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.

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.

Truthiness/exception-handling bugs allow silent validation bypass and uncaught crashes in validate.py

2 participants