diff --git a/core/wren/src/wren/connector/clickhouse.py b/core/wren/src/wren/connector/clickhouse.py index ad872dc83a..1e8310a7e6 100644 --- a/core/wren/src/wren/connector/clickhouse.py +++ b/core/wren/src/wren/connector/clickhouse.py @@ -13,7 +13,7 @@ import re from decimal import Decimal as PyDecimal from typing import Any -from urllib.parse import parse_qsl, unquote_plus, urlparse +from urllib.parse import parse_qsl, unquote, urlparse import pyarrow as pa import sqlglot @@ -258,9 +258,12 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict: if statement_timeout is not None: settings["max_execution_time"] = int(statement_timeout) - # urlparse leaves percent-encoded characters in userinfo, so decode - # them before clickhouse-connect sees the credentials. Matches the - # mssql / postgres URL handling elsewhere in this package. + # urlparse leaves percent-encoded characters in userinfo/path, so + # decode them before clickhouse-connect sees the credentials. Use + # ``unquote`` (not ``unquote_plus``): in a URL, ``+`` is only a space + # inside a query string, NOT in userinfo or the path — so a literal + # ``+`` in a password (e.g. ``pw+1``) must be preserved. This matches + # the mssql / mysql / oracle / trino URL handling in this package. secure = parsed.scheme == "clickhouse+https" # Pick the port-less default from the scheme: ClickHouse listens for # HTTPS on 8443 and plaintext HTTP on 8123. Defaulting an https URL to @@ -270,14 +273,12 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict: out: dict = { "host": parsed.hostname, "port": int(parsed.port) if parsed.port else default_port, - "username": ( - unquote_plus(parsed.username) if parsed.username else "default" - ), - "password": unquote_plus(parsed.password) if parsed.password else "", + "username": (unquote(parsed.username) if parsed.username else "default"), + "password": unquote(parsed.password) if parsed.password else "", "settings": settings, } if parsed.path and parsed.path != "/": - out["database"] = parsed.path.lstrip("/") + out["database"] = unquote(parsed.path.lstrip("/")) if secure: out["secure"] = True # ``settings`` already popped above, so ``out["settings"]`` survives. diff --git a/core/wren/tests/unit/test_clickhouse_helpers.py b/core/wren/tests/unit/test_clickhouse_helpers.py index aecd3d7288..a32227fcb6 100644 --- a/core/wren/tests/unit/test_clickhouse_helpers.py +++ b/core/wren/tests/unit/test_clickhouse_helpers.py @@ -42,7 +42,7 @@ def __init__(self, url: str, **extras) -> None: def test_clickhouse_url_decodes_username_and_password() -> None: - """user / password from the URL must be unquote_plus'd. + """user / password / database from the URL must be percent-decoded. urlparse leaves ``%40`` (``@``) and ``%20`` (space) literal in the userinfo, which would otherwise reach ClickHouse verbatim and fail auth. @@ -60,6 +60,35 @@ def test_clickhouse_url_decodes_username_and_password() -> None: assert out["database"] == "analytics" +def test_clickhouse_url_preserves_literal_plus_in_credentials() -> None: + """A literal ``+`` in userinfo is a plus, not a space. + + Regression: the parser used ``unquote_plus``, which turned a literal + ``+`` in a password into a space and corrupted the credential. In a URL, + ``+`` only means space inside a query string — never in userinfo/path. + """ + info = _FakeConnInfoFromUrl( + "clickhouse://svc+etl:pw+1@clickhouse-host:9000/an+db" + ) + + out = _build_clickhouse_client_kwargs(info) + + assert out["username"] == "svc+etl" + assert out["password"] == "pw+1" + assert out["database"] == "an+db" + + +def test_clickhouse_url_decodes_percent_encoded_database() -> None: + """A percent-encoded database name in the path must be decoded too.""" + info = _FakeConnInfoFromUrl( + "clickhouse://clickhouse-host:9000/my%20analytics" + ) + + out = _build_clickhouse_client_kwargs(info) + + assert out["database"] == "my analytics" + + def test_clickhouse_url_defaults_username_when_omitted() -> None: """When the URL has no userinfo, the default ``"default"`` user wins.""" info = _FakeConnInfoFromUrl("clickhouse://clickhouse-host/analytics")