-
Notifications
You must be signed in to change notification settings - Fork 280
fix(validation): report empty names/refs and survive nested input #414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,25 +176,28 @@ def validate_unique_names(data: dict) -> list[str]: | |
|
|
||
| model_name = model.get("name", "<unnamed>") | ||
|
|
||
| # Guard on "is not None", not truthiness: "" is a schema-valid name (no | ||
| # minLength) and must not slip past duplicate detection. | ||
|
|
||
| # 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] | ||
| for dup in find_duplicates(dataset_names): | ||
| errors.append(f"[Unique] Duplicate dataset name '{dup}' in model '{model_name}'") | ||
|
|
||
| # Check unique field names within each dataset | ||
| for dataset in model.get("datasets", []): | ||
| dataset_name = dataset.get("name", "<unnamed>") | ||
| field_names = [f.get("name") for f in dataset.get("fields", []) if f.get("name")] | ||
| field_names = [f.get("name") for f in dataset.get("fields", []) if f.get("name") is not None] | ||
| for dup in find_duplicates(field_names): | ||
| errors.append(f"[Unique] Duplicate field name '{dup}' in dataset '{dataset_name}'") | ||
|
|
||
| # Check unique metric names | ||
| metric_names = [m.get("name") for m in model.get("metrics", []) if m.get("name")] | ||
| metric_names = [m.get("name") for m in model.get("metrics", []) if m.get("name") is not None] | ||
| for dup in find_duplicates(metric_names): | ||
| errors.append(f"[Unique] Duplicate metric name '{dup}' in model '{model_name}'") | ||
|
|
||
| # Check unique relationship names | ||
| rel_names = [r.get("name") for r in model.get("relationships", []) if r.get("name")] | ||
| rel_names = [r.get("name") for r in model.get("relationships", []) if r.get("name") is not None] | ||
| for dup in find_duplicates(rel_names): | ||
| errors.append(f"[Unique] Duplicate relationship name '{dup}' in model '{model_name}'") | ||
|
|
||
|
|
@@ -211,16 +214,20 @@ def validate_references(data: dict) -> list[str]: | |
| errors = [] | ||
|
|
||
| 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")} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix (about This data lookup ( |
||
|
|
||
| for rel in model.get("relationships", []): | ||
| rel_name = rel.get("name", "<unnamed>") | ||
| from_ds = rel.get("from") | ||
| to_ds = rel.get("to") | ||
|
|
||
| if from_ds and from_ds not in datasets: | ||
| # "is not None", not truthiness: "" is a declared-but-invalid reference | ||
| # that must be reported, not skipped. | ||
| if from_ds is not None and from_ds not in datasets: | ||
| errors.append(f"[Reference] Relationship '{rel_name}' in model '{model_name}' references unknown dataset '{from_ds}'") | ||
| if to_ds and to_ds not in datasets: | ||
| if to_ds is not None and to_ds not in datasets: | ||
| errors.append(f"[Reference] Relationship '{rel_name}' in model '{model_name}' references unknown dataset '{to_ds}'") | ||
|
|
||
| # The spec defines to_columns as "Primary/unique key columns in the | ||
|
|
@@ -292,7 +299,9 @@ def validate_sql_expression(expr: str, dialect: str, context: str) -> str | None | |
| # Try parsing as expression first (for field expressions like "column_name") | ||
| sqlglot.parse_one(expr, dialect=sqlglot_dialect) | ||
| return None | ||
| except (ParseError, TokenError): | ||
| except Exception: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Broadening this from I recommend catching |
||
| # Any failure (a parse error, or a RecursionError on deeply nested | ||
| # input) falls through to the SELECT-wrapped retry below. | ||
| pass | ||
|
|
||
| try: | ||
|
|
@@ -301,6 +310,11 @@ def validate_sql_expression(expr: str, dialect: str, context: str) -> str | None | |
| return None | ||
| except (ParseError, TokenError) as e: | ||
| return f"[SQL] {context}: {str(e).split(chr(10))[0]}" | ||
| except RecursionError: | ||
| # Deeply nested input exhausts the recursion limit, not a parser error. | ||
| return f"[SQL] {context}: expression is too deeply nested to parse" | ||
| except Exception as e: | ||
| return f"[SQL] {context}: {str(e).split(chr(10))[0] or type(e).__name__}" | ||
|
|
||
|
|
||
| def validate_sql(data: dict) -> list[str]: | ||
|
|
@@ -384,6 +398,10 @@ def main(): | |
| except yaml.YAMLError as e: | ||
| print(f"Error: Invalid YAML: {e}") | ||
| sys.exit(1) | ||
| except RecursionError: | ||
| # Deeply nested input surfaces as RecursionError, not YAMLError. | ||
| print("Error: Invalid YAML: input is too deeply nested to parse") | ||
| sys.exit(1) | ||
|
|
||
| # Run validations | ||
| errors = [] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
is not Nonefix 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_namesonly fires on 2+ occurrences, and nothing else checks for empty names.I suggest fixing this at the schema level instead: add
minLength: 1to thename/from/tostring properties incore-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.