fix(clickhouse): sanitize brackets in connection-URL userinfo before parse - #2454
Conversation
…parse clickhouse:// URLs with '[' or ']' in credentials raised ValueError from urlparse's IPv6 netloc check, so such connections could never be opened. Mirror the MySQL connector's userinfo sanitiser so bracketed credentials parse while real IPv6 hosts still work.
|
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 with no reviewable changes (1)
WalkthroughAdds a ClickHouse connection URL helper that percent-encodes brackets in userinfo before parsing, updates client-kwargs construction to use it, and adds unit tests for bracketed passwords and IPv6 host literals. ChangesClickHouse URL parsing fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/wren/tests/unit/test_clickhouse_helpers.py (1)
398-414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider placing new tests inside
TestClickHouseUrlKwargsfor consistency.The two new test functions are module-level, while all other URL-parsing tests in this file are methods of the
TestClickHouseUrlKwargsclass (line 242). Moving them into the class would keep the test organization consistent and allow them to be collected under the same test class in reports.🤖 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_clickhouse_helpers.py` around lines 398 - 414, The two new ClickHouse URL parsing tests are currently defined at module scope, but the rest of the related URL kwargs tests live inside TestClickHouseUrlKwargs. Move the new bracketed-password cases into the TestClickHouseUrlKwargs class and keep using _FakeConnInfoFromUrl and _build_clickhouse_client_kwargs so the test grouping and reporting stay consistent with the existing suite.core/wren/src/wren/connector/clickhouse.py (1)
248-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting bracket sanitization into a shared utility.
The docstring notes this "Mirrors the MySQL connector helper," indicating the same userinfo-bracket-escaping logic exists in MySQL, MSSQL, and Oracle connectors. Extracting this into a shared helper (e.g.,
wren.connector._url_utils.sanitize_userinfo_brackets) would eliminate duplication and ensure fixes propagate to all connectors automatically.🤖 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/connector/clickhouse.py` around lines 248 - 278, The ClickHouse URL parser currently duplicates the userinfo bracket-escaping logic already used by other connectors, so extract that behavior into a shared URL helper and reuse it here. Move the sanitization from _parse_clickhouse_connection_url into a common utility such as sanitize_userinfo_brackets in a shared module, then call that helper from ClickHouse and the other connector parsers that mirror this behavior. Keep the existing parsing flow in _parse_clickhouse_connection_url intact, but replace the inline replace logic with the shared helper so future fixes apply consistently across all connectors.
🤖 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/connector/clickhouse.py`:
- Around line 248-278: The ClickHouse URL parser currently duplicates the
userinfo bracket-escaping logic already used by other connectors, so extract
that behavior into a shared URL helper and reuse it here. Move the sanitization
from _parse_clickhouse_connection_url into a common utility such as
sanitize_userinfo_brackets in a shared module, then call that helper from
ClickHouse and the other connector parsers that mirror this behavior. Keep the
existing parsing flow in _parse_clickhouse_connection_url intact, but replace
the inline replace logic with the shared helper so future fixes apply
consistently across all connectors.
In `@core/wren/tests/unit/test_clickhouse_helpers.py`:
- Around line 398-414: The two new ClickHouse URL parsing tests are currently
defined at module scope, but the rest of the related URL kwargs tests live
inside TestClickHouseUrlKwargs. Move the new bracketed-password cases into the
TestClickHouseUrlKwargs class and keep using _FakeConnInfoFromUrl and
_build_clickhouse_client_kwargs so the test grouping and reporting stay
consistent with the existing suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57415b41-a808-4224-b2da-3f4e3094adfc
📒 Files selected for processing (2)
core/wren/src/wren/connector/clickhouse.pycore/wren/tests/unit/test_clickhouse_helpers.py
2 participants
Summary
[/]in ClickHouse connection-URL userinfo beforeurlparse.Motivation
urlparsetreats[/]anywhere in the netloc as IPv6 delimiters. A password likep[a]sstherefore raisesValueError: Invalid IPv6 URLand the connector never opens. Same root cause already fixed for MySQL / MSSQL / Oracle on this tree — ClickHouse was still on bareurlparse.Verification
PYTHONPATH=src pytest tests/unit/test_clickhouse_helpers.py→ 50 passedtest_clickhouse_url_allows_raw_brackets_in_passwordtest_clickhouse_url_preserves_ipv6_host_with_bracketed_passwordclickhouse://user:p[a]ss@host:9000/db→ValueError: Invalid IPv6 URLuser, passwordp[a]ss, host intact;clickhouse://user:p[a]ss@[::1]:9000/dbkeeps host::1.%-encoded characters,unquote_pluscredential handling, secure-port defaults, or query wrapping.License
core/wren/**only (Apache-2.0).Summary by CodeRabbit
[or], preventing URL parsing failures.