rusackas opened a new pull request, #41309:
URL: https://github.com/apache/superset/pull/41309
### SUMMARY
On the role-edit screen, `can import on ImportExportRestApi` appears
**twice**. Only one of those entries is real.
- The live permission is `can_import_` (note the **trailing underscore**),
derived by Flask-AppBuilder from the `import_()` method on
`ImportExportRestApi` (`superset/importexport/api.py`). It is registered
exactly once.
- The visual "duplicate" is a **stale** `can_import` (no trailing
underscore) permission-view-menu (PVM) row tied to the `ImportExportRestApi`
view menu. It's a leftover from an older Superset/FAB era that has persisted
across upgrades. FAB renders `can_import_` as "can import " and `can_import` as
"can import", which look identical in the UI.
Current code can no longer create the stale row, so the right fix (Option A)
is a cleanup migration.
This migration is scoped to the `ImportExportRestApi` view menu **only**:
1. Ensures the live `can_import_` PVM exists (`add_pvms`, idempotent).
2. For every role that holds the stale `can_import` PVM, adds the live
`can_import_` PVM and removes the stale one — so **no role silently loses
import access** (`migrate_roles`).
3. Deletes the stale `can_import` PVM. The shared
`migrate_roles`/`_delete_old_permissions` helper deletes the `can_import`
permission row and the view-menu row **only if they become true orphans** (no
remaining PVMs reference them), so the global `can_import` permission name is
never dropped while anything still uses it.
On a clean install the stale PVM doesn't exist, so the lookup resolves to
`None` and the migration is a safe no-op.
`downgrade()` is intentionally a no-op (documented inline): recreating an
orphaned stale PVM would reintroduce the exact duplicate this fixes, and the
live `can_import_` permission is untouched by the upgrade, so there's nothing
meaningful to restore.
### TESTING INSTRUCTIONS
- Automated: `pytest
tests/integration_tests/migrations/a7d3f1b9c2e4_cleanup_stale_can_import_pvm__tests.py`
- `test_migration_reassigns_roles_and_removes_stale_pvm` — seeds a stale
`can_import`/`ImportExportRestApi` PVM on a role, runs upgrade, asserts the
stale PVM is gone, the live `can_import_` PVM exists, and the role was
reassigned to it.
- `test_migration_is_noop_on_clean_install` — asserts no error and the
live permission stays intact when no stale row exists.
- Manual: on a metadata DB that exhibits the duplicate, run `superset db
upgrade`, then open Settings → List Roles → edit a role and confirm `can import
on ImportExportRestApi` appears only once.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #36070
- [ ] Required feature flags:
- [ ] Changes UI
- [x] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [x] Migration is atomic, supports rollback & is backwards-compatible
- [x] Confirm DB migration upgrade and downgrade tested
- [x] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Runtime/downtime: negligible — touches at most a single PVM row plus its
role associations; no schema/DDL changes, no table scans, no downtime.
--
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]
