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
32 changes: 25 additions & 7 deletions core/wren/src/wren/memory/schema_indexer.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,16 @@ def describe_schema(manifest: dict) -> str:
lines.append("")

for model in manifest.get("models", []):
_describe_model(model, lines)
if isinstance(model, dict) and model.get("name"):
_describe_model(model, lines)

for rel in manifest.get("relationships", []):
_describe_relationship(rel, lines)
if isinstance(rel, dict):
_describe_relationship(rel, lines)
Comment on lines 57 to +59

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Skip nameless relationships and views in both paths.

These guards reject non-dictionaries but still pass {} or {"name": ""} to the relationship/view helpers and record builders, contrary to the PR’s stated contract. Require a truthy name here, and add regression cases for nameless relationships/views.

Proposed fix
     for rel in manifest.get("relationships", []):
-        if isinstance(rel, dict):
+        if isinstance(rel, dict) and rel.get("name"):
             _describe_relationship(rel, lines)

     for rel in manifest.get("relationships", []):
-        if not isinstance(rel, dict):
+        if not isinstance(rel, dict) or not rel.get("name"):
             continue

     for view in manifest.get("views", []):
-        if not isinstance(view, dict):
+        if not isinstance(view, dict) or not view.get("name"):
             continue

Also applies to: 247-255

🤖 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/memory/schema_indexer.py` around lines 57 - 59, Update the
relationship and view processing paths to require each dictionary entry to
contain a truthy name before invoking _describe_relationship, the corresponding
view helper, or record builders. Skip empty dictionaries and entries with blank
or missing names while preserving existing handling for valid named entries. Add
regression coverage for nameless relationships and views.


for view in manifest.get("views", []):
_describe_view(view, lines)
if isinstance(view, dict) and view.get("name"):
_describe_view(view, lines)

cubes = manifest.get("cubes", []) or []
if isinstance(cubes, list):
Expand Down Expand Up @@ -87,7 +90,9 @@ def _describe_model(model: dict, lines: list[str]) -> None:
if data_scope:
lines.append(f" Data scope: {data_scope}")

cols = model.get("columns", [])
cols = [
c for c in (model.get("columns") or []) if isinstance(c, dict) and c.get("name")
]
if cols:
lines.append(" Columns:")
for col in cols:
Expand All @@ -96,7 +101,9 @@ def _describe_model(model: dict, lines: list[str]) -> None:


def _describe_column(col: dict, lines: list[str]) -> None:
name = col["name"]
name = col.get("name")
if not name:
return
dtype = col.get("type", "?")
parts = [f" - {name} ({dtype})"]

Expand Down Expand Up @@ -229,14 +236,22 @@ def extract_schema_items(manifest: dict) -> list[dict]:
items: list[dict] = []

for model in manifest.get("models", []):
if not isinstance(model, dict) or not model.get("name"):
continue
items.append(_model_record(model, mdl_h, now))
for col in model.get("columns", []):
for col in model.get("columns", []) or []:
if not isinstance(col, dict) or not col.get("name"):
continue
items.append(_column_record(col, model["name"], mdl_h, now))

for rel in manifest.get("relationships", []):
if not isinstance(rel, dict):
continue
items.append(_relationship_record(rel, mdl_h, now))

for view in manifest.get("views", []):
if not isinstance(view, dict):
continue
items.append(_view_record(view, mdl_h, now))

cubes = manifest.get("cubes", []) or []
Expand Down Expand Up @@ -264,7 +279,9 @@ def extract_schema_items(manifest: dict) -> list[dict]:

def _model_record(model: dict, mdl_h: str, now: datetime) -> dict:
name = model["name"]
cols = model.get("columns", [])
cols = [
c for c in (model.get("columns") or []) if isinstance(c, dict) and c.get("name")
]
col_summaries = ", ".join(f"{c['name']} ({c.get('type', '?')})" for c in cols[:20])
pk = model.get("primaryKey") or ""

Expand Down Expand Up @@ -297,6 +314,7 @@ def _model_record(model: dict, mdl_h: str, now: datetime) -> dict:


def _column_record(col: dict, model_name: str, mdl_h: str, now: datetime) -> dict:
# Caller (extract_schema_items) already guarantees a truthy name.
name = col["name"]
dtype = col.get("type", "")
expr = col.get("expression") or None
Expand Down
88 changes: 88 additions & 0 deletions core/wren/tests/unit/test_schema_indexer_malformed_columns.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
"""schema_indexer must skip non-dict / nameless columns."""

from __future__ import annotations

from wren.memory.schema_indexer import describe_schema, extract_schema_items


def test_describe_schema_skips_non_dict_columns():
text = describe_schema(
{
"models": [
{
"name": "orders",
"columns": [None, {"name": "amount", "type": "int"}, {"type": "x"}],
}
]
}
)
assert "amount" in text
# Exactly one column entry rendered (the malformed ones are dropped).
assert text.count(" - ") == 1


def test_describe_schema_omits_dangling_columns_header():
# When every column is malformed, no dangling " Columns:" header (589e0bb).
text = describe_schema(
{"models": [{"name": "orders", "columns": [None, {"type": "x"}]}]}
)
assert "Columns:" not in text
assert "### Model: orders" in text


def test_describe_schema_skips_empty_string_name_and_null_columns():
text = describe_schema(
{
"models": [
{"name": "a", "columns": [{"name": "", "type": "int"}]},
{"name": "b", "columns": None},
]
}
)
assert "### Model: a" in text
assert "### Model: b" in text
assert " - " not in text


def test_describe_schema_skips_null_model_relationship_view_slots():
# Null slots in hand-edited manifests must not crash.
text = describe_schema({"models": [None], "relationships": [None], "views": [None]})
assert isinstance(text, str)


def test_extract_schema_items_skips_null_model_slots():
items = extract_schema_items({"models": [None, {"columns": []}]})
assert items == []


def test_extract_schema_items_skips_scalar_and_empty_name_columns():
items = extract_schema_items(
{
"models": [
{
"name": "orders",
"columns": ["amount", {"name": "", "type": "int"}],
}
]
}
)
col_items = [i for i in items if i["item_type"] == "column"]
assert col_items == []


def test_extract_schema_items_skips_non_dict_columns():
items = extract_schema_items(
{
"models": [
{
"name": "orders",
"columns": [None, {"name": "amount", "type": "int"}, {"type": "x"}],
}
]
}
)
col_items = [i for i in items if i["item_type"] == "column"]
assert len(col_items) == 1
assert col_items[0]["item_name"] == "amount"
model_items = [i for i in items if i["item_type"] == "model"]
assert "amount" in model_items[0]["text"]
Loading