sadpandajoe opened a new pull request, #43950:
URL: https://github.com/apache/superset/pull/43950
### SUMMARY
Importing a database connection can silently fail to create `schema_access`/
`catalog_access` permissions for schemas that exist on the target connection,
via two independent gaps:
1. `add_permissions()` retries nothing: a single transient exception while
listing a catalog's schemas discards that catalog's *entire* permission
set (schemas already known and newly-added alike), while the import still
reports success.
2. Importing a chart, dataset, saved-query, or dashboard bundle that
references a database already present in the target skips permission
sync for that database entirely, every time — this branch of
`import_database()` doesn't go through the top-level `validate()` that
only applies to the bundle's primary asset.
This PR:
- Retries the per-catalog schema-listing call once before giving up,
preserving the existing tolerance for a catalog that's genuinely
persistently unlistable (e.g. AWS RDS's `rdsadmin`, the case PR #31437
introduced this handler for).
- Backfills permissions on the existing/no-overwrite import branch when the
caller has database-write access, matching the same gate every other
permission-view creation in this function already uses.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
UI QA: not-required — this is a backend permission-record fix in the
database-import command layer. Neither triggering condition (a transient
schema-discovery exception, or a nested asset import referencing an existing
database) has a practical, deterministic browser-testable manifestation.
Verified instead via deterministic backend regression tests reproducing each
mechanism (RED before the fix, GREEN after) — see Testing Instructions.
### TESTING INSTRUCTIONS
New/updated tests, all confirmed failing (for the intended reason) before
this change and passing after:
-
`tests/unit_tests/commands/databases/importers/v1/command_test.py::test_transient_schema_listing_failure_is_recovered_by_retry`
-
`tests/unit_tests/databases/commands/importers/v1/import_test.py::test_import_database_existing_no_overwrite_backfills_permissions`
-
`tests/unit_tests/databases/commands/importers/v1/import_test.py::test_import_database_existing_no_overwrite_no_permission_skips_backfill`
-
`tests/unit_tests/databases/commands/importers/v1/import_test.py::test_import_database_existing_no_overwrite_backfill_oauth2_redirect_is_nonfatal`
-
`tests/unit_tests/databases/commands/utils_test.py::test_add_permissions_retries_transient_failure`
-
`tests/unit_tests/databases/commands/utils_test.py::test_add_permissions_tolerates_failure_creating_permission_view`
Run:
```
pytest tests/unit_tests/commands/databases/
tests/unit_tests/databases/commands/importers/v1/
tests/unit_tests/databases/commands/utils_test.py
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
