From d23460893dfbda63097a5da889432230208eca79 Mon Sep 17 00:00:00 2001 From: Bartok <259807879+Bartok9@users.noreply.github.com> Date: Sun, 9 Aug 2026 23:58:27 -0400 Subject: [PATCH] fix(context): harden relationships load + validate like views (#2613) Co-authored-by: Bartok9 --- core/wren/src/wren/context.py | 71 +++++++++++++++++++++++----- core/wren/tests/unit/test_context.py | 66 ++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 11 deletions(-) diff --git a/core/wren/src/wren/context.py b/core/wren/src/wren/context.py index 29caf9ee9..6b0bac8a1 100644 --- a/core/wren/src/wren/context.py +++ b/core/wren/src/wren/context.py @@ -703,12 +703,21 @@ def _cube_migration_target(cube: dict, source_file: str | None) -> tuple[str, st def load_relationships(project_path: Path) -> list[dict]: - """Load relationships from project_path/relationships.yml.""" + """Load relationships from project_path/relationships.yml. + + Non-list ``relationships`` values and non-mapping entries are dropped here + (matching ``_load_views_v1``). ``validate_project`` re-reads the raw YAML + so hand-edited mistakes are still reported rather than vanishing quietly. + """ rel_file = project_path / "relationships.yml" if not rel_file.exists(): return [] data = yaml.safe_load(rel_file.read_text(encoding="utf-8")) or {} - return data.get("relationships", []) if isinstance(data, dict) else [] + rels = data.get("relationships") if isinstance(data, dict) else None + # A bare ``relationships:`` parses to None and means "no relationships". + if not isinstance(rels, list): + return [] + return [r for r in rels if isinstance(r, dict)] def load_instructions(project_path: Path) -> str | None: @@ -1137,6 +1146,46 @@ def validate_project(project_path: Path) -> list[ValidationError]: ) ) + # relationships.yml may contain non-mapping entries (e.g. `- null`). + # load_relationships() silently drops those (matching the other loaders), + # but validate_project reports hand-edited mistakes rather than letting + # them vanish quietly — re-check the raw entries here. + # Also remember the raw list so the field checks below can keep file indices. + raw_relationships_list: list | None = None + rel_file = project_path / "relationships.yml" + if rel_file.exists(): + raw = yaml.safe_load(rel_file.read_text(encoding="utf-8")) or {} + if raw and not isinstance(raw, dict): + # Most likely hand-edit: bare list / scalar root (omitted `relationships:` key). + errors.append( + ValidationError( + "error", + "relationships.yml", + "relationships.yml must be a mapping with a 'relationships' key, " + f"got {type(raw).__name__}", + ) + ) + raw_rels = raw.get("relationships") if isinstance(raw, dict) else None + if isinstance(raw_rels, list): + raw_relationships_list = raw_rels + if raw_rels is not None and not isinstance(raw_rels, list): + errors.append( + ValidationError( + "error", + "relationships.yml > relationships", + f"'relationships' must be a list, got {type(raw_rels).__name__}", + ) + ) + for i, r in enumerate(raw_rels if isinstance(raw_rels, list) else []): + if not isinstance(r, dict): + errors.append( + ValidationError( + "error", + f"relationships.yml > relationships[{i}]", + f"relationship entry must be a mapping, got {type(r).__name__}", + ) + ) + # Check views for i, view in enumerate(views): src_dir = view.get("_source_dir", f"views[{i}]") @@ -1183,17 +1232,17 @@ def validate_project(project_path: Path) -> list[ValidationError]: ) ) - # Check relationships + # Check relationships — walk the raw list when available so indices match + # the file (filtered loader positions would renumber past dropped junk). all_entity_names = model_names | view_names - for i, rel in enumerate(relationships): + rel_entries = ( + list(enumerate(raw_relationships_list)) + if raw_relationships_list is not None + else list(enumerate(relationships)) + ) + for i, rel in rel_entries: if not isinstance(rel, dict): - errors.append( - ValidationError( - "error", - f"relationships[{i}]", - "relationship entry must be an object", - ) - ) + # Non-mappings already reported from the raw pass above. continue rel_name = rel.get("name", f"relationships[{i}]") ref_models = rel.get("models") or [] diff --git a/core/wren/tests/unit/test_context.py b/core/wren/tests/unit/test_context.py index 851242957..4277387ae 100644 --- a/core/wren/tests/unit/test_context.py +++ b/core/wren/tests/unit/test_context.py @@ -1733,3 +1733,69 @@ def test_validate_manifest_invalid_datasource(): manifest = {**_SEM_BASE_MANIFEST, "views": [_VALID_VIEW]} result = validate_manifest(_b64(manifest), "not-a-datasource") assert len(result["errors"]) == 1 + + +def test_load_relationships_filters_non_dict_entries(tmp_path: Path) -> None: + (tmp_path / "relationships.yml").write_text( + "relationships:\n - not-a-mapping\n - 42\n - name: ok\n models: [a, b]\n join_type: MANY_TO_ONE\n condition: a.id = b.id\n", + encoding="utf-8", + ) + rels = load_relationships(tmp_path) + assert len(rels) == 1 + assert rels[0]["name"] == "ok" + + +def test_validate_project_reports_non_dict_relationship_entries(tmp_path: Path) -> None: + # Minimal project scaffold for validate_project + (tmp_path / "wren_project.yml").write_text("schema_version: 1\n", encoding="utf-8") + (tmp_path / "relationships.yml").write_text( + "relationships:\n - not-a-mapping\n - 42\n", + encoding="utf-8", + ) + errors = validate_project(tmp_path) + msgs = [e.message for e in errors] + assert any("relationship entry must be a mapping, got str" in m for m in msgs) + assert any("relationship entry must be a mapping, got int" in m for m in msgs) + + +def test_validate_project_reports_relationships_not_list(tmp_path: Path) -> None: + (tmp_path / "wren_project.yml").write_text("schema_version: 1\n", encoding="utf-8") + (tmp_path / "relationships.yml").write_text("relationships: nope\n", encoding="utf-8") + errors = validate_project(tmp_path) + msgs = [e.message for e in errors] + assert any("'relationships' must be a list, got str" in m for m in msgs) + + +def test_validate_project_reports_relationships_bare_root(tmp_path: Path) -> None: + (tmp_path / "wren_project.yml").write_text("schema_version: 1\n", encoding="utf-8") + (tmp_path / "relationships.yml").write_text( + "- name: ok\n models: [a, b]\n join_type: MANY_TO_ONE\n condition: a.id = b.id\n", + encoding="utf-8", + ) + errors = validate_project(tmp_path) + msgs = [e.message for e in errors] + assert any( + "relationships.yml must be a mapping with a 'relationships' key, got list" in m + for m in msgs + ) + + +def test_validate_project_relationship_indices_match_file(tmp_path: Path) -> None: + """Junk at [0] must not renumber a later unnamed relationship's warnings.""" + (tmp_path / "wren_project.yml").write_text("schema_version: 1\n", encoding="utf-8") + (tmp_path / "relationships.yml").write_text( + "relationships:\n" + " - 42\n" + " - models: [a, b]\n" + " condition: a.id = b.id\n", + encoding="utf-8", + ) + errors = validate_project(tmp_path) + diagnostics = [ + f"{getattr(e, 'path', '')} {e.message}" for e in errors + ] + assert any("got int" in diagnostic for diagnostic in diagnostics) + assert any( + "relationships[1]" in diagnostic and "join_type" in diagnostic + for diagnostic in diagnostics + )