rusackas opened a new pull request, #44362: URL: https://github.com/apache/superset/pull/44362
### SUMMARY Follow-up to #42404, addressing the three items sadpandajoe flagged in [a review comment](https://github.com/apache/superset/pull/42404#issuecomment-5654516020) after approving that PR: 1. **Overwrite-import edge case.** `import_themes.py`'s overwrite gate checked `security_manager.is_editor(existing)`, but a theme's original creator isn't guaranteed to be in its `editors` list (e.g. a theme created via a path that bypasses `CreateThemeCommand`'s editor seeding). That user could get wrongly denied permission to re-import/overwrite their own theme despite holding `can_write`. Added a `created_by_fk` fallback, and backfill `editors` on first use so later overwrites don't need it. 2. **Missing index.** `theme_editors`' only index was the `UNIQUE(subject_id, theme_id)` constraint from its creation in #42404, which leads with `subject_id` — most engines can't use a composite index to serve a lookup filtered on the trailing column alone. Loading a theme's editor list filters on `theme_id` only, so every load was a table scan. Added `ix_theme_editors_theme_id`. 3. **DRY duplication.** `themes/api.py`, `charts/api.py`, and `dashboards/api.py` each had a near-identical copy of the guard-on-`EXTRA_EDITORS_RESOLVER`-and-attach-`extra_editors` logic, for both the single-object GET and `pre_get_list`. Extracted `attach_extra_editors`/`attach_extra_editors_to_rows` in `security/manager.py`, next to the `get_extra_editor_subject_ids`/ `get_extra_editors_by_pk` helpers they wrap, and pointed all three call sites at them. **Unplanned addition:** while testing (2), CI on master's own required "Enforce single Alembic migration head" check turned up failing as of #42404's merge commit. Two already-merged merge-migrations (`0884f655c0a6`, `e2f3a1b9c640`) each reconciled a disjoint set of prior forks without ever being reconciled with each other — two new revision files with different `down_revision`s produce no git merge conflict, so this slipped through. This PR includes a merge migration (`0e8bab8c31a3`) joining both heads, which the index migration is chained after. This is pre-existing breakage unrelated to #42404's own diff, but it blocks CI for this PR (and any other migration-touching PR) until fixed, so it's included here rather than filed separately. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/commands/theme/test_import_themes.py` — includes two new tests covering the original-creator overwrite fallback and confirming a non-creator/non-editor is still denied. - `pytest tests/integration_tests/dashboards/api_tests.py -k extra_editors` — unchanged behavior after the DRY refactor (dashboard + chart list coverage). - Migration chain verified end-to-end against a scratch SQLite DB: `flask db upgrade` from a fresh DB applies cleanly and creates `ix_theme_editors_theme_id`; `flask db downgrade 0e8bab8c31a3` drops it; re-`db upgrade` restores it. - `flask db heads` reports a single head (`00fab727cd0a`) after this PR. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] 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 estimate: the merge migration is a no-op (schema/data untouched). The index migration issues a single `CREATE INDEX` on `theme_editors`, a new junction table introduced in #42404 with no expected production rows yet — effectively instant on any real-world deployment size. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
