From 7457ce426f0563464dff2313249381afa9378f28 Mon Sep 17 00:00:00 2001 From: Martin Cech Date: Wed, 14 Feb 2018 17:02:00 -0500 Subject: [PATCH] optimize nonadmin and nonanon library list permission queries Before, the code would emit extra queries per each library to find out whether the current user has permissions on it. By pre-fetching all of them in one query and passing around as a dict object we should see a considerable speedup on Galaxies with many libraries. --- lib/galaxy/managers/libraries.py | 75 +++++++++++++++------- lib/galaxy/webapps/galaxy/api/libraries.py | 4 +- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index ed7ef58c39b..5f8c5c06ef6 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -3,7 +3,7 @@ Manager and Serializer for libraries. """ import logging -from sqlalchemy import and_, false, not_, or_, true +from sqlalchemy import false, not_, or_, true from sqlalchemy.orm.exc import MultipleResultsFound from sqlalchemy.orm.exc import NoResultFound @@ -109,8 +109,11 @@ class LibraryManager(object): :returns: query that will emit all accessible libraries :rtype: sqlalchemy query - :returns: set of library ids that have restricted access (not public) - :rtype: set + :returns: dict of 3 sets with available actions for user's accessible + libraries and a set of ids of all public libraries. These are + used for limiting the number of queries when dictifying the + libraries later on. + :rtype: dict """ is_admin = trans.user_is_admin() query = trans.sa_session.query(trans.app.model.Library) @@ -119,6 +122,7 @@ class LibraryManager(object): trans.sa_session.query(trans.model.LibraryPermissions).filter( trans.model.LibraryPermissions.table.c.action == library_access_action ).distinct())} + prefetched_ids = {'restricted_library_ids': restricted_library_ids} if is_admin: if deleted is None: # Flag is not specified, do not filter on it. @@ -129,18 +133,33 @@ class LibraryManager(object): query = query.filter(trans.app.model.Library.table.c.deleted == false()) else: # Nonadmins can't see deleted libraries + query = query.filter(trans.app.model.Library.table.c.deleted == false()) current_user_role_ids = [role.id for role in trans.get_current_user_roles()] - accessible_restricted_library_ids = [lp.library_id for lp in ( - trans.sa_session.query(trans.model.LibraryPermissions).filter( - and_( - trans.model.LibraryPermissions.table.c.action == library_access_action, - trans.model.LibraryPermissions.table.c.role_id.in_(current_user_role_ids) - )))] + all_actions = trans.sa_session.query(trans.model.LibraryPermissions).filter(trans.model.LibraryPermissions.table.c.role_id.in_(current_user_role_ids)) + library_add_action = trans.app.security_agent.permitted_actions.LIBRARY_ADD.action + library_modify_action = trans.app.security_agent.permitted_actions.LIBRARY_MODIFY.action + library_manage_action = trans.app.security_agent.permitted_actions.LIBRARY_MANAGE.action + accessible_restricted_library_ids = [] + allowed_library_add_ids = set([]) + allowed_library_modify_ids = set([]) + allowed_library_manage_ids = set([]) + for action in all_actions: + if action.action == library_access_action: + accessible_restricted_library_ids.add(action.library_id) + if action.action == library_add_action: + allowed_library_add_ids.add(action.library_id) + if action.action == library_modify_action: + allowed_library_modify_ids.add(action.library_id) + if action.action == library_manage_action: + allowed_library_manage_ids.add(action.library_id) query = query.filter(or_( not_(trans.model.Library.table.c.id.in_(restricted_library_ids)), trans.model.Library.table.c.id.in_(accessible_restricted_library_ids) )) - return query, restricted_library_ids + prefetched_ids['allowed_library_add_ids'] = allowed_library_add_ids + prefetched_ids['allowed_library_modify_ids'] = allowed_library_modify_ids + prefetched_ids['allowed_library_manage_ids'] = allowed_library_manage_ids + return query, prefetched_ids def secure(self, trans, library, check_accessible=True): """ @@ -172,30 +191,38 @@ class LibraryManager(object): else: return library - def get_library_dict(self, trans, library, restricted_library_ids=None): + def get_library_dict(self, trans, library, prefetched_ids=None): """ Return library data in the form of a dictionary. - :param library: library - :type library: galaxy.model.Library - :param restricted_library_ids: ids of restricted libraries to speed up the - detection of public libraries - :type restricted_library_ids: list of ints + :param library: library + :type library: galaxy.model.Library + :param prefetched_ids: dict of 3 sets with available actions for user's accessible + libraries and a set of ids of all public libraries. These are + used for limiting the number of queries when dictifying a + set of libraries. + :type prefetched_ids: dict :returns: dict with data about the library :rtype: dictionary """ + restricted_library_ids = prefetched_ids.get('restricted_library_ids', None) if prefetched_ids else None + allowed_library_add_ids = prefetched_ids.get('allowed_library_add_ids', None) if prefetched_ids else None + allowed_library_modify_ids = prefetched_ids.get('allowed_library_modify_ids', None) if prefetched_ids else None + allowed_library_manage_ids = prefetched_ids.get('allowed_library_manage_ids', None) if prefetched_ids else None library_dict = library.to_dict(view='element', value_mapper={'id': trans.security.encode_id, 'root_folder_id': trans.security.encode_id}) - if restricted_library_ids and library.id in restricted_library_ids: - library_dict['public'] = False - else: - library_dict['public'] = True + library_dict['public'] = False if (restricted_library_ids and library.id in restricted_library_ids) else True library_dict['create_time_pretty'] = pretty_print_time_interval(library.create_time, precise=True) if not trans.user_is_admin(): - current_user_roles = trans.get_current_user_roles() - library_dict['can_user_add'] = trans.app.security_agent.can_add_library_item(current_user_roles, library) - library_dict['can_user_modify'] = trans.app.security_agent.can_modify_library_item(current_user_roles, library) - library_dict['can_user_manage'] = trans.app.security_agent.can_manage_library_item(current_user_roles, library) + if prefetched_ids: + library_dict['can_user_add'] = True if (allowed_library_add_ids and library.id in allowed_library_add_ids) else False + library_dict['can_user_modify'] = True if (allowed_library_modify_ids and library.id in allowed_library_modify_ids) else False + library_dict['can_user_manage'] = True if (allowed_library_manage_ids and library.id in allowed_library_manage_ids) else False + else: + current_user_roles = trans.get_current_user_roles() + library_dict['can_user_add'] = trans.app.security_agent.can_add_library_item(current_user_roles, library) + library_dict['can_user_modify'] = trans.app.security_agent.can_modify_library_item(current_user_roles, library) + library_dict['can_user_manage'] = trans.app.security_agent.can_manage_library_item(current_user_roles, library) else: library_dict['can_user_add'] = True library_dict['can_user_modify'] = True diff --git a/lib/galaxy/webapps/galaxy/api/libraries.py b/lib/galaxy/webapps/galaxy/api/libraries.py index 7300b9962e3..5c22b9a4191 100644 --- a/lib/galaxy/webapps/galaxy/api/libraries.py +++ b/lib/galaxy/webapps/galaxy/api/libraries.py @@ -46,10 +46,10 @@ class LibrariesController(BaseAPIController): """ deleted = util.string_as_bool_or_none(kwd.get('deleted', None)) - query, restricted_library_ids = self.library_manager.list(trans, deleted) + query, prefetched_ids = self.library_manager.list(trans, deleted) libraries = [] for library in query: - libraries.append(self.library_manager.get_library_dict(trans, library, restricted_library_ids)) + libraries.append(self.library_manager.get_library_dict(trans, library, prefetched_ids)) return libraries def __decode_id(self, trans, encoded_id, object_name=None):