Skip to content

fix(databricks): migrate legacy connector URIs on upgrade - #45843

Open
aminghadersohi wants to merge 8 commits into
masterfrom
fix-databricks-legacy-connector-uri-migration
Open

aminghadersohi wants to merge 8 commits into
masterfrom
fix-databricks-legacy-connector-uri-migration

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

SUMMARY

Automatically migrate saved databricks+connector:// URIs to the registered SQLAlchemy 2 databricks:// dialect when upgrading from 6.0. The URI suffix and separate encrypted credential columns remain untouched. Converted connections use the native Databricks form, including legacy HTTP path and catalog settings. Unavailable-driver editing falls back to the saved parameter schema or an editable URI form. Saving a converted connection compares the stored and rebuilt URIs by the engine spec's effective destination (legacy connect_args and the native URI query are merged the way adjust_engine_params merges them), so an unchanged connection keeps its stored token; a changed warehouse, host or connect_args still requires a new token.

Malformed extra must not block an entire metadata database upgrade. The migration therefore rewrites only the scheme for these rows and preserves their original text (including empty or null values). A small migration-owned table records each converted row's id, a SHA-256 fingerprint of its resulting URI/extra/modification timestamp, and whether the migration owns its JSON marker; it stores no copies of settings or credentials. Downgrade restores locally converted URIs only when the fingerprint still matches, removes owned markers, and drops the bookkeeping table. This also protects later edits that retain the native Databricks scheme. The existing JSON marker remains on parseable settings for export/import compatibility; using the same fingerprint bookkeeping for every converted row avoids separate rollback rules for valid and malformed settings. Malformed settings are retained, not repaired, and may still require correction before queries can run.

The export/import rollback behavior is accepted: an imported row carries the same databricks:// URI and its marker means “converted by this migration,” so it remains eligible for downgrade in the importing workspace.

Related independent migration: #44278

If both changes land, that migration must target databricks://, not databricks+connector://; this change does not depend on or modify it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshots

Before After
Edit an upgraded Databricks (legacy) connection: blank form before; native Databricks form with host, catalog, schema, masked token and HTTP path after
A connection still on the legacy scheme (for example from an older export) opens in its saved field form
Converted connection at 1366x768 (new)
New Databricks connection: field-based form (new)

Captured at 54f95ada. Local docker-compose-light env with databricks-sqlalchemy 2.0.10 installed. Legacy rows were seeded at the previous revision and converted with superset db upgrade. Host and token are fake.

TESTING INSTRUCTIONS

  1. Save a legacy connection with HTTP path/catalog settings in extra.engine_params.connect_args; save a second legacy row with malformed extra and a separate native connection.
  2. Run superset db upgrade. Both legacy URI schemes should change, the malformed text should remain identical, and the native row should remain untouched. Open the valid converted connection and inspect its host, HTTP path, catalog, schema and masked token.
  3. Edit one converted connection. Run superset db downgrade f72b6e9c104a: unchanged converted rows should return to the legacy scheme, while edited and unmarked native rows should keep their URIs.
  4. Run superset db upgrade again.

Validation at 9670e47dd08e791b39c041e4fdf534c7136b2046:

  • PYTHONPATH=superset-core/src:. SUPERSET_CONFIG=tests.integration_tests.superset_test_config python -m pytest tests/unit_tests/db_engine_specs/test_databricks.py tests/unit_tests/migrations/test_migrate_databricks_connector_uri.py tests/unit_tests/models/core_test.py tests/unit_tests/migrations/test_single_migration_head.py -q -m 'not manual and not llm_compliance': 273 passed, 1 skipped (109 engine-spec passes, 20 migration passes, 143 model passes, 1 migration-graph pass).
  • test_upgrade_preserves_invalid_extra_and_downgrades_only_unchanged_rows: nine cases exercise malformed JSON, blank/null values, non-object JSON, marker collisions and deep nesting, with later batches, native rows, deletion and subsequent edits. Together with test_downgrade_preserves_edits_to_converted_native_connection, the focused comparison against incoming commit 7a49c6ce5c980966b6c34ce64af44301e16d05e3 produced 12 failures, 1 pass; the passing malformed-after-edit case preserves an existing behavior.
  • test_converted_connection_extra_can_be_imported passes at 9670e47dd08e791b39c041e4fdf534c7136b2046 and fails when the import schema is restored to 490bc015cff1fe52b4927d5c228b11b29ba7d355.
  • test_downgrade_imported_marker_and_unmarked_native_rows pins the accepted imported-marker behavior, native-row exclusion and re-upgrade. test_upgrade_preserves_uri_suffix_and_encrypted_credentials, test_converted_connection_parameters_include_legacy_settings and test_native_form_save_preserves_masked_token_and_updates_legacy_settings exercise suffix/ciphertext preservation and native form parameters/token handling at 9670e47dd08e791b39c041e4fdf534c7136b2046.
  • Superset CLI db heads at 9670e47dd08e791b39c041e4fdf534c7136b2046 (invoked through python -c 'from superset.cli.main import superset; superset()' db heads with the same Python/config paths): b8f3a2c91d64 (head), one head.
  • Staged-file pre-commit run passed before committing; pre-commit run --files <all branch-changed paths> passed at 9670e47dd08e791b39c041e4fdf534c7136b2046, including mypy and frontend type checking.

Limitations at 9670e47dd08e791b39c041e4fdf534c7136b2046: the local DatabaseModal Jest invocation (JEST_MAX_WORKERS=2 npx --no-install jest src/features/databases/DatabaseModal/index.test.tsx --maxWorkers=2) returned 37 failed, 52 passed; failures include You must call SupersetClient.configure(...) before calling other methods. No frontend execution pass is claimed. Live Databricks queries, charts, SQL Lab execution, remote Test Connection and browser acceptance were not run in this follow-up. No real model-provider API was used. No Docker containers were created in this follow-up; the final databricks-uri-fix2- container-name search returned no matches.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (approval process: [SIP-59] Proposal for Database migration standards #13351 )
    • Migration is atomic, supports rollback & is backwards-compatible (data writes use Alembic's transaction; bookkeeping DDL follows the metadata backend's transaction semantics)
    • Confirm DB migration upgrade and downgrade tested: test_upgrade_preserves_invalid_extra_and_downgrades_only_unchanged_rows, test_upgrade_batches_and_downgrade_only_converted_rows, and test_downgrade_preserves_edits_to_converted_native_connection at 9670e47dd08e791b39c041e4fdf534c7136b2046.
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Runtime/downtime: O(number of saved connections), keyset batches of at most 1,000 rows, with no intermediate data commits. One small bookkeeping table is created and retained until downgrade; only converted rows get entries. Run within the normal upgrade maintenance window; larger installations depend on connection count and metadata database performance.

@netlify

netlify Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 066d556
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ae4704bac0c6dfcc6279cb0
😎 Deploy Preview https://deploy-preview-45843--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added risk:db-migration PRs that require a DB migration doc Namespace | Anything related to documentation labels Oct 29, 2026
@codecov

codecov Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.56604% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.78%. Comparing base (2c5c47e) to head (066d556).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
superset/db_engine_specs/databricks.py 90.69% 3 Missing and 1 partial ⚠️
superset/commands/database/validate.py 82.35% 2 Missing and 1 partial ⚠️
superset/commands/database/utils.py 81.81% 2 Missing ⚠️
superset/databases/schemas.py 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #45843      +/-   ##
==========================================
+ Coverage   84.72%   84.78%   +0.05%     
==========================================
  Files        3069     3069              
  Lines      205678   205768      +90     
  Branches    48350    48375      +25     
==========================================
+ Hits       174265   174453     +188     
+ Misses      28120    28012     -108     
- Partials     3293     3303      +10     
Flag Coverage Δ
hive 34.50% <23.71%> (-0.01%) ⬇️
javascript 81.05% <100.00%> (+0.06%) ⬆️
mysql 52.57% <41.23%> (-0.02%) ⬇️
postgres 52.57% <41.23%> (-0.02%) ⬇️
presto 36.23% <23.71%> (-0.02%) ⬇️
python 87.79% <89.69%> (+0.04%) ⬆️
sqlite 52.33% <34.02%> (-0.02%) ⬇️
unit 82.54% <89.69%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Connections converted by the b8f3a2c91d64 migration carry
migrated_from_databricks_connector_uri in extra. Exports include it,
and ImportV1DatabaseExtraSchema rejected it as an unknown field, so
re-importing a converted connection (or any bundle containing it) failed.
Reading Database.parameters with an unparseable saved URI raised
DatabaseInvalidError for every engine before this branch. Moving URI parsing
into get_parameters_from_database put it inside the broad exception handler,
so such URIs silently returned empty parameters. Validate the URI before the
handler so other engines keep their previous behavior.

Also remove the PR screenshots from the docs tree (the PR body links them at
a pinned commit) and correct the downgrade note in UPDATING.md.
…tion

A converted connection keeps its legacy settings in connect_args, while the
native form rebuilds them into the URI query. The credential-reuse checks
compared the two URI spellings literally, so saving an unchanged converted
connection was treated as a destination move: validation tested it with the
masked token and the update was rejected until the token was re-entered.

Compare the stored and submitted URIs by the engine spec's effective
destination instead. The Databricks spec merges connect_args and the URI
query the same way adjust_engine_params does; other engines are unchanged.
A changed warehouse, host or connect_args still requires a new token.
@aminghadersohi

Copy link
Copy Markdown
Contributor Author

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 54f95ad
  • superset/commands/database/validate.py:136 — fixed in 54f95ad
  • 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 3cf49d0 and saved at 54f95ad, 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.

@aminghadersohi
aminghadersohi marked this pull request as ready for review October 29, 2026 14:14
Comment thread superset/models/core.py Outdated
Comment thread superset-frontend/src/features/databases/DatabaseModal/index.tsx
Comment thread superset/commands/database/utils.py
Comment thread superset/db_engine_specs/databricks.py
@codeant-ai-for-open-source

codeant-ai-for-open-source Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

CodeAnt  CodeAnt PR Risk

🟢  Low Risk

  • The PR appears safe to merge; the reported connection-edit and migration concerns are addressed in the current code with targeted regression tests.

Assessed commit: 066d5561e2b2

@rebenitez1802 rebenitez1802 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the migration, fingerprint-based rollback and stored-token destination checks hold up (we tried 12 ways to redirect the masked token and found none), but two upgrade-path bugs hit the same rows this PR converts: legacy connections that set only http_path, which is the common 6.0 shape. Details and fixes are inline.

Comment thread superset/db_engine_specs/databricks.py Outdated
Comment thread tests/unit_tests/commands/databases/validate_test.py
@github-actions github-actions Bot added the api Related to the REST API label Oct 29, 2026
@github-actions github-actions Bot added the i18n Namespace | Anything related to localization label Oct 29, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API doc Namespace | Anything related to documentation i18n Namespace | Anything related to localization risk:db-migration PRs that require a DB migration size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants