fix(trino): URL-decode credentials and identifiers from connection URL

[Bartok9]

Summary

• _parse_trino_url now unquote()s the username, password, catalog, and schema parsed out of the connection URL.
• Fixes auth/connection failures when a credential or identifier contains a reserved character that must be percent-encoded in the URL.

Motivation

_parse_trino_url (in core/wren/src/wren/connector/trino.py) parsed a trino://user:pass@host:port/catalog/schema URL and put parsed.username / parsed.password / the catalog+schema path parts straight into the connect kwargs.

urllib.parse.urlparse leaves percent-encoded characters in the userinfo and path. Credentials and identifiers can contain reserved characters (@, /, :, ?) that must be percent-encoded for the URL to parse — so a password like p@ss/word (...:p%40ss%2Fword@...) reached Trino as the literal p%40ss%2Fword, and a catalog named cat/alog (cat%2Falog) was mis-parsed.

The mssql, mysql, clickhouse, and oracle connectors already unquote() these components. This brings Trino in line. unquote (not unquote_plus) is used so a literal + in a credential is preserved.

Verification

Real output on current upstream/main (fix reverted, tests present):

E         - us@er
E         + us%40er
FAILED tests/connectors/test_trino.py::test_parse_trino_url_decodes_percent_encoded_credentials
1 failed, 4 passed

With the fix applied:

python3 -m pytest tests/connectors/test_trino.py -k parse_trino_url -q
5 passed, 66 deselected in 0.46s

• Two new regression tests (pure parser, no Trino container): one asserts percent-decoding of user/password/catalog/schema, one asserts a literal + is preserved.
• Path is Apache-2.0 (core/**).

Summary by CodeRabbit

• Bug Fixes
  • Improved Trino connection URL handling by percent-decoding credential and identifier components (including user, password, catalog/schema, and path).
  • Preserved literal + characters in Trino userinfo.
  • Kept existing URL validation and connection defaults unchanged.
• Tests
  • Added new unit-test coverage for Trino parsing helpers (data types, column building, trailing-semicolon stripping, and URL parsing/validation).
  • Added checks for install-hint errors when the Trino dependency is missing and ensured native connector import doesn’t load ibis.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 28dc6dd0-8ea8-45d5-85b9-62122ac0e3a2

📥 Commits

Reviewing files that changed from the base of the PR and between 93e8661 and e92d10e.

📒 Files selected for processing (1)

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

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

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

---

Walkthrough

The Trino connector now percent-decodes URL credentials and path identifiers while preserving literal + characters. New unit tests cover native parsing, conversion, validation, lazy imports, and import isolation; integration-test setup is reorganized.

Changes

Trino connector parsing and validation

Layer / File(s)	Summary
Percent-decode connection URL components core/wren/src/wren/connector/trino.py, core/wren/tests/unit/test_trino_parser.py	_parse_trino_url decodes username, password, catalog, and schema values with unquote; tests cover URL validation, decoding, and literal + preservation.
Validate native connector helpers core/wren/tests/unit/test_trino_parser.py	Unit tests cover Trino type parsing, column construction, trailing-semicolon handling, lazy import errors, and import isolation.
Organize integration test setup core/wren/tests/connectors/test_trino.py	Integration-test documentation and module-level Trino setup constants, markers, and imports are reorganized.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Canner/WrenAI#2434: Also updates connector URL parsing to decode percent-encoded components and tests literal + handling.

Suggested labels: ci

Suggested reviewers: goldmedal

Poem

A rabbit hops through Trino’s door,
Decoding paths as never before.
Types and maps pass every test,
While plus signs keep their cozy nest. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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: percent-decoding Trino URL credentials and identifiers.
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.}

[goldmedal]

The fix looks correct, but heads-up: the new regression tests don't run in CI.

wren-ci.yml only executes tests/unit/ (the test-unit job) and a connector matrix limited to postgres + mysql (test-connector). tests/connectors/test_trino.py is in neither, so no CI job runs these new tests — meaning a future re-break of the URL-decoding won't be caught automatically.

This is a pre-existing gap (the whole test_trino.py file, parser + integration, is un-run in CI), not something this PR introduced. But it does undercut the value of adding the regression tests. The parser tests are pure (no container), yet they can't simply move to tests/unit/ because the file's top-level from testcontainers.trino import TrinoContainer forces the extra even to collect them.

Cheapest durable fix: split the pure _parse_trino_url tests into a tests/unit/ module (no testcontainer import) so the test-unit job runs them. Not a blocker for this PR.

[Bartok9]

Good catch, thanks — done. I split the pure _parse_trino_url / _parse_trino_data_type / _build_trino_column / _strip_trailing_semicolon / lazy-import tests into a new tests/unit/test_trino_parser.py (no testcontainers.trino import), so the test-unit job now collects and runs the URL-decode regression coverage. The container-backed integration suite stays in tests/connectors/test_trino.py. Verified locally: 56 passed. Pushed in 93e8661.

[coderabbitai]

🧹 Nitpick comments (1)

core/wren/tests/unit/test_trino_parser.py (1)

235-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use monkeypatch for sys.modules mutations to ensure cleanup.

test_native_connector_does_not_import_ibis pops "ibis" and "wren.connector.trino" from sys.modules directly without monkeypatch, so these mutations persist after the test completes. If a later test in the same session depends on either module being cached, it could fail or experience altered behavior. Using monkeypatch.delitem (or monkeypatch.setitem with the original value) ensures automatic restoration.

♻️ Proposed refactor using monkeypatch

-def test_native_connector_does_not_import_ibis() -> None:
+def test_native_connector_does_not_import_ibis(monkeypatch) -> None:
     # Acceptance criterion: importing the native trino connector must not
     # pull ibis into sys.modules (the new module is independent of
     # ibis-framework[trino]).
     import sys  # noqa: PLC0415
 
-    sys.modules.pop("ibis", None)
-    sys.modules.pop("wren.connector.trino", None)
+    monkeypatch.delitem(sys.modules, "ibis", raising=False)
+    monkeypatch.delitem(sys.modules, "wren.connector.trino", raising=False)
     import wren.connector.trino  # noqa: F401, PLC0415
 
     assert "ibis" not in sys.modules

🤖 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_trino_parser.py` around lines 235 - 245, Update
test_native_connector_does_not_import_ibis to accept pytest’s monkeypatch
fixture and replace direct sys.modules.pop calls with monkeypatch.delitem(...,
raising=False), ensuring module entries are automatically restored after the
test.

🤖 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/tests/unit/test_trino_parser.py`:
- Around line 235-245: Update test_native_connector_does_not_import_ibis to
accept pytest’s monkeypatch fixture and replace direct sys.modules.pop calls
with monkeypatch.delitem(..., raising=False), ensuring module entries are
automatically restored after the test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 40f350d5-758d-4e04-9163-3b4a655509a1

📥 Commits

Reviewing files that changed from the base of the PR and between 2157cfd and 93e8661.

📒 Files selected for processing (2)

• core/wren/tests/connectors/test_trino.py
• core/wren/tests/unit/test_trino_parser.py

[goldmedal]

👍