Step 1 in separating update time from update ACLs

lib/SampleService/core/samples.py

@@ -278,7 +278,7 @@ def update_sample_acls(
             update.remove,
             update.public_read
         )
-        self._storage.update_sample_acls(id_, update)
+        self._storage.update_sample_acls(id_, update, self._now())
         if self._kafka:
             self._kafka.notify_sample_acl_change(id_)
 

lib/SampleService/core/storage/arango_sample_storage.py

@@ -84,10 +84,12 @@
 from SampleService.core.sample import SampleNode as _SampleNode, SubSampleType as _SubSampleType
 from SampleService.core.sample import SampleNodeAddress as _SampleNodeAddress
 from SampleService.core.sample import SampleAddress
-from SampleService.core.arg_checkers import not_falsy as _not_falsy
-from SampleService.core.arg_checkers import not_falsy_in_iterable as _not_falsy_in_iterable
-from SampleService.core.arg_checkers import check_string as _check_string
-from SampleService.core.arg_checkers import check_timestamp as _check_timestamp
+from SampleService.core.arg_checkers import (
+    not_falsy as _not_falsy,
+    not_falsy_in_iterable as _not_falsy_in_iterable,
+    check_string as _check_string,
+    check_timestamp as _check_timestamp,
+)
 from SampleService.core.errors import (
     ConcurrencyError as _ConcurrencyError,
     DataLinkExistsError as _DataLinkExistsError,
@@ -796,28 +798,31 @@ def replace_sample_acls(self, id_: UUID, acls: SampleACL):
         except _arango.exceptions.AQLQueryExecuteError as e:  # this is a real pain to test
             raise _SampleStorageError('Connection to database failed: ' + str(e)) from e
 
-    def update_sample_acls(self, id_: UUID, update: SampleACLDelta) -> None:
+    def update_sample_acls(
+            self, id_: UUID, update: SampleACLDelta, update_time: datetime.datetime) -> None:
         '''
         Update a sample's ACLs via a delta specification.
 
         :param id_: the sample ID.
         :param update: the update to apply to the ACLs.
+        :param update_time: the update time to save in the database.
         :raises NoSuchSampleError: if the sample does not exist.
         :raises SampleStorageError: if the sample could not be retrieved.
         :raises UnauthorizedError: if the update attempts to alter the sample owner.
         '''
         # Needs to ensure owner is not added to another ACL
         # could make an option to just ignore the update to the owner? YAGNI for now.
         _not_falsy(update, 'update')
+        _check_timestamp(update_time, 'update_time')
         s = self.get_sample_acls(id_)
         if not s.is_update(update):
             # noop. Theoretically the values in the DB may have changed since we pulled the ACLs,
             # but now we're talking about millisecond ordering differences, so don't worry
             # about it.
             return
-        self._update_sample_acls_pt2(id_, update, s.owner)
+        self._update_sample_acls_pt2(id_, update, s.owner, update_time)
 
-    def _update_sample_acls_pt2(self, id_, update, owner):
+    def _update_sample_acls_pt2(self, id_, update, owner, update_time):
         # this method is split solely to allow testing the owner change case.
 
         # At this point we're committed to a DB update and therefore an ACL update time bump
@@ -831,7 +836,7 @@ def _update_sample_acls_pt2(self, id_, update, owner):
         bind_vars = {'@col': self._col_sample.name,
                      'id': str(id_),
                      'owner': owner.id,
-                     'ts': update.lastupdate.timestamp(),
+                     'ts': update_time.timestamp(),
                      'admin': [u.id for u in update.admin],
                      'write': [u.id for u in update.write],
                      'read': [u.id for u in update.read],

test/core/samples_test.py

@@ -884,7 +884,8 @@ def _update_sample_acls(user: UserID, public_read):
             [u('z'), u('a')],
             [u('b'), u('c')],
             [u('r'), u('q')],
-            public_read))
+            public_read),
+        dt(6))
 
     kafka.notify_sample_acl_change.assert_called_once_with(
         UUID('1234567890abcdef1234567890abcde0'))
@@ -927,7 +928,8 @@ def test_update_sample_acls_as_admin_without_notifier():
             [u('z'), u('a')],
             [u('b'), u('c')],
             [u('r'), u('q')],
-            None))
+            None),
+        dt(6))
 
 
 def test_update_sample_acls_fail_bad_input():

test/core/storage/arango_sample_storage_test.py

@@ -1149,12 +1149,13 @@ def test_update_sample_acls(samplestorage):
         [UserID('foo'), UserID('bar1')],
         [UserID('baz1'), UserID('bat')],
         [UserID('whoo1')],
-        public_read=True))
+        public_read=True),
+        dt(101))
 
     res = samplestorage.get_sample_acls(id_)
     assert res == SampleACL(
         UserID('user'),
-        dt(58),
+        dt(101),
         [UserID('foo'), UserID('bar1')],
         [UserID('baz1'), UserID('bat')],
         [UserID('whoo1')],
@@ -1180,7 +1181,8 @@ def test_update_sample_acls_noop(samplestorage):
         [UserID('bat')],
         [UserID('whoo')],
         [UserID('nouser'), UserID('nouser2')],
-        public_read=True))
+        public_read=True),
+        dt(103))
 
     res = samplestorage.get_sample_acls(id_)
     print(res)
@@ -1211,13 +1213,13 @@ def test_update_sample_acls_with_remove_and_null_public(samplestorage):
         [UserID('admin')],
         [UserID('write'), UserID('write2')],
         [UserID('read')],
-        [UserID('foo'), UserID('bat'), UserID('whoo'), UserID('notauser')]
-    ))
+        [UserID('foo'), UserID('bat'), UserID('whoo'), UserID('notauser')]),
+        dt(102))
 
     res = samplestorage.get_sample_acls(id_)
     assert res == SampleACL(
         UserID('user'),
-        dt(98),
+        dt(102),
         [UserID('bar'), UserID('admin')],
         [UserID('baz'), UserID('write'), UserID('write2')],
         [UserID('read')],
@@ -1237,12 +1239,12 @@ def test_update_sample_acls_with_false_public(samplestorage):
         [UserID('whoo')],
         True))
 
-    samplestorage.update_sample_acls(id_, SampleACLDelta(dt(98), public_read=False))
+    samplestorage.update_sample_acls(id_, SampleACLDelta(dt(98), public_read=False), dt(89))
 
     res = samplestorage.get_sample_acls(id_)
     assert res == SampleACL(
         UserID('user'),
-        dt(98),
+        dt(89),
         [UserID('bar'), UserID('foo')],
         [UserID('baz'), UserID('bat')],
         [UserID('whoo')],
@@ -1252,11 +1254,16 @@ def test_update_sample_acls_with_false_public(samplestorage):
 def test_update_sample_acls_fail_bad_args(samplestorage):
     id_ = uuid.uuid4()
     s = SampleACLDelta(dt(2))
+    t = dt(1)
 
     _update_sample_acls_fail(
-        samplestorage, None, s, ValueError('id_ cannot be a value that evaluates to false'))
+        samplestorage, None, s, t, ValueError('id_ cannot be a value that evaluates to false'))
     _update_sample_acls_fail(
-        samplestorage, id_, None, ValueError('update cannot be a value that evaluates to false'))
+        samplestorage, id_, None, t, ValueError('update cannot be a value that evaluates to false'))
+    _update_sample_acls_fail(samplestorage, id_, s, None, ValueError(
+        'update_time cannot be a value that evaluates to false'))
+    _update_sample_acls_fail(samplestorage, id_, s, datetime.datetime.fromtimestamp(1), ValueError(
+        'update_time cannot be a naive datetime'))
 
 
 def test_update_sample_acls_fail_no_sample(samplestorage):
@@ -1265,7 +1272,7 @@ def test_update_sample_acls_fail_no_sample(samplestorage):
         SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True
 
     _update_sample_acls_fail(
-        samplestorage, uuid.UUID('1234567890abcdef1234567890abcde1'), SampleACLDelta(dt(1)),
+        samplestorage, uuid.UUID('1234567890abcdef1234567890abcde1'), SampleACLDelta(dt(1)), dt(1),
         NoSuchSampleError('12345678-90ab-cdef-1234-567890abcde1'))
 
 
@@ -1275,11 +1282,15 @@ def test_update_sample_acls_fail_alters_owner(samplestorage):
         SavedSample(id_, UserID('us'), [TEST_NODE], dt(1), 'foo')) is True
 
     err = UnauthorizedError('ACLs for the sample owner us may not be modified by a delta update.')
+    t = dt(1)
 
-    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), [UserID('us')]), err)
-    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), write=[UserID('us')]), err)
-    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), read=[UserID('us')]), err)
-    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), remove=[UserID('us')]), err)
+    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), [UserID('us')]), t, err)
+    _update_sample_acls_fail(
+        samplestorage, id_, SampleACLDelta(dt(1), write=[UserID('us')]), t, err)
+    _update_sample_acls_fail(
+        samplestorage, id_, SampleACLDelta(dt(1), read=[UserID('us')]), t, err)
+    _update_sample_acls_fail(
+        samplestorage, id_, SampleACLDelta(dt(1), remove=[UserID('us')]), t, err)
 
 
 def test_update_sample_acls_fail_owner_changed(samplestorage):
@@ -1294,16 +1305,16 @@ def test_update_sample_acls_fail_owner_changed(samplestorage):
 
     with raises(Exception) as got:
         samplestorage._update_sample_acls_pt2(
-            id_, SampleACLDelta(dt(2), [UserID('a')]), UserID('user2'))
+            id_, SampleACLDelta(dt(2), [UserID('a')]), UserID('user2'), dt(1))
     assert_exception_correct(got.value, OwnerChangedError(
         # we don't really ever expect this to happen, but just in case...
         'The sample owner unexpectedly changed during the operation. Please retry. ' +
         'If this error occurs frequently, code changes may be necessary.'))
 
 
-def _update_sample_acls_fail(samplestorage, id_, update, expected):
+def _update_sample_acls_fail(samplestorage, id_, update, update_time, expected):
     with raises(Exception) as got:
-        samplestorage.update_sample_acls(id_, update)
+        samplestorage.update_sample_acls(id_, update, update_time)
     assert_exception_correct(got.value, expected)