fix(types): coerce non-str inputs in parse_type

[Bartok9]

Summary

• Coerce None → empty string and non-str raw types via str(...) in parse_type
• Also tolerate bad dialect values; catch TypeError from sqlglot

License

Apache-2.0 (core/**).

Motivation

Dynamic schema exporters sometimes emit null/numeric type placeholders. parse_type assumed a string and crashed start-up mapping paths.

Verification

cd core/wren && .venv/bin/python -m pytest tests/unit/test_type_mapping.py::test_parse_type_none_and_non_str tests/unit/test_type_mapping.py::test_parse_types_none_type_field -v — 2 passed

Duplicate check

• No open PR on type_mapping.parse_type non-str coercion

Summary by CodeRabbit

• Bug Fixes

  • Improved type parsing to handle missing and non-text inputs more safely.
  • Added defensive handling for invalid or empty dialect values during parsing.
  • Expanded failure handling to prevent unexpected crashes and keep a usable fallback value.

• Tests

  • Added unit tests covering None and non-string inputs for type parsing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 486f7bc8-9339-421d-95fc-621ef8809f19

📥 Commits

Reviewing files that changed from the base of the PR and between 4d157df and 85cc7df.

📒 Files selected for processing (1)

• core/wren/tests/unit/test_type_mapping.py

🚧 Files skipped from review as they are similar to previous changes (1)

• core/wren/tests/unit/test_type_mapping.py

---

Walkthrough

parse_type now normalizes None and non-string inputs, handles invalid dialect values, catches TypeError, and preserves fallback behavior. Unit tests cover direct parsing and parse_types rows with None type fields.

Changes

Type parsing

Layer / File(s)	Summary
Defensive parsing and validation core/wren/src/wren/type_mapping.py, core/wren/tests/unit/test_type_mapping.py	parse_type normalizes inputs and dialects, catches additional parsing failures, and adds coverage for None and non-string values in parse_type and parse_types.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Canner/WrenAI#2410: Adds related cross-dialect type translation helpers that rely on the same parsing behavior.
• Canner/WrenAI#2433: Updates parse_type error handling and related tests for malformed inputs.
• Canner/WrenAI#2523: Broadens type parsing exception handling and preserves safe string fallbacks.

Suggested reviewers: goldmedal

Poem

I’m a rabbit guarding types with care,
Turning odd inputs into strings fair.
Empty fields now softly parse,
Tests hop along the fallback path.
None finds a safe burrow at last.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: making parse_type coerce non-string inputs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

core/wren/src/wren/type_mapping.py (2)

41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the type annotations with the accepted inputs.

parse_type now intentionally accepts None and arbitrary non-string values, but its signature remains str, forcing callers and tests to use type: ignore. Update the public annotations and docstring to reflect the defensive boundary.

Suggested annotation update

-def parse_type(type_str: str, dialect: str) -> str:
+def parse_type(type_str: object, dialect: object) -> str:

🤖 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/type_mapping.py` around lines 41 - 48, Update the public
parse_type annotations to accept None and arbitrary non-string type_str values,
and adjust its docstring to document this defensive coercion behavior; keep the
existing normalization logic unchanged.

---

47-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for invalid dialects.

The implementation adds dialect normalization and TypeError fallback, but the supplied tests cover only type values. Add a test for non-string and invalid string dialects, verifying the no-raise/fallback contract against sqlglot 29.0.0.

🤖 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/type_mapping.py` around lines 47 - 53, Add regression
tests for the type-mapping function around dialect handling, covering both
non-string dialect values and invalid string dialects. Verify against sqlglot
29.0.0 that each case does not raise and returns the expected fallback/result,
while preserving the existing type-value test coverage.

🤖 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/tests/unit/test_type_mapping.py`:
- Around line 383-388: Update test_parse_type_none_and_non_str to assert that
parse_type(38, "postgres") returns the exact string "38" instead of only
checking truthiness, while retaining the existing type assertion.

---

Nitpick comments:
In `@core/wren/src/wren/type_mapping.py`:
- Around line 41-48: Update the public parse_type annotations to accept None and
arbitrary non-string type_str values, and adjust its docstring to document this
defensive coercion behavior; keep the existing normalization logic unchanged.
- Around line 47-53: Add regression tests for the type-mapping function around
dialect handling, covering both non-string dialect values and invalid string
dialects. Verify against sqlglot 29.0.0 that each case does not raise and
returns the expected fallback/result, while preserving the existing type-value
test coverage.

🪄 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 Plus

Run ID: 6167a64a-b7aa-4962-be95-e02f74768329

📥 Commits

Reviewing files that changed from the base of the PR and between d472877 and 4d157df.

📒 Files selected for processing (2)

• core/wren/src/wren/type_mapping.py
• core/wren/tests/unit/test_type_mapping.py

[Bartok9]

Good call — tightened the assertion to check the exact stringified fallback (parse_type(38, "postgres") == "38") instead of just truthiness/isinstance. Pushed.

[goldmedal]

Closing this as-is — one half is arguable, the other is an unrelated behavior change.

The type_str half: coercing a None raw_type to "" is defensible, since parse_types is fed by catalog introspection and dbt manifests. But note it contradicts the documented contract two lines above — "Falls back to original string if parsing fails" — and "" for a column type propagates a silently wrong type into the project rather than surfacing the bad metadata. If you want this, please update the docstring and say in the description which exporter produced the None.

The dialect half is the blocker:

if not isinstance(dialect, str) or not dialect:
    dialect = ""

This silently swaps the SQL dialect for sqlglot's default whenever a caller passes something unexpected, which changes how types parse rather than guarding against a crash — and it isn't mentioned in the title or description. A wrong-dialect parse that returns a plausible-looking type is much harder to debug than a TypeError.

Adding TypeError to the except tuple is fine on its own.

If you'd like to pursue the type_str guard, please open a PR limited to that, with the docstring updated to match. I'd leave dialect alone.

[Bartok9]

Agreed on all three points. The dialect guard is the real problem — silently swapping to sqlglot's default is worse than a clean TypeError, and it wasn't in scope. Closing this. I'll open a focused follow-up limited to the type_str/raw_type None coercion, with the docstring updated to match and the producing exporter named in the description — and leave dialect alone. Thanks for the careful read.