jedcunningham commented on a change in pull request #14993:
URL: https://github.com/apache/airflow/pull/14993#discussion_r600924150
##########
File path: airflow/www/security.py
##########
@@ -457,25 +457,25 @@ def add_homepage_access_to_custom_roles(self):
self.get_session.commit()
- def get_all_permissions(self):
+ def get_all_permissions(self) -> Set[Tuple[str, str]]:
"""Returns all permissions as a set of tuples with the perm name and
view menu name"""
- perms = set()
- for permission_view in
self.get_session.query(self.permissionview_model).all():
- if permission_view.permission and permission_view.view_menu:
- perms.add((permission_view.permission.name,
permission_view.view_menu.name))
-
- return perms
+ return set(
+ self.get_session.query(self.permissionview_model)
+ .join(self.permission_model)
+ .join(self.viewmenu_model)
+ .with_entities(self.permission_model.name,
self.viewmenu_model.name)
+ .all()
+ )
Review comment:
Name isn't nullable on both permission and viewmodel, and with inner
joins we are guaranteed to get both which lets us return directly instead of
walking the whole list to check them.
##########
File path: airflow/www/security.py
##########
@@ -484,7 +484,7 @@ def create_dag_specific_permissions(self, session=None):
dag_id = row[0]
for perm_name in self.DAG_PERMS:
dag_resource_name = self.prefixed_dag_id(dag_id)
- if dag_resource_name and perm_name and (dag_resource_name,
perm_name) not in perms:
+ if (perm_name, dag_resource_name) not in perms:
Review comment:
Checking that we get a dag_id (which is the PK) and that it gets
prefixed properly didn't seem necessary, unless I've overlooked something?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
