Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions core/wren/src/wren/mdl/cte_rewriter.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,27 @@ def __init__(
self._model_cols: dict[str, list[str]] = {}

for model in self.manifest.get("models", []):
name = model["name"]
if not isinstance(model, dict):
continue
name = model.get("name")
if not isinstance(name, str) or not name:
continue
self.model_dict[name] = model
cols: dict[str, str] = {}
orig: dict[str, str] = {}
for col in model.get("columns", []):
raw_cols = model.get("columns", []) or []
if not isinstance(raw_cols, list):
raw_cols = []
for col in raw_cols:
if not isinstance(col, dict):
continue
if col.get("isHidden"):
continue
if col.get("relationship"):
continue
col_name = col["name"]
col_name = col.get("name")
if not isinstance(col_name, str) or not col_name:
continue
# Case-only collisions were already vetted by the pre-scan: on
# case-insensitive-column dialects they raised INVALID_MDL; on
# case-sensitive-column dialects they are kept distinct here and
Expand Down Expand Up @@ -180,9 +191,13 @@ def __init__(
# It is NOT expanded by wren-core — it becomes a CTE kept verbatim,
# preceded by model CTEs for the models it references. view_dict maps
# name → the view object so the statement can be emitted as-is.
self.view_dict: dict[str, dict] = {
view["name"]: view for view in self.manifest.get("views", [])
}
self.view_dict: dict[str, dict] = {}
for view in self.manifest.get("views", []) or []:
if not isinstance(view, dict):
continue
vname = view.get("name")
if isinstance(vname, str) and vname:
self.view_dict[vname] = view
self.view_names: set[str] = set(self.view_dict)

@staticmethod
Expand All @@ -192,14 +207,23 @@ def _iter_model_column_names(model: dict):
Mirrors the column filter used when populating the schema so the
case-collision pre-scan sees exactly the columns that get registered.
"""
for col in model.get("columns", []):
cols = model.get("columns", []) or []
if not isinstance(cols, list):
return
for col in cols:
if not isinstance(col, dict):
continue
if col.get("isHidden") or col.get("relationship"):
continue
yield col["name"]
col_name = col.get("name")
if isinstance(col_name, str) and col_name:
yield col_name

def _manifest_has_case_distinct_columns(self) -> bool:
"""True if any model has two visible columns differing only in case."""
for model in self.manifest.get("models", []):
if not isinstance(model, dict):
continue
Comment on lines 224 to +226

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Apply the valid-name filter to collision scans.

Initialization skips models whose name is missing, empty, or non-string, but the case-collision scans still process those dicts. A discarded model containing visible Year/year columns can therefore activate case-sensitive mode, or reach _raise_case_collision() and fail at model['name'] with KeyError.

Use the same name predicate in both scans and interpolate the validated local name in the error message.

Proposed fix
 for model in self.manifest.get("models", []):
     if not isinstance(model, dict):
         continue
+    model_name = model.get("name")
+    if not isinstance(model_name, str) or not model_name:
+        continue
-                            f"Model '{model['name']}' has columns that differ "
+                            f"Model '{model_name}' has columns that differ "

Also applies to: 242-244

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/src/wren/mdl/cte_rewriter.py` around lines 224 - 226, Update both
case-collision scans in the relevant rewriter logic to skip model dictionaries
unless their name is a non-empty string, matching the initialization filter.
Store the validated name locally and use it when calling or formatting
_raise_case_collision, avoiding direct model['name'] access for discarded
models.

seen: set[str] = set()
for col_name in self._iter_model_column_names(model):
low = col_name.lower()
Expand All @@ -216,6 +240,8 @@ def _raise_case_collision(self) -> None:
silently collide — and the backing database cannot represent them.
"""
for model in self.manifest.get("models", []):
if not isinstance(model, dict):
continue
seen: dict[str, str] = {}
for col_name in self._iter_model_column_names(model):
low = col_name.lower()
Expand Down
66 changes: 66 additions & 0 deletions core/wren/tests/unit/test_cte_rewriter_nonduct.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
"""CTERewriter must skip non-dict models/columns/views without TypeError."""

from __future__ import annotations

import base64
from types import SimpleNamespace

import orjson
import pytest

from wren.mdl.cte_rewriter import CTERewriter
from wren.model.data_source import DataSource

pytestmark = pytest.mark.unit


def _b64(manifest: dict) -> str:
return base64.b64encode(orjson.dumps(manifest)).decode()


def _rewriter(manifest: dict) -> CTERewriter:
session = SimpleNamespace()
return CTERewriter(_b64(manifest), session, DataSource.postgres, fallback=True)


def test_skips_nonduct_models_and_views() -> None:
manifest = {
"catalog": "wren",
"schema": "public",
"models": [
"bad",
None,
{
"name": "orders",
"columns": [
"x",
None,
{"name": "id", "type": "integer"},
{"name": "", "type": "integer"},
{"isHidden": True, "name": "secret"},
],
},
{"name": 123, "columns": []},
],
"views": [
"nope",
{"name": "v_ok", "statement": "SELECT 1"},
{"name": "", "statement": "SELECT 2"},
],
}
rw = _rewriter(manifest)
assert list(rw.model_dict) == ["orders"]
assert rw._model_cols["orders"] == ["id"]
assert list(rw.view_dict) == ["v_ok"]


def test_iter_model_column_names_tolerates_bad_columns() -> None:
names = list(
CTERewriter._iter_model_column_names(
{
"name": "t",
"columns": ["x", {"name": "a"}, {"name": "b", "isHidden": True}],
}
)
)
assert names == ["a"]
Loading