rad-pat commented on code in PR #28627:
URL: https://github.com/apache/superset/pull/28627#discussion_r3728312100
##########
superset/db_engine_specs/databend.py:
##########
@@ -273,7 +276,10 @@ def get_parameters_from_uri(
) -> BasicParametersType:
url = make_url_safe(uri)
query = url.query
- if "secure" in query:
Review Comment:
You were right and my earlier reply was wrong, on two counts. Migration
added.
The `if/elif` I was leaning on only repopulates the **connection form** — it
never touches the connection. `DatabendDialect.create_connect_args` does
`parameters = dict(url.query)` and concatenates every parameter straight into
the DSN handed to the Rust client, so a stored `?secure=false` reaches the
driver verbatim.
Worse, that branch could never actually run. `get_parameters_from_uri` was
declared `(cls, uri, *_args)` while `Database.parameters` calls it as
`get_parameters_from_uri(masked_uri, encrypted_extra=...)` — `TypeError`,
swallowed by that property's bare `except` into `parameters = {}`. Even called
positionally it then did `url.query.pop(...)` on an `immutabledict`. So there
was no runtime fallback at all; both are fixed here with regression tests.
What the driver actually does with the stale parameter (`core/src/client.rs`
in `databendlabs/bendsql`):
```rust
let mut scheme = "https";
...
"sslmode" => match v.as_ref() {
"disable" => scheme = "http",
"require" | "enable" => scheme = "https",
_ => return Err(Error::BadArgument(format!("Invalid value for sslmode:
{v}"))),
},
...
_ => { session_state.set(k, v); } // unknown keys are not an error
```
So `secure=false` is absorbed as a session variable and the scheme defaults
to `https` — plaintext installs stop connecting, while `secure=true` survives
by luck.
One thing that surprised me while writing it: connections with **no** TLS
parameter need migrating too. `databend-py` took `secure=False` and set
`self.schema = "http"`, and Superset only ever wrote `secure=true` (never
`secure=false`), so every connection saved with the encryption box unticked has
no parameter at all and was plaintext. Under the new `https` default those
break in exactly the way this migration exists to prevent, so `upgrade()` pins
them to `sslmode=disable`. That isn't a TLS downgrade — there was never any TLS
on those rows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
