Repository navigation
fix(databricks): migrate legacy connector URIs on upgrade - #45843
aminghadersohi wants to merge 8 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
|
Product acceptance at Works
Fixed in
In the env, the same save through Seen during testing, not changed by this PR (both reproduce at the merge base)
Not verified: real Databricks queries, SQL Lab and Test Connection against a workspace. |
rebenitez1802
left a comment
There was a problem hiding this comment.
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.
SUMMARY
Automatically migrate saved
databricks+connector://URIs to the registered SQLAlchemy 2databricks://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 (legacyconnect_argsand the native URI query are merged the wayadjust_engine_paramsmerges them), so an unchanged connection keeps its stored token; a changed warehouse, host orconnect_argsstill requires a new token.Malformed
extramust 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://, notdatabricks+connector://; this change does not depend on or modify it.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screenshots
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 withsuperset db upgrade. Host and token are fake.TESTING INSTRUCTIONS
extra.engine_params.connect_args; save a second legacy row with malformedextraand a separate native connection.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.superset db downgrade f72b6e9c104a: unchanged converted rows should return to the legacy scheme, while edited and unmarked native rows should keep their URIs.superset db upgradeagain.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 withtest_downgrade_preserves_edits_to_converted_native_connection, the focused comparison against incoming commit7a49c6ce5c980966b6c34ce64af44301e16d05e3produced 12 failures, 1 pass; the passing malformed-after-edit case preserves an existing behavior.test_converted_connection_extra_can_be_importedpasses at9670e47dd08e791b39c041e4fdf534c7136b2046and fails when the import schema is restored to490bc015cff1fe52b4927d5c228b11b29ba7d355.test_downgrade_imported_marker_and_unmarked_native_rowspins 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_settingsandtest_native_form_save_preserves_masked_token_and_updates_legacy_settingsexercise suffix/ciphertext preservation and native form parameters/token handling at9670e47dd08e791b39c041e4fdf534c7136b2046.db headsat9670e47dd08e791b39c041e4fdf534c7136b2046(invoked throughpython -c 'from superset.cli.main import superset; superset()' db headswith the same Python/config paths): b8f3a2c91d64 (head), one head.pre-commit runpassed before committing;pre-commit run --files <all branch-changed paths>passed at9670e47dd08e791b39c041e4fdf534c7136b2046, 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 includeYou 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 finaldatabricks-uri-fix2-container-name search returned no matches.ADDITIONAL INFORMATION
test_upgrade_preserves_invalid_extra_and_downgrades_only_unchanged_rows,test_upgrade_batches_and_downgrade_only_converted_rows, andtest_downgrade_preserves_edits_to_converted_native_connectionat9670e47dd08e791b39c041e4fdf534c7136b2046.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.