From 86d02818d3b5cf9be925170c6c777ef21942d164 Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 4 Jun 2020 13:29:59 -0700 Subject: [PATCH] Return expired link ID from create link Needed for notifications --- lib/SampleService/core/notification.py | 1 - .../core/storage/arango_sample_storage.py | 17 ++--- .../storage/arango_sample_storage_test.py | 62 +++++++++---------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/lib/SampleService/core/notification.py b/lib/SampleService/core/notification.py index 6dbeab80..4911c24a 100644 --- a/lib/SampleService/core/notification.py +++ b/lib/SampleService/core/notification.py @@ -136,7 +136,6 @@ def close(self): self._prod.close() self._closed = True - # TODO KAFKA notify on ACL change # TODO KAFKA notify on new link # TODO KAFKA notify on expired link via update # TODO KAFKA notify on expired link diff --git a/lib/SampleService/core/storage/arango_sample_storage.py b/lib/SampleService/core/storage/arango_sample_storage.py index cc68007b..d372a8db 100644 --- a/lib/SampleService/core/storage/arango_sample_storage.py +++ b/lib/SampleService/core/storage/arango_sample_storage.py @@ -505,9 +505,10 @@ def _meta_to_list(self, m: _Dict[str, _Dict[str, _PrimitiveType]]) -> List[_Dict ) return ret - def _list_to_meta(self, l: List[_Dict[str, _Any]]) -> _Dict[str, _Dict[str, _PrimitiveType]]: + def _list_to_meta( + self, list: List[_Dict[str, _Any]]) -> _Dict[str, _Dict[str, _PrimitiveType]]: ret: _Dict[str, _Dict[str, _PrimitiveType]] = defaultdict(dict) - for m in l: + for m in list: ret[m[_FLD_NODE_META_OUTER_KEY]][m[_FLD_NODE_META_KEY]] = m[_FLD_NODE_META_VALUE] return dict(ret) # some libs don't play nice with default dict, in particular maps @@ -782,7 +783,7 @@ def replace_sample_acls(self, id_: UUID, acls: SampleACL): # TODO change acls with more granularity - def create_data_link(self, link: DataLink, update: bool = False): + def create_data_link(self, link: DataLink, update: bool = False) -> Optional[UUID]: ''' Link data in the workspace to a sample. Each data unit can be linked to only one sample at a time. Expired links may exist to @@ -802,6 +803,7 @@ def create_data_link(self, link: DataLink, update: bool = False): :param link: the link to save, which cannot be expired. :param update: if the link from the object already exists and is linked to a different sample, update the link. If it is linked to the same sample take no action. + :returns: The ID of the link that is expired as part of the update process, if any. :raises NoSuchSampleError: if the sample does not exist. :raises NoSuchSampleVersionError: if the sample version does not exist. @@ -857,7 +859,7 @@ def create_data_link(self, link: DataLink, update: bool = False): oldlink = self._doc_to_link(oldlinkdoc) if link.is_equivalent(oldlink): self._abort_transaction(tdb) - return # I don't like having a return in the middle of the method, but + return None # I don't like having a return in the middle of the method, but # the alternative seems to be worse # See the notes in the expire method, many are relevant here. @@ -896,6 +898,7 @@ def create_data_link(self, link: DataLink, update: bool = False): self._commit_transaction(tdb) finally: self._abort_transaction(tdb) + return UUID(oldlinkdoc[_FLD_LINK_ID]) if oldlinkdoc else None def _commit_transaction(self, transaction_db): try: @@ -967,7 +970,7 @@ def _count_links(self, db, filters: str, bind_vars, created, expired): # links. Might not work in a NOT though. Alternate formulation is # (d.creatd >= @created AND d.created <= @expired) OR # (d.expired >= @created AND d.expired <= @expired) - q = (f''' + q = (''' FOR d in @@col ''' + filters + @@ -1236,8 +1239,8 @@ def get_links_from_sample( def _find_links_via_aql(self, query, bind_vars): duids = [] try: - for l in self._db.aql.execute(query, bind_vars=bind_vars): - duids.append(self._doc_to_link(l)) + for link in self._db.aql.execute(query, bind_vars=bind_vars): + duids.append(self._doc_to_link(link)) except _arango.exceptions.AQLQueryExecuteError as e: # this is a pain to test raise _SampleStorageError('Connection to database failed: ' + str(e)) from e return duids # a maxium of 10k can be returned based on the link creation function diff --git a/test/core/storage/arango_sample_storage_test.py b/test/core/storage/arango_sample_storage_test.py index 68ebe3c6..c30fa7d4 100644 --- a/test/core/storage/arango_sample_storage_test.py +++ b/test/core/storage/arango_sample_storage_test.py @@ -1100,40 +1100,40 @@ def test_create_and_get_data_link(samplestorage): assert samplestorage.save_sample( SavedSample(id2, UserID('user'), [SampleNode('mynode2')], dt(3), 'foo')) is True - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde1'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 2), 'mynode1'), dt(500), UserID('usera')) - ) + ) is None # test different workspace object and different sample version - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde2'), DataUnitID(UPA('42/42/42'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(600), UserID('userb')) - ) + ) is None # test data unit vs just UPA, different sample, and expiration date - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde3'), DataUnitID(UPA('5/89/32'), 'dataunit2'), SampleNodeAddress(SampleAddress(id2, 1), 'mynode2'), dt(700), UserID('u')) - ) + ) is None # test data units don't collide if they have different names - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde4'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(800), UserID('userd')) - ) + ) is None # this is naughty verdoc1 = samplestorage._col_version.find({'id': str(id1), 'ver': 1}).next() @@ -1288,23 +1288,23 @@ def test_creaate_data_link_with_update_no_extant_link(samplestorage): assert samplestorage.save_sample(SavedSample( id1, UserID('user'), [SampleNode('mynode'), SampleNode('mynode1')], dt(1), 'foo')) is True - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde1'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(500), UserID('usera')), update=True - ) + ) is None - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde2'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode1'), dt(550), UserID('user')), update=True - ) + ) is None # this is naughty verdoc1 = samplestorage._col_version.find({'id': str(id1), 'ver': 1}).next() @@ -1390,41 +1390,41 @@ def test_create_data_link_with_update_noop(samplestorage): assert samplestorage.save_sample(SavedSample( id1, UserID('user'), [SampleNode('mynode'), SampleNode('mynode1')], dt(1), 'foo')) is True - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde1'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(500), UserID('usera')) - ) + ) is None - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde2'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode1'), dt(550), UserID('user')) - ) + ) is None # expect noop - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde3'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(600), UserID('userb')), update=True - ) + ) is None # expect noop - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde4'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode1'), dt(700), UserID('userc')), update=True - ) + ) is None # this is naughty verdoc1 = samplestorage._col_version.find({'id': str(id1), 'ver': 1}).next() @@ -1508,39 +1508,39 @@ def test_create_data_link_with_update(samplestorage): id1, UserID('user'), [SampleNode('mynode'), SampleNode('mynode1'), SampleNode('mynode2')], dt(1), 'foo')) is True - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde1'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 1), 'mynode'), dt(500), UserID('usera')) - ) + ) is None - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde2'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode1'), dt(550), UserID('user')) - ) + ) is None - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde3'), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id1, 1), 'mynode1'), # update the node dt(600), UserID('userb')), update=True - ) + ) == uuid.UUID('1234567890abcdef1234567890abcde1') - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.UUID('1234567890abcdef1234567890abcde4'), DataUnitID(UPA('5/89/32'), 'dataunit1'), SampleNodeAddress(SampleAddress(id1, 1), 'mynode2'), # update the node dt(700), UserID('userc')), update=True - ) + ) == uuid.UUID('1234567890abcdef1234567890abcde2') # this is naughty verdoc1 = samplestorage._col_version.find({'id': str(id1), 'ver': 1}).next() @@ -1708,13 +1708,13 @@ def test_create_data_link_correct_missing_versions(samplestorage): samplestorage._col_version.update_match({}, {'ver': -1}) samplestorage._col_nodes.update_match({'name': 'kid2'}, {'ver': -1}) - samplestorage.create_data_link(DataLink( + assert samplestorage.create_data_link(DataLink( uuid.uuid4(), DataUnitID(UPA('5/89/32')), SampleNodeAddress(SampleAddress(id_, 1), 'kid1'), dt(500), UserID('user')) - ) + ) is None assert samplestorage._col_version.count() == 1 assert samplestorage._col_ver_edge.count() == 1 @@ -2630,7 +2630,7 @@ def test_expire_data_link_fail_race_condition(samplestorage): # ok, we have the link doc from the db. This is what part 1 of the code does, and then # passes to part 2. - linkdoc = samplestorage._col_data_link.get(f'1_1_1') + linkdoc = samplestorage._col_data_link.get('1_1_1') # Now we simulate a race condition by expiring that link and calling part 2 of the expire # method. Part 2 should fail without modifying the links collection.