Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Issue #25040; Refactored sync_role_definition function in order to reduce number of query. #25340

Merged
merged 6 commits into from
Oct 11, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 26 additions & 9 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
from jwt.api_jwt import _jwt_global_obj
from sqlalchemy import and_, inspect, or_
from sqlalchemy.engine.base import Connection
from sqlalchemy.orm import Session
from sqlalchemy.orm import eagerload, Session
from sqlalchemy.orm.mapper import Mapper
from sqlalchemy.orm.query import Query as SqlaQuery

Expand Down Expand Up @@ -736,7 +736,7 @@ def create_missing_perms(self) -> None:

logger.info("Fetching a set of all perms to lookup which ones are missing")
all_pvs = set()
for pv in self.get_session.query(self.permissionview_model).all():
for pv in self._get_all_pvms():
if pv.permission and pv.view_menu:
all_pvs.add((pv.permission.name, pv.view_menu.name))

Expand Down Expand Up @@ -784,11 +784,13 @@ def sync_role_definitions(self) -> None:

self.create_custom_permissions()

pvms = self._get_all_pvms()

# Creating default roles
self.set_role("Admin", self._is_admin_pvm)
self.set_role("Alpha", self._is_alpha_pvm)
self.set_role("Gamma", self._is_gamma_pvm)
self.set_role("sql_lab", self._is_sql_lab_pvm)
self.set_role("Admin", self._is_admin_pvm, pvms)
self.set_role("Alpha", self._is_alpha_pvm, pvms)
self.set_role("Gamma", self._is_gamma_pvm, pvms)
self.set_role("sql_lab", self._is_sql_lab_pvm, pvms)

# Configure public role
if current_app.config["PUBLIC_ROLE_LIKE"]:
Expand All @@ -804,6 +806,20 @@ def sync_role_definitions(self) -> None:
self.get_session.commit()
self.clean_perms()

def _get_all_pvms(self) -> list[PermissionView]:
"""
Gets list of all PVM
"""
pvms = (
self.get_session.query(self.permissionview_model)
.options(
eagerload(self.permissionview_model.permission),
eagerload(self.permissionview_model.view_menu),
)
.all()
)
return [p for p in pvms if p.permission and p.view_menu]

def _get_pvms_from_builtin_role(self, role_name: str) -> list[PermissionView]:
"""
Gets a list of model PermissionView permissions inferred from a builtin role
Expand Down Expand Up @@ -864,7 +880,10 @@ def copy_role(
self.get_session.commit()

def set_role(
self, role_name: str, pvm_check: Callable[[PermissionView], bool]
self,
role_name: str,
pvm_check: Callable[[PermissionView], bool],
pvms: list[PermissionView],
) -> None:
"""
Set the FAB permission/views for the role.
Expand All @@ -874,8 +893,6 @@ def set_role(
"""

logger.info("Syncing %s perms", role_name)
pvms = self.get_session.query(PermissionView).all()
pvms = [p for p in pvms if p.permission and p.view_menu]
role = self.add_role(role_name)
role_pvms = [
permission_view for permission_view in pvms if pvm_check(permission_view)
Expand Down