Skip to content

Commit

Permalink
refactor: Issue apache#25040; Refactored sync_role_definition functio…
Browse files Browse the repository at this point in the history
…n in order to reduce number of query. (apache#25340)
  • Loading branch information
sandeep-patel11 authored Oct 11, 2023
1 parent 3f85239 commit eb9cd2a
Showing 1 changed file with 26 additions and 9 deletions.
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 @@ -732,7 +732,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 @@ -780,11 +780,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 @@ -800,6 +802,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 @@ -860,7 +876,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 @@ -870,8 +889,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

0 comments on commit eb9cd2a

Please sign in to comment.