aminghadersohi commented on PR #45843:
URL: https://github.com/apache/superset/pull/45843#issuecomment-6495541145
Product acceptance at `3cf49d0292176998ec47d567deff7b02f921388e`, then at
`54f95ada005a48e5baa6ca86ea5d6f7501d8837c`. Local docker-compose-light env with
databricks-sqlalchemy 2.0.10 installed and a fake host and token. Legacy rows
were seeded at `f72b6e9c104a` and converted with `superset db upgrade`.
**Works**
- Upgrade converted the 4 `databricks+connector://` rows and left the native
Databricks and Postgres rows alone. The password ciphertext and `changed_on`
did not change, and the malformed `extra` stayed byte-identical.
- Downgrade restored all rows byte-identical to the pre-upgrade snapshot,
and re-upgrade matched the first run. A row saved after the upgrade kept its
URI on downgrade.
- Converted connections open in the native Databricks form with host, port,
catalog, schema, HTTP path and masked token (screenshots in the description).
At the merge base the same row opened a blank form.
- Export/import keeps the marker and the `databricks://` URI. A role with
read-only database access gets no `parameters` or `extra` from `GET
/api/v1/database/<id>` and gets 403 on `/connection`. Postgres save and Test
Connection are unchanged.
**Fixed in `54f95ada005a48e5baa6ca86ea5d6f7501d8837c`:** saving an unchanged
converted connection failed. The native form rebuilds
`…:443?catalog=…&http_path=…&schema=…`, while the stored URI is `…:443/default`
with the settings in `connect_args`. The credential-reuse checks treated that
as a destination change: validation tested the connection with the masked
token, and `PUT /api/v1/database/<id>` returned 422 ("would change the
connection's effective destination…") until the token was re-entered. Both
checks now compare the engine spec's effective destination. A changed
warehouse, host or `connect_args` still requires a new token.
- Regression tests (fail before, pass after):
`test_update_converted_databricks_connection_keeps_masked_token`,
`test_validate_converted_databricks_connection_token`,
`test_connection_identity_matches_legacy_settings_and_native_query`. Guard
cases that still reject or ignore the stored token:
`test_update_converted_databricks_http_path_change_requires_new_token` and the
other two `test_validate_converted_databricks_connection_token` cases.
Siblings checked:
- superset/commands/database/update.py:307 — fixed in
54f95ada005a48e5baa6ca86ea5d6f7501d8837c
- superset/commands/database/validate.py:136 — fixed in
54f95ada005a48e5baa6ca86ea5d6f7501d8837c
- superset/commands/database/test_connection.py:100 — not affected: it
compares the stored masked URI that the client sends back, not a URI rebuilt
from form fields
- superset/commands/database/importers/v1/utils.py:49 — not affected: the
import overwrite check keeps the stricter literal comparison and asks for the
password
In the env, the same save through `DatabasePutSchema` and
`UpdateDatabaseCommand` was rejected at 3cf49d02 and saved at 54f95ada, with
the token ciphertext unchanged.
**Seen during testing, not changed by this PR (both reproduce at the merge
base)**
- A URI that cannot be parsed (for example, a non-numeric port) still gets a
500 from `/connection`, and the modal shows an error panel instead of a form
(`Database.driver`).
- A row whose `extra` is not valid JSON makes `GET /api/v1/database/` return
400 (`Database.allows_cost_estimate`).
**Not verified:** real Databricks queries, SQL Lab and Test Connection
against a workspace.
--
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]
