opensource/WrenAI
3
0
Fork 0
mirror of https://github.com/Canner/WrenAI.git synced 2026-08-30 18:00:36 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(context): normalise model columns in load_models (#2614)

Browse Source 
Co-authored-by: Jax Liu <liugs963@gmail.com>
This commit is contained in:
Bartok
2026-08-21 02:40:44 -04:00
committed by GitHub
parent ce9513d68a
commit d48498fd55
4 changed files with 414 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/wren/src/wren/context.py
+144 -19
View File
@@ -526,6 +526,26 @@ def require_schema_version(project_path: Path) -> int:
# ── Loaders (all return snake_case dicts) ─────────────────────────────────
def _normalize_model_columns(model: dict) -> dict:
"""Normalise YAML-sourced ``columns`` to ``list[dict]``.
Hand-edited project YAML can set ``columns:`` to a scalar or mix bare
strings into the list. Loaders drop malformed entries (matching views/
relationships); ``validate_project`` re-reads the raw file to report them.
Omit the key when absent so ``target/mdl.json`` does not gain an empty
``columns: []`` solely from loader defaults.
"""
if "columns" not in model:
return model
raw = model.get("columns")
if not isinstance(raw, list):
model["columns"] = []
return model
model["columns"] = [c for c in raw if isinstance(c, dict)]
return model
def load_models(project_path: Path) -> list[dict]:
"""Load models — dispatches on schema_version.
@@ -549,7 +569,7 @@ def _load_models_v1(project_path: Path) -> list[dict]:
data = yaml.safe_load(f.read_text(encoding="utf-8"))
if isinstance(data, dict):
data["_source_dir"] = f.stem
models.append(data)
models.append(_normalize_model_columns(data))
return models
@@ -582,7 +602,7 @@ def _load_models_v2(project_path: Path) -> list[dict]:
if sql_content:
model["ref_sql"] = sql_content
models.append(model)
models.append(_normalize_model_columns(model))
return models
@@ -960,10 +980,76 @@ def validate_project(project_path: Path) -> list[ValidationError]:
model_names: set[str] = set()
view_names: set[str] = set()
# Hand-edited model YAML may set columns: to a non-list (e.g. a bare
# string). load_models() normalises to list[dict], but validate_project
# must still report the mistake rather than let it vanish quietly.
def _iter_raw_model_files() -> list[tuple[str, dict]]:
models_dir = project_path / "models"
if not models_dir.is_dir():
return []
out: list[tuple[str, dict]] = []
if sv == 1:
for f in sorted(models_dir.glob("*.yml")):
data = yaml.safe_load(f.read_text(encoding="utf-8")) or {}
if isinstance(data, dict):
out.append((f"models/{f.name}", data))
else:
for d in sorted(models_dir.iterdir()):
if not d.is_dir():
continue
meta = d / "metadata.yml"
if not meta.exists():
continue
data = yaml.safe_load(meta.read_text(encoding="utf-8")) or {}
if isinstance(data, dict):
out.append((f"models/{d.name}/metadata.yml", data))
return out
# source key (_source_dir) -> raw columns list (file order) for
# index-stable diagnostics. Key by source identity, not model name —
# duplicate names are reachable and must not cross-wire column lists.
raw_model_columns: dict[str, list] = {}
for src_path, raw_model in _iter_raw_model_files():
if "columns" not in raw_model:
continue
raw_cols = raw_model.get("columns")
# v2 path is models/<dir>/metadata.yml — prefer directory name over
# stem "metadata" when the YAML omits `name`.
if src_path.endswith("/metadata.yml"):
source_key = Path(src_path).parent.name
mname = raw_model.get("name") or source_key
else:
source_key = Path(src_path).stem
mname = raw_model.get("name") or source_key
if not isinstance(raw_cols, list):
errors.append(
ValidationError(
"error",
f"{src_path} > {mname} > columns",
f"must be a list, got {type(raw_cols).__name__}",
)
)
continue
raw_model_columns[source_key] = raw_cols
for j, col in enumerate(raw_cols):
if not isinstance(col, dict):
errors.append(
ValidationError(
"error",
f"{src_path} > {mname} > columns[{j}]",
"column entry must be an object",
)
)
# Check models
for i, model in enumerate(models):
src = model.get("_source_dir", f"models[{i}]")
src_path = f"models/{src}/metadata.yml"
# v1 is flat models/<stem>.yml; v2+ is models/<dir>/metadata.yml.
# Keep labels consistent with the raw re-read paths above.
if sv == 1:
src_path = f"models/{src}.yml"
else:
src_path = f"models/{src}/metadata.yml"
name = model.get("name")
if not name:
errors.append(ValidationError("error", src_path, "model missing 'name'"))
@@ -1005,14 +1091,8 @@ def validate_project(project_path: Path) -> list[ValidationError]:
)
)
# columns shape is owned by load_models + the raw re-read above.
columns = model.get("columns", [])
if not isinstance(columns, list):
errors.append(
ValidationError(
"error", f"{src_path} > {name}", "columns must be a list"
)
)
columns = []
if not columns:
errors.append(
ValidationError(
@@ -1020,16 +1100,19 @@ def validate_project(project_path: Path) -> list[ValidationError]:
)
)
# Walk the raw columns list when available so indices match the file
# (filtered loader positions would renumber past dropped junk).
# Keyed by _source_dir so duplicate model names cannot cross-wire.
raw_cols_for_model = raw_model_columns.get(src)
col_entries = (
list(enumerate(raw_cols_for_model))
if raw_cols_for_model is not None
else list(enumerate(columns))
)
col_names = set()
for j, col in enumerate(columns):
for j, col in col_entries:
if not isinstance(col, dict):
errors.append(
ValidationError(
"error",
f"{src_path} > {name} > columns[{j}]",
"column entry must be an object",
)
)
# Non-mappings already reported from the raw re-read above.
continue
col_name = col.get("name")
if not col_name:
@@ -1126,7 +1209,8 @@ def validate_project(project_path: Path) -> list[ValidationError]:
"properties must be a mapping",
)
)
for j, col in enumerate(columns):
# Prefer raw-file indices for properties diagnostics too (same list).
for j, col in col_entries:
if not isinstance(col, dict):
continue
col_props = col.get("properties")
@@ -1573,12 +1657,53 @@ def plan_upgrade(
)
def _reject_malformed_v1_model_columns(project_path: Path) -> None:
"""Abort v1→v2 upgrade if any model YAML has malformed ``columns``.
Loader normalisation is correct for validation/runtime, but migration
deletes the source file after writing the normalised model. Require a
clean columns shape up front so content is never silently discarded.
"""
models_dir = project_path / "models"
if not models_dir.is_dir():
return
problems: list[str] = []
for f in sorted(models_dir.glob("*.yml")):
data = yaml.safe_load(f.read_text(encoding="utf-8")) or {}
if not isinstance(data, dict) or "columns" not in data:
continue
raw_cols = data.get("columns")
src = f"models/{f.name}"
name = data.get("name") or f.stem
if not isinstance(raw_cols, list):
problems.append(
f"{src} > {name} > columns: must be a list, "
f"got {type(raw_cols).__name__}"
)
continue
for j, col in enumerate(raw_cols):
if not isinstance(col, dict):
problems.append(
f"{src} > {name} > columns[{j}]: column entry must be an object"
)
if problems:
detail = "; ".join(problems)
raise UpgradeError(
"Cannot upgrade: malformed model columns would be discarded — " + detail
)
def _plan_v1_to_v2(project_path: Path) -> tuple[list[str], list[str]]:
"""Plan the v1→v2 file restructuring. Returns (files_created, files_deleted)."""
created: list[str] = []
deleted: list[str] = []
project_root = project_path.resolve()
# Reject malformed model columns before any write: load_models normalises
# (drops non-list / non-dict entries), and _apply_v1_to_v2 then unlinks the
# v1 source. Abort in preflight so upgrade cannot silently discard data.
_reject_malformed_v1_model_columns(project_path)
# Models: flat files → directories
models = _load_models_v1(project_path)
for model in models:
core/wren/src/wren/genbi/composer.py
+9 -2
View File
@@ -52,12 +52,19 @@ def _data_mode_guidance(data_mode: str) -> str:
def _format_model_inventory(models: list[dict]) -> str:
"""One markdown bullet per model with its column names."""
"""One markdown bullet per model with its column names.
Callers pass models from ``load_models`` (``columns`` already
``list[dict]``). Coerce each ``name`` with ``str(...)`` so a non-string
YAML name (e.g. ``name: 3``) cannot crash ``", ".join``.
"""
if not models:
return "- (no models found — run `wren context build` first)"
lines = []
for model in models:
cols = ", ".join(c.get("name", "?") for c in model.get("columns", []))
raw_cols = model.get("columns") or []
names = [str(c.get("name", "?")) for c in raw_cols]
cols = ", ".join(names)
lines.append(f"- **{model.get('name', '?')}**: {cols}")
return "\n".join(lines)
core/wren/tests/unit/test_context.py
+234 -11
View File
@@ -1391,9 +1391,7 @@ def test_plan_upgrade_v1_to_v2_requires_portable_path_component(tmp_path, entity
@pytest.mark.parametrize("statement", [42, 0])
def test_plan_upgrade_v1_to_v2_rejects_non_string_view_statement(
tmp_path, statement
):
def test_plan_upgrade_v1_to_v2_rejects_non_string_view_statement(tmp_path, statement):
_make_v1_project(tmp_path)
_set_v1_view_statement(tmp_path, statement)
source_contents = _snapshot_v1_sources(tmp_path)
@@ -1605,6 +1603,234 @@ def test_validate_project_reports_non_list_v1_views_container(
assert not (tmp_path / "views").exists()
# ── Regression: model columns non-list / non-dict entries ─────────────────
def test_load_models_v2_normalises_non_list_columns(tmp_path):
"""columns: scalar is dropped to [] by the loader."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: orders\ntable_reference:\n table: orders\ncolumns: id, customer_id\n"
)
models = load_models(tmp_path)
assert len(models) == 1
assert models[0]["columns"] == []
def test_load_models_v2_omits_missing_columns_key(tmp_path):
"""Absent columns key stays absent (no empty list injection)."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text("name: orders\ntable_reference:\n table: orders\n")
models = load_models(tmp_path)
assert len(models) == 1
assert "columns" not in models[0]
def test_load_models_v2_drops_non_dict_column_entries(tmp_path):
"""Bare string / junk column entries are not coerced to column names."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "customers"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: customers\n"
"table_reference:\n table: customers\n"
"columns:\n"
" - name: id\n type: INTEGER\n"
" - bare\n"
" - 3\n"
)
models = load_models(tmp_path)
assert len(models) == 1
assert models[0]["columns"] == [{"name": "id", "type": "INTEGER"}]
def test_validate_project_reports_non_list_model_columns(tmp_path):
"""validate_project reports hand-edited non-list columns (loader already empty)."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: orders\ntable_reference:\n table: orders\ncolumns: id, customer_id\n"
)
errors = validate_project(tmp_path)
msgs = [f"{e.path}: {e.message}" for e in errors]
assert any("must be a list, got str" in m for m in msgs), msgs
assert any("models/orders/metadata.yml > orders > columns" in m for m in msgs), msgs
def test_validate_project_reports_null_model_columns(tmp_path):
"""Explicit `columns:` (YAML null) is present and non-list — report it."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: orders\ntable_reference:\n table: orders\ncolumns:\n"
)
errors = validate_project(tmp_path)
msgs = [f"{e.path}: {e.message}" for e in errors]
assert any("columns" in m and "must be a list, got NoneType" in m for m in msgs), (
msgs
)
def test_validate_project_reports_non_dict_column_entries(tmp_path):
"""Bare-string column entries must error, not vanish silently."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: orders\n"
"table_reference:\n table: orders\n"
"columns:\n"
" - name: id\n type: INTEGER\n"
" - bare_column\n"
)
errors = validate_project(tmp_path)
msgs = [f"{e.path}: {e.message}" for e in errors]
assert any(
"columns[1]" in m and "column entry must be an object" in m for m in msgs
), msgs
def test_validate_project_duplicate_model_names_do_not_crosswire_columns(tmp_path):
"""Duplicate model names must not share/steal each other's raw column lists."""
_make_v2_project(tmp_path)
for dirname, columns_yaml, clean in (
(
"a",
" - bare_junk\n - type: INTEGER\n - name: ok\n type: INT\n",
False,
),
(
"b",
" - name: only\n type: INT\n",
True,
),
):
d = tmp_path / "models" / dirname
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
f"name: orders\ntable_reference:\n table: orders\ncolumns:\n{columns_yaml}"
)
errors = validate_project(tmp_path)
diagnostics = [f"{e.path}: {e.message}" for e in errors]
assert any("duplicate model name" in d for d in diagnostics), diagnostics
# Real errors on a/ must remain.
assert any(
"models/a/metadata.yml" in d
and "columns[0]" in d
and "column entry must be an object" in d
for d in diagnostics
), diagnostics
assert any(
"models/a/metadata.yml" in d
and "columns[1]" in d
and "column missing 'name'" in d
for d in diagnostics
), diagnostics
# No phantom column errors against clean b/.
b_col_errs = [
d
for d in diagnostics
if "models/b/metadata.yml" in d
and ("column entry must be an object" in d or "column missing 'name'" in d)
]
assert b_col_errs == [], b_col_errs
def test_plan_upgrade_v1_to_v2_rejects_malformed_model_columns(tmp_path):
"""v1→v2 must abort before discarding non-list / non-object columns."""
_make_v1_project(tmp_path)
models_dir = tmp_path / "models"
(models_dir / "orders.yml").write_text(
"name: orders\ntable_reference:\n table: orders\ncolumns: id, customer_id\n",
encoding="utf-8",
)
(models_dir / "items.yml").write_text(
"name: items\n"
"table_reference:\n table: items\n"
"columns:\n"
" - name: id\n type: INTEGER\n"
" - bare_column\n",
encoding="utf-8",
)
source_contents = _snapshot_v1_sources(tmp_path)
from wren.context import UpgradeError as _UE # noqa: PLC0415
with pytest.raises(_UE, match="malformed model columns"):
plan_upgrade(tmp_path, target_version=2)
_assert_v1_sources_unchanged(tmp_path, source_contents)
# Source content still intact (not normalised away).
assert "id, customer_id" in (models_dir / "orders.yml").read_text(encoding="utf-8")
assert "bare_column" in (models_dir / "items.yml").read_text(encoding="utf-8")
def test_validate_project_column_indices_match_file(tmp_path):
"""Junk at [0] must not renumber a later unnamed column's error."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"name: orders\n"
"table_reference:\n table: orders\n"
"columns:\n"
" - bare_junk\n"
" - type: INTEGER\n"
" - name: amt\n type: DOUBLE\n"
)
errors = validate_project(tmp_path)
diagnostics = [f"{e.path}: {e.message}" for e in errors]
assert any(
"columns[0]" in d and "column entry must be an object" in d for d in diagnostics
), diagnostics
assert any(
"columns[1]" in d and "column missing 'name'" in d for d in diagnostics
), diagnostics
# Must not report missing-name against the renumbered filtered index 0.
missing_name_paths = [d for d in diagnostics if "column missing 'name'" in d]
assert all("columns[0]" not in d for d in missing_name_paths), missing_name_paths
def test_validate_project_v1_model_paths_use_flat_yml(tmp_path):
"""v1 errors should label models/<stem>.yml, not models/<stem>/metadata.yml."""
(tmp_path / "wren_project.yml").write_text("schema_version: 1\n", encoding="utf-8")
models_dir = tmp_path / "models"
models_dir.mkdir()
(models_dir / "orders.yml").write_text(
"name: orders\ncolumns: id, customer_id\n",
encoding="utf-8",
)
errors = validate_project(tmp_path)
diagnostics = [f"{e.path} {e.message}" for e in errors]
assert any(
"models/orders.yml > orders > columns" in d and "must be a list" in d
for d in diagnostics
), diagnostics
assert not any("models/orders/metadata.yml" in d for d in diagnostics), diagnostics
def test_validate_project_uses_dir_name_when_model_name_missing(tmp_path):
"""v2 error path should use models/<dir>, not stem 'metadata'."""
_make_v2_project(tmp_path)
d = tmp_path / "models" / "orders"
d.mkdir(parents=True)
(d / "metadata.yml").write_text(
"table_reference:\n table: orders\ncolumns: not-a-list\n"
)
errors = validate_project(tmp_path)
msgs = [f"{e.path}: {e.message}" for e in errors]
assert any(
"models/orders/metadata.yml > orders > columns" in m and "must be a list" in m
for m in msgs
), msgs
def test_build_manifest_drops_v1_views_yml_non_mapping_entries(tmp_path):
_make_v1_project(tmp_path)
_corrupt_v1_views_yml(tmp_path)
@@ -1991,7 +2217,9 @@ def test_validate_project_reports_non_dict_relationship_entries(tmp_path: Path)
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")
(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)
@@ -2015,16 +2243,11 @@ def test_validate_project_relationship_indices_match_file(tmp_path: Path) -> Non
"""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",
"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
]
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
core/wren/tests/unit/test_genbi_build.py
+27
View File
@@ -94,6 +94,33 @@ def test_build_includes_model_inventory(tmp_path: Path) -> None:
assert "duckdb" in result.output
def test_format_inventory_stringifies_non_string_column_names() -> None:
"""Non-string column names (YAML ``name: 3``) must not crash join."""
from wren.genbi.composer import _format_model_inventory
models = [{"name": "customers", "columns": [{"name": 3}, {"name": "id"}]}]
assert _format_model_inventory(models) == "- **customers**: 3, id"
def test_build_inventory_stringifies_non_string_column_names(tmp_path: Path) -> None:
"""compose_build_instruction survives non-string column names."""
from wren.genbi.composer import compose_build_instruction
models = [
{"name": "customers", "columns": [{"name": 3}, {"name": "id"}]},
]
text = compose_build_instruction(
app_name="demo",
data_mode="snapshot",
user_prompt="hi",
mdl_path=tmp_path / "mdl.json",
app_dir=tmp_path / "app",
models=models,
data_source="duckdb",
)
assert "- **customers**: 3, id" in text
def test_build_includes_wasm_wiring_and_final_steps(tmp_path: Path) -> None:
project = _make_project(tmp_path)