From bf6fadb7522b41817bb71736af0033c7c4eac42e Mon Sep 17 00:00:00 2001
From: Nicola Soranzo <nicola.soranzo@earlham.ac.uk>
Date: Tue, 3 Sep 2024 19:46:13 +0100
Subject: [PATCH 1/2] Refactor and type annotate LibraryDatasetsManager

and related code to reduce the use of trans.
---
 lib/galaxy/managers/base.py                   |  6 +-
 lib/galaxy/managers/datasets.py               | 85 ++++++++++---------
 lib/galaxy/managers/folders.py                |  2 +-
 lib/galaxy/managers/library_datasets.py       | 34 ++++----
 lib/galaxy/model/__init__.py                  | 17 ++--
 .../webapps/galaxy/api/library_datasets.py    |  2 +-
 .../webapps/galaxy/services/datasets.py       |  2 +-
 .../galaxy/services/history_contents.py       |  2 +-
 8 files changed, 78 insertions(+), 72 deletions(-)

diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py
index 9866fb91ed15..2a0b4765c0de 100644
--- a/lib/galaxy/managers/base.py
+++ b/lib/galaxy/managers/base.py
@@ -503,18 +503,18 @@ def copy(self, item, **kwargs) -> U:
         """
         raise exceptions.NotImplemented("Abstract method")
 
-    def update(self, item, new_values, flush=True, **kwargs) -> U:
+    def update(self, item: U, new_values: Dict[str, Any], flush: bool = True, **kwargs) -> U:
         """
         Given a dictionary of new values, update `item` and return it.
 
         ..note: NO validation or deserialization occurs here.
         """
-        self.session().add(item)
         for key, value in new_values.items():
             if hasattr(item, key):
                 setattr(item, key, value)
+        session = self.session()
+        session.add(item)
         if flush:
-            session = self.session()
             with transaction(session):
                 session.commit()
         return item
diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py
index 13e523ba4ae0..08f0d3c05f2a 100644
--- a/lib/galaxy/managers/datasets.py
+++ b/lib/galaxy/managers/datasets.py
@@ -31,6 +31,7 @@
 from galaxy.model import (
     Dataset,
     DatasetHash,
+    DatasetInstance,
 )
 from galaxy.model.base import transaction
 from galaxy.schema.tasks import (
@@ -45,12 +46,12 @@
 T = TypeVar("T")
 
 
-class DatasetManager(base.ModelManager[model.Dataset], secured.AccessibleManagerMixin, deletable.PurgableManagerMixin):
+class DatasetManager(base.ModelManager[Dataset], secured.AccessibleManagerMixin, deletable.PurgableManagerMixin):
     """
     Manipulate datasets: the components contained in DatasetAssociations/DatasetInstances/HDAs/LDDAs
     """
 
-    model_class = model.Dataset
+    model_class = Dataset
     foreign_key_name = "dataset"
     app: MinimalManagerApp
 
@@ -62,7 +63,6 @@ def __init__(self, app: MinimalManagerApp):
         # needed for admin test
         self.user_manager = users.UserManager(app)
         self.quota_agent = app.quota_agent
-        self.security_agent = app.model.security_agent
 
     def copy(self, item, **kwargs):
         raise exceptions.NotImplemented("Datasets cannot be copied")
@@ -142,7 +142,7 @@ def update_object_store_id(self, trans, dataset, object_store_id: str):
                 "Cannot swap object store IDs for object stores that don't share a device ID."
             )
 
-        if not self.security_agent.can_change_object_store_id(trans.user, dataset):
+        if not self.app.security_agent.can_change_object_store_id(trans.user, dataset):
             # TODO: probably want separate exceptions for doesn't own the dataset and dataset
             # has been shared.
             raise exceptions.InsufficientPermissionsException("Cannot change dataset permissions...")
@@ -152,7 +152,7 @@ def update_object_store_id(self, trans, dataset, object_store_id: str):
             new_label = quota_source_map.get_quota_source_label(new_object_store_id)
             if old_label != new_label:
                 self.quota_agent.relabel_quota_for_dataset(dataset, old_label, new_label)
-        sa_session = self.app.model.context
+        sa_session = self.session()
         with transaction(sa_session):
             dataset.object_store_id = new_object_store_id
             sa_session.add(dataset)
@@ -321,7 +321,7 @@ def serialize_permissions(self, item, key, user=None, **context):
 
 # ============================================================================= AKA DatasetInstanceManager
 class DatasetAssociationManager(
-    base.ModelManager[model.DatasetInstance],
+    base.ModelManager[DatasetInstance],
     secured.AccessibleManagerMixin,
     secured.OwnableManagerMixin,
     deletable.PurgableManagerMixin,
@@ -338,7 +338,7 @@ class DatasetAssociationManager(
 
     # NOTE: model_manager_class should be set in HDA/LDA subclasses
 
-    def __init__(self, app):
+    def __init__(self, app: MinimalManagerApp):
         super().__init__(app)
         self.dataset_manager = DatasetManager(app)
 
@@ -404,7 +404,7 @@ def stop_creating_job(self, dataset_assoc, flush=False):
 
         # Optimize this to skip other checks if this dataset is terminal - we can infer the
         # job is already complete.
-        if dataset_assoc.state in model.Dataset.terminal_states:
+        if dataset_assoc.state in Dataset.terminal_states:
             return False
 
         if dataset_assoc.parent_id is None and len(dataset_assoc.creating_job_associations) > 0:
@@ -438,7 +438,7 @@ def extra_files(self, dataset_assoc):
             return []
         return glob.glob(os.path.join(dataset_assoc.dataset.extra_files_path, "*"))
 
-    def serialize_dataset_association_roles(self, trans, dataset_assoc):
+    def serialize_dataset_association_roles(self, dataset_assoc):
         if hasattr(dataset_assoc, "library_dataset_dataset_association"):
             library_dataset = dataset_assoc
             dataset = library_dataset.library_dataset_dataset_association.dataset
@@ -447,25 +447,24 @@ def serialize_dataset_association_roles(self, trans, dataset_assoc):
             dataset = dataset_assoc.dataset
 
         # Omit duplicated roles by converting to set
-        security_agent = trans.app.security_agent
-        access_roles = set(dataset.get_access_roles(security_agent))
-        manage_roles = set(dataset.get_manage_permissions_roles(security_agent))
+        access_roles = set(dataset.get_access_roles(self.app.security_agent))
+        manage_roles = set(dataset.get_manage_permissions_roles(self.app.security_agent))
 
         access_dataset_role_list = [
-            (access_role.name, trans.security.encode_id(access_role.id)) for access_role in access_roles
+            (access_role.name, self.app.security.encode_id(access_role.id)) for access_role in access_roles
         ]
         manage_dataset_role_list = [
-            (manage_role.name, trans.security.encode_id(manage_role.id)) for manage_role in manage_roles
+            (manage_role.name, self.app.security.encode_id(manage_role.id)) for manage_role in manage_roles
         ]
         rval = dict(access_dataset_roles=access_dataset_role_list, manage_dataset_roles=manage_dataset_role_list)
         if library_dataset is not None:
             modify_roles = set(
-                security_agent.get_roles_for_action(
-                    library_dataset, trans.app.security_agent.permitted_actions.LIBRARY_MODIFY
+                self.app.security_agent.get_roles_for_action(
+                    library_dataset, self.app.security_agent.permitted_actions.LIBRARY_MODIFY
                 )
             )
             modify_item_role_list = [
-                (modify_role.name, trans.security.encode_id(modify_role.id)) for modify_role in modify_roles
+                (modify_role.name, self.app.security.encode_id(modify_role.id)) for modify_role in modify_roles
             ]
             rval["modify_item_roles"] = modify_item_role_list
         return rval
@@ -493,7 +492,7 @@ def ensure_dataset_on_disk(self, trans, dataset):
                 "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:
+    def ensure_can_change_datatype(self, dataset: DatasetInstance, raiseException: bool = True) -> bool:
         if not dataset.datatype.is_datatype_change_allowed():
             if not raiseException:
                 return False
@@ -502,7 +501,7 @@ def ensure_can_change_datatype(self, dataset: model.DatasetInstance, raiseExcept
             )
         return True
 
-    def ensure_can_set_metadata(self, dataset: model.DatasetInstance, raiseException: bool = True) -> bool:
+    def ensure_can_set_metadata(self, dataset: DatasetInstance, raiseException: bool = True) -> bool:
         if not dataset.ok_to_edit_metadata():
             if not raiseException:
                 return False
@@ -511,29 +510,31 @@ def ensure_can_set_metadata(self, dataset: model.DatasetInstance, raiseException
             )
         return True
 
-    def detect_datatype(self, trans, dataset_assoc):
+    def detect_datatype(self, trans, dataset_assoc: DatasetInstance):
         """Sniff and assign the datatype to a given dataset association (ldda or hda)"""
-        data = trans.sa_session.get(self.model_class, dataset_assoc.id)
-        self.ensure_can_change_datatype(data)
-        self.ensure_can_set_metadata(data)
-        path = data.dataset.get_file_name()
-        datatype = sniff.guess_ext(path, trans.app.datatypes_registry.sniff_order)
-        trans.app.datatypes_registry.change_datatype(data, datatype)
-        with transaction(trans.sa_session):
-            trans.sa_session.commit()
+        session = self.session()
+        self.ensure_can_change_datatype(dataset_assoc)
+        self.ensure_can_set_metadata(dataset_assoc)
+        assert dataset_assoc.dataset
+        path = dataset_assoc.dataset.get_file_name()
+        datatype = sniff.guess_ext(path, self.app.datatypes_registry.sniff_order)
+        self.app.datatypes_registry.change_datatype(dataset_assoc, datatype)
+        with transaction(session):
+            session.commit()
         self.set_metadata(trans, dataset_assoc)
 
-    def set_metadata(self, trans, dataset_assoc, overwrite=False, validate=True):
+    def set_metadata(
+        self, trans, dataset_assoc: DatasetInstance, overwrite: bool = False, validate: bool = True
+    ) -> None:
         """Trigger a job that detects and sets metadata on a given dataset association (ldda or hda)"""
-        data = trans.sa_session.get(self.model_class, dataset_assoc.id)
-        self.ensure_can_set_metadata(data)
+        self.ensure_can_set_metadata(dataset_assoc)
         if overwrite:
-            self.overwrite_metadata(data)
+            self.overwrite_metadata(dataset_assoc)
 
         job, *_ = self.app.datatypes_registry.set_external_metadata_tool.tool_action.execute_via_trans(
             self.app.datatypes_registry.set_external_metadata_tool,
             trans,
-            incoming={"input1": data, "validate": validate},
+            incoming={"input1": dataset_assoc, "validate": validate},
             overwrite=overwrite,
         )
         self.app.job_manager.enqueue(job, tool=self.app.datatypes_registry.set_external_metadata_tool)
@@ -560,26 +561,26 @@ def update_permissions(self, trans, dataset_assoc, **kwd):
             dataset = dataset_assoc.dataset
 
         current_user_roles = trans.get_current_user_roles()
-        can_manage = trans.app.security_agent.can_manage_dataset(current_user_roles, dataset) or trans.user_is_admin
+        can_manage = self.app.security_agent.can_manage_dataset(current_user_roles, dataset) or trans.user_is_admin
         if not can_manage:
             raise exceptions.InsufficientPermissionsException(
                 "You do not have proper permissions to manage permissions on this dataset."
             )
 
         if action == "remove_restrictions":
-            trans.app.security_agent.make_dataset_public(dataset)
-            if not trans.app.security_agent.dataset_is_public(dataset):
+            self.app.security_agent.make_dataset_public(dataset)
+            if not self.app.security_agent.dataset_is_public(dataset):
                 raise exceptions.InternalServerError("An error occurred while making dataset public.")
         elif action == "make_private":
-            if not trans.app.security_agent.dataset_is_private_to_user(trans, dataset):
-                private_role = trans.app.security_agent.get_private_user_role(trans.user)
-                dp = trans.app.model.DatasetPermissions(
-                    trans.app.security_agent.permitted_actions.DATASET_ACCESS.action, dataset, private_role
+            if not self.app.security_agent.dataset_is_private_to_user(trans, dataset):
+                private_role = self.app.security_agent.get_private_user_role(trans.user)
+                dp = self.app.model.DatasetPermissions(
+                    self.app.security_agent.permitted_actions.DATASET_ACCESS.action, dataset, private_role
                 )
                 trans.sa_session.add(dp)
                 with transaction(trans.sa_session):
                     trans.sa_session.commit()
-            if not trans.app.security_agent.dataset_is_private_to_user(trans, dataset):
+            if not self.app.security_agent.dataset_is_private_to_user(trans, dataset):
                 # Check again and inform the user if dataset is not private.
                 raise exceptions.InternalServerError("An error occurred and the dataset is NOT private.")
         elif action == "set_permissions":
@@ -869,7 +870,7 @@ def deserialize_datatype(self, item, key, val, **context):
         job, *_ = self.app.datatypes_registry.set_external_metadata_tool.tool_action.execute_via_trans(
             self.app.datatypes_registry.set_external_metadata_tool, trans, incoming={"input1": item}, overwrite=False
         )  # overwrite is False as per existing behavior
-        trans.app.job_manager.enqueue(job, tool=trans.app.datatypes_registry.set_external_metadata_tool)
+        self.app.job_manager.enqueue(job, tool=self.app.datatypes_registry.set_external_metadata_tool)
         return item.datatype
 
 
diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py
index fcbbed0cb9c5..ff13b16083b1 100644
--- a/lib/galaxy/managers/folders.py
+++ b/lib/galaxy/managers/folders.py
@@ -505,7 +505,7 @@ def _get_contained_datasets_statement(
             stmt = stmt.where(
                 or_(
                     func.lower(ldda.name).contains(search_text, autoescape=True),
-                    func.lower(ldda.message).contains(search_text, autoescape=True),  # type:ignore[attr-defined]
+                    func.lower(ldda.message).contains(search_text, autoescape=True),
                 )
             )
         sort_column = LDDA_SORT_COLUMN_MAP[payload.order_by](ldda, associated_dataset)
diff --git a/lib/galaxy/managers/library_datasets.py b/lib/galaxy/managers/library_datasets.py
index 3666838b2bb0..e2bbfea35763 100644
--- a/lib/galaxy/managers/library_datasets.py
+++ b/lib/galaxy/managers/library_datasets.py
@@ -35,7 +35,7 @@ class LibraryDatasetsManager(datasets.DatasetAssociationManager):
     def __init__(self, app: MinimalManagerApp):
         self.app = app
 
-    def get(self, trans, decoded_library_dataset_id, check_accessible=True):
+    def get(self, trans, decoded_library_dataset_id, check_accessible=True) -> LibraryDataset:
         """
         Get the library dataset from the DB.
 
@@ -129,7 +129,7 @@ def _validate_and_parse_update_payload(self, payload):
         for key, val in payload.items():
             if val is None:
                 continue
-            if key in ("name"):
+            if key in ("name",):
                 if len(val) < MINIMUM_STRING_LENGTH:
                     raise RequestParameterInvalidException(
                         f"{key} must have at least length of {MINIMUM_STRING_LENGTH}"
@@ -139,19 +139,19 @@ def _validate_and_parse_update_payload(self, payload):
             if key in ("misc_info", "message"):
                 val = validation.validate_and_sanitize_basestring(key, val)
                 validated_payload[key] = val
-            if key in ("file_ext"):
+            if key in ("file_ext",):
                 datatype = self.app.datatypes_registry.get_datatype_by_extension(val)
                 if datatype is None and val not in ("auto",):
                     raise RequestParameterInvalidException(f"This Galaxy does not recognize the datatype of: {val}")
                 validated_payload[key] = val
-            if key in ("genome_build"):
+            if key in ("genome_build",):
                 if len(val) < MINIMUM_STRING_LENGTH:
                     raise RequestParameterInvalidException(
                         f"{key} must have at least length of {MINIMUM_STRING_LENGTH}"
                     )
                 val = validation.validate_and_sanitize_basestring(key, val)
                 validated_payload[key] = val
-            if key in ("tags"):
+            if key in ("tags",):
                 val = validation.validate_and_sanitize_basestring_list(key, util.listify(val))
                 validated_payload[key] = val
         return validated_payload
@@ -187,7 +187,7 @@ def check_accessible(self, trans, ld):
 
         :raises:    ObjectNotFound
         """
-        if not trans.app.security_agent.can_access_library_item(trans.get_current_user_roles(), ld, trans.user):
+        if not self.app.security_agent.can_access_library_item(trans.get_current_user_roles(), ld, trans.user):
             raise ObjectNotFound("Library dataset with the id provided was not found.")
         elif ld.deleted:
             raise ObjectNotFound("Library dataset with the id provided is deleted.")
@@ -210,27 +210,27 @@ def check_modifiable(self, trans, ld):
             raise ObjectNotFound("Library dataset with the id provided is deleted.")
         elif trans.user_is_admin:
             return ld
-        if not trans.app.security_agent.can_modify_library_item(trans.get_current_user_roles(), ld):
+        if not self.app.security_agent.can_modify_library_item(trans.get_current_user_roles(), ld):
             raise InsufficientPermissionsException("You do not have proper permission to modify this library dataset.")
         else:
             return ld
 
-    def serialize(self, trans, ld):
+    def serialize(self, trans, ld: LibraryDataset) -> Dict[str, Any]:
         """Serialize the library dataset into a dictionary."""
         current_user_roles = trans.get_current_user_roles()
 
         # Build the full path for breadcrumb purposes.
         full_path = self._build_path(trans, ld.folder)
-        dataset_item = (trans.security.encode_id(ld.id), ld.name)
+        dataset_item = (self.app.security.encode_id(ld.id), ld.name)
         full_path.insert(0, dataset_item)
         full_path = full_path[::-1]
 
         # Find expired versions of the library dataset
         expired_ldda_versions = []
         for expired_ldda in ld.expired_datasets:
-            expired_ldda_versions.append((trans.security.encode_id(expired_ldda.id), expired_ldda.name))
+            expired_ldda_versions.append((self.app.security.encode_id(expired_ldda.id), expired_ldda.name))
 
-        rval = trans.security.encode_all_ids(ld.to_dict())
+        rval = self.app.security.encode_all_ids(ld.to_dict())
         if len(expired_ldda_versions) > 0:
             rval["has_versions"] = True
             rval["expired_versions"] = expired_ldda_versions
@@ -249,14 +249,14 @@ def serialize(self, trans, ld):
         rval["file_size"] = util.nice_size(int(ldda.get_size(calculate_size=False)))
         rval["date_uploaded"] = ldda.create_time.isoformat()
         rval["update_time"] = ldda.update_time.isoformat()
-        rval["can_user_modify"] = trans.user_is_admin or trans.app.security_agent.can_modify_library_item(
+        rval["can_user_modify"] = trans.user_is_admin or self.app.security_agent.can_modify_library_item(
             current_user_roles, ld
         )
-        rval["is_unrestricted"] = trans.app.security_agent.dataset_is_public(ldda.dataset)
+        rval["is_unrestricted"] = self.app.security_agent.dataset_is_public(ldda.dataset)
         rval["tags"] = trans.tag_handler.get_tags_list(ldda.tags)
 
         #  Manage dataset permission is always attached to the dataset itself, not the ld or ldda to maintain consistency
-        rval["can_user_manage"] = trans.user_is_admin or trans.app.security_agent.can_manage_dataset(
+        rval["can_user_manage"] = trans.user_is_admin or self.app.security_agent.can_manage_dataset(
             current_user_roles, ldda.dataset
         )
         return rval
@@ -275,15 +275,15 @@ def _build_path(self, trans, folder):
         path_to_root = []
         if folder.parent_id is None:
             # We are almost in root
-            path_to_root.append((f"F{trans.security.encode_id(folder.id)}", folder.name))
+            path_to_root.append((f"F{self.app.security.encode_id(folder.id)}", folder.name))
         else:
             # We add the current folder and traverse up one folder.
-            path_to_root.append((f"F{trans.security.encode_id(folder.id)}", folder.name))
+            path_to_root.append((f"F{self.app.security.encode_id(folder.id)}", folder.name))
             upper_folder = trans.sa_session.get(LibraryFolder, folder.parent_id)
             path_to_root.extend(self._build_path(trans, upper_folder))
         return path_to_root
 
 
-def get_library_dataset(session, library_dataset_id):
+def get_library_dataset(session, library_dataset_id) -> LibraryDataset:
     stmt = select(LibraryDataset).where(LibraryDataset.id == library_dataset_id)
     return session.scalars(stmt).one()
diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index b01620b59c2e..75599f69eed9 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -5337,7 +5337,7 @@ def to_library_dataset_dataset_association(
         roles=None,
         ldda_message="",
         element_identifier=None,
-    ):
+    ) -> "LibraryDatasetDatasetAssociation":
         """
         Copy this HDA to a library optionally replacing an existing LDDA.
         """
@@ -5371,7 +5371,9 @@ def to_library_dataset_dataset_association(
             user=user,
         )
         library_dataset.library_dataset_dataset_association = ldda
-        object_session(self).add(library_dataset)
+        session = object_session(self)
+        assert session
+        session.add(library_dataset)
         # If roles were selected on the upload form, restrict access to the Dataset to those roles
         roles = roles or []
         for role in roles:
@@ -5381,7 +5383,6 @@ def to_library_dataset_dataset_association(
             trans.sa_session.add(dp)
         # Must set metadata after ldda flushed, as MetadataFiles require ldda.id
         if self.set_metadata_requires_flush:
-            session = object_session(self)
             with transaction(session):
                 session.commit()
         ldda.metadata = self.metadata
@@ -5390,10 +5391,9 @@ def to_library_dataset_dataset_association(
             ldda.message = ldda_message
         if not replace_dataset:
             target_folder.add_library_dataset(library_dataset, genome_build=ldda.dbkey)
-            object_session(self).add(target_folder)
-        object_session(self).add(library_dataset)
+            session.add(target_folder)
+        session.add(library_dataset)
 
-        session = object_session(self)
         with transaction(session):
             session.commit()
 
@@ -5980,6 +5980,11 @@ def to_dict(self, view="collection"):
 
 
 class LibraryDatasetDatasetAssociation(DatasetInstance, HasName, Serializable):
+    message: Mapped[Optional[str]] = mapped_column(TrimmedString(255))
+    tags: Mapped[List["LibraryDatasetDatasetAssociationTagAssociation"]] = relationship(
+        order_by=lambda: LibraryDatasetDatasetAssociationTagAssociation.id,
+        back_populates="library_dataset_dataset_association",
+    )
 
     def __init__(
         self,
diff --git a/lib/galaxy/webapps/galaxy/api/library_datasets.py b/lib/galaxy/webapps/galaxy/api/library_datasets.py
index 35cc5ae350f9..270fcec96b6f 100644
--- a/lib/galaxy/webapps/galaxy/api/library_datasets.py
+++ b/lib/galaxy/webapps/galaxy/api/library_datasets.py
@@ -171,7 +171,7 @@ def _get_current_roles(self, trans, library_dataset):
         :rtype:     dictionary
         :returns:   dict of current roles for all available permission types
         """
-        return self.ldda_manager.serialize_dataset_association_roles(trans, library_dataset)
+        return self.ldda_manager.serialize_dataset_association_roles(library_dataset)
 
     @expose_api
     def update(self, trans, encoded_dataset_id, payload=None, **kwd):
diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py
index 25dc26b5bb09..6d4598bf30d2 100644
--- a/lib/galaxy/webapps/galaxy/services/datasets.py
+++ b/lib/galaxy/webapps/galaxy/services/datasets.py
@@ -569,7 +569,7 @@ def update_permissions(
         dataset_manager = self.dataset_manager_by_type[hda_ldda]
         dataset = dataset_manager.get_accessible(dataset_id, trans.user)
         dataset_manager.update_permissions(trans, dataset, **payload_dict)
-        return dataset_manager.serialize_dataset_association_roles(trans, dataset)
+        return dataset_manager.serialize_dataset_association_roles(dataset)
 
     def extra_files(
         self,
diff --git a/lib/galaxy/webapps/galaxy/services/history_contents.py b/lib/galaxy/webapps/galaxy/services/history_contents.py
index 54ef988f990c..b1edebd3e897 100644
--- a/lib/galaxy/webapps/galaxy/services/history_contents.py
+++ b/lib/galaxy/webapps/galaxy/services/history_contents.py
@@ -604,7 +604,7 @@ def update_permissions(
         assert hda is not None
         self.history_manager.error_unless_mutable(hda.history)
         self.hda_manager.update_permissions(trans, hda, **payload_dict)
-        roles = self.hda_manager.serialize_dataset_association_roles(trans, hda)
+        roles = self.hda_manager.serialize_dataset_association_roles(hda)
         return DatasetAssociationRoles(**roles)
 
     def update(

From c065fa94e295b0c94653f4ec95a274fc14cbd965 Mon Sep 17 00:00:00 2001
From: Nicola Soranzo <nicola.soranzo@earlham.ac.uk>
Date: Tue, 3 Sep 2024 19:50:29 +0100
Subject: [PATCH 2/2] Change model_class of LibraryDatasetsManager to
 LibraryDataset

and make `update()` method compatible with supertype.
Fix the following mypy error:

```
lib/galaxy/managers/library_datasets.py:57: error: Signature of "update"
incompatible with supertype "ModelManager"  [override]
        def update(self, trans, ld, payload):
        ^
lib/galaxy/managers/library_datasets.py:57: note:      Superclass:
lib/galaxy/managers/library_datasets.py:57: note:          def update(self, item: Any, new_values: Any, flush: Any = ..., **kwargs: Any) -> DatasetInstance
lib/galaxy/managers/library_datasets.py:57: note:      Subclass:
lib/galaxy/managers/library_datasets.py:57: note:          def update(self, trans: Any, ld: Any, payload: Any) -> Any
```

Also:
- Move `check_modifiable()` check to API method.
- More type annotations.
---
 lib/galaxy/managers/library_datasets.py       | 65 +++++++++++--------
 .../webapps/galaxy/api/library_datasets.py    |  3 +-
 lib/galaxy_test/api/test_libraries.py         |  5 +-
 3 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/lib/galaxy/managers/library_datasets.py b/lib/galaxy/managers/library_datasets.py
index e2bbfea35763..f4f046551f84 100644
--- a/lib/galaxy/managers/library_datasets.py
+++ b/lib/galaxy/managers/library_datasets.py
@@ -1,23 +1,26 @@
 """Manager and Serializer for library datasets."""
 
 import logging
+from typing import (
+    Any,
+    Dict,
+)
 
 from sqlalchemy import select
 
-from galaxy import (
-    model,
-    util,
-)
+from galaxy import util
 from galaxy.exceptions import (
     InsufficientPermissionsException,
     InternalServerError,
     ObjectNotFound,
     RequestParameterInvalidException,
 )
-from galaxy.managers import datasets
+from galaxy.managers.base import ModelManager
 from galaxy.managers.context import ProvidesUserContext
+from galaxy.managers.datasets import DatasetAssociationManager
 from galaxy.model import (
     LibraryDataset,
+    LibraryDatasetDatasetAssociation,
     LibraryFolder,
 )
 from galaxy.model.base import transaction
@@ -27,13 +30,14 @@
 log = logging.getLogger(__name__)
 
 
-class LibraryDatasetsManager(datasets.DatasetAssociationManager):
+class LibraryDatasetsManager(ModelManager[LibraryDataset]):
     """Interface/service object for interacting with library datasets."""
 
-    model_class = model.LibraryDatasetDatasetAssociation
+    model_class = LibraryDataset
 
     def __init__(self, app: MinimalManagerApp):
-        self.app = app
+        super().__init__(app)
+        self.dataset_assoc_manager = DatasetAssociationManager(app)
 
     def get(self, trans, decoded_library_dataset_id, check_accessible=True) -> LibraryDataset:
         """
@@ -54,14 +58,14 @@ def get(self, trans, decoded_library_dataset_id, check_accessible=True) -> Libra
         ld = self.secure(trans, ld, check_accessible)
         return ld
 
-    def update(self, trans, ld, payload):
+    def update(self, item: LibraryDataset, new_values: Dict[str, Any], flush: bool = True, **kwargs) -> LibraryDataset:
         """
         Update the given library dataset - the latest linked ldda.
         Updating older lddas (versions) is not allowed.
 
-        :param  ld:                 library dataset to change
-        :type   ld:                 LibraryDataset
-        :param  payload:            dictionary structure containing::
+        :param  item:               library dataset to change
+        :type   item:               LibraryDataset
+        :param  new_values:         dictionary structure containing::
             :param name:            new ld's name, must be longer than 0
             :type  name:            str
             :param misc_info:       new ld's misc info
@@ -72,19 +76,27 @@ def update(self, trans, ld, payload):
             :type  genome_build:    str
             :param tags:            list of dataset tags
             :type  tags:            list
-        :type   payload: dict
+        :type   new_values: dict
 
         :returns:   the changed library dataset
         :rtype:     galaxy.model.LibraryDataset
         """
-        self.check_modifiable(trans, ld)
+        trans = kwargs.get("trans")
+        if not trans:
+            raise ValueError("Missing trans parameter")
         # we are going to operate on the actual latest ldda
-        ldda = ld.library_dataset_dataset_association
-        payload = self._validate_and_parse_update_payload(payload)
-        self._set_from_dict(trans, ldda, payload)
-        return ld
-
-    def _set_from_dict(self, trans: ProvidesUserContext, ldda, new_data):
+        ldda = item.library_dataset_dataset_association
+        new_values = self._validate_and_parse_update_payload(new_values)
+        self._set_from_dict(trans, ldda, new_values, flush=flush)
+        return item
+
+    def _set_from_dict(
+        self,
+        trans: ProvidesUserContext,
+        ldda: LibraryDatasetDatasetAssociation,
+        new_data: Dict[str, Any],
+        flush: bool = True,
+    ) -> None:
         changed = False
         new_name = new_data.get("name", None)
         if new_name is not None and new_name != ldda.name:
@@ -100,10 +112,10 @@ def _set_from_dict(self, trans: ProvidesUserContext, ldda, new_data):
             changed = True
         new_file_ext = new_data.get("file_ext", None)
         if new_file_ext == "auto":
-            self.detect_datatype(trans, ldda)
+            self.dataset_assoc_manager.detect_datatype(trans, ldda)
         elif new_file_ext is not None and new_file_ext != ldda.extension:
             ldda.extension = new_file_ext
-            self.set_metadata(trans, ldda)
+            self.dataset_assoc_manager.set_metadata(trans, ldda)
             changed = True
         new_genome_build = new_data.get("genome_build", None)
         if new_genome_build is not None and new_genome_build != ldda.dbkey:
@@ -118,10 +130,11 @@ def _set_from_dict(self, trans: ProvidesUserContext, ldda, new_data):
             changed = True
         if changed:
             ldda.update_parent_folder_update_times()
-            trans.sa_session.add(ldda)
-            with transaction(trans.sa_session):
-                trans.sa_session.commit()
-        return changed
+            session = self.session()
+            session.add(ldda)
+            if flush:
+                with transaction(session):
+                    session.commit()
 
     def _validate_and_parse_update_payload(self, payload):
         MINIMUM_STRING_LENGTH = 1
diff --git a/lib/galaxy/webapps/galaxy/api/library_datasets.py b/lib/galaxy/webapps/galaxy/api/library_datasets.py
index 270fcec96b6f..e8f3c64957a5 100644
--- a/lib/galaxy/webapps/galaxy/api/library_datasets.py
+++ b/lib/galaxy/webapps/galaxy/api/library_datasets.py
@@ -199,7 +199,8 @@ def update(self, trans, encoded_dataset_id, payload=None, **kwd):
         :rtype:     dictionary
         """
         library_dataset = self.ld_manager.get(trans, managers_base.decode_id(self.app, encoded_dataset_id))
-        updated = self.ld_manager.update(trans, library_dataset, payload)
+        self.ld_manager.check_modifiable(trans, library_dataset)
+        updated = self.ld_manager.update(library_dataset, payload, trans=trans)
         serialized = self.ld_manager.serialize(trans, updated)
         return serialized
 
diff --git a/lib/galaxy_test/api/test_libraries.py b/lib/galaxy_test/api/test_libraries.py
index 440d004db146..6cbfaa21e0fe 100644
--- a/lib/galaxy_test/api/test_libraries.py
+++ b/lib/galaxy_test/api/test_libraries.py
@@ -447,10 +447,11 @@ def test_update_dataset_in_folder(self):
             "name": "updated_name",
             "file_ext": "fasta",
             "misc_info": "updated_info",
-            "genome_build": "updated_genome_build",
+            "message": "update message",
         }
         ld_updated = self._patch_library_dataset(ld["id"], data)
-        self._assert_has_keys(ld_updated, "name", "file_ext", "misc_info", "genome_build")
+        for key, value in data.items():
+            assert ld_updated[key] == value
 
     @requires_new_library
     def test_update_dataset_tags(self):