Skip to content

Commit

Permalink
optimize nonadmin and nonanon library list permission queries
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@martenson
martenson committed Feb 15, 2018
1 parent c20b68a commit 7457ce4
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 26 deletions.
75 changes: 51 additions & 24 deletions lib/galaxy/managers/libraries.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"""
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

Expand Down Expand Up @@ -109,8 +109,11 @@ def list(self, trans, deleted=False):
: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)
Expand All @@ -119,6 +122,7 @@ def list(self, trans, deleted=False):
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.
Expand All @@ -129,18 +133,33 @@ def list(self, trans, deleted=False):
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):
"""
Expand Down Expand Up @@ -172,30 +191,38 @@ def check_accessible(self, trans, library):
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
Expand Down
4 changes: 2 additions & 2 deletions lib/galaxy/webapps/galaxy/api/libraries.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ def index(self, trans, **kwd):
"""
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):
Expand Down

0 comments on commit 7457ce4

Please sign in to comment.