From 9bdae39c99a818faa91229bd4a3972c00643f041 Mon Sep 17 00:00:00 2001 From: Jax Liu Date: Fri, 7 Aug 2026 11:44:12 +0800 Subject: [PATCH] fix(context): filter non-mapping entries in _load_views_v1 (#2604) Co-authored-by: Claude Opus 5 (1M context) --- core/wren/src/wren/context.py | 37 +++++++++- core/wren/tests/unit/test_context.py | 105 +++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/core/wren/src/wren/context.py b/core/wren/src/wren/context.py index cf4aee13c..29caf9ee9 100644 --- a/core/wren/src/wren/context.py +++ b/core/wren/src/wren/context.py @@ -605,7 +605,13 @@ def _load_views_v1(project_path: Path) -> list[dict]: if not views_file.exists(): return [] data = yaml.safe_load(views_file.read_text(encoding="utf-8")) or {} - return data.get("views", []) if isinstance(data, dict) else [] + views = data.get("views") if isinstance(data, dict) else None + # A bare ``views:`` parses to None and means "no views", same as a missing + # key. Any other non-list value is malformed; return nothing here and let + # ``validate_project`` be the one to report it. + if not isinstance(views, list): + return [] + return [v for v in views if isinstance(v, dict)] def _load_views_v2(project_path: Path) -> list[dict]: @@ -1102,6 +1108,35 @@ def validate_project(project_path: Path) -> list[ValidationError]: ) ) + # v1 legacy views.yml may contain non-mapping entries (e.g. `- null`). + # load_views() silently drops those (matching the other loaders), but + # validate_project's job is to tell the user about hand-edited mistakes + # rather than let them vanish quietly, so re-check the raw entries here. + if sv == 1: + views_file = project_path / "views.yml" + if views_file.exists(): + raw = yaml.safe_load(views_file.read_text(encoding="utf-8")) or {} + raw_views = raw.get("views") if isinstance(raw, dict) else None + if raw_views is not None and not isinstance(raw_views, list): + # A bare ``views:`` (None) legitimately means "no views"; a + # scalar or mapping there does not. + errors.append( + ValidationError( + "error", + "views.yml > views", + f"'views' must be a list, got {type(raw_views).__name__}", + ) + ) + for i, v in enumerate(raw_views if isinstance(raw_views, list) else []): + if not isinstance(v, dict): + errors.append( + ValidationError( + "error", + f"views.yml > views[{i}]", + f"view entry must be a mapping, got {type(v).__name__}", + ) + ) + # Check views for i, view in enumerate(views): src_dir = view.get("_source_dir", f"views[{i}]") diff --git a/core/wren/tests/unit/test_context.py b/core/wren/tests/unit/test_context.py index 3bef80df5..851242957 100644 --- a/core/wren/tests/unit/test_context.py +++ b/core/wren/tests/unit/test_context.py @@ -1377,6 +1377,111 @@ def test_apply_upgrade_v1_to_v2(tmp_path): assert cubes[0]["name"] == "order_metrics" +# ── Regression: v1 views.yml with non-mapping entries (issue #2597) ──────── +# +# A hand-edited legacy views.yml can contain a non-mapping list entry (e.g. +# `- null`). _load_views_v1 must drop it, matching the other v1/v2 loaders, +# so every consumer below still works instead of crashing with a bare +# AttributeError — and validate_project must additionally report it rather +# than silently ignore it. + + +def _corrupt_v1_views_yml(tmp_path: Path) -> None: + """Overwrite views.yml with a non-mapping entry alongside a valid one.""" + (tmp_path / "views.yml").write_text( + 'views:\n - null\n - "junk"\n - name: summary\n statement: SELECT 1\n' + ) + + +def test_validate_project_reports_v1_views_yml_non_mapping_entries(tmp_path): + _make_v1_project(tmp_path) + _corrupt_v1_views_yml(tmp_path) + errors = validate_project(tmp_path) + hard = [e for e in errors if e.level == "error"] + # Both malformed entries are reported, each at its own index. + entry_errors = [e for e in hard if "must be a mapping" in e.message] + assert {e.path for e in entry_errors} == { + "views.yml > views[0]", + "views.yml > views[1]", + } + assert any("NoneType" in e.message for e in entry_errors) + assert any("str" in e.message for e in entry_errors) + # The well-formed sibling entry is unaffected. + assert not any("summary" in e.message for e in errors) + + +def test_validate_project_accepts_empty_v1_views_key(tmp_path): + """A bare ``views:`` means "no views" and must not be reported.""" + _make_v1_project(tmp_path) + (tmp_path / "views.yml").write_text("views:\n") + assert build_manifest(tmp_path)["views"] == [] + assert not [e for e in validate_project(tmp_path) if "views" in e.path] + + +@pytest.mark.parametrize( + ("views_yml", "expected_type"), + [ + ("views: junk\n", "str"), + ("views: 3\n", "int"), + ("views:\n a: 1\n", "dict"), + ], +) +def test_validate_project_reports_non_list_v1_views_container( + tmp_path, views_yml, expected_type +): + """A non-list under ``views:`` is malformed — report it once, don't crash.""" + _make_v1_project(tmp_path) + (tmp_path / "views.yml").write_text(views_yml) + + hard = [e for e in validate_project(tmp_path) if e.level == "error"] + container = [e for e in hard if e.path == "views.yml > views"] + assert len(container) == 1 + assert f"must be a list, got {expected_type}" in container[0].message + # Not additionally reported once per character/key of the container. + assert not [e for e in hard if "must be a mapping" in e.message] + + # Every consumer degrades to "no views" rather than raising. + assert build_manifest(tmp_path)["views"] == [] + assert build_json(tmp_path)["views"] == [] + plan = plan_upgrade(tmp_path, target_version=2) + assert not [f for f in plan.files_created if f.startswith("views/")] + apply_upgrade(tmp_path, plan) + assert not (tmp_path / "views").exists() + + +def test_build_manifest_drops_v1_views_yml_non_mapping_entries(tmp_path): + _make_v1_project(tmp_path) + _corrupt_v1_views_yml(tmp_path) + manifest = build_manifest(tmp_path) + assert [v["name"] for v in manifest["views"]] == ["summary"] + + +def test_build_json_does_not_crash_on_v1_views_yml_non_mapping_entries(tmp_path): + _make_v1_project(tmp_path) + _corrupt_v1_views_yml(tmp_path) + manifest = build_json(tmp_path) + assert [v["name"] for v in manifest["views"]] == ["summary"] + + +def test_plan_upgrade_v1_to_v2_does_not_crash_on_non_mapping_view(tmp_path): + _make_v1_project(tmp_path) + _corrupt_v1_views_yml(tmp_path) + result = plan_upgrade(tmp_path, target_version=2) + view_files = [f for f in result.files_created if f.startswith("views/")] + assert view_files == ["views/summary/metadata.yml"] + + +def test_apply_upgrade_v1_to_v2_does_not_crash_on_non_mapping_view(tmp_path): + _make_v1_project(tmp_path) + _corrupt_v1_views_yml(tmp_path) + result = plan_upgrade(tmp_path, target_version=2) + apply_upgrade(tmp_path, result) + assert (tmp_path / "views" / "summary" / "metadata.yml").exists() + assert not (tmp_path / "views.yml").exists() + # Only the well-formed view became a directory — no junk siblings. + assert [d.name for d in (tmp_path / "views").iterdir()] == ["summary"] + + def test_apply_upgrade_v2_to_v3(tmp_path): _make_v2_project(tmp_path) result = plan_upgrade(tmp_path, target_version=3)