rad-pat commented on PR #28627:
URL: https://github.com/apache/superset/pull/28627#issuecomment-5203948558
Rebased onto master and pushed — both outstanding review points are
addressed, and the branch is conflict-free again.
@rusackas — thanks for the nudge, and sorry it sat so long. Two things
resolved themselves on the rebase: master is **already** on
`databend-sqlalchemy>=0.5.5`, so the `pyproject.toml` bump this PR carried is
simply gone, and I confirmed `sslmode` is still the parameter the current
driver reads. Master had also merged `DatabendConnectEngineSpec` into
`DatabendEngineSpec` since; the changes moved across cleanly.
The migration you asked for is in. Digging into it turned up something I
hadn't expected — connections with *no* TLS parameter need migrating too, not
just `secure=*` ones. `databend-py` defaulted to an `http` scheme and Superset
only ever wrote `secure=true`, so every connection saved with the encryption
box unticked has no parameter at all and was plaintext; the Rust core defaults
to `https` and breaks them. Details in the thread above.
@betodealmeida — `encryption_disable_parameters` is on
`BasicParametersMixin` as you asked, defaulting to `{}` so nothing else changes
behaviour, and I've answered my own question from that thread (short version:
no inverse boolean is needed — the disable set only ever needs *stripping* on
the read path).
I also fixed some pre-existing faults in the two methods this PR touches,
which is the bulk of the extra diff. `Database.parameters` was returning `{}`
for every Databend connection — `get_parameters_from_uri` took `*_args` and so
raised `TypeError` on the keyword call, and it mutated an `immutabledict`; both
were swallowed by that property's bare `except`. Net effect: you could create a
Databend connection through the dynamic form but reopening it showed an empty
form, and the legacy-`secure` fallback was unreachable. Also `sslmode=enable`
read as unencrypted (the driver accepts it alongside `require`), which meant a
save could silently rewrite a TLS connection to `sslmode=disable`. Happy to
split those into a separate PR if you'd prefer them reviewed apart from the
`secure`→`sslmode` change — say the word.
@hantmac — you approved this a while back; the scope has grown since, so
it's worth another look from a Databend angle if you have time, particularly
the migration's treatment of parameter-less URIs.
Tests: unit coverage added for the build/parse round trip, the legacy
fallback, the `encrypted_extra` keyword regression, the mixin's opt-out
parameters, and the migration's upgrade/downgrade. There were none for these
paths before, which is how all of the above survived.
--
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]
