fix(clickhouse): reconcile default port when secure is toggled via kwargs or query params (#2426)

core/wren/src/wren/connector/clickhouse.py

@@ -231,6 +231,19 @@ def _build_clickhouse_column(values: list, arrow_type: pa.DataType) -> pa.Array:
 # Client kwargs assembly
 # --------------------------------------------------------------------------
 
+_FALSY_SECURE_VALUES = frozenset({"", "0", "false", "no", "off"})
+
+
+def _parse_secure_flag(value: Any) -> bool:
+    """Interpret a ``secure`` override from kwargs or a URL query string.
+
+    Query parameters always arrive as strings, so ``?secure=false`` must not
+    be treated as truthy the way ``bool("false")`` would.
+    """
+    if isinstance(value, str):
+        return value.strip().lower() not in _FALSY_SECURE_VALUES
+    return bool(value)
+
 
 def _build_clickhouse_client_kwargs(connection_info: Any) -> dict:
     """Translate ``ClickHouseConnectionInfo`` / ``ConnectionUrl`` into
@@ -246,7 +259,17 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict:
                 "ClickHouse connection URL must use clickhouse:// scheme",
             )
 
-        kwargs: dict = dict(parse_qsl(parsed.query))
+        # ``keep_blank_values`` so a blank parameter (``?secure=``) still
+        # reaches the override handling below instead of being dropped —
+        # blank means "falsy", matching ``_parse_secure_flag``. Blank is only
+        # meaningful for the params handled below, though: any other blank
+        # param keeps the old dropped behaviour rather than leaking
+        # ``key=""`` into the client kwargs via ``out.update(kwargs)``.
+        kwargs: dict = {
+            key: value
+            for key, value in parse_qsl(parsed.query, keep_blank_values=True)
+            if value != "" or key in ("secure", "statement_timeout")
+        }
         info_kwargs = getattr(connection_info, "kwargs", None)
         if info_kwargs:
             kwargs.update(info_kwargs)
@@ -255,17 +278,27 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict:
             dict(kwargs.pop("settings", {})) if "settings" in kwargs else {}
         )
         statement_timeout = kwargs.pop("statement_timeout", None)
-        if statement_timeout is not None:
+        # Blank (``?statement_timeout=``) is treated the same as absent.
+        if statement_timeout not in (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.
         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
-        # 8123 silently dialed the plaintext port with ``secure=True``, so the
-        # TLS handshake hit a non-TLS listener and the connection failed.
+        # TLS can also be toggled *after* the scheme is inspected — via a
+        # ``?secure=...`` query parameter or a ``kwargs`` override. Reconcile
+        # the effective value here (kwargs win over query params, which win
+        # over the scheme, matching the merge above) so the port-less default
+        # tracks the real TLS setting instead of just the scheme.
+        secure_override = kwargs.pop("secure", None)
+        if secure_override is not None:
+            secure = _parse_secure_flag(secure_override)
+        # Pick the port-less default from the effective TLS setting: ClickHouse
+        # listens for HTTPS on 8443 and plaintext HTTP on 8123. Defaulting a
+        # secure connection to 8123 silently dialed the plaintext port with
+        # ``secure=True``, so the TLS handshake hit a non-TLS listener and the
+        # connection failed. An explicit URL port always wins.
         default_port = 8443 if secure else 8123
         out: dict = {
             "host": parsed.hostname,

core/wren/tests/connectors/test_clickhouse.py

@@ -289,45 +289,3 @@ class TestClickHouseClientKwargs:
         info = _FakeChInfo(kwargs={"statement_timeout": 10})
         out = _build_clickhouse_client_kwargs(info)
         assert out["settings"] == {"max_execution_time": 10}
-
-
-class _FakeChUrl:
-    """Stand-in exposing a ``connection_url`` for the URL kwargs path."""
-
-    def __init__(self, url: str) -> None:
-        from pydantic import SecretStr
-
-        self.connection_url = SecretStr(url)
-        self.kwargs = None
-
-
-@pytest.mark.clickhouse
-class TestClickHouseUrlKwargs:
-    """Pure-Python tests for the ``connection_url`` branch of the kwargs builder."""
-
-    def test_https_url_without_port_uses_secure_default_port(self) -> None:
-        """A port-less clickhouse+https URL must dial 8443 (TLS), not 8123.
-
-        ClickHouse serves HTTPS on 8443 and plaintext HTTP on 8123. Defaulting
-        an https URL to 8123 while also setting ``secure=True`` made the TLS
-        client connect to the plaintext listener — the handshake fails.
-        """
-        out = _build_clickhouse_client_kwargs(
-            _FakeChUrl("clickhouse+https://user:pw@host/db")
-        )
-        assert out["secure"] is True
-        assert out["port"] == 8443
-
-    def test_http_url_without_port_uses_plaintext_default_port(self) -> None:
-        out = _build_clickhouse_client_kwargs(
-            _FakeChUrl("clickhouse+http://user:pw@host/db")
-        )
-        assert "secure" not in out
-        assert out["port"] == 8123
-
-    def test_explicit_port_is_respected_for_https(self) -> None:
-        out = _build_clickhouse_client_kwargs(
-            _FakeChUrl("clickhouse+https://user:pw@host:9440/db")
-        )
-        assert out["secure"] is True
-        assert out["port"] == 9440

core/wren/tests/unit/test_clickhouse_helpers.py

@@ -228,3 +228,168 @@ def test_clickhouse_arrow_table_nullable_columns_preserve_none_values() -> None:
     assert table.column("id").to_pylist() == [1, None, 3]
     assert table.column("name").to_pylist() == ["Alice", "Bob", None]
     assert table.column("score").to_pylist() == [9.5, None, 7.0]
+
+
+# ---------------------------------------------------------------------------
+# 5. Default port tracks the effective ``secure`` value (connection_url branch)
+#
+# Regression for Canner/WrenAI#2412 / #2416: the port-less default must follow
+# the *effective* TLS setting (kwargs > query param > scheme), not the scheme
+# alone, and string overrides like ``?secure=false`` must not be truthy.
+# ---------------------------------------------------------------------------
+
+
+class TestClickHouseUrlKwargs:
+    """Pure-Python tests for the ``connection_url`` branch of the kwargs builder."""
+
+    def test_https_url_without_port_uses_secure_default_port(self) -> None:
+        """A port-less clickhouse+https URL must dial 8443 (TLS), not 8123.
+
+        ClickHouse serves HTTPS on 8443 and plaintext HTTP on 8123. Defaulting
+        an https URL to 8123 while also setting ``secure=True`` made the TLS
+        client connect to the plaintext listener — the handshake fails.
+        """
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db")
+        )
+        assert out["secure"] is True
+        assert out["port"] == 8443
+
+    def test_http_url_without_port_uses_plaintext_default_port(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse+http://user:pw@host/db")
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    def test_explicit_port_is_respected_for_https(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse+https://user:pw@host:9440/db")
+        )
+        assert out["secure"] is True
+        assert out["port"] == 9440
+
+    # --- secure enabled after the scheme is inspected (#2416) ---------------
+
+    def test_secure_query_param_uses_secure_default_port(self) -> None:
+        """``?secure=true`` on a plain scheme must dial 8443, not 8123.
+
+        The scheme alone said plaintext, so the old code picked 8123 — then
+        ``secure=True`` arrived via the query string and the TLS handshake
+        hit the plaintext listener.
+        """
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?secure=true")
+        )
+        assert out["secure"] is True
+        assert out["port"] == 8443
+
+    def test_secure_kwargs_override_uses_secure_default_port(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(
+                "clickhouse://user:pw@host/db", kwargs={"secure": True}
+            )
+        )
+        assert out["secure"] is True
+        assert out["port"] == 8443
+
+    def test_secure_false_query_param_uses_plaintext_default_port(self) -> None:
+        """``?secure=false`` must not be truthy just because it is a string."""
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db?secure=false")
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    def test_secure_false_kwargs_override_uses_plaintext_default_port(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(
+                "clickhouse+https://user:pw@host/db", kwargs={"secure": False}
+            )
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    def test_explicit_port_wins_over_secure_query_param(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse://user:pw@host:9000/db?secure=true")
+        )
+        assert out["secure"] is True
+        assert out["port"] == 9000
+
+    def test_explicit_port_wins_over_secure_false_override(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(
+                "clickhouse+https://user:pw@host:9440/db", kwargs={"secure": "false"}
+            )
+        )
+        assert "secure" not in out
+        assert out["port"] == 9440
+
+    def test_secure_kwargs_win_over_query_param(self) -> None:
+        """kwargs are merged after query params, so they take precedence."""
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(
+                "clickhouse://user:pw@host/db?secure=true", kwargs={"secure": False}
+            )
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    @pytest.mark.parametrize("raw", ["1", "true", "TRUE", "yes", "on"])
+    def test_truthy_secure_strings(self, raw: str) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(f"clickhouse://user:pw@host/db?secure={raw}")
+        )
+        assert out["secure"] is True
+        assert out["port"] == 8443
+
+    @pytest.mark.parametrize("raw", ["0", "false", "FALSE", "no", "off", ""])
+    def test_falsy_secure_strings(self, raw: str) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl(
+                "clickhouse+https://user:pw@host/db", kwargs={"secure": raw}
+            )
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    def test_blank_secure_query_param_is_a_falsy_override(self) -> None:
+        """``?secure=`` must reach the override handling, not be dropped.
+
+        ``parse_qsl`` discards blank values by default, which silently turned
+        ``?secure=`` into "unspecified". Blank is falsy, like everywhere else
+        in ``_parse_secure_flag``.
+        """
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db?secure=")
+        )
+        assert "secure" not in out
+        assert out["port"] == 8123
+
+    def test_blank_statement_timeout_query_param_is_ignored(self) -> None:
+        """``?statement_timeout=`` is treated as absent, not ``int("")``."""
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?statement_timeout=")
+        )
+        assert out["settings"] == {}
+        assert "statement_timeout" not in out
+
+    def test_blank_unhandled_query_param_is_dropped(self) -> None:
+        """``?connect_timeout=`` must not reach the client kwargs as ``""``.
+
+        ``keep_blank_values`` exists so ``?secure=`` / ``?statement_timeout=``
+        can act as explicit "unset" overrides; every other blank param keeps
+        the old dropped behaviour instead of leaking ``key=""`` into
+        ``clickhouse_connect.get_client``.
+        """
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?connect_timeout=")
+        )
+        assert "connect_timeout" not in out
+
+    def test_non_blank_query_param_passes_through(self) -> None:
+        out = _build_clickhouse_client_kwargs(
+            _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?connect_timeout=10")
+        )
+        assert out["connect_timeout"] == "10"