From b70a23bcf07d5ecae8ac567805fc8ab190060edf Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 09:36:11 +0200 Subject: [PATCH 01/30] Make most base managers generic and parameterize concrete item managers This means that when we list/filter/retrieve items from a given manager we get the correct return type, instead of `any`. --- lib/galaxy/managers/base.py | 9 +++-- lib/galaxy/managers/datasets.py | 27 +++++++------ lib/galaxy/managers/deletable.py | 17 ++++---- lib/galaxy/managers/hdas.py | 8 ++-- lib/galaxy/managers/histories.py | 4 +- lib/galaxy/managers/lddas.py | 13 ++++++- lib/galaxy/managers/library_datasets.py | 2 +- lib/galaxy/managers/pages.py | 2 +- lib/galaxy/managers/secured.py | 26 ++++++------- lib/galaxy/managers/sharable.py | 52 +++++++++++++++---------- lib/galaxy/managers/visualizations.py | 2 +- lib/galaxy/managers/workflows.py | 2 +- 12 files changed, 97 insertions(+), 67 deletions(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index be487b4da675..0e86310d57f2 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -196,8 +196,12 @@ def get_object(trans, id, class_name, check_ownership=False, check_accessible=Fa U = TypeVar("U", bound=model._HasTable) +class HasModelClass(Generic[U]): + model_class: Type[U] + + # ----------------------------------------------------------------------------- -class ModelManager(Generic[U]): +class ModelManager(Generic[U], HasModelClass[U]): """ Base class for all model/resource managers. @@ -205,7 +209,6 @@ class ModelManager(Generic[U]): over the ORM. """ - model_class: Type[U] foreign_key_name: str app: BasicSharedApp @@ -215,7 +218,7 @@ def __init__(self, app: BasicSharedApp): def session(self) -> scoped_session: return self.app.model.context - def _session_setattr(self, item: model.Base, attr: str, val: Any, flush: bool = True): + def _session_setattr(self, item: U, attr: str, val: Any, flush: bool = True): setattr(item, attr, val) self.session().add(item) diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py index eb137a2bcfc2..834e5d5cea70 100644 --- a/lib/galaxy/managers/datasets.py +++ b/lib/galaxy/managers/datasets.py @@ -8,6 +8,7 @@ from typing import ( Any, Dict, + Generic, List, Optional, Type, @@ -336,12 +337,16 @@ def serialize_permissions(self, item, key, user=None, **context): return permissions +DI = TypeVar("DI", bound=model.DatasetInstance) + + # ============================================================================= AKA DatasetInstanceManager class DatasetAssociationManager( - base.ModelManager[model.DatasetInstance], + Generic[DI], + base.ModelManager[DI], secured.AccessibleManagerMixin, - secured.OwnableManagerMixin, - deletable.PurgableManagerMixin, + secured.OwnableManagerMixin[DI], + deletable.PurgableManagerMixin[DI], ): """ DatasetAssociation/DatasetInstances are intended to be working @@ -359,14 +364,14 @@ def __init__(self, app): super().__init__(app) self.dataset_manager = DatasetManager(app) - def is_accessible(self, item, user: Optional[model.User], **kwargs: Any) -> bool: + def is_accessible(self, item: DI, user: Optional[model.User], **kwargs: Any) -> bool: """ Is this DA accessible to `user`? """ # defer to the dataset return self.dataset_manager.is_accessible(item.dataset, user, **kwargs) - def delete(self, item, flush: bool = True, stop_job: bool = False, **kwargs): + def delete(self, item: DI, flush: bool = True, stop_job: bool = False, **kwargs): """ Marks this dataset association as deleted. If `stop_job` is True, will stop the creating job if all other outputs are deleted. @@ -376,7 +381,7 @@ def delete(self, item, flush: bool = True, stop_job: bool = False, **kwargs): self.stop_creating_job(item, flush=flush) return item - def purge(self, dataset_assoc, flush=True): + def purge(self, item: DI, flush=True, **kwargs) -> DI: """ Purge this DatasetInstance and the dataset underlying it. """ @@ -388,15 +393,15 @@ def purge(self, dataset_assoc, flush=True): # so that job cleanup associated with stop_creating_job will see # the dataset as purged. flush_required = not self.app.config.track_jobs_in_database - super().purge(dataset_assoc, flush=flush or flush_required) + super().purge(item, flush=flush or flush_required) # stop any jobs outputing the dataset_assoc - self.stop_creating_job(dataset_assoc, flush=True) + self.stop_creating_job(item, flush=True) # more importantly, purge underlying dataset as well - if dataset_assoc.dataset.user_can_purge: - self.dataset_manager.purge(dataset_assoc.dataset) - return dataset_assoc + if item.dataset.user_can_purge: + self.dataset_manager.purge(item.dataset) + return item def by_user(self, user): raise exceptions.NotImplemented("Abstract Method") diff --git a/lib/galaxy/managers/deletable.py b/lib/galaxy/managers/deletable.py index 113cfa22dc99..ca2d8cee9ce8 100644 --- a/lib/galaxy/managers/deletable.py +++ b/lib/galaxy/managers/deletable.py @@ -12,19 +12,20 @@ from typing import ( Any, Dict, + Generic, Set, ) -from galaxy.model import Base from .base import ( Deserializer, ModelValidator, OrmFilterParsersType, parse_bool, + U, ) -class DeletableManagerMixin: +class DeletableManagerMixin(Generic[U]): """ A mixin/interface for a model that is deletable (i.e. has a 'deleted' attr). @@ -33,15 +34,15 @@ class DeletableManagerMixin: removed by an admin/script. """ - def _session_setattr(self, item: Base, attr: str, val: Any, flush: bool = True): ... + def _session_setattr(self, item: U, attr: str, val: Any, flush: bool = True): ... - def delete(self, item, flush=True, **kwargs): + def delete(self, item: U, flush=True, **kwargs): """ Mark as deleted and return. """ return self._session_setattr(item, "deleted", True, flush=flush) - def undelete(self, item, flush=True, **kwargs): + def undelete(self, item: U, flush=True, **kwargs): """ Mark as not deleted and return. """ @@ -84,16 +85,16 @@ def _add_parsers(self): self.orm_filter_parsers.update({"deleted": {"op": ("eq"), "val": parse_bool}}) -class PurgableManagerMixin(DeletableManagerMixin): +class PurgableManagerMixin(Generic[U], DeletableManagerMixin[U]): """ A manager interface/mixin for a resource that allows deleting and purging where purging is often removal of some additional, non-db resource (e.g. a dataset's file). """ - def _session_setattr(self, item: Base, attr: str, val: Any, flush: bool = True): ... + def _session_setattr(self, item: U, attr: str, val: Any, flush: bool = True): ... - def purge(self, item, flush=True, **kwargs): + def purge(self, item: U, flush=True, **kwargs): """ Mark as purged and return. diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py index 48e69eb356b5..19cfcfcbb553 100644 --- a/lib/galaxy/managers/hdas.py +++ b/lib/galaxy/managers/hdas.py @@ -78,8 +78,8 @@ class HistoryDatasetAssociationNoHistoryException(Exception): class HDAManager( - datasets.DatasetAssociationManager, - secured.OwnableManagerMixin, + datasets.DatasetAssociationManager[model.HistoryDatasetAssociation], + secured.OwnableManagerMixin[model.HistoryDatasetAssociation], annotatable.AnnotatableManagerMixin, ): """ @@ -126,7 +126,9 @@ def is_accessible(self, item: model.HistoryDatasetAssociation, user: Optional[mo # return True return super().is_accessible(item, user, **kwargs) - def is_owner(self, item, user: Optional[model.User], current_history=None, **kwargs: Any) -> bool: + def is_owner( + self, item: model.HistoryDatasetAssociation, user: Optional[model.User], current_history=None, **kwargs: Any + ) -> bool: """ Use history to see if current user owns HDA. """ diff --git a/lib/galaxy/managers/histories.py b/lib/galaxy/managers/histories.py index 84db5dd38c2f..eb4103173281 100644 --- a/lib/galaxy/managers/histories.py +++ b/lib/galaxy/managers/histories.py @@ -96,7 +96,7 @@ } -class HistoryManager(sharable.SharableModelManager, deletable.PurgableManagerMixin, SortableManager): +class HistoryManager(sharable.SharableModelManager[History], deletable.PurgableManagerMixin[History], SortableManager): model_class = model.History foreign_key_name = "history" user_share_model = model.HistoryUserShareAssociation @@ -259,7 +259,7 @@ def by_user( def is_owner( self, - item: model.Base, + item: model.History, user: Optional[model.User], current_history: Optional[model.History] = None, **kwargs: Any, diff --git a/lib/galaxy/managers/lddas.py b/lib/galaxy/managers/lddas.py index 1cb2fe6da3f0..d20423db524e 100644 --- a/lib/galaxy/managers/lddas.py +++ b/lib/galaxy/managers/lddas.py @@ -1,4 +1,5 @@ import logging +from typing import Optional from galaxy import model from galaxy.managers import base as manager_base @@ -8,7 +9,7 @@ log = logging.getLogger(__name__) -class LDDAManager(DatasetAssociationManager): +class LDDAManager(DatasetAssociationManager[model.LibraryDatasetDatasetAssociation]): """ A fairly sparse manager for LDDAs. """ @@ -21,6 +22,16 @@ def __init__(self, app: MinimalManagerApp): """ super().__init__(app) + def is_owner( + self, item: model.LibraryDatasetDatasetAssociation, user: Optional[model.User], trans=None, **kwargs + ) -> bool: + return manager_base.get_object( + trans, + id, + "LibraryDatasetDatasetAssociation", + check_ownership=True, + ) + def get(self, trans, id: int, check_accessible=True) -> model.LibraryDatasetDatasetAssociation: return manager_base.get_object( trans, id, "LibraryDatasetDatasetAssociation", check_ownership=False, check_accessible=check_accessible diff --git a/lib/galaxy/managers/library_datasets.py b/lib/galaxy/managers/library_datasets.py index 3666838b2bb0..7be35e608ff5 100644 --- a/lib/galaxy/managers/library_datasets.py +++ b/lib/galaxy/managers/library_datasets.py @@ -27,7 +27,7 @@ log = logging.getLogger(__name__) -class LibraryDatasetsManager(datasets.DatasetAssociationManager): +class LibraryDatasetsManager(datasets.DatasetAssociationManager[model.LibraryDatasetDatasetAssociation]): """Interface/service object for interacting with library datasets.""" model_class = model.LibraryDatasetDatasetAssociation diff --git a/lib/galaxy/managers/pages.py b/lib/galaxy/managers/pages.py index 581b5b93562b..238d5dd5ee33 100644 --- a/lib/galaxy/managers/pages.py +++ b/lib/galaxy/managers/pages.py @@ -122,7 +122,7 @@ } -class PageManager(sharable.SharableModelManager, UsesAnnotations): +class PageManager(sharable.SharableModelManager[Page], UsesAnnotations): """Provides operations for managing a Page.""" model_class = model.Page diff --git a/lib/galaxy/managers/secured.py b/lib/galaxy/managers/secured.py index 64721d8b137e..8531c7dd044b 100644 --- a/lib/galaxy/managers/secured.py +++ b/lib/galaxy/managers/secured.py @@ -6,8 +6,8 @@ from typing import ( Any, + Generic, Optional, - Type, TYPE_CHECKING, ) @@ -15,21 +15,22 @@ exceptions, model, ) +from galaxy.managers.base import ( + HasModelClass, + U, +) if TYPE_CHECKING: from sqlalchemy.orm import Query -class AccessibleManagerMixin: +class AccessibleManagerMixin(Generic[U], HasModelClass[U]): """ A security interface to check if a User can read/view an item's. This can also be thought of as 'read but not modify' privileges. """ - # declare what we are using from base ModelManager - model_class: Type[Any] - def by_id(self, id: int): ... # don't want to override by_id since consumers will also want to fetch w/o any security checks @@ -83,7 +84,7 @@ def filter_accessible(self, user, **kwargs): # return filter( lambda item: self.is_accessible( trans, item, user ), items ) -class OwnableManagerMixin: +class OwnableManagerMixin(Generic[U], HasModelClass[U]): """ A security interface to check if a User is an item's owner. @@ -93,19 +94,16 @@ class OwnableManagerMixin: This can also be thought of as write/edit privileges. """ - # declare what we are using from base ModelManager - model_class: Type[Any] - def by_id(self, id: int): ... - def is_owner(self, item: model.Base, user: Optional[model.User], **kwargs: Any) -> bool: + def is_owner(self, item: U, user: Optional[model.User], **kwargs: Any) -> bool: """ Return True if user owns the item. """ # override in subclasses raise exceptions.NotImplemented("Abstract interface Method") - def get_owned(self, id: int, user: Optional[model.User], **kwargs: Any) -> Any: + def get_owned(self, id: int, user: Optional[model.User], **kwargs: Any) -> U: """ Return the item with the given id if owned by the user, otherwise raise an error. @@ -115,7 +113,7 @@ def get_owned(self, id: int, user: Optional[model.User], **kwargs: Any) -> Any: item = self.by_id(id) return self.error_unless_owner(item, user, **kwargs) - def error_unless_owner(self, item, user: Optional[model.User], **kwargs: Any): + def error_unless_owner(self, item: U, user: Optional[model.User], **kwargs: Any) -> U: """ Raise an error if the item is NOT owned by user, otherwise return the item. @@ -143,7 +141,7 @@ def filter_owned(self, user, **kwargs): # just alias to list_owned return self.list_owned(user, **kwargs) - def get_mutable(self, id: int, user: Optional[model.User], **kwargs: Any) -> Any: + def get_mutable(self, id: int, user: Optional[model.User], **kwargs: Any) -> U: """ Return the item with the given id if the user can mutate it, otherwise raise an error. The user must be the owner of the item. @@ -154,7 +152,7 @@ def get_mutable(self, id: int, user: Optional[model.User], **kwargs: Any) -> Any self.error_unless_mutable(item) return item - def error_unless_mutable(self, item): + def error_unless_mutable(self, item: U): """ Raise an error if the item is NOT mutable. diff --git a/lib/galaxy/managers/sharable.py b/lib/galaxy/managers/sharable.py index 1f607f5d2589..349a54faff43 100644 --- a/lib/galaxy/managers/sharable.py +++ b/lib/galaxy/managers/sharable.py @@ -14,10 +14,13 @@ import re from typing import ( Any, + Generic, List, Optional, Set, Type, + TypeVar, + Union, ) from sqlalchemy import ( @@ -41,8 +44,12 @@ ) from galaxy.managers.base import combine_lists from galaxy.model import ( + History, + Page, + StoredWorkflow, User, UserShareAssociation, + Visualization, ) from galaxy.model.base import transaction from galaxy.model.tags import GalaxyTagHandler @@ -56,10 +63,13 @@ log = logging.getLogger(__name__) +U = TypeVar("U", bound=Union[History, Page, Visualization, StoredWorkflow]) + class SharableModelManager( - base.ModelManager, - secured.OwnableManagerMixin, + Generic[U], + base.ModelManager[U], + secured.OwnableManagerMixin[U], secured.AccessibleManagerMixin, annotatable.AnnotatableManagerMixin, ratable.RatableManagerMixin, @@ -80,7 +90,7 @@ def __init__(self, app: MinimalManagerApp): self.tag_handler = app[GalaxyTagHandler] # .... has a user - def by_user(self, user: User, **kwargs: Any) -> List[Any]: + def by_user(self, user: User, **kwargs: Any) -> List[U]: """ Return list for all items (of model_class type) associated with the given `user`. @@ -90,7 +100,7 @@ def by_user(self, user: User, **kwargs: Any) -> List[Any]: return self.list(filters=filters, **kwargs) # .... owned/accessible interfaces - def is_owner(self, item: model.Base, user: Optional[User], **kwargs: Any) -> bool: + def is_owner(self, item: U, user: Optional[User], **kwargs: Any) -> bool: """ Return true if this sharable belongs to `user` (or `user` is an admin). """ @@ -99,7 +109,7 @@ def is_owner(self, item: model.Base, user: Optional[User], **kwargs: Any) -> boo return True return item.user == user # type:ignore[attr-defined] - def is_accessible(self, item, user: Optional[User], **kwargs: Any) -> bool: + def is_accessible(self, item: U, user: Optional[User], **kwargs: Any) -> bool: """ If the item is importable, is owned by `user`, or (the valid) `user` is in 'users shared with' list for the item: return True. @@ -116,7 +126,7 @@ def is_accessible(self, item, user: Optional[User], **kwargs: Any) -> bool: return False # .... importable - def make_importable(self, item, flush=True): + def make_importable(self, item: U, flush=True): """ Makes item accessible--viewable and importable--and sets item's slug. Does not flush/commit changes, however. Item must have name, user, @@ -125,7 +135,7 @@ def make_importable(self, item, flush=True): self.create_unique_slug(item, flush=False) return self._session_setattr(item, "importable", True, flush=flush) - def make_non_importable(self, item, flush=True): + def make_non_importable(self, item: U, flush=True): """ Makes item accessible--viewable and importable--and sets item's slug. Does not flush/commit changes, however. Item must have name, user, @@ -137,7 +147,7 @@ def make_non_importable(self, item, flush=True): return self._session_setattr(item, "importable", False, flush=flush) # .... published - def publish(self, item, flush=True): + def publish(self, item: U, flush=True): """ Set both the importable and published flags on `item` to True. """ @@ -146,13 +156,13 @@ def publish(self, item, flush=True): self.make_importable(item, flush=False) return self._session_setattr(item, "published", True, flush=flush) - def unpublish(self, item, flush=True): + def unpublish(self, item: U, flush=True): """ Set the published flag on `item` to False. """ return self._session_setattr(item, "published", False, flush=flush) - def list_published(self, filters=None, **kwargs): + def list_published(self, filters=None, **kwargs) -> List[U]: """ Return a list of all published items. """ @@ -162,7 +172,7 @@ def list_published(self, filters=None, **kwargs): # .... user sharing # sharing is often done via a 3rd table btwn a User and an item -> a UserShareAssociation - def get_share_assocs(self, item, user=None): + def get_share_assocs(self, item: U, user=None): """ Get the UserShareAssociations for the `item`. @@ -173,7 +183,7 @@ def get_share_assocs(self, item, user=None): query = query.filter_by(user=user) return query.all() - def share_with(self, item, user: User, flush: bool = True): + def share_with(self, item: U, user: User, flush: bool = True): """ Get or create a share for the given user. """ @@ -184,7 +194,7 @@ def share_with(self, item, user: User, flush: bool = True): return existing.pop(0) return self._create_user_share_assoc(item, user, flush=flush) - def _create_user_share_assoc(self, item, user, flush=True): + def _create_user_share_assoc(self, item: U, user, flush=True): """ Create a share for the given user. """ @@ -203,7 +213,7 @@ def _create_user_share_assoc(self, item, user, flush=True): session.commit() return user_share_assoc - def unshare_with(self, item, user: User, flush: bool = True): + def unshare_with(self, item: U, user: User, flush: bool = True): """ Delete a user share from the database. """ @@ -228,7 +238,7 @@ def _query_shared_with(self, user, eagerloads=True, **kwargs): query = query.filter(self.user_share_model.user == user) return self._filter_and_order_query(query, **kwargs) - def list_shared_with(self, user, filters=None, order_by=None, limit=None, offset=None, **kwargs): + def list_shared_with(self, user, filters=None, order_by=None, limit=None, offset=None, **kwargs) -> List[U]: """ Return a list of those models shared with a particular user. """ @@ -248,7 +258,7 @@ def list_shared_with(self, user, filters=None, order_by=None, limit=None, offset return list(self._apply_fn_limit_offset_gen(items, limit, offset)) def get_sharing_extra_information( - self, trans, item, users: Set[User], errors: Set[str], option: Optional[SharingOptions] = None + self, trans, item: U, users: Set[User], errors: Set[str], option: Optional[SharingOptions] = None ) -> Optional[ShareWithExtra]: """Returns optional extra information about the shareability of the given item. @@ -256,14 +266,14 @@ def get_sharing_extra_information( to provide the extra information, otherwise, it will be None by default.""" return None - def make_members_public(self, trans, item): + def make_members_public(self, trans, item: U): """Make potential elements of this item public. This method must be overridden in managers that need to change permissions of internal elements contained associated with the given item. """ - def update_current_sharing_with_users(self, item, new_users_shared_with: Set[User], flush=True): + def update_current_sharing_with_users(self, item: U, new_users_shared_with: Set[User], flush=True): """Updates the currently list of users this item is shared with by adding new users and removing missing ones.""" current_shares = self.get_share_assocs(item) @@ -286,7 +296,7 @@ def update_current_sharing_with_users(self, item, new_users_shared_with: Set[Use # .... slugs # slugs are human readable strings often used to link to sharable resources (replacing ids) # TODO: as validator, deserializer, etc. (maybe another object entirely?) - def set_slug(self, item, new_slug, user, flush=True): + def set_slug(self, item: U, new_slug, user, flush=True): """ Validate and set the new slug for `item`. """ @@ -332,7 +342,7 @@ def _default_slug_base(self, item): return item.title.lower() return item.name.lower() - def get_unique_slug(self, item): + def get_unique_slug(self, item: U): """ Returns a slug that is unique among user's importable items for item's class. @@ -357,7 +367,7 @@ def get_unique_slug(self, item): return new_slug - def create_unique_slug(self, item, flush=True): + def create_unique_slug(self, item: U, flush=True): """ Set a new, unique slug on the item. """ diff --git a/lib/galaxy/managers/visualizations.py b/lib/galaxy/managers/visualizations.py index 80fbf13348c8..5292ee89bc19 100644 --- a/lib/galaxy/managers/visualizations.py +++ b/lib/galaxy/managers/visualizations.py @@ -59,7 +59,7 @@ } -class VisualizationManager(sharable.SharableModelManager): +class VisualizationManager(sharable.SharableModelManager[model.Visualization]): """ Handle operations outside and between visualizations and other models. """ diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 35089bc5e4a3..a90f8c73ec59 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -145,7 +145,7 @@ } -class WorkflowsManager(sharable.SharableModelManager, deletable.DeletableManagerMixin): +class WorkflowsManager(sharable.SharableModelManager[StoredWorkflow], deletable.DeletableManagerMixin[StoredWorkflow]): """Handle CRUD type operations related to workflows. More interesting stuff regarding workflow execution, step sorting, etc... can be found in the galaxy.workflow module. From 76b7288b939df8a308be245d1295622b03d8170e Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 09:39:12 +0200 Subject: [PATCH 02/30] Split UserManager into shared base manager and galaxy-specific user manager --- lib/galaxy/managers/users.py | 399 ++++++++++++++++++----------------- 1 file changed, 211 insertions(+), 188 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 6ffa1ebabc21..aa022bac483d 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -12,8 +12,13 @@ from typing import ( Any, Dict, + Generic, List, Optional, + overload, + Type, + TYPE_CHECKING, + Union, ) from markupsafe import escape @@ -25,6 +30,7 @@ true, ) from sqlalchemy.exc import NoResultFound +from typing_extensions import TypeVar from galaxy import ( exceptions, @@ -55,10 +61,14 @@ ) from galaxy.structured_app import ( BasicSharedApp, + MinimalApp, MinimalManagerApp, ) from galaxy.util.hash_util import new_secure_hash_v2 +if TYPE_CHECKING: + from tool_shed.webapp.model import User as ToolShedUser + log = logging.getLogger(__name__) PASSWORD_RESET_TEMPLATE = """ @@ -76,8 +86,10 @@ TXT_ACTIVATION_EMAIL_TEMPLATE_RELPATH = "mail/activation-email.txt" HTML_ACTIVATION_EMAIL_TEMPLATE_RELPATH = "mail/activation-email.html" +U = TypeVar("U", bound=Union[User, "ToolShedUser"], default=User) + -class UserManager(base.ModelManager, deletable.PurgableManagerMixin): +class BaseUserManager(Generic[U], base.ModelManager[U], deletable.PurgableManagerMixin[U]): foreign_key_name = "user" # TODO: there is quite a bit of functionality around the user (authentication, permissions, quotas, groups/roles) @@ -155,117 +167,23 @@ def create(self, email=None, username=None, password=None, **kwargs): self.app.security_agent.create_user_role(user, self.app) return user - def delete(self, user, flush=True): + def delete(self, item: U, flush=True, **kwargs): """Mark the given user deleted.""" if not self.app.config.allow_user_deletion: raise exceptions.ConfigDoesNotAllowException( - "The configuration of this Galaxy instance does not allow admins to delete users." + f"The configuration of this {self.app_type.capitalize()} instance does not allow admins to delete users." ) - super().delete(user, flush=flush) - self._stop_all_jobs_from_user(user) + super().delete(item, flush=flush) - def _stop_all_jobs_from_user(self, user): - active_jobs = self._get_all_active_jobs_from_user(user) - session = self.session() - for job in active_jobs: - job.mark_deleted(self.app.config.track_jobs_in_database) - with transaction(session): - session.commit() - - def _get_all_active_jobs_from_user(self, user: User) -> List[Job]: - """Get all jobs that are not ready yet and belong to the given user.""" - stmt = select(Job).where(and_(Job.user_id == user.id, Job.state.in_(Job.non_ready_states))) - jobs = self.session().scalars(stmt) - return jobs # type:ignore[return-value] - - def undelete(self, user, flush=True): + def undelete(self, item: U, flush=True, **kwargs): """Remove the deleted flag for the given user.""" if not self.app.config.allow_user_deletion: raise exceptions.ConfigDoesNotAllowException( "The configuration of this Galaxy instance does not allow admins to undelete users." ) - if user.purged: + if item.purged: raise exceptions.ItemDeletionException("Purged user cannot be undeleted.") - super().undelete(user, flush=flush) - - def purge(self, user, flush=True): - """Purge the given user. They must have the deleted flag already.""" - if not self.app.config.allow_user_deletion: - raise exceptions.ConfigDoesNotAllowException( - "The configuration of this Galaxy instance does not allow admins to delete or purge users." - ) - if not user.deleted: - raise exceptions.MessageException(f"User '{user.email}' has not been deleted, so they cannot be purged.") - private_role = self.app.security_agent.get_private_user_role(user) - # Delete History - for active_history in user.active_histories: - self.session().refresh(active_history) - for hda in active_history.active_datasets: - # Delete HistoryDatasetAssociation - hda.deleted = True - self.session().add(hda) - active_history.deleted = True - self.session().add(active_history) - # Delete UserGroupAssociations - for uga in user.groups: - self.session().delete(uga) - # Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE - for ura in user.roles: - if ura.role_id != private_role.id: - self.session().delete(ura) - # Delete UserAddresses - for address in user.addresses: - self.session().delete(address) - compliance_log = logging.getLogger("COMPLIANCE") - compliance_log.info(f"delete-user-event: {user.username}") - # Maybe there is some case in the future where an admin needs - # to prove that a user was using a server for some reason (e.g. - # a court case.) So we make this painfully hard to recover (and - # not immediately reversable) in line with GDPR, but still - # leave open the possibility to prove someone was part of the - # server just in case. By knowing the exact email + approximate - # time of deletion, one could run through hashes for every - # second of the surrounding days/weeks. - pseudorandom_value = str(int(time.time())) - # Replace email + username with a (theoretically) unreversable - # hash. If provided with the username we can probably re-hash - # to identify if it is needed for some reason. - # - # Deleting multiple times will re-hash the username/email - email_hash = new_secure_hash_v2(user.email + pseudorandom_value) - uname_hash = new_secure_hash_v2(user.username + pseudorandom_value) - # Redact all roles user has - for role in user.all_roles(): - if self.app.config.redact_username_during_deletion: - role.name = role.name.replace(user.username, uname_hash) - role.description = role.description.replace(user.username, uname_hash) - - if self.app.config.redact_email_during_deletion: - role.name = role.name.replace(user.email, email_hash) - role.description = role.description.replace(user.email, email_hash) - self.session().add(role) - private_role.name = email_hash - private_role.description = f"Private Role for {email_hash}" - self.session().add(private_role) - # Redact user's email and username - user.email = email_hash - user.username = uname_hash - # Redact user addresses as well - if self.app.config.redact_user_address_during_deletion: - stmt = select(UserAddress).where(UserAddress.user_id == user.id) - for addr in self.session().scalars(stmt): - addr.desc = new_secure_hash_v2(addr.desc + pseudorandom_value) - addr.name = new_secure_hash_v2(addr.name + pseudorandom_value) - addr.institution = new_secure_hash_v2(addr.institution + pseudorandom_value) - addr.address = new_secure_hash_v2(addr.address + pseudorandom_value) - addr.city = new_secure_hash_v2(addr.city + pseudorandom_value) - addr.state = new_secure_hash_v2(addr.state + pseudorandom_value) - addr.postal_code = new_secure_hash_v2(addr.postal_code + pseudorandom_value) - addr.country = new_secure_hash_v2(addr.country + pseudorandom_value) - addr.phone = new_secure_hash_v2(addr.phone + pseudorandom_value) - self.session().add(addr) - # Purge the user - super().purge(user, flush=flush) + super().undelete(item, flush=flush) def _error_on_duplicate_email(self, email: str) -> None: """ @@ -277,11 +195,11 @@ def _error_on_duplicate_email(self, email: str) -> None: if self.by_email(email) is not None: raise exceptions.Conflict("Email must be unique", email=email) - def by_id(self, user_id: int) -> model.User: + def by_id(self, user_id: int) -> U: return self.app.model.session.get(self.model_class, user_id) # ---- filters - def by_email(self, email: str, filters=None, **kwargs) -> Optional[model.User]: + def by_email(self, email: str, filters=None, **kwargs) -> Optional[U]: """ Find a user by their email. """ @@ -292,7 +210,7 @@ def by_email(self, email: str, filters=None, **kwargs) -> Optional[model.User]: except exceptions.ObjectNotFound: return None - def by_api_key(self, api_key: str, sa_session=None): + def by_api_key(self, api_key: str, sa_session=None) -> Union[U, schema.BootstrapAdminUser]: """ Find a user by API key. """ @@ -312,7 +230,7 @@ def by_api_key(self, api_key: str, sa_session=None): raise exceptions.AuthenticationFailed("Provided API key has expired.") return provided_key.user - def by_oidc_access_token(self, access_token: str): + def by_oidc_access_token(self, access_token: str) -> Optional[U]: if hasattr(self.app, "authnz_manager") and self.app.authnz_manager: user = self.app.authnz_manager.match_access_token_to_user(self.app.model.session, access_token) return user @@ -329,7 +247,7 @@ def check_bootstrap_admin_api_key(self, api_key): return util.safe_str_cmp(bootstrap_hash, provided_hash) # ---- admin - def is_admin(self, user: Optional[model.User], trans=None) -> bool: + def is_admin(self, user: Optional[U], trans=None) -> bool: """Return True if this user is an admin (or session is authenticated as admin). Do not pass trans to simply check if an existing user object is an admin user, @@ -341,7 +259,7 @@ def is_admin(self, user: Optional[model.User], trans=None) -> bool: return trans and trans.user_is_admin return self.app.config.is_admin_user(user) - def admins(self, filters=None, **kwargs): + def admins(self, filters=None, **kwargs) -> List[U]: """ Return a list of admin Users. """ @@ -361,14 +279,14 @@ def error_unless_admin(self, user, msg="Administrators only", **kwargs): return user # ---- anonymous - def is_anonymous(self, user: Optional[model.User]) -> bool: + def is_anonymous(self, user: Optional[U]) -> bool: """ Return True if `user` is anonymous. """ # define here for single point of change and make more readable return user is None - def error_if_anonymous(self, user, msg="Log-in required", **kwargs): + def error_if_anonymous(self, user: Optional[U], msg="Log-in required", **kwargs): """ Raise an error if `user` is anonymous. """ @@ -377,9 +295,8 @@ def error_if_anonymous(self, user, msg="Log-in required", **kwargs): raise exceptions.AuthenticationFailed(msg, **kwargs) return user - def get_user_by_identity(self, identity): + def get_user_by_identity(self, identity) -> Optional[U]: """Get user by username or email.""" - user = None if VALID_EMAIL_RE.match(identity): # VALID_PUBLICNAME and VALID_EMAIL do not overlap, so 'identity' here is an email address user = get_user_by_email(self.session(), identity, self.model_class) @@ -391,71 +308,11 @@ def get_user_by_identity(self, identity): return user # ---- current - def current_user(self, trans): + def current_user(self, trans) -> Optional[U]: # define here for single point of change and make more readable # TODO: trans return trans.user - def user_can_do_run_as(self, user) -> bool: - run_as_users = [u for u in self.app.config.get("api_allow_run_as", "").split(",") if u] - if not run_as_users: - return False - user_in_run_as_users = user and user.email in run_as_users - # Can do if explicitly in list or master_api_key supplied. - can_do_run_as = user_in_run_as_users or user.bootstrap_admin_user - return can_do_run_as - - # ---- preferences - def preferences(self, user): - return dict(user.preferences.items()) - - # ---- roles and permissions - def private_role(self, user): - return self.app.security_agent.get_private_user_role(user) - - def sharing_roles(self, user): - return self.app.security_agent.get_sharing_roles(user) - - def default_permissions(self, user): - return self.app.security_agent.user_get_default_permissions(user) - - def quota(self, user, total=False, quota_source_label=None): - if total: - return self.app.quota_agent.get_quota_nice_size(user, quota_source_label=quota_source_label) - return self.app.quota_agent.get_percent(user=user, quota_source_label=quota_source_label) - - def quota_bytes(self, user, quota_source_label: Optional[str] = None): - return self.app.quota_agent.get_quota(user=user, quota_source_label=quota_source_label) - - def tags_used(self, user, tag_models=None): - """ - Return a list of distinct 'user_tname:user_value' strings that the - given user has used. - """ - # TODO: simplify and unify with tag manager - if self.is_anonymous(user): - return [] - - # get all the taggable model TagAssociations - if not tag_models: - tag_models = [v.tag_assoc_class for v in self.app.tag_handler.item_tag_assoc_info.values()] - - if not tag_models: - return [] - - # create a union of select statements for each tag model for this user - getting only the tname and user_value - all_stmts = [] - for tag_model in tag_models: - stmt = select(tag_model.user_tname, tag_model.user_value).where(tag_model.user == user) - all_stmts.append(stmt) - union_stmt = all_stmts[0].union(*all_stmts[1:]) # union the first select with the rest - - # boil the tag tuples down into a sorted list of DISTINCT name:val strings - tag_tuples = self.session().execute(union_stmt) # no need for DISTINCT: union is a set operation. - tags = [(f"{name}:{val}" if val else name) for name, val in tag_tuples] - # consider named tags while sorting - return sorted(tags, key=lambda str: re.sub("^name:", "#", str)) - def change_password(self, trans, password=None, confirm=None, token=None, id=None, current=None): """ Allows to change a user password with a token. @@ -488,7 +345,7 @@ def change_password(self, trans, password=None, confirm=None, token=None, id=Non else: return user, "User not found." - def __set_password(self, trans, user, password, confirm): + def __set_password(self, trans, user: U, password, confirm): if not password: return "Please provide a new password." if user: @@ -518,14 +375,14 @@ def __set_password(self, trans, user, password, confirm): else: return "Failed to determine user, access denied." - def impersonate(self, trans, user): + def impersonate(self, trans, user: U): if not trans.app.config.allow_user_impersonation: - raise exceptions.Message("User impersonation is not enabled in this instance of Galaxy.") + raise exceptions.MessageException("User impersonation is not enabled in this instance of Galaxy.") if user: trans.handle_user_logout() trans.handle_user_login(user) else: - raise exceptions.Message("Please provide a valid user.") + raise exceptions.MessageException("Please provide a valid user.") def send_activation_email(self, trans, email, username): """ @@ -566,7 +423,7 @@ def __get_activation_token(self, trans, email): """ Check for the activation token. Create new activation token and store it in the database if no token found. """ - user = get_user_by_email(trans.sa_session, email, self.app.model.User) + user = get_user_by_email(trans.sa_session, email, self.model_class) activation_token = user.activation_token if activation_token is None: activation_token = util.hash_util.new_secure_hash_v2(str(random.getrandbits(256))) @@ -635,22 +492,169 @@ def send_subscription_email(self, email): log.exception("Subscribing to the mailing list has failed.") return "Subscribing to the mailing list has failed." - def activate(self, user): + def activate(self, user: User): user.active = True self.session().add(user) session = self.session() with transaction(session): session.commit() - def get_or_create_remote_user(self, remote_user_email): + def _get_user_by_email_case_insensitive(self, session, email): + stmt = select(self.app.model.User).where(func.lower(self.app.model.User.email) == email.lower()).limit(1) + return session.scalars(stmt).first() + + +class UserManager(BaseUserManager[User]): + + def purge(self, item: User, flush=True, **kwargs): + """Purge the given user. They must have the deleted flag already.""" + if not self.app.config.allow_user_deletion: + raise exceptions.ConfigDoesNotAllowException( + "The configuration of this Galaxy instance does not allow admins to delete or purge users." + ) + if not item.deleted: + raise exceptions.MessageException(f"User '{item.email}' has not been deleted, so they cannot be purged.") + private_role = self.app.security_agent.get_private_user_role(item) + # Delete History + for active_history in item.active_histories: + self.session().refresh(active_history) + for hda in active_history.active_datasets: + # Delete HistoryDatasetAssociation + hda.deleted = True + self.session().add(hda) + active_history.deleted = True + self.session().add(active_history) + # Delete UserGroupAssociations + for uga in item.groups: + self.session().delete(uga) + # Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE + for ura in item.roles: + if ura.role_id != private_role.id: + self.session().delete(ura) + # Delete UserAddresses + for address in item.addresses: + self.session().delete(address) + compliance_log = logging.getLogger("COMPLIANCE") + compliance_log.info(f"delete-user-event: {item.username}") + # Maybe there is some case in the future where an admin needs + # to prove that a user was using a server for some reason (e.g. + # a court case.) So we make this painfully hard to recover (and + # not immediately reversable) in line with GDPR, but still + # leave open the possibility to prove someone was part of the + # server just in case. By knowing the exact email + approximate + # time of deletion, one could run through hashes for every + # second of the surrounding days/weeks. + pseudorandom_value = str(int(time.time())) + # Replace email + username with a (theoretically) unreversable + # hash. If provided with the username we can probably re-hash + # to identify if it is needed for some reason. + # + # Deleting multiple times will re-hash the username/email + email_hash = new_secure_hash_v2(item.email + pseudorandom_value) + uname_hash = new_secure_hash_v2(item.username + pseudorandom_value) + # Redact all roles user has + for role in item.all_roles(): + if self.app.config.redact_username_during_deletion: + role.name = role.name.replace(item.username, uname_hash) + role.description = role.description.replace(item.username, uname_hash) + + if self.app.config.redact_email_during_deletion: + role.name = role.name.replace(item.email, email_hash) + role.description = role.description.replace(item.email, email_hash) + self.session().add(role) + private_role.name = email_hash + private_role.description = f"Private Role for {email_hash}" + self.session().add(private_role) + # Redact user's email and username + item.email = email_hash + item.username = uname_hash + # Redact user addresses as well + if self.app.config.redact_user_address_during_deletion: + stmt = select(UserAddress).where(UserAddress.user_id == item.id) + for addr in self.session().scalars(stmt): + addr.desc = new_secure_hash_v2(addr.desc + pseudorandom_value) + addr.name = new_secure_hash_v2(addr.name + pseudorandom_value) + addr.institution = new_secure_hash_v2(addr.institution + pseudorandom_value) + addr.address = new_secure_hash_v2(addr.address + pseudorandom_value) + addr.city = new_secure_hash_v2(addr.city + pseudorandom_value) + addr.state = new_secure_hash_v2(addr.state + pseudorandom_value) + addr.postal_code = new_secure_hash_v2(addr.postal_code + pseudorandom_value) + addr.country = new_secure_hash_v2(addr.country + pseudorandom_value) + addr.phone = new_secure_hash_v2(addr.phone + pseudorandom_value) + self.session().add(addr) + # Purge the user + super().purge(item, flush=flush) + + def user_can_do_run_as(self, user: Union[U, schema.BootstrapAdminUser]) -> bool: + run_as_users = [u for u in self.app.config.get("api_allow_run_as", "").split(",") if u] + if not run_as_users: + return False + user_in_run_as_users = user and user.email in run_as_users + # Can do if explicitly in list or master_api_key supplied. + can_do_run_as = user_in_run_as_users or user.bootstrap_admin_user + return can_do_run_as + + # ---- preferences + def preferences(self, user: User): + return dict(user.preferences.items()) + + # ---- roles and permissions + def private_role(self, user: User): + return self.app.security_agent.get_private_user_role(user) + + def sharing_roles(self, user: User): + return self.app.security_agent.get_sharing_roles(user) + + def default_permissions(self, user: User): + return self.app.security_agent.user_get_default_permissions(user) + + def quota(self, user: User, total=False, quota_source_label=None): + if total: + return self.app.quota_agent.get_quota_nice_size(user, quota_source_label=quota_source_label) + return self.app.quota_agent.get_percent(user=user, quota_source_label=quota_source_label) + + def quota_bytes(self, user: User, quota_source_label: Optional[str] = None): + return self.app.quota_agent.get_quota(user=user, quota_source_label=quota_source_label) + + def tags_used(self, user: User, tag_models=None): + """ + Return a list of distinct 'user_tname:user_value' strings that the + given user has used. + """ + # TODO: simplify and unify with tag manager + if self.is_anonymous(user): + return [] + + # get all the taggable model TagAssociations + if not tag_models and isinstance(self.app, MinimalApp): + tag_models = [v.tag_assoc_class for v in self.app.tag_handler.item_tag_assoc_info.values()] + + if not tag_models: + return [] + + # create a union of select statements for each tag model for this user - getting only the tname and user_value + all_stmts = [] + for tag_model in tag_models: + stmt = select(tag_model.user_tname, tag_model.user_value).where(tag_model.user == user) + all_stmts.append(stmt) + union_stmt = all_stmts[0].union(*all_stmts[1:]) # union the first select with the rest + + # boil the tag tuples down into a sorted list of DISTINCT name:val strings + tag_tuples = self.session().execute(union_stmt) # no need for DISTINCT: union is a set operation. + tags = [(f"{name}:{val}" if val else name) for name, val in tag_tuples] + # consider named tags while sorting + return sorted(tags, key=lambda str: re.sub("^name:", "#", str)) + + def get_or_create_remote_user(self, remote_user_email) -> User: """ Create a remote user with the email remote_user_email and return it """ if not self.app.config.use_remote_user: - return None + # All calling code checks this, so should be unreachable. + raise Exception("Remote user creation not enabled on this instance") if getattr(self.app.config, "normalize_remote_user_email", False): remote_user_email = remote_user_email.lower() - user = get_user_by_email(self.session(), remote_user_email, self.app.model.User) + user = get_user_by_email(self.session(), remote_user_email, self.model_class) if user: # GVK: June 29, 2009 - This is to correct the behavior of a previous bug where a private # role and default user / history permissions were not set for remote users. When a @@ -689,9 +693,23 @@ def get_or_create_remote_user(self, remote_user_email): # self.log_event( "Automatically created account '%s'", user.email ) return user - def _get_user_by_email_case_insensitive(self, session, email): - stmt = select(self.app.model.User).where(func.lower(self.app.model.User.email) == email.lower()).limit(1) - return session.scalars(stmt).first() + def delete(self, item: User, flush=True, **kwargs): + super().delete(item, flush, **kwargs) + self._stop_all_jobs_from_user(item) + + def _stop_all_jobs_from_user(self, user): + active_jobs = self._get_all_active_jobs_from_user(user) + session = self.session() + for job in active_jobs: + job.mark_deleted(self.app.config.track_jobs_in_database) + with transaction(session): + session.commit() + + def _get_all_active_jobs_from_user(self, user: User) -> List[Job]: + """Get all jobs that are not ready yet and belong to the given user.""" + stmt = select(Job).where(and_(Job.user_id == user.id, Job.state.in_(Job.non_ready_states))) + jobs = self.session().scalars(stmt) + return jobs # type:ignore[return-value] class UserSerializer(base.ModelSerializer, deletable.PurgableSerializerMixin): @@ -879,10 +897,15 @@ def get_users_by_ids(session: galaxy_scoped_session, user_ids): return session.scalars(stmt).all() -# The get_user_by_email and get_user_by_username functions may be called from -# the tool_shed app, which has its own User model, which is different from -# galaxy.model.User. In that case, the tool_shed user model should be passed as -# the model_class argument. +# https://github.com/python/mypy/issues/3737#issuecomment-316263133 + + +@overload +def get_user_by_email(session, email: str, model_class: Type[User] = User, case_sensitive=True) -> Optional[User]: ... +@overload +def get_user_by_email(session, email: str, model_class: Type[U], case_sensitive=True) -> Optional[U]: ... + + def get_user_by_email(session, email: str, model_class=User, case_sensitive=True): filter_clause = model_class.email == email if not case_sensitive: From d116ea9a00c711ab5af62bf532de16d2dee8f115 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 09:42:37 +0200 Subject: [PATCH 03/30] Add ToolShed specific user manager --- lib/galaxy/webapps/base/webapp.py | 9 ++++++++- lib/galaxy/webapps/galaxy/api/__init__.py | 7 +++++-- lib/tool_shed/managers/users.py | 5 +++++ lib/tool_shed/webapp/api2/__init__.py | 2 +- lib/tool_shed/webapp/api2/users.py | 2 +- lib/tool_shed/webapp/model/__init__.py | 3 ++- 6 files changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/webapps/base/webapp.py b/lib/galaxy/webapps/base/webapp.py index 0cd502606613..6a23ea92f295 100644 --- a/lib/galaxy/webapps/base/webapp.py +++ b/lib/galaxy/webapps/base/webapp.py @@ -14,6 +14,8 @@ Any, Dict, Optional, + TYPE_CHECKING, + Union, ) from urllib.parse import urlparse @@ -65,6 +67,10 @@ ) from galaxy.web.framework.middleware.static import CacheableStaticURLParser as Static +if TYPE_CHECKING: + from galaxy.model import User + from galaxy.schema import BootstrapAdminUser + log = logging.getLogger(__name__) @@ -310,7 +316,7 @@ def __init__( ) -> None: self._app = app self.webapp = webapp - self.user_manager = app[UserManager] + self.user_manager: UserManager = app[UserManager] self.session_manager = app[GalaxySessionManager] super().__init__(environ) config = self.app.config @@ -544,6 +550,7 @@ def _authenticate_api(self, session_cookie: str) -> Optional[str]: api_key = self.request.params.get("key", None) or self.request.headers.get("x-api-key", None) secure_id = self.get_cookie(name=session_cookie) api_key_supplied = self.environ.get("is_api_request", False) and api_key + user: Optional[Union[User, BootstrapAdminUser]] if api_key_supplied: # Sessionless API transaction, we just need to associate a user. try: diff --git a/lib/galaxy/webapps/galaxy/api/__init__.py b/lib/galaxy/webapps/galaxy/api/__init__.py index 3f19076a2367..e88327c25d60 100644 --- a/lib/galaxy/webapps/galaxy/api/__init__.py +++ b/lib/galaxy/webapps/galaxy/api/__init__.py @@ -14,6 +14,7 @@ Tuple, Type, TypeVar, + Union, ) from urllib.parse import ( urlencode, @@ -72,6 +73,7 @@ from galaxy.managers.session import GalaxySessionManager from galaxy.managers.users import UserManager from galaxy.model import User +from galaxy.schema import BootstrapAdminUser from galaxy.schema.fields import DecodedDatabaseIdField from galaxy.security.idencoding import IdEncodingHelper from galaxy.structured_app import StructuredApp @@ -156,7 +158,8 @@ def get_api_user( "Only admins and designated users can make API calls on behalf of other users." ), ), -) -> Optional[User]: +) -> Optional[Union[User, BootstrapAdminUser]]: + user: Optional[Union[User, BootstrapAdminUser]] if api_key := key or x_api_key: user = user_manager.by_api_key(api_key=api_key) elif bearer_token: @@ -164,7 +167,7 @@ def get_api_user( else: return None if run_as: - if user_manager.user_can_do_run_as(user): + if user and user_manager.user_can_do_run_as(user): return user_manager.by_id(run_as) else: raise UserCannotRunAsException diff --git a/lib/tool_shed/managers/users.py b/lib/tool_shed/managers/users.py index ef1ca01ff6f5..fddddc71b622 100644 --- a/lib/tool_shed/managers/users.py +++ b/lib/tool_shed/managers/users.py @@ -3,6 +3,7 @@ from sqlalchemy import select from galaxy.exceptions import RequestParameterInvalidException +from galaxy.managers.users import BaseUserManager from galaxy.model.base import transaction from galaxy.security.validate_user_input import ( validate_email, @@ -18,6 +19,10 @@ ) +class UserManager(BaseUserManager[User]): + pass + + def index(app: ToolShedApp, deleted: bool) -> List[ApiUser]: users: List[ApiUser] = [] for user in get_users_by_deleted(app.model.context, app.model.User, deleted): diff --git a/lib/tool_shed/webapp/api2/__init__.py b/lib/tool_shed/webapp/api2/__init__.py index 845e1a9a2cfc..9af1a57bb058 100644 --- a/lib/tool_shed/webapp/api2/__init__.py +++ b/lib/tool_shed/webapp/api2/__init__.py @@ -28,7 +28,6 @@ from galaxy.exceptions import AdminRequiredException from galaxy.managers.session import GalaxySessionManager -from galaxy.managers.users import UserManager from galaxy.model.base import transaction from galaxy.security.idencoding import IdEncodingHelper from galaxy.util import unicodify @@ -46,6 +45,7 @@ SessionRequestContext, SessionRequestContextImpl, ) +from tool_shed.managers.users import UserManager from tool_shed.structured_app import ToolShedApp from tool_shed.webapp import app as tool_shed_app_mod from tool_shed.webapp.model import ( diff --git a/lib/tool_shed/webapp/api2/users.py b/lib/tool_shed/webapp/api2/users.py index f55e51438cbc..ea119cc6d888 100644 --- a/lib/tool_shed/webapp/api2/users.py +++ b/lib/tool_shed/webapp/api2/users.py @@ -23,7 +23,6 @@ RequestParameterInvalidException, ) from galaxy.managers.api_keys import ApiKeyManager -from galaxy.managers.users import UserManager from galaxy.model.base import transaction from galaxy.webapps.base.webapp import create_new_session from tool_shed.context import SessionRequestContext @@ -31,6 +30,7 @@ api_create_user, get_api_user, index, + UserManager, ) from tool_shed.structured_app import ToolShedApp from tool_shed.webapp.model import ( diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index a0b1a178cdd5..d1c32354253d 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -41,6 +41,7 @@ import tool_shed.repository_types.util as rt_util from galaxy import util +from galaxy.model import _HasTable from galaxy.model.custom_types import ( MutableJSONType, TrimmedString, @@ -76,7 +77,7 @@ class DeclarativeMeta(_DeclarativeMeta, type): mapper_registry = registry() -class Base(metaclass=DeclarativeMeta): +class Base(_HasTable, metaclass=DeclarativeMeta): __abstract__ = True registry = mapper_registry metadata = mapper_registry.metadata From d4b2cc51dbb24eff6aa1e575fcbf03c3db6e3aaf Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 09:44:08 +0200 Subject: [PATCH 04/30] Fix bugs and type hints reveealed by improved type hints --- lib/galaxy/visualization/genomes.py | 2 +- lib/galaxy/webapps/galaxy/api/users.py | 2 +- lib/galaxy/webapps/galaxy/services/histories.py | 3 ++- lib/galaxy/webapps/galaxy/services/quotas.py | 7 ++++++- lib/tool_shed/webapp/api2/__init__.py | 4 +++- test/integration/test_user_preferences.py | 1 + 6 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/visualization/genomes.py b/lib/galaxy/visualization/genomes.py index 066785feae7a..eb21c5956047 100644 --- a/lib/galaxy/visualization/genomes.py +++ b/lib/galaxy/visualization/genomes.py @@ -380,7 +380,7 @@ def reference(self, trans, dbkey, chrom, low, high): else: dbkey_user = trans.user - if not self.has_reference_data(dbkey, dbkey_user): + if not self.has_reference_data(dbkey, dbkey_user) or not dbkey_user: raise ReferenceDataError(f"No reference data for {dbkey}") # diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index a5427042c5bd..83449c182a5c 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -636,7 +636,7 @@ def create( else: raise exceptions.NotImplemented() item = user.to_dict(view="element", value_mapper={"total_disk_usage": float}) - return item + return CreatedUserModel(**item) @router.get( "/api/users", diff --git a/lib/galaxy/webapps/galaxy/services/histories.py b/lib/galaxy/webapps/galaxy/services/histories.py index d5a6a75ffba7..d007919bf042 100644 --- a/lib/galaxy/webapps/galaxy/services/histories.py +++ b/lib/galaxy/webapps/galaxy/services/histories.py @@ -505,7 +505,8 @@ def count( Returns number of histories for the current user. """ current_user = self.user_manager.current_user(trans) - if self.user_manager.is_anonymous(current_user): + if current_user is None: + # user is anonymous current_history = self.manager.get_current(trans) return 1 if current_history else 0 return self.manager.get_active_count(current_user) diff --git a/lib/galaxy/webapps/galaxy/services/quotas.py b/lib/galaxy/webapps/galaxy/services/quotas.py index 38b6a69fe849..812897151d6b 100644 --- a/lib/galaxy/webapps/galaxy/services/quotas.py +++ b/lib/galaxy/webapps/galaxy/services/quotas.py @@ -130,7 +130,12 @@ def get_user_id(item): try: return trans.security.decode_id(item) except Exception: - return get_user_by_email(trans.sa_session, item).id + user = get_user_by_email(trans.sa_session, item) + if user: + return user.id + else: + # caught in loop below + raise Exception("No user found") def get_group_id(item): try: diff --git a/lib/tool_shed/webapp/api2/__init__.py b/lib/tool_shed/webapp/api2/__init__.py index 9af1a57bb058..6e1516d73c7f 100644 --- a/lib/tool_shed/webapp/api2/__init__.py +++ b/lib/tool_shed/webapp/api2/__init__.py @@ -7,6 +7,7 @@ Optional, Type, TypeVar, + Union, ) from fastapi import ( @@ -29,6 +30,7 @@ from galaxy.exceptions import AdminRequiredException from galaxy.managers.session import GalaxySessionManager from galaxy.model.base import transaction +from galaxy.schema import BootstrapAdminUser from galaxy.security.idencoding import IdEncodingHelper from galaxy.util import unicodify from galaxy.web.framework.decorators import require_admin_message @@ -88,7 +90,7 @@ def get_api_user( user_manager: UserManager = depends(UserManager), key: str = Security(api_key_query), x_api_key: str = Security(api_key_header), -) -> Optional[User]: +) -> Optional[Union[User, BootstrapAdminUser]]: api_key = key or x_api_key if not api_key: return None diff --git a/test/integration/test_user_preferences.py b/test/integration/test_user_preferences.py index b01f4850489d..94788d25af8f 100644 --- a/test/integration/test_user_preferences.py +++ b/test/integration/test_user_preferences.py @@ -21,6 +21,7 @@ def test_user_theme(self): app = cast(Any, self._test_driver.app if self._test_driver else None) db_user = get_user_by_email(app.model.session, user["email"]) + assert db_user # create some initial data put(url) From 6d046f5db43af9cb1dfdf1305ce422ab19a1d879 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 10:40:59 +0200 Subject: [PATCH 05/30] Drop unnecessary type ignores --- lib/galaxy/celery/tasks.py | 2 +- lib/galaxy/managers/sharable.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/celery/tasks.py b/lib/galaxy/celery/tasks.py index e1ac7456ad41..c7490b15bac2 100644 --- a/lib/galaxy/celery/tasks.py +++ b/lib/galaxy/celery/tasks.py @@ -196,7 +196,7 @@ def set_metadata( try: if overwrite: hda_manager.overwrite_metadata(dataset_instance) - dataset_instance.datatype.set_meta(dataset_instance) # type:ignore [arg-type] + dataset_instance.datatype.set_meta(dataset_instance) dataset_instance.set_peek() # Reset SETTING_METADATA state so the dataset instance getter picks the dataset state dataset_instance.set_metadata_success_state() diff --git a/lib/galaxy/managers/sharable.py b/lib/galaxy/managers/sharable.py index 349a54faff43..49d09fd30941 100644 --- a/lib/galaxy/managers/sharable.py +++ b/lib/galaxy/managers/sharable.py @@ -30,10 +30,7 @@ true, ) -from galaxy import ( - exceptions, - model, -) +from galaxy import exceptions from galaxy.managers import ( annotatable, base, @@ -107,7 +104,7 @@ def is_owner(self, item: U, user: Optional[User], **kwargs: Any) -> bool: # ... effectively a good fit to have this here, but not semantically if self.user_manager.is_admin(user, trans=kwargs.get("trans", None)): return True - return item.user == user # type:ignore[attr-defined] + return item.user == user def is_accessible(self, item: U, user: Optional[User], **kwargs: Any) -> bool: """ From d32b6dd344f8501e3c030201327be3d35ef132ad Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 12 May 2024 20:53:34 +0200 Subject: [PATCH 06/30] Make ApiKeyManager generic for galaxy or ts models --- lib/galaxy/app.py | 2 +- lib/galaxy/managers/api_keys.py | 33 ++++++++++++++++++------ lib/galaxy/tools/evaluation.py | 4 ++- lib/tool_shed/managers/api_keys.py | 6 +++++ lib/tool_shed/webapp/app.py | 8 ++++-- lib/tool_shed/webapp/controllers/user.py | 6 ++++- 6 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 lib/tool_shed/managers/api_keys.py diff --git a/lib/galaxy/app.py b/lib/galaxy/app.py index eec2318de4e9..eb8f25d231c0 100644 --- a/lib/galaxy/app.py +++ b/lib/galaxy/app.py @@ -722,7 +722,7 @@ def __init__(self, **kwargs) -> None: self.test_data_resolver = self._register_singleton( TestDataResolver, TestDataResolver(file_dirs=self.config.tool_test_data_directories) ) - self.api_keys_manager = self._register_singleton(ApiKeyManager) + self.api_keys_manager = self._register_singleton(ApiKeyManager[galaxy.model.APIKeys, galaxy.model.User]) # Tool Data Tables self._configure_tool_data_tables(from_shed_config=False) diff --git a/lib/galaxy/managers/api_keys.py b/lib/galaxy/managers/api_keys.py index e306049259cb..5ee501deaac6 100644 --- a/lib/galaxy/managers/api_keys.py +++ b/lib/galaxy/managers/api_keys.py @@ -1,29 +1,45 @@ +from typing import ( + Generic, + Optional, + TYPE_CHECKING, + TypeVar, + Union, +) + from sqlalchemy import ( false, select, update, ) -from typing_extensions import Protocol from galaxy.model.base import transaction from galaxy.structured_app import BasicSharedApp +if TYPE_CHECKING: + from galaxy.model import ( + APIKeys as GalaxyAPIKeys, + User as GalaxyUser, + ) + from tool_shed.webapp.model import ( + APIKeys as ToolShedAPIKeys, + User as ToolShedUser, + ) -class IsUserModel(Protocol): - id: int +A = TypeVar("A", bound=Union["GalaxyAPIKeys", "ToolShedAPIKeys"]) +U = TypeVar("U", bound=Union["GalaxyUser", "ToolShedUser"]) -class ApiKeyManager: +class ApiKeyManager(Generic[A, U]): def __init__(self, app: BasicSharedApp): self.app = app self.session = self.app.model.context - def get_api_key(self, user: IsUserModel): + def get_api_key(self, user: U) -> Optional[A]: APIKeys = self.app.model.APIKeys stmt = select(APIKeys).filter_by(user_id=user.id, deleted=False).order_by(APIKeys.create_time.desc()).limit(1) return self.session.scalars(stmt).first() - def create_api_key(self, user: IsUserModel): + def create_api_key(self, user: U) -> A: guid = self.app.security.get_new_guid() new_key = self.app.model.APIKeys() new_key.user_id = user.id @@ -33,15 +49,16 @@ def create_api_key(self, user: IsUserModel): self.session.commit() return new_key - def get_or_create_api_key(self, user: IsUserModel) -> str: + def get_or_create_api_key(self, user: U) -> str: # Logic Galaxy has always used - but it would appear to have a race # condition. Worth fixing? Would kind of need a message queue to fix # in multiple process mode. api_key = self.get_api_key(user) key = api_key.key if api_key else self.create_api_key(user).key + assert key return key - def delete_api_key(self, user: IsUserModel) -> None: + def delete_api_key(self, user: U) -> None: """Marks the current user API key as deleted.""" # Before it was possible to create multiple API keys for the same user although they were not considered valid # So all non-deleted keys are marked as deleted for backward compatibility diff --git a/lib/galaxy/tools/evaluation.py b/lib/galaxy/tools/evaluation.py index 07c04ab6055a..4af32d4469d5 100644 --- a/lib/galaxy/tools/evaluation.py +++ b/lib/galaxy/tools/evaluation.py @@ -672,7 +672,9 @@ def _build_environment_variables(self): if self._user and isinstance(self.app, BasicSharedApp): from galaxy.managers import api_keys - environment_variable_template = api_keys.ApiKeyManager(self.app).get_or_create_api_key(self._user) + environment_variable_template = api_keys.ApiKeyManager[model.APIKeys, model.User]( + self.app + ).get_or_create_api_key(self._user) else: environment_variable_template = "" is_template = False diff --git a/lib/tool_shed/managers/api_keys.py b/lib/tool_shed/managers/api_keys.py new file mode 100644 index 000000000000..d38ed6b1a537 --- /dev/null +++ b/lib/tool_shed/managers/api_keys.py @@ -0,0 +1,6 @@ +from galaxy.managers.api_keys import ApiKeyManager +from tool_shed.webapp.model import APIKeys, User + + +class ToolShedApiKeyManager(ApiKeyManager[APIKeys, User]): + pass diff --git a/lib/tool_shed/webapp/app.py b/lib/tool_shed/webapp/app.py index c71ad3938c68..66ed229f902f 100644 --- a/lib/tool_shed/webapp/app.py +++ b/lib/tool_shed/webapp/app.py @@ -22,7 +22,6 @@ from galaxy.managers.api_keys import ApiKeyManager from galaxy.managers.citations import CitationsManager from galaxy.managers.dbkeys import GenomeBuilds -from galaxy.managers.users import UserManager from galaxy.model.base import SharedModelMapping from galaxy.model.tags import CommunityTagHandler from galaxy.quota import ( @@ -33,8 +32,13 @@ from galaxy.structured_app import BasicSharedApp from galaxy.web_stack import application_stack_instance from tool_shed.grids.repository_grid_filter_manager import RepositoryGridFilterManager +from tool_shed.managers.users import UserManager from tool_shed.structured_app import ToolShedApp from tool_shed.util.hgweb_config import hgweb_config_manager +from tool_shed.webapp.model import ( + APIKeys, + User, +) from tool_shed.webapp.model.migrations import verify_database from . import config @@ -84,7 +88,7 @@ def __init__(self, **kwd) -> None: self._register_singleton(mapping.ToolShedModelMapping, model) self._register_singleton(scoped_session, self.model.context) self.user_manager = self._register_singleton(UserManager, UserManager(self, app_type="tool_shed")) - self.api_keys_manager = self._register_singleton(ApiKeyManager) + self.api_keys_manager = self._register_singleton(ApiKeyManager[APIKeys, User]) # initialize the Tool Shed tag handler. self.tag_handler = CommunityTagHandler(self) # Initialize the Tool Shed tool data tables. Never pass a configuration file here diff --git a/lib/tool_shed/webapp/controllers/user.py b/lib/tool_shed/webapp/controllers/user.py index eb9a11878eba..ee79d31971cd 100644 --- a/lib/tool_shed/webapp/controllers/user.py +++ b/lib/tool_shed/webapp/controllers/user.py @@ -19,6 +19,10 @@ from galaxy.web.form_builder import CheckboxField from galaxy.webapps.galaxy.controllers.user import User as BaseUser from tool_shed.webapp.framework.decorators import require_login +from tool_shed.webapp.model import ( + APIKeys, + User as ToolShedUser, +) log = logging.getLogger(__name__) @@ -312,7 +316,7 @@ def api_keys(self, trans, cntrller, **kwd): message = escape(util.restore_text(params.get("message", ""))) status = params.get("status", "done") if params.get("new_api_key_button", False): - ApiKeyManager(trans.app).create_api_key(trans.user) + ApiKeyManager[APIKeys, ToolShedUser](trans.app).create_api_key(trans.user) message = "Generated a new web API key" status = "done" return trans.fill_template( From f82bdd73d47e35640ff1c941c7b6211fc24161d1 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 12 May 2024 20:54:37 +0200 Subject: [PATCH 07/30] Fix and adjust model type optionality The model optionality on the sqlalchemy side seems weird. --- lib/galaxy/managers/users.py | 9 ++++++--- lib/galaxy/model/__init__.py | 10 +++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index aa022bac483d..5b24c5cbc168 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -572,15 +572,18 @@ def purge(self, item: User, flush=True, **kwargs): if self.app.config.redact_user_address_during_deletion: stmt = select(UserAddress).where(UserAddress.user_id == item.id) for addr in self.session().scalars(stmt): - addr.desc = new_secure_hash_v2(addr.desc + pseudorandom_value) + if addr.desc: + addr.desc = new_secure_hash_v2(addr.desc + pseudorandom_value) addr.name = new_secure_hash_v2(addr.name + pseudorandom_value) - addr.institution = new_secure_hash_v2(addr.institution + pseudorandom_value) + if addr.institution: + addr.institution = new_secure_hash_v2(addr.institution + pseudorandom_value) addr.address = new_secure_hash_v2(addr.address + pseudorandom_value) addr.city = new_secure_hash_v2(addr.city + pseudorandom_value) addr.state = new_secure_hash_v2(addr.state + pseudorandom_value) addr.postal_code = new_secure_hash_v2(addr.postal_code + pseudorandom_value) addr.country = new_secure_hash_v2(addr.country + pseudorandom_value) - addr.phone = new_secure_hash_v2(addr.phone + pseudorandom_value) + if addr.phone: + addr.phone = new_secure_hash_v2(addr.phone + pseudorandom_value) self.session().add(addr) # Purge the user super().purge(item, flush=flush) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 73f6296d2a18..4c8b9c9d21cd 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -777,7 +777,7 @@ class User(Base, Dictifiable, RepresentById): create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True) email: Mapped[str] = mapped_column(TrimmedString(255), index=True) - username: Mapped[Optional[str]] = mapped_column(TrimmedString(255), index=True, unique=True) + username: Mapped[str] = mapped_column(TrimmedString(255), index=True, unique=True) password: Mapped[str] = mapped_column(TrimmedString(255)) last_password_change: Mapped[Optional[datetime]] = mapped_column(default=now) external: Mapped[Optional[bool]] = mapped_column(default=False) @@ -6834,10 +6834,10 @@ class HistoryDatasetCollectionAssociation( __tablename__ = "history_dataset_collection_association" id: Mapped[int] = mapped_column(primary_key=True) - collection_id: Mapped[Optional[int]] = mapped_column(ForeignKey("dataset_collection.id"), index=True) - history_id: Mapped[Optional[int]] = mapped_column(ForeignKey("history.id"), index=True) + collection_id: Mapped[int] = mapped_column(ForeignKey("dataset_collection.id"), index=True) + history_id: Mapped[int] = mapped_column(ForeignKey("history.id"), index=True) name: Mapped[Optional[str]] = mapped_column(TrimmedString(255)) - hid: Mapped[Optional[int]] + hid: Mapped[int] visible: Mapped[Optional[bool]] deleted: Mapped[Optional[bool]] = mapped_column(default=False) copied_from_history_dataset_collection_association_id: Mapped[Optional[int]] = mapped_column( @@ -6852,7 +6852,7 @@ class HistoryDatasetCollectionAssociation( update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, index=True, nullable=True) collection = relationship("DatasetCollection") - history: Mapped[Optional["History"]] = relationship(back_populates="dataset_collections") + history: Mapped["History"] = relationship(back_populates="dataset_collections") copied_from_history_dataset_collection_association = relationship( "HistoryDatasetCollectionAssociation", From ce735536b6445ec5566e6a33d37bd112d500e708 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 12 May 2024 21:04:11 +0200 Subject: [PATCH 08/30] Cannot use union in lagom --- lib/galaxy/app.py | 4 ++-- lib/galaxy/managers/api_keys.py | 4 ++++ lib/galaxy/structured_app.py | 3 ++- lib/galaxy/tools/evaluation.py | 6 +++--- lib/galaxy/webapps/galaxy/services/authenticate.py | 4 ++-- lib/galaxy/webapps/galaxy/services/users.py | 2 +- lib/tool_shed/webapp/api2/users.py | 4 ++-- lib/tool_shed/webapp/app.py | 8 ++------ lib/tool_shed/webapp/controllers/user.py | 8 ++------ 9 files changed, 20 insertions(+), 23 deletions(-) diff --git a/lib/galaxy/app.py b/lib/galaxy/app.py index eb8f25d231c0..b97393f745f7 100644 --- a/lib/galaxy/app.py +++ b/lib/galaxy/app.py @@ -47,7 +47,7 @@ from galaxy.files.templates import ConfiguredFileSourceTemplates from galaxy.job_metrics import JobMetrics from galaxy.jobs.manager import JobManager -from galaxy.managers.api_keys import ApiKeyManager +from galaxy.managers.api_keys import GalaxyApiKeyManager from galaxy.managers.citations import CitationsManager from galaxy.managers.collections import DatasetCollectionManager from galaxy.managers.dbkeys import GenomeBuilds @@ -722,7 +722,7 @@ def __init__(self, **kwargs) -> None: self.test_data_resolver = self._register_singleton( TestDataResolver, TestDataResolver(file_dirs=self.config.tool_test_data_directories) ) - self.api_keys_manager = self._register_singleton(ApiKeyManager[galaxy.model.APIKeys, galaxy.model.User]) + self.api_keys_manager = self._register_singleton(GalaxyApiKeyManager) # Tool Data Tables self._configure_tool_data_tables(from_shed_config=False) diff --git a/lib/galaxy/managers/api_keys.py b/lib/galaxy/managers/api_keys.py index 5ee501deaac6..d0046374970b 100644 --- a/lib/galaxy/managers/api_keys.py +++ b/lib/galaxy/managers/api_keys.py @@ -76,3 +76,7 @@ def _mark_all_api_keys_as_deleted(self, user_id: int): .execution_options(synchronize_session="evaluate") ) return self.session.execute(stmt) + + +class GalaxyApiKeyManager(ApiKeyManager["GalaxyAPIKeys", "GalaxyUser"]): + pass diff --git a/lib/galaxy/structured_app.py b/lib/galaxy/structured_app.py index ebab917098fc..896c7e272038 100644 --- a/lib/galaxy/structured_app.py +++ b/lib/galaxy/structured_app.py @@ -43,6 +43,7 @@ if TYPE_CHECKING: from galaxy.config_watchers import ConfigWatchers from galaxy.jobs import JobConfiguration + from galaxy.managers.api_keys import GalaxyApiKeyManager from galaxy.managers.collections import DatasetCollectionManager from galaxy.managers.hdas import HDAManager from galaxy.managers.histories import HistoryManager @@ -159,5 +160,5 @@ class StructuredApp(MinimalManagerApp): watchers: "ConfigWatchers" workflow_scheduling_manager: Any # 'galaxy.workflow.scheduling_manager.WorkflowSchedulingManager' interactivetool_manager: Any - api_keys_manager: Any # 'galaxy.managers.api_keys.ApiKeyManager' + api_keys_manager: "GalaxyApiKeyManager" # 'galaxy.managers.api_keys.ApiKeyManager' visualizations_registry: Any # 'galaxy.visualization.plugins.registry.VisualizationsRegistry' diff --git a/lib/galaxy/tools/evaluation.py b/lib/galaxy/tools/evaluation.py index 4af32d4469d5..72c12ed4b94f 100644 --- a/lib/galaxy/tools/evaluation.py +++ b/lib/galaxy/tools/evaluation.py @@ -672,9 +672,9 @@ def _build_environment_variables(self): if self._user and isinstance(self.app, BasicSharedApp): from galaxy.managers import api_keys - environment_variable_template = api_keys.ApiKeyManager[model.APIKeys, model.User]( - self.app - ).get_or_create_api_key(self._user) + environment_variable_template = api_keys.GalaxyApiKeyManager(self.app).get_or_create_api_key( + self._user + ) else: environment_variable_template = "" is_template = False diff --git a/lib/galaxy/webapps/galaxy/services/authenticate.py b/lib/galaxy/webapps/galaxy/services/authenticate.py index f71caf88cdbd..774a024161cb 100644 --- a/lib/galaxy/webapps/galaxy/services/authenticate.py +++ b/lib/galaxy/webapps/galaxy/services/authenticate.py @@ -13,7 +13,7 @@ from galaxy import exceptions from galaxy.auth import AuthManager -from galaxy.managers.api_keys import ApiKeyManager +from galaxy.managers.api_keys import GalaxyApiKeyManager from galaxy.managers.users import UserManager from galaxy.util import ( smart_str, @@ -29,7 +29,7 @@ class APIKeyResponse(BaseModel): class AuthenticationService: - def __init__(self, user_manager: UserManager, auth_manager: AuthManager, api_keys_manager: ApiKeyManager): + def __init__(self, user_manager: UserManager, auth_manager: AuthManager, api_keys_manager: GalaxyApiKeyManager): self._user_manager = user_manager self._auth_manager = auth_manager self._api_keys_manager = api_keys_manager diff --git a/lib/galaxy/webapps/galaxy/services/users.py b/lib/galaxy/webapps/galaxy/services/users.py index 909a9898f193..0fee8af25a7c 100644 --- a/lib/galaxy/webapps/galaxy/services/users.py +++ b/lib/galaxy/webapps/galaxy/services/users.py @@ -55,7 +55,7 @@ def __init__( self, security: IdEncodingHelper, user_manager: UserManager, - api_key_manager: api_keys.ApiKeyManager, + api_key_manager: api_keys.GalaxyApiKeyManager, user_serializer: UserSerializer, user_deserializer: UserDeserializer, quota_agent: QuotaAgent, diff --git a/lib/tool_shed/webapp/api2/users.py b/lib/tool_shed/webapp/api2/users.py index ea119cc6d888..b8d349eafed5 100644 --- a/lib/tool_shed/webapp/api2/users.py +++ b/lib/tool_shed/webapp/api2/users.py @@ -22,10 +22,10 @@ ObjectNotFound, RequestParameterInvalidException, ) -from galaxy.managers.api_keys import ApiKeyManager from galaxy.model.base import transaction from galaxy.webapps.base.webapp import create_new_session from tool_shed.context import SessionRequestContext +from tool_shed.managers.api_keys import ToolShedApiKeyManager from tool_shed.managers.users import ( api_create_user, get_api_user, @@ -102,7 +102,7 @@ class UiChangePasswordRequest(BaseModel): class FastAPIUsers: app: ToolShedApp = depends(ToolShedApp) user_manager: UserManager = depends(UserManager) - api_key_manager: ApiKeyManager = depends(ApiKeyManager) + api_key_manager: ToolShedApiKeyManager = depends(ToolShedApiKeyManager) @router.get( "/api/users", diff --git a/lib/tool_shed/webapp/app.py b/lib/tool_shed/webapp/app.py index 66ed229f902f..0c245f029871 100644 --- a/lib/tool_shed/webapp/app.py +++ b/lib/tool_shed/webapp/app.py @@ -19,7 +19,6 @@ SentryClientMixin, ) from galaxy.config import configure_logging -from galaxy.managers.api_keys import ApiKeyManager from galaxy.managers.citations import CitationsManager from galaxy.managers.dbkeys import GenomeBuilds from galaxy.model.base import SharedModelMapping @@ -32,13 +31,10 @@ from galaxy.structured_app import BasicSharedApp from galaxy.web_stack import application_stack_instance from tool_shed.grids.repository_grid_filter_manager import RepositoryGridFilterManager +from tool_shed.managers.api_keys import ToolShedApiKeyManager from tool_shed.managers.users import UserManager from tool_shed.structured_app import ToolShedApp from tool_shed.util.hgweb_config import hgweb_config_manager -from tool_shed.webapp.model import ( - APIKeys, - User, -) from tool_shed.webapp.model.migrations import verify_database from . import config @@ -88,7 +84,7 @@ def __init__(self, **kwd) -> None: self._register_singleton(mapping.ToolShedModelMapping, model) self._register_singleton(scoped_session, self.model.context) self.user_manager = self._register_singleton(UserManager, UserManager(self, app_type="tool_shed")) - self.api_keys_manager = self._register_singleton(ApiKeyManager[APIKeys, User]) + self.api_keys_manager = self._register_singleton(ToolShedApiKeyManager) # initialize the Tool Shed tag handler. self.tag_handler = CommunityTagHandler(self) # Initialize the Tool Shed tool data tables. Never pass a configuration file here diff --git a/lib/tool_shed/webapp/controllers/user.py b/lib/tool_shed/webapp/controllers/user.py index ee79d31971cd..c9e4d3d34103 100644 --- a/lib/tool_shed/webapp/controllers/user.py +++ b/lib/tool_shed/webapp/controllers/user.py @@ -7,7 +7,6 @@ util, web, ) -from galaxy.managers.api_keys import ApiKeyManager from galaxy.managers.users import get_user_by_email from galaxy.model.base import transaction from galaxy.security.validate_user_input import ( @@ -18,11 +17,8 @@ from galaxy.web import url_for from galaxy.web.form_builder import CheckboxField from galaxy.webapps.galaxy.controllers.user import User as BaseUser +from tool_shed.managers.api_keys import ToolShedApiKeyManager from tool_shed.webapp.framework.decorators import require_login -from tool_shed.webapp.model import ( - APIKeys, - User as ToolShedUser, -) log = logging.getLogger(__name__) @@ -316,7 +312,7 @@ def api_keys(self, trans, cntrller, **kwd): message = escape(util.restore_text(params.get("message", ""))) status = params.get("status", "done") if params.get("new_api_key_button", False): - ApiKeyManager[APIKeys, ToolShedUser](trans.app).create_api_key(trans.user) + ToolShedApiKeyManager(trans.app).create_api_key(trans.user) message = "Generated a new web API key" status = "done" return trans.fill_template( From fed8b1a4ff54237571accb4a5576884077d2ec4a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 May 2024 10:17:14 +0200 Subject: [PATCH 09/30] Fix type annotation for api_keys.key --- lib/galaxy/model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 4c8b9c9d21cd..4933acc83e9f 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -11142,7 +11142,7 @@ class APIKeys(Base, RepresentById): id: Mapped[int] = mapped_column(primary_key=True) create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) user_id: Mapped[Optional[int]] = mapped_column(ForeignKey("galaxy_user.id"), index=True) - key: Mapped[Optional[str]] = mapped_column(TrimmedString(32), index=True, unique=True) + key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True) user: Mapped[Optional["User"]] = relationship(back_populates="api_keys") deleted: Mapped[bool] = mapped_column(index=True, server_default=false()) From 668deb26d7b928fc37ccab6b5f146390643870d0 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 May 2024 15:47:49 +0200 Subject: [PATCH 10/30] Fix nullability of TS api key --- lib/tool_shed/webapp/model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index d1c32354253d..5916325c03cd 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -96,7 +96,7 @@ class APIKeys(Base): id: Mapped[int] = mapped_column(Integer, primary_key=True) create_time: Mapped[Optional[datetime]] = mapped_column(DateTime, default=now) user_id: Mapped[Optional[int]] = mapped_column(ForeignKey("galaxy_user.id"), index=True) - key: Mapped[Optional[str]] = mapped_column(TrimmedString(32), index=True, unique=True) + key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True) user = relationship("User", back_populates="api_keys") deleted: Mapped[Optional[bool]] = mapped_column(Boolean, index=True, default=False) From 3076954a193e966d62ce12df5d43adae88302642 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 May 2024 15:52:57 +0200 Subject: [PATCH 11/30] Fixup setup_users in celery rate limit integration test --- lib/tool_shed/managers/api_keys.py | 5 ++++- test/integration/test_celery_user_rate_limit.py | 6 ++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/tool_shed/managers/api_keys.py b/lib/tool_shed/managers/api_keys.py index d38ed6b1a537..9f66c9512b45 100644 --- a/lib/tool_shed/managers/api_keys.py +++ b/lib/tool_shed/managers/api_keys.py @@ -1,5 +1,8 @@ from galaxy.managers.api_keys import ApiKeyManager -from tool_shed.webapp.model import APIKeys, User +from tool_shed.webapp.model import ( + APIKeys, + User, +) class ToolShedApiKeyManager(ApiKeyManager[APIKeys, User]): diff --git a/test/integration/test_celery_user_rate_limit.py b/test/integration/test_celery_user_rate_limit.py index b9b832c5cddc..6aff9a68317d 100644 --- a/test/integration/test_celery_user_rate_limit.py +++ b/test/integration/test_celery_user_rate_limit.py @@ -49,8 +49,10 @@ def setup_users(dburl: str, num_users: int = 2): user_ids_to_add = set(expected_user_ids).difference(found_user_ids) for user_id in user_ids_to_add: conn.execute( - text("insert into galaxy_user(id, active, email, password) values (:id, :active, :email, :pw)"), - [{"id": user_id, "active": True, "email": "e", "pw": "p"}], + text( + "insert into galaxy_user(id, active, email, password, username) values (:id, :active, :email, :pw, :username)" + ), + [{"id": user_id, "active": True, "email": "e", "pw": "p", "username": f"u{user_id}"}], ) From 9866f4ec5e4f37248b7d82be89fa2864ccf2d23a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 May 2024 22:03:09 +0200 Subject: [PATCH 12/30] Fix unit tests --- test/unit/app/jobs/test_job_context.py | 2 +- test/unit/app/jobs/test_rule_helper.py | 6 +- .../managers/test_JobConnectionsManager.py | 20 ++++ test/unit/app/tools/test_error_reporting.py | 4 +- test/unit/app/tools/test_history_imp_exp.py | 4 +- test/unit/app/tools/test_toolbox.py | 4 +- test/unit/data/model/test_model_discovery.py | 4 +- test/unit/data/model/test_model_store.py | 16 +-- test/unit/data/test_galaxy_mapping.py | 100 +++++++++--------- test/unit/data/test_model_copy.py | 6 +- test/unit/data/test_quota.py | 13 +-- test/unit/test_galaxy_transactions.py | 2 +- test/unit/workflows/test_run_parameters.py | 4 +- test/unit/workflows/workflow_support.py | 2 +- 14 files changed, 102 insertions(+), 85 deletions(-) diff --git a/test/unit/app/jobs/test_job_context.py b/test/unit/app/jobs/test_job_context.py index 29b030982595..5f454c9aea1c 100644 --- a/test/unit/app/jobs/test_job_context.py +++ b/test/unit/app/jobs/test_job_context.py @@ -46,7 +46,7 @@ def test_job_context_discover_outputs_flushes_once(mocker): sa_session = app.model.context # mocker is a pytest-mock fixture - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) tool = Tool(app) diff --git a/test/unit/app/jobs/test_rule_helper.py b/test/unit/app/jobs/test_rule_helper.py index 1d1197a0dc78..2dfefcd93f00 100644 --- a/test/unit/app/jobs/test_rule_helper.py +++ b/test/unit/app/jobs/test_rule_helper.py @@ -64,9 +64,9 @@ def __setup_fixtures(app): # user1 has 3 jobs queued and 2 jobs running on cluster1 and one queued and # on running job on local. user2 has a queued and running job on the cluster. # user3 has no jobs. - user1 = model.User(email=USER_EMAIL_1, password="pass1") - user2 = model.User(email=USER_EMAIL_2, password="pass2") - user3 = model.User(email=USER_EMAIL_2, password="pass2") + user1 = model.User(email=USER_EMAIL_1, password="pass1", username="u1") + user2 = model.User(email=USER_EMAIL_2, password="pass2", username="u2") + user3 = model.User(email=USER_EMAIL_2, password="pass2", username="u3") app.add(user1, user2, user3) diff --git a/test/unit/app/managers/test_JobConnectionsManager.py b/test/unit/app/managers/test_JobConnectionsManager.py index 5ec06f388fd7..2c12ea52aced 100644 --- a/test/unit/app/managers/test_JobConnectionsManager.py +++ b/test/unit/app/managers/test_JobConnectionsManager.py @@ -3,6 +3,8 @@ from galaxy.managers.job_connections import JobConnectionsManager from galaxy.model import ( + DatasetCollection, + History, HistoryDatasetAssociation, HistoryDatasetCollectionAssociation, Job, @@ -25,11 +27,19 @@ def job_connections_manager(sa_session) -> JobConnectionsManager: # ============================================================================= def setup_connected_dataset(sa_session: galaxy_scoped_session): + history = History() + sa_session.add(history) center_hda = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) input_hda = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) + input_dc = DatasetCollection(collection_type="list") input_hdca = HistoryDatasetCollectionAssociation() + input_hdca.collection = input_dc output_hda = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) + output_dc = DatasetCollection(collection_type="list") output_hdca = HistoryDatasetCollectionAssociation() + output_hdca.collection = output_dc + history.stage_addition([center_hda, input_hda, input_hdca, output_hda, output_hdca]) + history.add_pending_items() input_job = Job() output_job = Job() input_job.add_output_dataset("output_hda", center_hda) @@ -55,12 +65,22 @@ def setup_connected_dataset(sa_session: galaxy_scoped_session): def setup_connected_dataset_collection(sa_session: galaxy_scoped_session): + history = History() + sa_session.add(history) + center_dc = DatasetCollection(collection_type="list") center_hdca = HistoryDatasetCollectionAssociation() + center_hdca.collection = center_dc input_hda1 = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) input_hda2 = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) + input_dc = DatasetCollection(collection_type="list") input_hdca = HistoryDatasetCollectionAssociation() + input_hdca.collection = input_dc output_hda = HistoryDatasetAssociation(sa_session=sa_session, create_dataset=True) + output_dc = DatasetCollection(collection_type="list") output_hdca = HistoryDatasetCollectionAssociation() + output_hdca.collection = output_dc + history.stage_addition([center_hdca, input_hda1, input_hda2, input_hdca, output_hda, output_hdca]) + history.add_pending_items() input_job = Job() output_job = Job() input_job.add_output_dataset_collection("output_hdca", center_hdca) diff --git a/test/unit/app/tools/test_error_reporting.py b/test/unit/app/tools/test_error_reporting.py index 7db9cb4bf60e..5651c1c73bf9 100644 --- a/test/unit/app/tools/test_error_reporting.py +++ b/test/unit/app/tools/test_error_reporting.py @@ -57,7 +57,7 @@ def test_hda_security(self, tmp_path): permissions = {access_action: [private_role], manage_action: [private_role]} security_agent.set_all_dataset_permissions(hda.dataset, permissions) - other_user = model.User(email="otheruser@galaxyproject.org", password="mockpass2") + other_user = model.User(email="otheruser@galaxyproject.org", password="mockpass2", username="otheruser") self._commit_objects([other_user]) security_agent = self.app.security_agent email_path = self.email_path @@ -125,7 +125,7 @@ def test_no_redact_user_details_in_bugreport(self, tmp_path): ) def _setup_model_objects(self): - user = model.User(email=TEST_USER_EMAIL, password="mockpass") + user = model.User(email=TEST_USER_EMAIL, password="mockpass", username=TEST_USER_EMAIL.split("@")[0]) job = model.Job() job.tool_id = "cat1" job.history = model.History() diff --git a/test/unit/app/tools/test_history_imp_exp.py b/test/unit/app/tools/test_history_imp_exp.py index 1f54029642da..097940bc1b84 100644 --- a/test/unit/app/tools/test_history_imp_exp.py +++ b/test/unit/app/tools/test_history_imp_exp.py @@ -36,7 +36,7 @@ def t_data_path(name): def _run_jihaw_cleanup(archive_dir, app=None): app = app or _mock_app() job = model.Job() - job.user = model.User(email="test@test.org", password="test") + job.user = model.User(email="test@test.org", password="test", username="test") job.tool_stderr = "" jiha = model.JobImportHistoryArchive(job=job, archive_dir=archive_dir) app.model.context.current.add_all([job, jiha]) @@ -661,7 +661,7 @@ def _setup_history_for_export(history_name): sa_session = app.model.context email = history_name.replace(" ", "-") + "-user@example.org" - u = model.User(email=email, password="password") + u = model.User(email=email, password="password", username=email.split("@")[0]) h = model.History(name=history_name, user=u) return app, sa_session, h diff --git a/test/unit/app/tools/test_toolbox.py b/test/unit/app/tools/test_toolbox.py index f4faaa370a1a..54bf1aad6c9d 100644 --- a/test/unit/app/tools/test_toolbox.py +++ b/test/unit/app/tools/test_toolbox.py @@ -356,9 +356,7 @@ def __test_workflow(self): workflow = model.Workflow() workflow.stored_workflow = stored_workflow stored_workflow.latest_workflow = workflow - user = model.User() - user.email = "test@example.com" - user.password = "passw0rD1" + user = model.User(email="test@example.com", password="passw0rD1", username="test") stored_workflow.user = user self.app.model.context.add(workflow) self.app.model.context.add(stored_workflow) diff --git a/test/unit/data/model/test_model_discovery.py b/test/unit/data/model/test_model_discovery.py index 7237d37f0ef0..b4bcfdf01289 100644 --- a/test/unit/data/model/test_model_discovery.py +++ b/test/unit/data/model/test_model_discovery.py @@ -225,7 +225,7 @@ def _import_library_target(target, work_directory): with store.DirectoryModelExportStore(temp_directory, app=app, serialize_dataset_objects=True) as export_store: persist_target_to_export_store(target, export_store, app.object_store, work_directory) - u = model.User(email="library@example.com", password="password") + u = model.User(email="library@example.com", password="password", username="library") import_options = store.ImportOptions(allow_dataset_object_edit=True, allow_library_creation=True) import_model_store = store.get_import_model_store_for_directory( @@ -240,7 +240,7 @@ def _import_library_target(target, work_directory): def _import_directory_to_history(app, target, work_directory): sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") import_history = model.History(name="Test History for Import", user=u) sa_session = app.model.context diff --git a/test/unit/data/model/test_model_store.py b/test/unit/data/model/test_model_store.py index a30410dd0207..7ac5b05a724a 100644 --- a/test/unit/data/model/test_model_store.py +++ b/test/unit/data/model/test_model_store.py @@ -312,7 +312,7 @@ def test_import_library_require_permissions(): app = _mock_app() sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") library = model.Library(name="my library 1", description="my library description", synopsis="my synopsis") root_folder = model.LibraryFolder(name="my library 1", description="folder description") @@ -340,7 +340,7 @@ def test_import_export_library(): app = _mock_app() sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") library = model.Library(name="my library 1", description="my library description", synopsis="my synopsis") root_folder = model.LibraryFolder(name="my library 1", description="folder description") @@ -684,7 +684,7 @@ def test_import_export_edit_collection(): app = _mock_app() sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) c1 = model.DatasetCollection(collection_type="list", populated=False) @@ -761,7 +761,7 @@ def test_import_export_composite_datasets(): app = _mock_app() sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) d1 = _create_datasets(sa_session, h, 1, extension="html")[0] @@ -804,7 +804,7 @@ def test_edit_metadata_files(): app = _mock_app(store_by="uuid") sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) d1 = _create_datasets(sa_session, h, 1, extension="bam")[0] @@ -910,7 +910,7 @@ def _assert_simple_cat_job_imported(imported_history, state="ok"): def _setup_simple_cat_job(app, state="ok"): sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) d1, d2 = _create_datasets(sa_session, h, 2) @@ -966,7 +966,7 @@ def _setup_invocation(app): def _setup_simple_collection_job(app, state="ok"): sa_session = app.model.context - u = model.User(email="collection@example.com", password="password") + u = model.User(email="collection@example.com", password="password", username="collection") h = model.History(name="Test History", user=u) d1, d2, d3, d4 = _create_datasets(sa_session, h, 4) @@ -1170,7 +1170,7 @@ def setup_fixture_context_with_user( ) -> StoreFixtureContextWithUser: app = _mock_app(store_by=store_by) sa_session = app.model.context - user = model.User(email=user_email, password="password") + user = model.User(email=user_email, password="password", username=user_email.split("@")[0]) return StoreFixtureContextWithUser(app=app, sa_session=sa_session, user=user) diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index 1a88a82f78ed..c8f812a75131 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -40,6 +40,11 @@ PRIVATE_OBJECT_STORE_ID = "my_private_data" +@pytest.fixture() +def u(): + return model.User(f"{uuid.uuid4()}@#user.com", str(uuid.uuid4()), str(uuid.uuid4())) + + class BaseModelTestCase(TestCase): model: mapping.GalaxyModelMapping @@ -79,7 +84,7 @@ def expunge(cls): class TestMappings(BaseModelTestCase): def test_ratings(self): user_email = "rater@example.com" - u = model.User(email=user_email, password="password") + u = model.User(email=user_email, password="password", username=user_email.split("@")[0]) self.persist(u) def persist_and_check_rating(rating_class, item): @@ -119,6 +124,8 @@ def persist_and_check_rating(rating_class, item): dataset_collection = model.DatasetCollection(collection_type="paired") history_dataset_collection = model.HistoryDatasetCollectionAssociation(collection=dataset_collection) + self.session().add_all([h, history_dataset_collection]) + h.add_dataset_collection(history_dataset_collection) self.persist(history_dataset_collection) persist_and_check_rating(model.HistoryDatasetCollectionRatingAssociation, history_dataset_collection) @@ -154,7 +161,7 @@ def assert_display_name_converts_to_unicode(item, name): def test_hda_to_library_dataset_dataset_association(self): model = self.model - u = self.model.User(email="mary@example.com", password="password") + u = self.model.User(email="mary@example.com", password="password", username="mary") h1 = model.History(name="History 1", user=u) hda = model.HistoryDatasetAssociation( name="hda_name", create_dataset=True, history=h1, sa_session=model.session @@ -186,7 +193,7 @@ def test_hda_to_library_dataset_dataset_association(self): def test_hda_to_library_dataset_dataset_association_fails_if_private(self): model = self.model - u = model.User(email="mary2@example.com", password="password") + u = model.User(email="mary2@example.com", password="password", username="mary2") h1 = model.History(name="History 1", user=u) hda = model.HistoryDatasetAssociation( name="hda_name", create_dataset=True, history=h1, sa_session=model.session @@ -205,7 +212,7 @@ def test_hda_to_library_dataset_dataset_association_fails_if_private(self): def test_tags(self): TAG_NAME = "Test Tag" my_tag = model.Tag(name=TAG_NAME) - u = model.User(email="tagger@example.com", password="password") + u = model.User(email="tagger@example.com", password="password", username="tagger") self.persist(my_tag, u) def tag_and_test(taggable_object, tag_association_class): @@ -239,13 +246,13 @@ def tag_and_test(taggable_object, tag_association_class): dataset_collection = model.DatasetCollection(collection_type="paired") history_dataset_collection = model.HistoryDatasetCollectionAssociation(collection=dataset_collection) + h.add_dataset_collection(history_dataset_collection) tag_and_test(history_dataset_collection, model.HistoryDatasetCollectionTagAssociation) library_dataset_collection = model.LibraryDatasetCollectionAssociation(collection=dataset_collection) tag_and_test(library_dataset_collection, model.LibraryDatasetCollectionTagAssociation) - def test_collection_get_interface(self): - u = model.User(email="mary@example.com", password="password") + def test_collection_get_interface(self, u): h1 = model.History(name="History 1", user=u) d1 = model.HistoryDatasetAssociation( extension="txt", history=h1, create_dataset=True, sa_session=self.model.session @@ -261,8 +268,7 @@ def test_collection_get_interface(self): for i in range(elements): assert c1[i] == dces[i] - def test_dataset_instance_order(self) -> None: - u = model.User(email="mary@example.com", password="password") + def test_dataset_instance_order(self, u) -> None: h1 = model.History(name="History 1", user=u) elements = [] list_pair = model.DatasetCollection(collection_type="list:paired") @@ -308,8 +314,7 @@ def test_dataset_instance_order(self) -> None: assert all(d.name == f"forward_{i}" for i, d in enumerate(forward_hdas)) assert all(d.name == f"reverse_{i}" for i, d in enumerate(reverse_hdas)) - def test_collections_in_histories(self): - u = model.User(email="mary@example.com", password="password") + def test_collections_in_histories(self, u): h1 = model.History(name="History 1", user=u) d1 = model.HistoryDatasetAssociation( extension="txt", history=h1, create_dataset=True, sa_session=self.model.session @@ -323,6 +328,8 @@ def test_collections_in_histories(self): dce1 = model.DatasetCollectionElement(collection=c1, element=d1, element_identifier="left") dce2 = model.DatasetCollectionElement(collection=c1, element=d2, element_identifier="right") + h1.stage_addition([d1, d2, hc1]) + h1.add_pending_items() self.persist(u, h1, d1, d2, c1, hc1, dce1, dce2) @@ -337,8 +344,7 @@ def test_collections_in_histories(self): assert loaded_dataset_collection["left"] == dce1 assert loaded_dataset_collection["right"] == dce2 - def test_collections_in_library_folders(self): - u = model.User(email="mary2@example.com", password="password") + def test_collections_in_library_folders(self, u): lf = model.LibraryFolder(name="RootFolder") library = model.Library(name="Library1", root_folder=lf) ld1 = model.LibraryDataset() @@ -358,7 +364,7 @@ def test_collections_in_library_folders(self): # assert loaded_dataset_collection.collection_type == "pair" def test_dataset_action_tuples(self): - u = model.User(email="foo", password="foo") + u = model.User(email="foo", password="foo", username="foo") h1 = model.History(user=u) hda1 = model.HistoryDatasetAssociation(history=h1, create_dataset=True, sa_session=self.model.session) hda2 = model.HistoryDatasetAssociation(history=h1, create_dataset=True, sa_session=self.model.session) @@ -373,8 +379,7 @@ def test_dataset_action_tuples(self): self.model.session.flush() assert c1.dataset_action_tuples == [("action1", r1.id), ("action3", r1.id)] - def test_nested_collection_attributes(self): - u = model.User(email="mary2@example.com", password="password") + def test_nested_collection_attributes(self, u): h1 = model.History(name="History 1", user=u) d1 = model.HistoryDatasetAssociation( extension="bam", history=h1, create_dataset=True, sa_session=self.model.session @@ -461,8 +466,7 @@ def test_nested_collection_attributes(self): ] assert c4.dataset_elements == [dce1, dce2] - def test_dataset_dbkeys_and_extensions_summary(self): - u = model.User(email="mary2@example.com", password="password") + def test_dataset_dbkeys_and_extensions_summary(self, u): h1 = model.History(name="History 1", user=u) d1 = model.HistoryDatasetAssociation( extension="bam", dbkey="hg19", history=h1, create_dataset=True, sa_session=self.model.session @@ -475,12 +479,13 @@ def test_dataset_dbkeys_and_extensions_summary(self): dce2 = model.DatasetCollectionElement(collection=c1, element=d2, element_identifier="reverse", element_index=1) hdca = model.HistoryDatasetCollectionAssociation(collection=c1, history=h1) self.model.session.add_all([d1, d2, c1, dce1, dce2, hdca]) + h1.stage_addition([d1, d2, hdca]) + h1.add_pending_items() self.model.session.flush() assert hdca.dataset_dbkeys_and_extensions_summary[0] == {"hg19"} assert hdca.dataset_dbkeys_and_extensions_summary[1] == {"bam", "txt"} - def test_populated_optimized_ok(self): - u = model.User(email="mary2@example.com", password="password") + def test_populated_optimized_ok(self, u): h1 = model.History(name="History 1", user=u) d1 = model.HistoryDatasetAssociation( extension="txt", history=h1, create_dataset=True, sa_session=self.model.session @@ -492,6 +497,8 @@ def test_populated_optimized_ok(self): dce1 = model.DatasetCollectionElement(collection=c1, element=d1, element_identifier="forward", element_index=0) dce2 = model.DatasetCollectionElement(collection=c1, element=d2, element_identifier="reverse", element_index=1) self.model.session.add_all([d1, d2, c1, dce1, dce2]) + h1.stage_addition([d1, d2]) + h1.add_pending_items() self.model.session.flush() assert c1.populated assert c1.populated_optimized @@ -524,7 +531,7 @@ def test_populated_optimized_list_list_not_populated(self): assert not c2.populated_optimized def test_default_disk_usage(self): - u = model.User(email="disk_default@test.com", password="password") + u = model.User(email="disk_default@test.com", password="password", username="disk_default") self.persist(u) u.adjust_total_disk_usage(1, None) u_id = u.id @@ -536,7 +543,7 @@ def test_basic(self): original_user_count = len(self.model.session.scalars(select(model.User)).all()) # Make some changes and commit them - u = model.User(email="james@foo.bar.baz", password="password") + u = model.User(email="james@foo.bar.baz", password="password", username="james") h1 = model.History(name="History 1", user=u) h2 = model.History(name=("H" * 1024)) self.persist(u, h1, h2) @@ -592,8 +599,7 @@ def test_dataset_job_relationship(self): ).scalar_one() assert loaded_dataset.job_id == job.id - def test_jobs(self): - u = model.User(email="jobtest@foo.bar.baz", password="password") + def test_jobs(self, u): job = model.Job() job.user = u job.tool_id = "cat1" @@ -603,8 +609,7 @@ def test_jobs(self): loaded_job = self.model.session.scalars(select(model.Job).filter(model.Job.user == u).limit(1)).first() assert loaded_job.tool_id == "cat1" - def test_job_metrics(self): - u = model.User(email="jobtest@foo.bar.baz", password="password") + def test_job_metrics(self, u): job = model.Job() job.user = u job.tool_id = "cat1" @@ -624,8 +629,7 @@ def test_job_metrics(self): # Ensure big values truncated assert len(task.text_metrics[1].metric_value) <= 1023 - def test_tasks(self): - u = model.User(email="jobtest@foo.bar.baz", password="password") + def test_tasks(self, u): job = model.Job() task = model.Task(job=job, working_directory="/tmp", prepare_files_cmd="split.sh") job.user = u @@ -634,8 +638,7 @@ def test_tasks(self): loaded_task = self.model.session.scalars(select(model.Task).filter(model.Task.job == job).limit(1)).first() assert loaded_task.prepare_input_files_cmd == "split.sh" - def test_history_contents(self): - u = model.User(email="contents@foo.bar.baz", password="password") + def test_history_contents(self, u): # gs = model.GalaxySession() h1 = model.History(name="HistoryContentsHistory1", user=u) @@ -668,8 +671,7 @@ def contents_iter_names(**kwds): assert contents_iter_names(ids=[d1.id, d3.id]) == ["1", "3"] - def test_history_audit(self): - u = model.User(email="contents@foo.bar.baz", password="password") + def test_history_audit(self, u): h1 = model.History(name="HistoryAuditHistory", user=u) h2 = model.History(name="HistoryAuditHistory", user=u) @@ -723,8 +725,8 @@ def _non_empty_flush(self): session.add(lf) session.flush() - def test_current_session(self): - user = model.User(email="testworkflows@bx.psu.edu", password="password") + def test_current_session(self, u): + user = u galaxy_session = model.GalaxySession() galaxy_session.user = user self.persist(user, galaxy_session) @@ -734,12 +736,12 @@ def test_current_session(self): self.persist(user, new_galaxy_session) assert user.current_galaxy_session == new_galaxy_session - def test_flush_refreshes(self): + def test_flush_refreshes(self, u): # Normally I don't believe in unit testing library code, but the behaviors around attribute # states and flushing in SQL Alchemy is very subtle and it is good to have a executable # reference for how it behaves in the context of Galaxy objects. model = self.model - user = model.User(email="testworkflows@bx.psu.edu", password="password") + user = u galaxy_session = model.GalaxySession() galaxy_session_other = model.GalaxySession() galaxy_session.user = user @@ -811,10 +813,8 @@ def test_flush_refreshes(self): session.flush(model.GalaxySession()) assert "id" not in inspect(galaxy_model_object_new).unloaded - def test_workflows(self): - user = model.User(email="testworkflows@bx.psu.edu", password="password") - - child_workflow = _workflow_from_steps(user, []) + def test_workflows(self, u): + child_workflow = _workflow_from_steps(u, []) self.persist(child_workflow) workflow_step_1 = model.WorkflowStep() @@ -831,19 +831,19 @@ def test_workflows(self): workflow_step_2.get_or_add_input("moo") workflow_step_1.add_connection("foo", "cow", workflow_step_2) - workflow = _workflow_from_steps(user, [workflow_step_1, workflow_step_2]) + workflow = _workflow_from_steps(u, [workflow_step_1, workflow_step_2]) self.persist(workflow) workflow_id = workflow.id annotation = model.WorkflowStepAnnotationAssociation() annotation.annotation = "Test Step Annotation" - annotation.user = user + annotation.user = u add_object_to_object_session(annotation, workflow_step_1) annotation.workflow_step = workflow_step_1 self.persist(annotation) assert workflow_step_1.id is not None - workflow_invocation = _invocation_for_workflow(user, workflow) + workflow_invocation = _invocation_for_workflow(u, workflow) invocation_uuid = uuid.uuid1() @@ -931,7 +931,7 @@ def check_private_role(private_role, email): assert private_role.description == "Private Role for " + email email = "rule_user_1@example.com" - u = model.User(email=email, password="password") + u = model.User(email=email, password="password", username=email.split("@")[0]) self.persist(u) role = security_agent.get_private_user_role(u) @@ -941,7 +941,7 @@ def check_private_role(private_role, email): check_private_role(role, email) email = "rule_user_2@example.com" - u = model.User(email=email, password="password") + u = model.User(email=email, password="password", username=email.split("@")[0]) self.persist(u) role = security_agent.get_private_user_role(u) assert role is None @@ -1035,8 +1035,7 @@ def test_can_manage_private_dataset(self): assert security_agent.can_manage_dataset(u_from.all_roles(), d1.dataset) assert not security_agent.can_manage_dataset(u_other.all_roles(), d1.dataset) - def test_history_hid_counter_is_expired_after_next_hid_call(self): - u = model.User(email="hid_abuser@example.com", password="password") + def test_history_hid_counter_is_expired_after_next_hid_call(self, u): h = model.History(name="History for hid testing", user=u) self.persist(u, h) state = inspect(h) @@ -1050,8 +1049,7 @@ def test_history_hid_counter_is_expired_after_next_hid_call(self): assert "id" not in state.unloaded # but other attributes have NOT been expired assert h.hid_counter == 2 # check this last: this causes thie hid_counter to be reloaded - def test_next_hid(self): - u = model.User(email="hid_abuser@example.com", password="password") + def test_next_hid(self, u): h = model.History(name="History for hid testing", user=u) self.persist(u, h) assert h.hid_counter == 1 @@ -1136,9 +1134,9 @@ def _three_users(self, suffix): email_to = f"user_{suffix}e2@example.com" email_other = f"user_{suffix}e3@example.com" - u_from = model.User(email=email_from, password="password") - u_to = model.User(email=email_to, password="password") - u_other = model.User(email=email_other, password="password") + u_from = model.User(email=email_from, password="password", username=email_from.split("@")[0]) + u_to = model.User(email=email_to, password="password", username=email_to.split("@")[0]) + u_other = model.User(email=email_other, password="password", username=email_other.split("@")[0]) self.persist(u_from, u_to, u_other) return u_from, u_to, u_other diff --git a/test/unit/data/test_model_copy.py b/test/unit/data/test_model_copy.py index 117ce7ab0c20..6c1f0ab4cff6 100644 --- a/test/unit/data/test_model_copy.py +++ b/test/unit/data/test_model_copy.py @@ -88,12 +88,10 @@ def test_history_collection_copy(list_size=NUM_DATASETS): list_elements.append(paired_collection_element) model.context.add_all([forward_dce, reverse_dce, paired_collection_element]) history_dataset_collection = model.HistoryDatasetCollectionAssociation(collection=list_collection) - history_dataset_collection.user = old_history.user + history_dataset_collection.user = old_history.username model.context.add(history_dataset_collection) session = model.context - with transaction(session): - session.commit() old_history.add_dataset_collection(history_dataset_collection) history_dataset_collection.add_item_annotation( @@ -148,7 +146,7 @@ def _setup_mapping_and_user(): ) setup_global_object_store_for_models(object_store) - u = User(email="historycopy@example.com", password="password") + u = User(email="historycopy@example.com", password="password", username="historycopy") h1 = History(name="HistoryCopyHistory1", user=u) model.context.add_all([u, h1]) session = model.context diff --git a/test/unit/data/test_quota.py b/test/unit/data/test_quota.py index 5c34ea7cb056..143968ac809d 100644 --- a/test/unit/data/test_quota.py +++ b/test/unit/data/test_quota.py @@ -16,7 +16,7 @@ class TestPurgeUsage(BaseModelTestCase): def setUp(self): super().setUp() model = self.model - u = model.User(email="purge_usage@example.com", password="password") + u = model.User(email="purge_usage@example.com", password="password", username=str(uuid.uuid4())) u.disk_usage = 25 self.persist(u) @@ -67,7 +67,8 @@ def test_calculate_usage_per_source(self): class TestCalculateUsage(BaseModelTestCase): def setUp(self): model = self.model - u = model.User(email=f"calc_usage{uuid.uuid1()}@example.com", password="password") + email = f"calc_usage{uuid.uuid1()}@example.com" + u = model.User(email, password="password", username=email.split("@")[0]) self.persist(u) h = model.History(name="History for Calculated Usage", user=u) self.persist(h) @@ -359,7 +360,7 @@ def setUp(self): self.quota_agent = DatabaseQuotaAgent(model) def test_quota(self): - u = model.User(email="quota@example.com", password="password") + u = model.User(email="quota@example.com", password="password", username="quota") self.persist(u) self._assert_user_quota_is(u, None) @@ -405,7 +406,7 @@ def test_quota(self): def test_labeled_quota(self): model = self.model - u = model.User(email="labeled_quota@example.com", password="password") + u = model.User(email="labeled_quota@example.com", password="password", username="labeled_quota") self.persist(u) label1 = "coollabel1" @@ -456,7 +457,7 @@ def _assert_user_quota_is(self, user, amount, quota_source_label=None): class TestUsage(BaseModelTestCase): def test_usage(self): model = self.model - u = model.User(email="usage@example.com", password="password") + u = model.User(email="usage@example.com", password="password", username="usage") self.persist(u) u.adjust_total_disk_usage(123, None) @@ -466,7 +467,7 @@ def test_usage(self): def test_labeled_usage(self): model = self.model - u = model.User(email="labeled.usage@example.com", password="password") + u = model.User(email="labeled.usage@example.com", password="password", username="labeled") self.persist(u) assert len(u.quota_source_usages) == 0 diff --git a/test/unit/test_galaxy_transactions.py b/test/unit/test_galaxy_transactions.py index 8093df91c0ce..581c32f2c2d5 100644 --- a/test/unit/test_galaxy_transactions.py +++ b/test/unit/test_galaxy_transactions.py @@ -62,7 +62,7 @@ def test_logging_actions_on(transaction): def test_expunge_all(transaction): - user = model.User("foo", "bar1") + user = model.User("foo", "bar1", username="foo") transaction.sa_session.add(user) user.password = "bar2" diff --git a/test/unit/workflows/test_run_parameters.py b/test/unit/workflows/test_run_parameters.py index 76eae8955744..a45c34b0f44d 100644 --- a/test/unit/workflows/test_run_parameters.py +++ b/test/unit/workflows/test_run_parameters.py @@ -1,3 +1,5 @@ +import uuid + from galaxy import model from galaxy.model.base import transaction from galaxy.workflow.run_request import ( @@ -89,7 +91,7 @@ def __new_input(): def __workflow_fixure(trans): - user = model.User(email="testworkflow_params@bx.psu.edu", password="pass") + user = model.User(email="testworkflow_params@bx.psu.edu", password="pass", username=str(uuid.uuid4())) stored_workflow = model.StoredWorkflow() stored_workflow.user = user workflow = model.Workflow() diff --git a/test/unit/workflows/workflow_support.py b/test/unit/workflows/workflow_support.py index 05064e722fac..0e89a20ec91c 100644 --- a/test/unit/workflows/workflow_support.py +++ b/test/unit/workflows/workflow_support.py @@ -28,7 +28,7 @@ def save_workflow(self, workflow): @property def user(self): if self._user is None: - self._user = model.User(email="testworkflows@bx.psu.edu", password="password") + self._user = model.User(email="testworkflows@bx.psu.edu", password="password", username="testworkflows") return self._user From 1aedb8589e68a8b9d248da4ae6663321dffd7476 Mon Sep 17 00:00:00 2001 From: Marius van den Beek Date: Thu, 23 May 2024 17:09:43 +0200 Subject: [PATCH 13/30] Add more type hints in manager signatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David López <46503462+davelopez@users.noreply.github.com> --- lib/galaxy/managers/users.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 5b24c5cbc168..1ce9e21146f1 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -295,7 +295,7 @@ def error_if_anonymous(self, user: Optional[U], msg="Log-in required", **kwargs) raise exceptions.AuthenticationFailed(msg, **kwargs) return user - def get_user_by_identity(self, identity) -> Optional[U]: + def get_user_by_identity(self, identity: str) -> Optional[U]: """Get user by username or email.""" if VALID_EMAIL_RE.match(identity): # VALID_PUBLICNAME and VALID_EMAIL do not overlap, so 'identity' here is an email address @@ -499,7 +499,7 @@ def activate(self, user: User): with transaction(session): session.commit() - def _get_user_by_email_case_insensitive(self, session, email): + def _get_user_by_email_case_insensitive(self, session, email: str): stmt = select(self.app.model.User).where(func.lower(self.app.model.User.email) == email.lower()).limit(1) return session.scalars(stmt).first() From da79c31f5d88f59138c266b8db1282f21347756d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 14:57:02 +0200 Subject: [PATCH 14/30] Fix up history pruner unit test --- test/unit/data/model/conftest.py | 2 ++ test/unit/data/model/db/test_history_table_pruner.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/data/model/conftest.py b/test/unit/data/model/conftest.py index 034be09e51b8..4187d95c592c 100644 --- a/test/unit/data/model/conftest.py +++ b/test/unit/data/model/conftest.py @@ -158,7 +158,9 @@ def f(**kwd): @pytest.fixture def make_history_dataset_collection_association(session): def f(**kwd): + collection = m.DatasetCollection(collection_type="list") model = m.HistoryDatasetCollectionAssociation(**kwd) + model.collection = collection write_to_db(session, model) return model diff --git a/test/unit/data/model/db/test_history_table_pruner.py b/test/unit/data/model/db/test_history_table_pruner.py index 376b0c52939c..3b65d96f6c20 100644 --- a/test/unit/data/model/db/test_history_table_pruner.py +++ b/test/unit/data/model/db/test_history_table_pruner.py @@ -55,7 +55,7 @@ def setup_db( make_job_import_history_archive(history=histories[20]) make_job_export_history_archive(history=histories[21]) make_workflow_invocation(history=histories[22]) - make_history_dataset_collection_association(history=histories[23]) + make_history_dataset_collection_association(history=histories[23], hid=histories[23].hid_counter + 1) make_history_dataset_association(history=histories[25]) make_job().history = histories[24] From a1760cc879e6210e09616395c47c44e3c051a136 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 11 May 2024 09:46:46 +0200 Subject: [PATCH 15/30] Ensure dataset on disk befoe access --- lib/galaxy/managers/datasets.py | 19 +++++++++++++++++++ .../webapps/galaxy/services/datasets.py | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py index 834e5d5cea70..d52f96422fb4 100644 --- a/lib/galaxy/managers/datasets.py +++ b/lib/galaxy/managers/datasets.py @@ -492,6 +492,25 @@ def serialize_dataset_association_roles(self, trans, dataset_assoc): rval["modify_item_roles"] = modify_item_role_list return rval + def ensure_dataset_on_disk(self, trans, dataset: DI): + # Not a guarantee data is really present, but excludes a lot of expected cases + if dataset.purged or dataset.dataset.purged: + raise exceptions.ItemDeletionException("The dataset you are attempting to view has been purged.") + elif dataset.deleted and not (trans.user_is_admin or self.is_owner(dataset, trans.get_user())): + raise exceptions.ItemDeletionException("The dataset you are attempting to view has been deleted.") + elif dataset.state == Dataset.states.UPLOAD: + raise exceptions.Conflict("Please wait until this dataset finishes uploading before attempting to view it.") + elif dataset.state == Dataset.states.DISCARDED: + raise exceptions.ItemDeletionException("The dataset you are attempting to view has been discarded.") + elif dataset.state == Dataset.states.DEFERRED: + raise exceptions.Conflict( + "The dataset you are attempting to view has deferred data. You can only use this dataset as input for jobs." + ) + elif dataset.state == Dataset.states.PAUSED: + raise exceptions.Conflict( + "The dataset you are attempting to view is in paused state. One of the inputs for the job that creates this dataset has failed." + ) + def ensure_can_change_datatype(self, dataset: model.DatasetInstance, raiseException: bool = True) -> bool: if not dataset.datatype.is_datatype_change_allowed(): if not raiseException: diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index fc67929c3c24..de89199202b8 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -606,7 +606,8 @@ def display( headers = {} rval: Any = "" try: - dataset_instance = self.dataset_manager_by_type[hda_ldda].get_accessible(dataset_id, trans.user) + dataset_manager = self.dataset_manager_by_type[hda_ldda] + dataset_instance = dataset_manager.get_accessible(dataset_id, trans.user) if raw: if filename and filename != "index": object_store = trans.app.object_store From 2523f472abc062efb7b46fc7df20abd3299ff0f5 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:49:01 +0200 Subject: [PATCH 16/30] Tighten up use of DatasetProtocol and DatasetHasHidProtocol --- lib/galaxy/datatypes/binary.py | 5 ++--- lib/galaxy/datatypes/blast.py | 7 ++----- lib/galaxy/datatypes/data.py | 8 ++++---- lib/galaxy/datatypes/isa.py | 3 +-- lib/galaxy/datatypes/protocols.py | 10 ++++++++-- lib/galaxy/datatypes/sequence.py | 3 +-- lib/galaxy/datatypes/spaln.py | 3 +-- lib/galaxy/datatypes/tabular.py | 3 +-- lib/galaxy/datatypes/text.py | 5 ++--- lib/galaxy/model/__init__.py | 18 ++++++++++++++---- lib/galaxy/model/store/__init__.py | 2 +- 11 files changed, 37 insertions(+), 30 deletions(-) diff --git a/lib/galaxy/datatypes/binary.py b/lib/galaxy/datatypes/binary.py index 853a6e402977..20613b15c559 100644 --- a/lib/galaxy/datatypes/binary.py +++ b/lib/galaxy/datatypes/binary.py @@ -69,7 +69,6 @@ MetadataParameter, ) from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasExtraFilesAndMetadata, HasFileName, @@ -692,7 +691,7 @@ def get_chunk(self, trans, dataset: HasFileName, offset: int = 0, ck_size: Optio def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, @@ -2094,7 +2093,7 @@ def display_peek(self, dataset: DatasetProtocol) -> str: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/blast.py b/lib/galaxy/datatypes/blast.py index c19244acb28f..d05389991820 100644 --- a/lib/galaxy/datatypes/blast.py +++ b/lib/galaxy/datatypes/blast.py @@ -40,10 +40,7 @@ Optional, ) -from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, - DatasetProtocol, -) +from galaxy.datatypes.protocols import DatasetProtocol from galaxy.datatypes.sniff import ( build_sniff_from_prefix, FilePrefix, @@ -210,7 +207,7 @@ def display_peek(self, dataset: DatasetProtocol) -> str: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 308e691a02cc..44797c0339f7 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -422,7 +422,7 @@ def __archive_extra_files_path(self, extra_files_path: str) -> Generator[Tuple[s yield fpath, rpath def _serve_raw( - self, dataset: DatasetHasHidProtocol, to_ext: Optional[str], headers: Headers, **kwd + self, dataset: DatasetProtocol, to_ext: Optional[str], headers: Headers, **kwd ) -> Tuple[IO, Headers]: headers["Content-Length"] = str(os.stat(dataset.get_file_name()).st_size) headers["content-type"] = ( @@ -516,7 +516,7 @@ def _serve_file_contents(self, trans, data, headers, preview, file_size, max_pee def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, @@ -648,7 +648,7 @@ def _yield_user_file_content(self, trans, from_dataset: HasCreatingJob, filename def _download_filename( self, - dataset: DatasetHasHidProtocol, + dataset: Union[DatasetProtocol, DatasetHasHidProtocol], to_ext: Optional[str] = None, hdca: Optional[DatasetHasHidProtocol] = None, element_identifier: Optional[str] = None, @@ -665,7 +665,7 @@ def escape(raw_identifier): template_values = { "name": escape(dataset.name), "ext": to_ext, - "hid": dataset.hid, + "hid": getattr(dataset, "hid", "-library"), } if not filename_pattern: diff --git a/lib/galaxy/datatypes/isa.py b/lib/galaxy/datatypes/isa.py index 68cc2c38752f..5f44eef4dc34 100644 --- a/lib/galaxy/datatypes/isa.py +++ b/lib/galaxy/datatypes/isa.py @@ -28,7 +28,6 @@ from galaxy import util from galaxy.datatypes.data import Data from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasExtraFilesAndMetadata, HasExtraFilesPath, @@ -266,7 +265,7 @@ def groom_dataset_content(self, file_name: str) -> None: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/protocols.py b/lib/galaxy/datatypes/protocols.py index 5bc07c20c9f2..a764358fe519 100644 --- a/lib/galaxy/datatypes/protocols.py +++ b/lib/galaxy/datatypes/protocols.py @@ -2,10 +2,16 @@ Location of protocols used in datatypes """ -from typing import Any +from typing import ( + Any, + TYPE_CHECKING, +) from typing_extensions import Protocol +if TYPE_CHECKING: + from sqlalchemy.orm.base import Mapped + class HasClearAssociatedFiles(Protocol): def clear_associated_files(self, metadata_safe: bool = False, purge: bool = False) -> None: ... @@ -35,7 +41,7 @@ class HasHid(Protocol): class HasId(Protocol): - id: int + id: "Mapped[int]" class HasInfo(Protocol): diff --git a/lib/galaxy/datatypes/sequence.py b/lib/galaxy/datatypes/sequence.py index c88abfcde5cd..f4feb6611a31 100644 --- a/lib/galaxy/datatypes/sequence.py +++ b/lib/galaxy/datatypes/sequence.py @@ -31,7 +31,6 @@ MetadataElement, ) from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasMetadata, ) @@ -766,7 +765,7 @@ def sniff_prefix(self, file_prefix: FilePrefix) -> bool: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/spaln.py b/lib/galaxy/datatypes/spaln.py index c3728ef6b4ea..4f5498b6fc9d 100644 --- a/lib/galaxy/datatypes/spaln.py +++ b/lib/galaxy/datatypes/spaln.py @@ -14,7 +14,6 @@ from galaxy.datatypes.data import Data from galaxy.datatypes.metadata import MetadataElement from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasExtraFilesAndMetadata, ) @@ -117,7 +116,7 @@ def display_peek(self, dataset: DatasetProtocol) -> str: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/tabular.py b/lib/galaxy/datatypes/tabular.py index 5b4e1d523e12..82503b654262 100644 --- a/lib/galaxy/datatypes/tabular.py +++ b/lib/galaxy/datatypes/tabular.py @@ -53,7 +53,6 @@ MetadataParameter, ) from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasFileName, HasMetadata, @@ -168,7 +167,7 @@ def _read_chunk(self, trans, dataset: HasFileName, offset: int, ck_size: Optiona def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/datatypes/text.py b/lib/galaxy/datatypes/text.py index 95fa625aa789..8c328ce31cd2 100644 --- a/lib/galaxy/datatypes/text.py +++ b/lib/galaxy/datatypes/text.py @@ -27,7 +27,6 @@ MetadataParameter, ) from galaxy.datatypes.protocols import ( - DatasetHasHidProtocol, DatasetProtocol, HasCreatingJob, HasExtraFilesAndMetadata, @@ -208,7 +207,7 @@ def sniff_prefix(self, file_prefix: FilePrefix) -> bool: def display_data( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, @@ -224,7 +223,7 @@ def display_data( def _display_data_trusted( self, trans, - dataset: DatasetHasHidProtocol, + dataset: DatasetProtocol, preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 4933acc83e9f..9dd8db7a06c9 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -135,6 +135,10 @@ import galaxy.model.tags import galaxy.security.passwords import galaxy.util +from galaxy.datatypes.protocols import ( + DatasetHasHidProtocol, + DatasetProtocol, +) from galaxy.files.templates import ( FileSourceConfiguration, FileSourceTemplate, @@ -475,6 +479,7 @@ def get_display_name(self): class UsesCreateAndUpdateTime: + create_time: Mapped[Optional[datetime]] update_time: Mapped[Optional[datetime]] @property @@ -4530,7 +4535,7 @@ def datatype_for_extension(extension, datatypes_registry=None) -> "Data": return ret -class DatasetInstance(RepresentById, UsesCreateAndUpdateTime, _HasTable): +class DatasetInstance(DatasetProtocol, RepresentById, UsesCreateAndUpdateTime, _HasTable): """A base class for all 'dataset instances', HDAs, LDAs, etc""" states = Dataset.states @@ -4973,7 +4978,9 @@ def find_conversion_destination( ) -> Tuple[bool, Optional[str], Optional["DatasetInstance"]]: """Returns ( target_ext, existing converted dataset )""" return self.datatype.find_conversion_destination( - self, accepted_formats, _get_datatypes_registry(), **kwd # type:ignore[arg-type] + self, + accepted_formats, + _get_datatypes_registry(), ) def add_validation_error(self, validation_error): @@ -5172,12 +5179,15 @@ def _handle_serialize_files(self, id_encoder, serialization_options, rval): rval["file_metadata"] = file_metadata -class HistoryDatasetAssociation(DatasetInstance, HasTags, Dictifiable, UsesAnnotations, HasName, Serializable): +class HistoryDatasetAssociation( + DatasetHasHidProtocol, DatasetInstance, HasTags, Dictifiable, UsesAnnotations, HasName, Serializable +): """ Resource class that creates a relation between a dataset and a user history. """ history_id: Optional[int] + dataset_id: Mapped[int] def __init__( self, @@ -5965,7 +5975,7 @@ def to_dict(self, view="collection"): return rval -class LibraryDatasetDatasetAssociation(DatasetInstance, HasName, Serializable): +class LibraryDatasetDatasetAssociation(DatasetInstance, HasName, Serializable, UsesCreateAndUpdateTime): def __init__( self, diff --git a/lib/galaxy/model/store/__init__.py b/lib/galaxy/model/store/__init__.py index 3be82b1b48b3..cfb975b3699e 100644 --- a/lib/galaxy/model/store/__init__.py +++ b/lib/galaxy/model/store/__init__.py @@ -716,7 +716,7 @@ def handle_dataset_object_edit(dataset_instance, dataset_attrs): # Try to set metadata directly. @mvdbeek thinks we should only record the datasets try: if dataset_instance.has_metadata_files: - dataset_instance.datatype.set_meta(dataset_instance) # type:ignore[arg-type] + dataset_instance.datatype.set_meta(dataset_instance) except Exception: log.debug(f"Metadata setting failed on {dataset_instance}", exc_info=True) dataset_instance.state = dataset_instance.dataset.states.FAILED_METADATA From 5b856b577a881ec6322882dfa96edfc07c023484 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:51:10 +0200 Subject: [PATCH 17/30] Make AccessibleManagerMixin generic --- lib/galaxy/managers/base.py | 7 +++++++ lib/galaxy/managers/secured.py | 26 +++++++++++--------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index 0e86310d57f2..f6ba5140359f 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -200,6 +200,13 @@ class HasModelClass(Generic[U]): model_class: Type[U] +HasIdT = TypeVar("HasIdT", covariant=True) + + +class HasById(Protocol[HasIdT]): + def by_id(self, id: int) -> HasIdT: ... + + # ----------------------------------------------------------------------------- class ModelManager(Generic[U], HasModelClass[U]): """ diff --git a/lib/galaxy/managers/secured.py b/lib/galaxy/managers/secured.py index 8531c7dd044b..57970bafc269 100644 --- a/lib/galaxy/managers/secured.py +++ b/lib/galaxy/managers/secured.py @@ -7,8 +7,8 @@ from typing import ( Any, Generic, + List, Optional, - TYPE_CHECKING, ) from galaxy import ( @@ -16,32 +16,28 @@ model, ) from galaxy.managers.base import ( + HasById, HasModelClass, U, ) -if TYPE_CHECKING: - from sqlalchemy.orm import Query - -class AccessibleManagerMixin(Generic[U], HasModelClass[U]): +class AccessibleManagerMixin(Generic[U], HasModelClass[U], HasById[U]): """ A security interface to check if a User can read/view an item's. This can also be thought of as 'read but not modify' privileges. """ - def by_id(self, id: int): ... - # don't want to override by_id since consumers will also want to fetch w/o any security checks - def is_accessible(self, item, user: model.User, **kwargs: Any) -> bool: + def is_accessible(self, item: U, user: model.User, **kwargs: Any) -> bool: """ Return True if the item accessible to user. """ # override in subclasses raise exceptions.NotImplemented("Abstract interface Method") - def get_accessible(self, id: int, user: model.User, **kwargs: Any): + def get_accessible(self, id: int, user: model.User, **kwargs: Any) -> U: """ Return the item with the given id if it's accessible to user, otherwise raise an error. @@ -51,7 +47,7 @@ def get_accessible(self, id: int, user: model.User, **kwargs: Any): item = self.by_id(id) return self.error_unless_accessible(item, user, **kwargs) - def error_unless_accessible(self, item: "Query", user, **kwargs): + def error_unless_accessible(self, item: U, user, **kwargs) -> U: """ Raise an error if the item is NOT accessible to user, otherwise return the item. @@ -62,7 +58,7 @@ def error_unless_accessible(self, item: "Query", user, **kwargs): raise exceptions.ItemAccessibilityException(f"{self.model_class.__name__} is not accessible by user") # TODO:?? are these even useful? - def list_accessible(self, user, **kwargs): + def list_accessible(self, user, **kwargs) -> List[U]: """ Return a list of items accessible to the user, raising an error if ANY are inaccessible. @@ -74,7 +70,7 @@ def list_accessible(self, user, **kwargs): # items = ModelManager.list( self, trans, **kwargs ) # return [ self.error_unless_accessible( trans, item, user ) for item in items ] - def filter_accessible(self, user, **kwargs): + def filter_accessible(self, user, **kwargs) -> List[U]: """ Return a list of items accessible to the user. """ @@ -123,7 +119,7 @@ def error_unless_owner(self, item: U, user: Optional[model.User], **kwargs: Any) return item raise exceptions.ItemOwnershipException(f"{self.model_class.__name__} is not owned by user") - def list_owned(self, user, **kwargs): + def list_owned(self, user, **kwargs) -> List[U]: """ Return a list of items owned by the user, raising an error if ANY are not. @@ -134,7 +130,7 @@ def list_owned(self, user, **kwargs): # just alias to by_user (easier/same thing) # return self.by_user( trans, user, **kwargs ) - def filter_owned(self, user, **kwargs): + def filter_owned(self, user, **kwargs) -> List[U]: """ Return a list of items owned by the user. """ @@ -152,7 +148,7 @@ def get_mutable(self, id: int, user: Optional[model.User], **kwargs: Any) -> U: self.error_unless_mutable(item) return item - def error_unless_mutable(self, item: U): + def error_unless_mutable(self, item: U) -> None: """ Raise an error if the item is NOT mutable. From 1c6124276f66c605cee5fdab30fa543b527f1b1d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:52:10 +0200 Subject: [PATCH 18/30] Specialize generated query in ModelManager --- lib/galaxy/managers/base.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index f6ba5140359f..fb10f0600618 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -243,7 +243,7 @@ def query( order_by=None, limit: Optional[int] = None, offset: Optional[int] = None, - ) -> Query: + ) -> Query[U]: """ Return a basic query from model_class, filters, order_by, and limit and offset. @@ -256,8 +256,8 @@ def query( return self._filter_and_order_query(query, filters=filters, order_by=order_by, limit=limit, offset=offset) def _filter_and_order_query( - self, query: Query, filters=None, order_by=None, limit: Optional[int] = None, offset: Optional[int] = None - ) -> Query: + self, query: Query[U], filters=None, order_by=None, limit: Optional[int] = None, offset: Optional[int] = None + ) -> Query[U]: # TODO: not a lot of functional cohesion here query = self._apply_orm_filters(query, filters) query = self._apply_order_by(query, order_by) @@ -265,7 +265,7 @@ def _filter_and_order_query( return query # .... filters - def _apply_orm_filters(self, query: Query, filters) -> Query: + def _apply_orm_filters(self, query: Query[U], filters) -> Query[U]: """ Add any filters to the given query. """ @@ -280,7 +280,7 @@ def _apply_orm_filters(self, query: Query, filters) -> Query: return query # .... order, limit, and offset - def _apply_order_by(self, query: Query, order_by) -> Query: + def _apply_order_by(self, query: Query[U], order_by) -> Query[U]: """ Return the query after adding the order_by clauses. @@ -299,7 +299,7 @@ def _default_order_by(self): """ return (self.model_class.__table__.c.create_time,) - def _apply_orm_limit_offset(self, query: Query, limit: Optional[int], offset: Optional[int]) -> Query: + def _apply_orm_limit_offset(self, query: Query[U], limit: Optional[int], offset: Optional[int]) -> Query[U]: """ Return the query after applying the given limit and offset (if not None). """ From 67024f0e08a2bec367b14f10bd7533967fe3bf85 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:53:32 +0200 Subject: [PATCH 19/30] Add type parameters to DatasetManager --- lib/galaxy/managers/datasets.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py index d52f96422fb4..ddb8d8b3f19c 100644 --- a/lib/galaxy/managers/datasets.py +++ b/lib/galaxy/managers/datasets.py @@ -46,7 +46,11 @@ T = TypeVar("T") -class DatasetManager(base.ModelManager[model.Dataset], secured.AccessibleManagerMixin, deletable.PurgableManagerMixin): +class DatasetManager( + base.ModelManager[model.Dataset], + secured.AccessibleManagerMixin[model.Dataset], + deletable.PurgableManagerMixin[model.Dataset], +): """ Manipulate datasets: the components contained in DatasetAssociations/DatasetInstances/HDAs/LDDAs """ @@ -344,7 +348,7 @@ def serialize_permissions(self, item, key, user=None, **context): class DatasetAssociationManager( Generic[DI], base.ModelManager[DI], - secured.AccessibleManagerMixin, + secured.AccessibleManagerMixin[DI], secured.OwnableManagerMixin[DI], deletable.PurgableManagerMixin[DI], ): From f388f05c56b0446200fe6b7a663c142d39106a62 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:56:27 +0200 Subject: [PATCH 20/30] Add type annotation to HDAManager copy signature --- lib/galaxy/managers/hdas.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py index 19cfcfcbb553..1e44817467f6 100644 --- a/lib/galaxy/managers/hdas.py +++ b/lib/galaxy/managers/hdas.py @@ -196,7 +196,12 @@ def materialize(self, request: MaterializeDatasetInstanceTaskRequest) -> None: session.commit() def copy( - self, item: Any, history=None, hide_copy: bool = False, flush: bool = True, **kwargs: Any + self, + item: model.HistoryDatasetAssociation, + history=None, + hide_copy: bool = False, + flush: bool = True, + **kwargs: Any, ) -> model.HistoryDatasetAssociation: """ Copy hda, including annotation and tags, add to history and return the given HDA. From e8d0e094c442d00c826878c593fd02e8b72424f0 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 15:56:51 +0200 Subject: [PATCH 21/30] Eliminate unnecessary type ignores --- lib/galaxy/managers/jobs.py | 6 +++--- lib/galaxy/managers/users.py | 2 +- lib/galaxy/model/__init__.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index bf26cad31a0a..b9768ac6d731 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -529,7 +529,7 @@ def _build_stmt_for_hda(self, stmt, data_conditions, used_ids, k, v, identifier) model.HistoryDatasetAssociation.id == e.history_dataset_association_id ) # b is the HDA used for the job - stmt = stmt.join(b, a.dataset_id == b.id).join(c, c.dataset_id == b.dataset_id) # type:ignore[attr-defined] + stmt = stmt.join(b, a.dataset_id == b.id).join(c, c.dataset_id == b.dataset_id) name_condition = [] if identifier: stmt = stmt.join(d) @@ -627,7 +627,7 @@ def _build_stmt_for_dce(self, stmt, data_conditions, used_ids, k, v): ), ) .outerjoin(d, d.id == c.hda_id) - .outerjoin(e, e.dataset_id == d.dataset_id) # type:ignore[attr-defined] + .outerjoin(e, e.dataset_id == d.dataset_id) ) data_conditions.append( and_( @@ -637,7 +637,7 @@ def _build_stmt_for_dce(self, stmt, data_conditions, used_ids, k, v): and_( c.hda_id == b.hda_id, d.id == c.hda_id, - e.dataset_id == d.dataset_id, # type:ignore[attr-defined] + e.dataset_id == d.dataset_id, ), ), c.id == v, diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 1ce9e21146f1..b96a2c3354cb 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -196,7 +196,7 @@ def _error_on_duplicate_email(self, email: str) -> None: raise exceptions.Conflict("Email must be unique", email=email) def by_id(self, user_id: int) -> U: - return self.app.model.session.get(self.model_class, user_id) + return self.one(id=user_id) # ---- filters def by_email(self, email: str, filters=None, **kwargs) -> Optional[U]: diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 9dd8db7a06c9..92e626071f2e 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -4074,7 +4074,7 @@ class Dataset(Base, StorableObject, Serializable): active_history_associations: Mapped[List["HistoryDatasetAssociation"]] = relationship( primaryjoin=( lambda: and_( - Dataset.id == HistoryDatasetAssociation.dataset_id, # type: ignore[attr-defined] + Dataset.id == HistoryDatasetAssociation.dataset_id, HistoryDatasetAssociation.deleted == false(), # type: ignore[has-type] HistoryDatasetAssociation.purged == false(), # type: ignore[arg-type] ) @@ -4084,7 +4084,7 @@ class Dataset(Base, StorableObject, Serializable): purged_history_associations: Mapped[List["HistoryDatasetAssociation"]] = relationship( primaryjoin=( lambda: and_( - Dataset.id == HistoryDatasetAssociation.dataset_id, # type: ignore[attr-defined] + Dataset.id == HistoryDatasetAssociation.dataset_id, HistoryDatasetAssociation.purged == true(), # type: ignore[arg-type] ) ), From 35b1e125d0d7686586fcd09a15bb6249f3e41b81 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 16:13:42 +0200 Subject: [PATCH 22/30] Add type hint for dataset_manager_by_type --- lib/galaxy/webapps/galaxy/services/datasets.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index de89199202b8..322fc30628b7 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -11,6 +11,7 @@ List, Optional, Tuple, + TypedDict, Union, ) @@ -32,10 +33,7 @@ from galaxy.datatypes.dataproviders.exceptions import NoProviderAvailable from galaxy.managers.base import ModelSerializer from galaxy.managers.context import ProvidesHistoryContext -from galaxy.managers.datasets import ( - DatasetAssociationManager, - DatasetManager, -) +from galaxy.managers.datasets import DatasetManager from galaxy.managers.hdas import ( HDAManager, HDASerializer, @@ -286,6 +284,11 @@ class DeleteDatasetBatchResult(Model): ) +class DatasetManagerByType(TypedDict): + hda: HDAManager + ldda: LDDAManager + + class DatasetsService(ServiceBase, UsesVisualizationMixin): def __init__( self, @@ -316,7 +319,7 @@ def serializer_by_type(self) -> Dict[str, ModelSerializer]: return {"dataset": self.hda_serializer, "dataset_collection": self.hdca_serializer} @property - def dataset_manager_by_type(self) -> Dict[str, DatasetAssociationManager]: + def dataset_manager_by_type(self) -> DatasetManagerByType: return {"hda": self.hda_manager, "ldda": self.ldda_manager} def index( From b5e1ddcb96fda45785398666e5b225b597a3eb8a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 16:12:11 +0200 Subject: [PATCH 23/30] Undo DatasetSourceType change --- lib/galaxy/managers/hdas.py | 8 +++++--- lib/galaxy/schema/schema.py | 17 ++++++++++------- lib/galaxy/webapps/galaxy/api/datasets.py | 4 ++-- .../webapps/galaxy/api/history_contents.py | 3 +-- lib/galaxy/webapps/galaxy/api/jobs.py | 8 ++++---- lib/galaxy/webapps/galaxy/services/jobs.py | 5 ++++- 6 files changed, 26 insertions(+), 19 deletions(-) diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py index 1e44817467f6..194f9c069343 100644 --- a/lib/galaxy/managers/hdas.py +++ b/lib/galaxy/managers/hdas.py @@ -14,6 +14,7 @@ List, Optional, Set, + Union, ) from sqlalchemy import ( @@ -51,7 +52,6 @@ ) from galaxy.model.base import transaction from galaxy.model.deferred import materializer_factory -from galaxy.schema.schema import DatasetSourceType from galaxy.schema.storage_cleaner import ( CleanableItemsSummary, StorageItemCleanupError, @@ -184,8 +184,10 @@ def materialize(self, request: MaterializeDatasetInstanceTaskRequest) -> None: sa_session=self.app.model.session(), ) user = self.user_manager.by_id(request_user.user_id) - if request.source == DatasetSourceType.hda: - dataset_instance = self.get_accessible(request.content, user) + if request.source == "hda": + dataset_instance: Union[model.HistoryDatasetAssociation, model.LibraryDatasetDatasetAssociation] = ( + self.get_accessible(request.content, user) + ) else: dataset_instance = self.ldda_manager.get_accessible(request.content, user) history = self.app.history_manager.by_id(request.history_id) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 66a331ad0ef8..d59056088739 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -699,11 +699,14 @@ class HDAInaccessible(HDACommon): state: DatasetStateField -HdaLddaField = Field( - DatasetSourceType.hda, - title="HDA or LDDA", - description="Whether this dataset belongs to a history (HDA) or a library (LDDA).", -) +HdaLddaField = Annotated[ + DataItemSourceType, + Field( + "hda", + title="HDA or LDDA", + description="Whether this dataset belongs to a history (HDA) or a library (LDDA).", + ), +] class DatasetValidatedState(str, Enum): @@ -760,7 +763,7 @@ class HDADetailed(HDASummary, WithModelClass): """History Dataset Association detailed information.""" model_class: Annotated[HDA_MODEL_CLASS, ModelClassField(HDA_MODEL_CLASS)] - hda_ldda: DatasetSourceType = HdaLddaField + hda_ldda: HdaLddaField accessible: bool = AccessibleField misc_info: Optional[str] = Field( default=None, @@ -942,7 +945,7 @@ class HDAObject(Model, WithModelClass): id: HistoryDatasetAssociationId model_class: HDA_MODEL_CLASS = ModelClassField(HDA_MODEL_CLASS) state: DatasetStateField - hda_ldda: DatasetSourceType = HdaLddaField + hda_ldda: HdaLddaField history_id: HistoryID tags: List[str] copied_from_ldda_id: Optional[EncodedDatabaseIdField] = None diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 53006b4ee8c1..ea16afa18ad3 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -81,7 +81,7 @@ ] DatasetSourceQueryParam: DatasetSourceType = Query( - default=DatasetSourceType.hda, + default="hda", description="Whether this dataset belongs to a history (HDA) or a library (LDDA).", ) @@ -421,7 +421,7 @@ def show( dataset_id: HistoryDatasetIDPathParam, trans=DependsOnTrans, hda_ldda: DatasetSourceType = Query( - default=DatasetSourceType.hda, + default="hda", description=("The type of information about the dataset to be requested."), ), data_type: Optional[RequestDataType] = Query( diff --git a/lib/galaxy/webapps/galaxy/api/history_contents.py b/lib/galaxy/webapps/galaxy/api/history_contents.py index 052ea3a2e465..ddfe3c2ce642 100644 --- a/lib/galaxy/webapps/galaxy/api/history_contents.py +++ b/lib/galaxy/webapps/galaxy/api/history_contents.py @@ -39,7 +39,6 @@ AsyncFile, AsyncTaskResultSummary, DatasetAssociationRoles, - DatasetSourceType, DeleteHistoryContentPayload, DeleteHistoryContentResult, HistoryContentBulkOperationPayload, @@ -1066,7 +1065,7 @@ def materialize_dataset( # values are already validated, use model_construct materialize_request = MaterializeDatasetInstanceRequest.model_construct( history_id=history_id, - source=DatasetSourceType.hda, + source="hda", content=id, ) rval = self.service.materialize(trans, materialize_request) diff --git a/lib/galaxy/webapps/galaxy/api/jobs.py b/lib/galaxy/webapps/galaxy/api/jobs.py index 6c6f7757f9a7..6093684682b7 100644 --- a/lib/galaxy/webapps/galaxy/api/jobs.py +++ b/lib/galaxy/webapps/galaxy/api/jobs.py @@ -377,7 +377,7 @@ def outputs( def parameters_display_by_job( self, job_id: JobIdPathParam, - hda_ldda: Annotated[Optional[DatasetSourceType], DeprecatedHdaLddaQueryParam] = DatasetSourceType.hda, + hda_ldda: Annotated[Optional[DatasetSourceType], DeprecatedHdaLddaQueryParam] = "hda", trans: ProvidesUserContext = DependsOnTrans, ) -> JobDisplayParametersSummary: """ @@ -398,7 +398,7 @@ def parameters_display_by_job( def parameters_display_by_dataset( self, dataset_id: DatasetIdPathParam, - hda_ldda: Annotated[DatasetSourceType, HdaLddaQueryParam] = DatasetSourceType.hda, + hda_ldda: Annotated[DatasetSourceType, HdaLddaQueryParam] = "hda", trans: ProvidesUserContext = DependsOnTrans, ) -> JobDisplayParametersSummary: """ @@ -417,7 +417,7 @@ def parameters_display_by_dataset( def metrics_by_job( self, job_id: JobIdPathParam, - hda_ldda: Annotated[Optional[DatasetSourceType], DeprecatedHdaLddaQueryParam] = DatasetSourceType.hda, + hda_ldda: Annotated[Optional[DatasetSourceType], DeprecatedHdaLddaQueryParam] = "hda", trans: ProvidesUserContext = DependsOnTrans, ) -> List[Optional[JobMetric]]: hda_ldda_str = hda_ldda or "hda" @@ -433,7 +433,7 @@ def metrics_by_job( def metrics_by_dataset( self, dataset_id: DatasetIdPathParam, - hda_ldda: Annotated[DatasetSourceType, HdaLddaQueryParam] = DatasetSourceType.hda, + hda_ldda: Annotated[DatasetSourceType, HdaLddaQueryParam] = "hda", trans: ProvidesUserContext = DependsOnTrans, ) -> List[Optional[JobMetric]]: job = self.service.get_job(trans, dataset_id=dataset_id, hda_ldda=hda_ldda) diff --git a/lib/galaxy/webapps/galaxy/services/jobs.py b/lib/galaxy/webapps/galaxy/services/jobs.py index 240d841a67ea..739269cd6eba 100644 --- a/lib/galaxy/webapps/galaxy/services/jobs.py +++ b/lib/galaxy/webapps/galaxy/services/jobs.py @@ -4,6 +4,7 @@ Dict, List, Optional, + Union, ) from galaxy import ( @@ -122,7 +123,9 @@ def get_job( elif dataset_id is not None: # Following checks dataset accessible if hda_ldda == "hda": - dataset_instance = self.hda_manager.get_accessible(id=dataset_id, user=trans.user) + dataset_instance: Union[model.HistoryDatasetAssociation, model.LibraryDatasetDatasetAssociation] = ( + self.hda_manager.get_accessible(id=dataset_id, user=trans.user) + ) else: dataset_instance = self.hda_manager.ldda_manager.get(trans, id=dataset_id) if not dataset_instance.creating_job: From f5ed1f14d7b61aa49e69978352fa419e871580aa Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 16:30:11 +0200 Subject: [PATCH 24/30] Turn DatasetSourceType into literal This allows type lookups using TypedDict, which isn't possible with enum members. --- client/src/api/schema/schema.ts | 36 +++++++++---------- lib/galaxy/schema/schema.py | 4 +-- .../webapps/galaxy/services/datasets.py | 17 ++++----- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index 2584707d0d76..645fc8d052bb 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -4019,14 +4019,10 @@ export interface components { /** * Source * @description The source of this dataset, either `hda` or `ldda` depending of its origin. + * @enum {string} */ - src: components["schemas"]["DatasetSourceType"]; + src: "hda" | "ldda"; }; - /** - * DatasetSourceType - * @enum {string} - */ - DatasetSourceType: "hda" | "ldda"; /** * DatasetState * @enum {string} @@ -4645,8 +4641,9 @@ export interface components { /** * Source * @description The source of this dataset, either `hda` or `ldda` depending of its origin. + * @enum {string} */ - src: components["schemas"]["DatasetSourceType"]; + src: "hda" | "ldda"; }; /** EncodedHdcaSourceId */ EncodedHdcaSourceId: { @@ -5605,7 +5602,7 @@ export interface components { * HDA or LDDA * @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ - hda_ldda?: components["schemas"]["DatasetSourceType"] | null; + hda_ldda?: components["schemas"]["DataItemSourceType"] | null; /** * HID * @description The index position of this item in the History. @@ -5851,7 +5848,7 @@ export interface components { * @description Whether this dataset belongs to a history (HDA) or a library (LDDA). * @default hda */ - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: components["schemas"]["DataItemSourceType"]; /** * HID * @description The index position of this item in the History. @@ -6000,7 +5997,7 @@ export interface components { * @description Whether this dataset belongs to a history (HDA) or a library (LDDA). * @default hda */ - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: components["schemas"]["DataItemSourceType"]; /** * History ID * @example 0123456789ABCDEF @@ -9370,8 +9367,9 @@ export interface components { /** * Source * @description The source of the content. Can be other history element to be copied or library elements. + * @enum {string} */ - source: components["schemas"]["DatasetSourceType"]; + source: "hda" | "ldda"; }; /** MessageNotificationContent */ MessageNotificationContent: { @@ -13847,7 +13845,7 @@ export interface operations { /** @description View to be passed to the serializer */ /** @description Comma-separated list of keys to be passed to the serializer */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; data_type?: components["schemas"]["RequestDataType"] | null; view?: string | null; keys?: string | null; @@ -14148,7 +14146,7 @@ export interface operations { parameters: { /** @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -14184,7 +14182,7 @@ export interface operations { parameters: { /** @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -14218,7 +14216,7 @@ export interface operations { parameters: { /** @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -14287,7 +14285,7 @@ export interface operations { parameters: { /** @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -14353,7 +14351,7 @@ export interface operations { parameters: { /** @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"]; + hda_ldda?: "hda" | "ldda"; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -19697,7 +19695,7 @@ export interface operations { * @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"] | null; + hda_ldda?: ("hda" | "ldda") | null; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { @@ -19798,7 +19796,7 @@ export interface operations { * @description Whether this dataset belongs to a history (HDA) or a library (LDDA). */ query?: { - hda_ldda?: components["schemas"]["DatasetSourceType"] | null; + hda_ldda?: ("hda" | "ldda") | null; }; /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ header?: { diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index d59056088739..43a79127ffd8 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -503,9 +503,7 @@ class DCEType(str, Enum): # TODO: suspiciously similar to HistoryContentType dataset_collection = "dataset_collection" -class DatasetSourceType(str, Enum): - hda = "hda" - ldda = "ldda" +DatasetSourceType = Annotated[Literal["hda", "ldda"], Field(description="Type of dataset association")] class DataItemSourceType(str, Enum): diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index 322fc30628b7..7551d2a95f07 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -394,7 +394,7 @@ def show( rval = self._dataset_in_use_state(dataset) else: # Default: return dataset as dict. - if hda_ldda == DatasetSourceType.hda: + if hda_ldda == "hda": return self.hda_serializer.serialize_to_view( dataset, view=serialization_params.view or "detailed", user=trans.user, trans=trans ) @@ -407,7 +407,7 @@ def show_storage( self, trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, - hda_ldda: DatasetSourceType = DatasetSourceType.hda, + hda_ldda: DatasetSourceType = "hda", ) -> DatasetStorageDetails: """ Display user-facing storage details related to the objectstore a @@ -457,7 +457,7 @@ def show_inheritance_chain( self, trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, - hda_ldda: DatasetSourceType = DatasetSourceType.hda, + hda_ldda: DatasetSourceType = "hda", ) -> DatasetInheritanceChain: """ Display inheritance chain for the given dataset. @@ -475,7 +475,7 @@ def compute_hash( trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, payload: ComputeDatasetHashPayload, - hda_ldda: DatasetSourceType = DatasetSourceType.hda, + hda_ldda: DatasetSourceType = "hda", ) -> AsyncTaskResultSummary: dataset_instance = self.dataset_manager_by_type[hda_ldda].get_accessible(dataset_id, trans.user) request = ComputeDatasetHashTaskRequest( @@ -488,12 +488,13 @@ def compute_hash( return async_task_summary(result) def drs_dataset_instance(self, object_id: str) -> Tuple[int, DatasetSourceType]: + hda_ldda: DatasetSourceType if object_id.startswith("hda-"): decoded_object_id = self.decode_id(object_id[len("hda-") :], kind="drs") - hda_ldda = DatasetSourceType.hda + hda_ldda = "hda" elif object_id.startswith("ldda-"): decoded_object_id = self.decode_id(object_id[len("ldda-") :], kind="drs") - hda_ldda = DatasetSourceType.ldda + hda_ldda = "ldda" else: raise galaxy_exceptions.RequestParameterInvalidException( "Invalid object_id format specified for this Galaxy server" @@ -552,7 +553,7 @@ def update_permissions( trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, payload: UpdateDatasetPermissionsPayload, - hda_ldda: DatasetSourceType = DatasetSourceType.hda, + hda_ldda: DatasetSourceType = "hda", ) -> DatasetAssociationRoles: """ Updates permissions of a dataset. @@ -590,7 +591,7 @@ def display( self, trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, - hda_ldda: DatasetSourceType = DatasetSourceType.hda, + hda_ldda: DatasetSourceType = "hda", preview: bool = False, filename: Optional[str] = None, to_ext: Optional[str] = None, From 57f008525adbabc3c8bdbe503ba1e957de0046b4 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 16:38:39 +0200 Subject: [PATCH 25/30] Do proper type narrowing in delete_batch --- .../webapps/galaxy/services/datasets.py | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index 7551d2a95f07..6ad3932e89f2 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -725,15 +725,23 @@ def delete_batch( errors: List[DatasetErrorMessage] = [] for dataset in payload.datasets: try: - manager = self.dataset_manager_by_type[dataset.src] - dataset_instance = manager.get_owned(dataset.id, trans.user) - manager.error_unless_mutable(dataset_instance.history) - if dataset.src == DatasetSourceType.hda: - self.hda_manager.error_if_uploading(dataset_instance) - if payload.purge: - manager.purge(dataset_instance, flush=True) + if dataset.src == "hda": + hda_manager = self.dataset_manager_by_type[dataset.src] + hda = hda_manager.get_owned(dataset.id, trans.user) + hda_manager.error_if_uploading(hda) + hda_manager.error_unless_mutable(hda.history) + if payload.purge: + hda_manager.purge(hda, flush=True) + else: + hda_manager.delete(hda, flush=False) else: - manager.delete(dataset_instance, flush=False) + ldda_manager = self.dataset_manager_by_type[dataset.src] + ldda = ldda_manager.get_owned(dataset.id, trans.user) + if payload.purge: + ldda_manager.purge(ldda, flush=True) + else: + ldda_manager.delete(ldda, flush=False) + success_count += 1 except galaxy_exceptions.MessageException as e: errors.append( From 796fcd141e256f73e8139d9edef692ed821d83d1 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 16:42:57 +0200 Subject: [PATCH 26/30] Ensure dataset should be on disk before opening dataset --- lib/galaxy/webapps/galaxy/services/datasets.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index 6ad3932e89f2..dbb899467245 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -610,8 +610,15 @@ def display( headers = {} rval: Any = "" try: - dataset_manager = self.dataset_manager_by_type[hda_ldda] - dataset_instance = dataset_manager.get_accessible(dataset_id, trans.user) + dataset_instance: Union[model.HistoryDatasetAssociation, model.LibraryDatasetDatasetAssociation] + if hda_ldda == "hda": + hda_manager = self.dataset_manager_by_type[hda_ldda] + dataset_instance = hda = hda_manager.get_accessible(dataset_id, trans.user) + hda_manager.ensure_dataset_on_disk(trans, hda) + else: + ldda_manager = self.dataset_manager_by_type[hda_ldda] + dataset_instance = ldda = ldda_manager.get_accessible(dataset_id, trans.user) + ldda_manager.ensure_dataset_on_disk(trans, ldda) if raw: if filename and filename != "index": object_store = trans.app.object_store From 26b6c7412b4d698d7ea89aca3b8bfca3ab3b3046 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 17:05:00 +0200 Subject: [PATCH 27/30] Remove unnecessary by_id implementation --- lib/galaxy/managers/users.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index b96a2c3354cb..639e1b65c2ff 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -195,9 +195,6 @@ def _error_on_duplicate_email(self, email: str) -> None: if self.by_email(email) is not None: raise exceptions.Conflict("Email must be unique", email=email) - def by_id(self, user_id: int) -> U: - return self.one(id=user_id) - # ---- filters def by_email(self, email: str, filters=None, **kwargs) -> Optional[U]: """ From 0eced9030d84fdc366637ec9b7da57865ec421cf Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 17:05:59 +0200 Subject: [PATCH 28/30] Add API test to ensure that a proper error is returned if the dataset can't be opened --- lib/galaxy_test/api/test_datasets.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/galaxy_test/api/test_datasets.py b/lib/galaxy_test/api/test_datasets.py index 3c8c9daf3420..9705d2d2b577 100644 --- a/lib/galaxy_test/api/test_datasets.py +++ b/lib/galaxy_test/api/test_datasets.py @@ -343,6 +343,17 @@ def test_display(self, history_id): self._assert_status_code_is(display_response, 200) assert display_response.text == contents + def test_display_error_handling(self, history_id): + hda1 = self.dataset_populator.create_deferred_hda( + history_id, "https://raw.githubusercontent.com/galaxyproject/galaxy/dev/test-data/1.bed" + ) + display_response = self._get(f"histories/{history_id}/contents/{hda1['id']}/display", {"raw": "True"}) + self._assert_status_code_is(display_response, 409) + assert ( + display_response.json()["err_msg"] + == "The dataset you are attempting to view has deferred data. You can only use this dataset as input for jobs." + ) + def test_get_content_as_text(self, history_id): contents = textwrap.dedent( """\ From 70aa9d71a76d69fca044a82b0d6c0bd077f19c95 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 23 May 2024 17:16:16 +0200 Subject: [PATCH 29/30] Restore nullability of column definition --- lib/galaxy/model/__init__.py | 6 +++--- lib/tool_shed/webapp/model/__init__.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 92e626071f2e..cd2b79631e3c 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -782,7 +782,7 @@ class User(Base, Dictifiable, RepresentById): create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True) email: Mapped[str] = mapped_column(TrimmedString(255), index=True) - username: Mapped[str] = mapped_column(TrimmedString(255), index=True, unique=True) + username: Mapped[str] = mapped_column(TrimmedString(255), index=True, unique=True, nullable=True) password: Mapped[str] = mapped_column(TrimmedString(255)) last_password_change: Mapped[Optional[datetime]] = mapped_column(default=now) external: Mapped[Optional[bool]] = mapped_column(default=False) @@ -6847,7 +6847,7 @@ class HistoryDatasetCollectionAssociation( collection_id: Mapped[int] = mapped_column(ForeignKey("dataset_collection.id"), index=True) history_id: Mapped[int] = mapped_column(ForeignKey("history.id"), index=True) name: Mapped[Optional[str]] = mapped_column(TrimmedString(255)) - hid: Mapped[int] + hid: Mapped[int] = mapped_column(nullabled=True) visible: Mapped[Optional[bool]] deleted: Mapped[Optional[bool]] = mapped_column(default=False) copied_from_history_dataset_collection_association_id: Mapped[Optional[int]] = mapped_column( @@ -11152,7 +11152,7 @@ class APIKeys(Base, RepresentById): id: Mapped[int] = mapped_column(primary_key=True) create_time: Mapped[datetime] = mapped_column(default=now, nullable=True) user_id: Mapped[Optional[int]] = mapped_column(ForeignKey("galaxy_user.id"), index=True) - key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True) + key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True, nullable=True) user: Mapped[Optional["User"]] = relationship(back_populates="api_keys") deleted: Mapped[bool] = mapped_column(index=True, server_default=false()) diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index 5916325c03cd..fa29287ae741 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -96,7 +96,7 @@ class APIKeys(Base): id: Mapped[int] = mapped_column(Integer, primary_key=True) create_time: Mapped[Optional[datetime]] = mapped_column(DateTime, default=now) user_id: Mapped[Optional[int]] = mapped_column(ForeignKey("galaxy_user.id"), index=True) - key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True) + key: Mapped[str] = mapped_column(TrimmedString(32), index=True, unique=True, nullable=True) user = relationship("User", back_populates="api_keys") deleted: Mapped[Optional[bool]] = mapped_column(Boolean, index=True, default=False) From 2e0fe2a34223cf91e584cc8e38de5ce4b754f6a3 Mon Sep 17 00:00:00 2001 From: Marius van den Beek Date: Thu, 23 May 2024 17:51:40 +0200 Subject: [PATCH 30/30] Fix nullable typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David López <46503462+davelopez@users.noreply.github.com> --- lib/galaxy/model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index cd2b79631e3c..81f8481006dd 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -6847,7 +6847,7 @@ class HistoryDatasetCollectionAssociation( collection_id: Mapped[int] = mapped_column(ForeignKey("dataset_collection.id"), index=True) history_id: Mapped[int] = mapped_column(ForeignKey("history.id"), index=True) name: Mapped[Optional[str]] = mapped_column(TrimmedString(255)) - hid: Mapped[int] = mapped_column(nullabled=True) + hid: Mapped[int] = mapped_column(nullable=True) visible: Mapped[Optional[bool]] deleted: Mapped[Optional[bool]] = mapped_column(default=False) copied_from_history_dataset_collection_association_id: Mapped[Optional[int]] = mapped_column(