From 6e2aad1416eedaf08c30bfb071ab3ec9839ddf4d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 29 Oct 2020 09:16:36 +0100 Subject: [PATCH] Resolve LDDA <-> Library Dataset circular dependency This eliminates a lot of excessive flushes that were needed to persist new `LibraryDatasetDatasetAssociation` (LDDA) and `LibraryDataset` (LD) objects. These intermittent flushes would make LD objects visible before they are associated with a LDDA, causing the Exception in https://github.com/galaxyproject/galaxy/pull/10556#issuecomment-717175641 ``` galaxy.web.framework.decorators ERROR 2020-10-27 17:26:25,817 Uncaught exception in exposed API method: Traceback (most recent call last): File "/Users/mvandenb/src/galaxy/lib/galaxy/web/framework/decorators.py", line 294, in decorator rval = func(self, trans, *args, **kwargs) File "/Users/mvandenb/src/galaxy/lib/galaxy/webapps/galaxy/api/folder_contents.py", line 111, in index dataset = content_item.library_dataset_dataset_association.dataset AttributeError: 'NoneType' object has no attribute 'dataset' ``` The trick here is to use `post_update=True`, so that ldda and ld rows are INSERTED, followed by an UPDATE to create the link. This is explained at https://docs.sqlalchemy.org/en/13/orm/relationship_persistence.html. This is also the groundwork for speeding up library additions by batching flushes. --- lib/galaxy/model/__init__.py | 28 +++++++++--------------- lib/galaxy/model/mapping.py | 6 +++--- test/unit/data/test_galaxy_mapping.py | 31 +++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index da2db4d8717..af55c39e1aa 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -3193,7 +3193,7 @@ class HistoryDatasetAssociation(DatasetInstance, HasTags, Dictifiable, UsesAnnot new_dataset.hid = self.hid def to_library_dataset_dataset_association(self, trans, target_folder, replace_dataset=None, - parent_id=None, user=None, roles=None, ldda_message='', element_identifier=None): + parent_id=None, roles=None, ldda_message='', element_identifier=None): """ Copy this HDA to a library optionally replacing an existing LDDA. """ @@ -3206,11 +3206,7 @@ class HistoryDatasetAssociation(DatasetInstance, HasTags, Dictifiable, UsesAnnot # applied to the new LibraryDataset, and the current user's DefaultUserPermissions will be applied # to the associated Dataset. library_dataset = LibraryDataset(folder=target_folder, name=self.name, info=self.info) - object_session(self).add(library_dataset) - object_session(self).flush() - if not user: - # This should never happen since users must be authenticated to upload to a data library - user = self.history.user + user = trans.user or self.history.user ldda = LibraryDatasetDatasetAssociation(name=element_identifier or self.name, info=self.info, blurb=self.blurb, @@ -3225,16 +3221,19 @@ class HistoryDatasetAssociation(DatasetInstance, HasTags, Dictifiable, UsesAnnot parent_id=parent_id, copied_from_history_dataset_association=self, user=user) - object_session(self).add(ldda) - object_session(self).flush() + library_dataset.library_dataset_dataset_association = ldda + object_session(self).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: dp = trans.model.DatasetPermissions(trans.app.security_agent.permitted_actions.DATASET_ACCESS.action, ldda.dataset, role) trans.sa_session.add(dp) - trans.sa_session.flush() # Must set metadata after ldda flushed, as MetadataFiles require ldda.id + flushed = False + if self.set_metadata_requires_flush: + flushed = True + object_session(self).flush() ldda.metadata = self.metadata # TODO: copy #tags from history if ldda_message: @@ -3242,12 +3241,11 @@ class HistoryDatasetAssociation(DatasetInstance, HasTags, Dictifiable, UsesAnnot if not replace_dataset: target_folder.add_library_dataset(library_dataset, genome_build=ldda.dbkey) object_session(self).add(target_folder) - object_session(self).flush() - library_dataset.library_dataset_dataset_association_id = ldda.id object_session(self).add(library_dataset) - object_session(self).flush() if not self.datatype.copy_safe_peek: # In some instances peek relies on dataset_id, i.e. gmaj.zip for viewing MAFs + if not flushed: + object_session(self).flush() ldda.set_peek() object_session(self).flush() return ldda @@ -3597,12 +3595,6 @@ class LibraryDataset(RepresentById): self.info = info self.library_dataset_dataset_association = library_dataset_dataset_association - def set_library_dataset_dataset_association(self, ldda): - self.library_dataset_dataset_association = ldda - ldda.library_dataset = self - object_session(self).add_all((ldda, self)) - object_session(self).flush() - def get_info(self): if self.library_dataset_dataset_association: return self.library_dataset_dataset_association.info diff --git a/lib/galaxy/model/mapping.py b/lib/galaxy/model/mapping.py index 7d68dd3a8d4..38622dc1f3b 100644 --- a/lib/galaxy/model/mapping.py +++ b/lib/galaxy/model/mapping.py @@ -2088,8 +2088,8 @@ mapper(model.LibraryFolderInfoAssociation, model.LibraryFolderInfoAssociation.ta mapper(model.LibraryDataset, model.LibraryDataset.table, properties=dict( folder=relation(model.LibraryFolder), library_dataset_dataset_association=relation(model.LibraryDatasetDatasetAssociation, - primaryjoin=(model.LibraryDataset.table.c.library_dataset_dataset_association_id == - model.LibraryDatasetDatasetAssociation.table.c.id)), + foreign_keys=model.LibraryDataset.table.c.library_dataset_dataset_association_id, + post_update=True), expired_datasets=relation(model.LibraryDatasetDatasetAssociation, foreign_keys=[model.LibraryDataset.table.c.id, model.LibraryDataset.table.c.library_dataset_dataset_association_id], primaryjoin=( @@ -2104,7 +2104,7 @@ mapper(model.LibraryDataset, model.LibraryDataset.table, properties=dict( mapper(model.LibraryDatasetDatasetAssociation, model.LibraryDatasetDatasetAssociation.table, properties=dict( dataset=relation(model.Dataset), library_dataset=relation(model.LibraryDataset, - primaryjoin=(model.LibraryDatasetDatasetAssociation.table.c.library_dataset_id == model.LibraryDataset.table.c.id)), + foreign_keys=model.LibraryDatasetDatasetAssociation.table.c.library_dataset_id), # user=relation( model.User.mapper ), user=relation(model.User), copied_from_library_dataset_dataset_association=relation(model.LibraryDatasetDatasetAssociation, diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index caba5ffbf8c..b30e7298463 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -1,4 +1,5 @@ # -*- coding: utf-8 -*- +import collections import unittest import uuid @@ -154,6 +155,36 @@ class MappingTests(unittest.TestCase): assert isinstance(history.get_display_name(), text_type) assert history.get_display_name() == u'Hello₩◎ґʟⅾ' + def test_hda_to_library_dataset_dataset_association(self): + u = self.model.User(email="mary@example.com", password="password") + hda = self.model.HistoryDatasetAssociation(name='hda_name') + self.persist(hda) + trans = collections.namedtuple('trans', 'user') + target_folder = self.model.LibraryFolder(name='library_folder') + ldda = hda.to_library_dataset_dataset_association( + trans=trans(user=u), + target_folder=target_folder, + ) + assert target_folder.item_count == 1 + assert ldda.id + assert ldda.library_dataset.id + assert ldda.library_dataset_id + assert ldda.library_dataset.library_dataset_dataset_association + assert ldda.library_dataset.library_dataset_dataset_association_id + library_dataset_id = ldda.library_dataset_id + replace_dataset = ldda.library_dataset + new_ldda = hda.to_library_dataset_dataset_association( + trans=trans(user=u), + target_folder=target_folder, + replace_dataset=replace_dataset + ) + assert new_ldda.id != ldda.id + assert new_ldda.library_dataset_id == library_dataset_id + assert new_ldda.library_dataset.library_dataset_dataset_association_id == new_ldda.id + assert len(new_ldda.library_dataset.expired_datasets) == 1 + assert new_ldda.library_dataset.expired_datasets[0] == ldda + assert target_folder.item_count == 1 + def test_tags(self): model = self.model