Skip to content

Return expired link ID from create link #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 4, 2020
Merged

Return expired link ID from create link #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/SampleService/core/notification.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
17 changes: 10 additions & 7 deletions lib/SampleService/core/storage/arango_sample_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 +
Expand Down Expand Up @@ -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
Expand Down
62 changes: 31 additions & 31 deletions test/core/storage/arango_sample_storage_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down