From 141e22e34980485dd15b4e257d4159ab5727ce0a Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Tue, 18 Aug 2026 19:12:52 +0000 Subject: [PATCH 1/2] fix(wren): report a validation error for non-scalar model/view names validate_project() only checked model/view names for truthiness before using them as set members for duplicate detection, so a hand-edited name: [a, b] or name: {x: 1} in a model's or view's metadata.yml (both unhashable) reached `name in model_names` / `name in view_names` and raised an unhandled TypeError, crashing `wren context validate` with a raw traceback instead of reporting the malformed field. Add a type guard next to the existing `if not name` check in both the model and the view loop: a list/dict name is now reported as a clean ValidationError and the entry is skipped, matching the pattern already used for malformed relationship/column entries elsewhere in this file. A hashable non-string name (e.g. an int) is unaffected. Fixes #2673 --- core/wren/src/wren/context.py | 18 ++++++++ core/wren/tests/unit/test_context.py | 61 ++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/core/wren/src/wren/context.py b/core/wren/src/wren/context.py index 31df239bfd..b96ab04d69 100644 --- a/core/wren/src/wren/context.py +++ b/core/wren/src/wren/context.py @@ -968,6 +968,15 @@ def validate_project(project_path: Path) -> list[ValidationError]: if not name: errors.append(ValidationError("error", src_path, "model missing 'name'")) continue + if isinstance(name, (list, dict)): + errors.append( + ValidationError( + "error", + src_path, + f"model 'name' must be a scalar value, got {type(name).__name__}", + ) + ) + continue if name in model_names: errors.append( @@ -1220,6 +1229,15 @@ def validate_project(project_path: Path) -> list[ValidationError]: ) ) continue + if isinstance(name, (list, dict)): + errors.append( + ValidationError( + "error", + f"views/{src_dir}/metadata.yml", + f"view 'name' must be a scalar value, got {type(name).__name__}", + ) + ) + continue if name in view_names or name in model_names: errors.append( ValidationError("error", f"views/{src_dir}", f"duplicate name '{name}'") diff --git a/core/wren/tests/unit/test_context.py b/core/wren/tests/unit/test_context.py index 49f45ba666..00a00fc43c 100644 --- a/core/wren/tests/unit/test_context.py +++ b/core/wren/tests/unit/test_context.py @@ -555,6 +555,45 @@ def test_validate_duplicate_model(tmp_path): assert any("duplicate model name" in e.message for e in errors) +def test_validate_model_name_list_reports_error(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "models" / "orders" + d.mkdir(parents=True) + (d / "metadata.yml").write_text( + "name: [a, b]\ntable_reference:\n table: orders\ncolumns: []\n" + ) + errors = validate_project(tmp_path) + assert any( + "model 'name' must be a scalar value, got list" in e.message for e in errors + ) + + +def test_validate_model_name_dict_reports_error(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "models" / "orders" + d.mkdir(parents=True) + (d / "metadata.yml").write_text( + "name: {x: 1}\ntable_reference:\n table: orders\ncolumns: []\n" + ) + errors = validate_project(tmp_path) + assert any( + "model 'name' must be a scalar value, got dict" in e.message for e in errors + ) + + +def test_validate_model_name_int_is_not_rejected(tmp_path): + # An int name is unusual but hashable, so it never hit the crash this guard + # exists for; the guard must not turn it into a new validation error. + _make_v2_project(tmp_path) + d = tmp_path / "models" / "orders" + d.mkdir(parents=True) + (d / "metadata.yml").write_text( + "name: 3\ntable_reference:\n table: orders\ncolumns: []\n" + ) + errors = validate_project(tmp_path) + assert not any("must be a scalar value" in e.message for e in errors) + + def test_validate_both_tref_and_ref_sql(tmp_path): _make_v2_project(tmp_path) d = tmp_path / "models" / "conflict" @@ -682,6 +721,28 @@ def test_validate_view_no_statement(tmp_path): assert any("missing 'statement'" in e.message for e in errors) +def test_validate_view_name_list_reports_error(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "views" / "monthly" + d.mkdir(parents=True) + (d / "metadata.yml").write_text("name: [a, b]\nstatement: SELECT 1\n") + errors = validate_project(tmp_path) + assert any( + "view 'name' must be a scalar value, got list" in e.message for e in errors + ) + + +def test_validate_view_name_dict_reports_error(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "views" / "monthly" + d.mkdir(parents=True) + (d / "metadata.yml").write_text("name: {x: 1}\nstatement: SELECT 1\n") + errors = validate_project(tmp_path) + assert any( + "view 'name' must be a scalar value, got dict" in e.message for e in errors + ) + + def test_validate_missing_join_type(tmp_path): _make_valid_project(tmp_path) (tmp_path / "relationships.yml").write_text( From 046aafe6e20479b6669ac89b19906bc56fe71a92 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Wed, 19 Aug 2026 00:15:41 +0000 Subject: [PATCH 2/2] fix(wren): check the scalar-type guard before the falsy missing-name check CodeRabbit caught it on our own PR (#2681): an empty list/dict name is falsy, so the pre-existing 'if not name' check fired first and reported 'missing name' instead of the scalar-type error the new guard exists for. Moved the isinstance(name, (list, dict)) check ahead of the truthiness check in both the model and view loops. None and empty string still report 'missing name' as before; non-empty list/dict and now empty list/dict both report the scalar-type error. Added regression tests for empty list/dict on both loops, verified they fail against the old ordering and pass against this one. --- core/wren/src/wren/context.py | 18 ++++----- core/wren/tests/unit/test_context.py | 56 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/core/wren/src/wren/context.py b/core/wren/src/wren/context.py index b96ab04d69..7e0d65f975 100644 --- a/core/wren/src/wren/context.py +++ b/core/wren/src/wren/context.py @@ -965,9 +965,6 @@ def validate_project(project_path: Path) -> list[ValidationError]: src = model.get("_source_dir", f"models[{i}]") src_path = f"models/{src}/metadata.yml" name = model.get("name") - if not name: - errors.append(ValidationError("error", src_path, "model missing 'name'")) - continue if isinstance(name, (list, dict)): errors.append( ValidationError( @@ -977,6 +974,9 @@ def validate_project(project_path: Path) -> list[ValidationError]: ) ) continue + if not name: + errors.append(ValidationError("error", src_path, "model missing 'name'")) + continue if name in model_names: errors.append( @@ -1222,19 +1222,19 @@ def validate_project(project_path: Path) -> list[ValidationError]: for i, view in enumerate(views): src_dir = view.get("_source_dir", f"views[{i}]") name = view.get("name") - if not name: + if isinstance(name, (list, dict)): errors.append( ValidationError( - "error", f"views/{src_dir}/metadata.yml", "view missing 'name'" + "error", + f"views/{src_dir}/metadata.yml", + f"view 'name' must be a scalar value, got {type(name).__name__}", ) ) continue - if isinstance(name, (list, dict)): + if not name: errors.append( ValidationError( - "error", - f"views/{src_dir}/metadata.yml", - f"view 'name' must be a scalar value, got {type(name).__name__}", + "error", f"views/{src_dir}/metadata.yml", "view missing 'name'" ) ) continue diff --git a/core/wren/tests/unit/test_context.py b/core/wren/tests/unit/test_context.py index 00a00fc43c..0d20b35155 100644 --- a/core/wren/tests/unit/test_context.py +++ b/core/wren/tests/unit/test_context.py @@ -594,6 +594,36 @@ def test_validate_model_name_int_is_not_rejected(tmp_path): assert not any("must be a scalar value" in e.message for e in errors) +def test_validate_model_name_empty_list_reports_scalar_error_not_missing(tmp_path): + # An empty list is falsy, so the type guard must run before the missing-name + # check or this reports "missing 'name'" instead of the malformed type. + _make_v2_project(tmp_path) + d = tmp_path / "models" / "orders" + d.mkdir(parents=True) + (d / "metadata.yml").write_text( + "name: []\ntable_reference:\n table: orders\ncolumns: []\n" + ) + errors = validate_project(tmp_path) + assert any( + "model 'name' must be a scalar value, got list" in e.message for e in errors + ) + assert not any("model missing 'name'" in e.message for e in errors) + + +def test_validate_model_name_empty_dict_reports_scalar_error_not_missing(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "models" / "orders" + d.mkdir(parents=True) + (d / "metadata.yml").write_text( + "name: {}\ntable_reference:\n table: orders\ncolumns: []\n" + ) + errors = validate_project(tmp_path) + assert any( + "model 'name' must be a scalar value, got dict" in e.message for e in errors + ) + assert not any("model missing 'name'" in e.message for e in errors) + + def test_validate_both_tref_and_ref_sql(tmp_path): _make_v2_project(tmp_path) d = tmp_path / "models" / "conflict" @@ -743,6 +773,32 @@ def test_validate_view_name_dict_reports_error(tmp_path): ) +def test_validate_view_name_empty_list_reports_scalar_error_not_missing(tmp_path): + # An empty list is falsy, so the type guard must run before the missing-name + # check or this reports "missing 'name'" instead of the malformed type. + _make_v2_project(tmp_path) + d = tmp_path / "views" / "monthly" + d.mkdir(parents=True) + (d / "metadata.yml").write_text("name: []\nstatement: SELECT 1\n") + errors = validate_project(tmp_path) + assert any( + "view 'name' must be a scalar value, got list" in e.message for e in errors + ) + assert not any("view missing 'name'" in e.message for e in errors) + + +def test_validate_view_name_empty_dict_reports_scalar_error_not_missing(tmp_path): + _make_v2_project(tmp_path) + d = tmp_path / "views" / "monthly" + d.mkdir(parents=True) + (d / "metadata.yml").write_text("name: {}\nstatement: SELECT 1\n") + errors = validate_project(tmp_path) + assert any( + "view 'name' must be a scalar value, got dict" in e.message for e in errors + ) + assert not any("view missing 'name'" in e.message for e in errors) + + def test_validate_missing_join_type(tmp_path): _make_valid_project(tmp_path) (tmp_path / "relationships.yml").write_text(