fix(dbt): skip unusable models/columns in seed payloads - #2549
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthroughdbt seed-query generation now validates model, relationship, column, and property inputs. Invalid payloads are skipped, malformed properties are normalized, and tests cover helper guards plus filtered query-pair generation. Changesdbt seed payload guards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/wren/tests/unit/test_dbt_seed_payload_guards.py (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type ignore for intentionally malformed test data.
Other tests explicitly suppress static typing warnings when intentionally bypassing the function signatures with non-dictionary data. Passing
Noneand"bad"inside these lists will likely trigger a mypy error against thelist[dict[str, Any]]arguments expected by_build_dbt_query_pairs.🔕 Proposed fix to silence mypy
pairs = _build_dbt_query_pairs( - [None, {"name": "ok", "columns": []}, "bad"], - [None, {"name": "r1"}], + [None, {"name": "ok", "columns": []}, "bad"], # type: ignore[list-item] + [None, {"name": "r1"}], # type: ignore[list-item] datasource="postgres", )🤖 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/tests/unit/test_dbt_seed_payload_guards.py` around lines 50 - 54, Add a targeted type-ignore annotation to the _build_dbt_query_pairs test call for the intentionally malformed None and string entries, matching the suppression style used by nearby tests while preserving the invalid payloads needed by this guard test.core/wren/src/wren/dbt.py (1)
1278-1305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant dictionary checks for properties.
Since
_camelize_propshas been hardened to acceptdict | Noneand already returns{}when the input is not a dictionary (lines 1319-1321), the inline validation forpropsandcol_propsis redundant and can be safely removed.♻️ Proposed refactor
def _seed_model_payload(model: dict[str, Any]) -> dict[str, Any] | None: """Build seed payload for one imported model, or None if unusable.""" if not isinstance(model, dict) or not model.get("name"): return None - props = model.get("properties") or {} - if not isinstance(props, dict): - props = {} columns_out: list[dict[str, Any]] = [] for column in model.get("columns") or []: if not isinstance(column, dict) or not column.get("name"): continue - col_props = column.get("properties") or {} - if not isinstance(col_props, dict): - col_props = {} columns_out.append( { "name": column["name"], "type": column.get("type"), "isCalculated": column.get("is_calculated", False), - "properties": _camelize_props(col_props), + "properties": _camelize_props(column.get("properties")), } ) return { "name": model["name"], "primaryKey": model.get("primary_key"), - "properties": _camelize_props(props), + "properties": _camelize_props(model.get("properties")), "columns": columns_out, }🤖 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/dbt.py` around lines 1278 - 1305, Remove the redundant dictionary type checks and fallback assignments for props and col_props in _seed_model_payload; pass model.get("properties") and column.get("properties") directly to _camelize_props, relying on its existing dict-or-None handling while preserving the current payload behavior.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@core/wren/src/wren/dbt.py`:
- Around line 1278-1305: Remove the redundant dictionary type checks and
fallback assignments for props and col_props in _seed_model_payload; pass
model.get("properties") and column.get("properties") directly to
_camelize_props, relying on its existing dict-or-None handling while preserving
the current payload behavior.
In `@core/wren/tests/unit/test_dbt_seed_payload_guards.py`:
- Around line 50-54: Add a targeted type-ignore annotation to the
_build_dbt_query_pairs test call for the intentionally malformed None and string
entries, matching the suppression style used by nearby tests while preserving
the invalid payloads needed by this guard test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2f7b5aa-ebd9-4ddb-a490-a245076a112f
📒 Files selected for processing (2)
core/wren/src/wren/dbt.pycore/wren/tests/unit/test_dbt_seed_payload_guards.py
_seed_model_payload assumed every column was a dict with name; junk import rows crashed dbt→seed-query generation. Filter and camelize safely.
12abbed to
3cb64a0
Compare
|
Suggesting we close this one — and here the guard doesn't just fail to fire, it would hide a bug if it ever did. Both inputs are produced by our own code, not by dbt. At the only callsite ( (imported_models, ...) = _build_imported_models(artifacts)
relationships, test_events = _apply_dbt_test_enrichment(artifacts, imported_models)
query_pairs = _build_dbt_query_pairs(imported_models, relationships, datasource=...)The dbt artifacts are user input, yes — but And the failure mode gets worse, not better. Today a nameless model raises If there's an actual reproduction where |
|
Good point — |
2 participants
Summary
_seed_model_payload/_build_dbt_query_pairsassumed dict models and named columns.TypeError/KeyErrorduring seed pair generation.Nonefor unusable models/relationships, skip bad columns, harden_camelize_props.Verification
Apache-2.0:
core/wren/**only.Summary by CodeRabbit