Skip to content

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

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

OGsiji wants to merge 2 commits 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.

Address review feedback on the apache#408 fix:

- Collapse the redundant except (ParseError, TokenError) in the first
  parse attempt into the broader except Exception that already covers
  it and falls through identically.
- Drop the inert noqa: BLE001 markers; validation/ has no ruff config.
- Condense the new comments to the file's one-line style instead of
  restating the PR description.

No behaviour change; validation suites remain green.

Refs apache#408
@OGsiji

OGsiji commented Sep 18, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review! Addressed both nits in 2c8c4a5:

  • Redundant except: collapsed the first parse attempt's except (ParseError, TokenError): pass into the broader except Exception: pass that already subsumed it. ParseError/TokenError stay imported for the second attempt's actual [SQL] diagnostic.
  • Inert noqa: BLE001: dropped both, since validation/ has no ruff config.
  • Comments: trimmed to the file's one-line style, keeping only the non-obvious "why" (schema has no minLength; deep input surfaces as RecursionError, not the parser/YAML error types).

No behaviour change; validation-ci steps (unittest 41, pytest 55, canonical example) stay green on 3.11 to 3.14.

On the #271 overlap: happy to take the second-mover rebase whenever it lands. The conflict is limited to the main() except ladder and the added pytest cases.

@jbonofre
jbonofre self-requested a review September 19, 2026 11:44
Comment thread validation/validate.py

# Check unique dataset names
dataset_names = [d.get("name") for d in model.get("datasets", []) if d.get("name")]
dataset_names = [d.get("name") for d in model.get("datasets", []) if d.get("name") is not None]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The is not None fix here only helps duplicate-name detection see an empty string. It doesn't fix the actual bug: a single (non duplicate) dataset/field/metric/relationship named "" still passes validation entirely, validate_unique_names only fires on 2+ occurrences, and nothing else checks for empty names.

I suggest fixing this at the schema level instead: add minLength: 1 to the name/from/to string properties in core-spec/ossie-schema.json (Dataset, Field, Metric, Relationship, SemanticModel). That rejects "" with a clear [Schema] error before semantic checks even run, and fixes this consistently across all four call sites instead of patching them individually.

Comment thread validation/validate.py
model_name = model.get("name", "<unnamed>")
# Exclude falsy dataset names so a relationship pointing at "" is reported
# as unknown rather than matched.
datasets = {d.get("name"): d for d in model.get("datasets", []) if d.get("name")}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix (about minLength: 1) is helping there too.

This data lookup ({d.get("name"): d for d in ... if d.get("name")}) still uses a truth filter, while the from_ds/to_ds checks two lines below were fixed to is not None. That inconsistency means a dataset actually named "" gets excluded from this dict, so any relationship referencing it is wrongly reporter as "unknown dataset ''" instead of being caught as an invalid name.

Comment thread validation/validate.py
sqlglot.parse_one(expr, dialect=sqlglot_dialect)
return None
except (ParseError, TokenError):
except Exception:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Broadening this from except (ParseError, TokenError) to bare exception Exception: pass silently swallows any exception type on the first parse attempt, not just the RecursionError this seems aimed at.
It means validate_sql_expression(123, 'ANSI_SQL', 'ctx') (non string expr) used to raise a loud TypeError. Now it's swallowed and the SELECT wrapped retry (parse_one('SELECT 123', ...)) spuriously succeeds, so the function reports the input as valid SQL.

I recommend catching RecursionError specifically here (in addition to ParseError/TokenError) rather than a bare Exception, so genuine bugs still surface instead of being masked as "valid".

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

3 participants