fix(memory): skip non-dict models/columns in seed query generation#2542
fix(memory): skip non-dict models/columns in seed query generation#2542Bartok9 wants to merge 3 commits into
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSeed query generation now validates manifest models, relationships, and columns before processing them. Invalid entries are skipped, malformed collections are treated as empty, and unit coverage verifies valid model seeds remain generated. ChangesSeed query robustness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/wren/src/wren/memory/seed_queries.py (2)
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant dictionary fallbacks.
The
or []fallback and[]default argument are unnecessary because the subsequentisinstance(rels, list)checks naturally evaluate toFalseforNoneor other non-list types, safely bypassing the logic.
core/wren/src/wren/memory/seed_queries.py#L55-L56: simplify torels = manifest.get("relationships")core/wren/src/wren/memory/seed_queries.py#L177-L179: simplify torels = manifest.get("relationships")🤖 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/seed_queries.py` around lines 55 - 56, Simplify both relationship lookups in core/wren/src/wren/memory/seed_queries.py at lines 55-56 and 177-179 by removing the redundant [] fallbacks and assigning manifest.get("relationships") directly; leave the subsequent isinstance(rels, list) checks unchanged so non-list values bypass processing safely.
34-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtract valid models to avoid redundant iteration and strictly check for string names.
Iterating over
modelsonce to extract a list ofvalid_modelssimplifies the logic and removes duplicated checks.Additionally, verifying that the model
nameis a string provides complete safety. The currentis not Nonecheck allows non-string values (like numbers or booleans) through, which would still result in a runtimeAttributeErrorwhen_norm_identsubsequently calls.strip(). Finally, theor []default fallback can be safely omitted since theisinstancecheck naturally handlesNone.♻️ Proposed refactor
- models = manifest.get("models", []) or [] - if not isinstance(models, list): - models = [] - model_layers = { - model["name"]: _prop_value(model, "dbtLayer", "dbt_layer") - for model in models - if isinstance(model, dict) and model.get("name") is not None - } - relationship_keys = _relationship_key_columns(manifest) - - for model in models: - if not isinstance(model, dict) or model.get("name") is None: - continue + models = manifest.get("models") + if not isinstance(models, list): + models = [] + + valid_models = [ + model for model in models + if isinstance(model, dict) and isinstance(model.get("name"), str) + ] + + model_layers = { + model["name"]: _prop_value(model, "dbtLayer", "dbt_layer") + for model in valid_models + } + relationship_keys = _relationship_key_columns(manifest) + + for model in valid_models:🤖 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/seed_queries.py` around lines 34 - 46, In the model-processing flow, replace the separate model-layer comprehension and later validation loop with one extraction of valid_models. Treat manifest["models"] as invalid unless it is a list, without relying on an or [] fallback, and retain only dictionary entries whose name is a string. Use valid_models for both model_layers construction and subsequent processing so validation is not duplicated and _norm_ident cannot receive non-string names.
🤖 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.
Inline comments:
In `@core/wren/src/wren/memory/seed_queries.py`:
- Around line 71-74: Update the column normalization logic to use the raw value
from model.get("columns", []) without the redundant “or []” fallback, retain the
list-type guard, and filter entries only when c.get("name") is a string.
Preserve the existing dictionary validation while preventing malformed
non-string names from reaching _norm_ident.
---
Nitpick comments:
In `@core/wren/src/wren/memory/seed_queries.py`:
- Around line 55-56: Simplify both relationship lookups in
core/wren/src/wren/memory/seed_queries.py at lines 55-56 and 177-179 by removing
the redundant [] fallbacks and assigning manifest.get("relationships") directly;
leave the subsequent isinstance(rels, list) checks unchanged so non-list values
bypass processing safely.
- Around line 34-46: In the model-processing flow, replace the separate
model-layer comprehension and later validation loop with one extraction of
valid_models. Treat manifest["models"] as invalid unless it is a list, without
relying on an or [] fallback, and retain only dictionary entries whose name is a
string. Use valid_models for both model_layers construction and subsequent
processing so validation is not duplicated and _norm_ident cannot receive
non-string names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0abe0a7a-0752-4673-920c-6768fcfe35b3
📒 Files selected for processing (2)
core/wren/src/wren/memory/seed_queries.pycore/wren/tests/unit/test_seed_queries_nonduct.py
Guards against malformed column names (int/bool) that pass 'is not None' but fail on _norm_ident .strip(). Addresses CodeRabbit review on Canner#2542.
1 participant
Summary
generate_seed_queriesskips non-dict models/relationships._model_seedsfilters non-dict / nameless columns before readingcol["name"].License
Apache-2.0 (
core/wren).Verification
cd core/wren && .venv/bin/python -m pytest tests/unit/test_seed_queries_nonduct.py -qTest plan
Summary by CodeRabbit