mikebridge commented on PR #43783: URL: https://github.com/apache/superset/pull/43783#issuecomment-5535130884
@aminghadersohi — thanks for the careful permission-direction pass. Both of the "other way" moves are now documented at `7ddae079f0` (docs only, no behaviour change, matching your read that both are benign): **1. `add_pvms` runs unconditionally → not a whole-migration no-op on clean installs.** You're right that the docstring's justification was off. Reworded it: `migrate_roles` is the no-op part (stale PVMs resolve to `None`), but `add_pvms` still seeds the `SemanticLayer` view menu and its `can_read` PVM, which `sync_role_definitions` then grants to Gamma even on a fresh/flag-off install — benign (the endpoints 404 while the flag is off), not a no-op. I called out your guard alternative (`if _find_pvm(session, VIEW_MENU, "can_views") or _find_pvm(session, VIEW_MENU, "can_connections"):` around both calls, `_find_pvm` is already exported) in the docstring — happy to switch to it if you'd rather the migration be a literal no-op on clean installs; I kept to the docs-only path since the behaviour is benign and this PR is scoped to the classification. **2. `can_views`/`can_connections` → `can_read` remap is a small widening.** Agreed the code is the right trade (the alternative strands those roles). UPDATING.md now says so explicitly: a custom role that held only `can_views` or `can_connections` gains `can_read` (the layer list + masked-configuration detail) it could not previously reach, and operators who hand-rolled semantic-layer roles should re-audit after upgrading. **On the `DatabaseFilter` gap (`_fetch_connection_items`, database branch not access-filtered):** deferred as a follow-up, per your own read — pre-existing on master, not regressed here (Gamma's reach is unchanged; what changed is the endpoint is now gated on the common `can_read`). Filed as **sc-119878** with your analysis (the semantic-layer branch filters on `SemanticLayer.perm`, the database branch issues a bare `db.session.query(Database)`, and `DatabaseFilter` is the fix). It's the natural place to add it, but since this PR doesn't regress it I've kept it out of scope here. Would appreciate another look. -- 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]
