Merge pull request #454 from kbase/develop

Merge Develop into Master (pre 0.2.4)

.github/workflows/non_sdk_test.yml

@@ -4,7 +4,7 @@ on:
   push:
     branches: [ master ]
   pull_request:
-    branches: [ master ]
+    branches: [ master, develop ]
   workflow_dispatch:
 
 jobs:

RELEASE_NOTES.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+## 0.2.4
+* Changes github actions: creates images from releases off master, adds test running on develop branch
+* Bugfix for write-write error
+* Bugfix for incorrectly thrown `owner unexpectedly changed` errors
+* Improved error handling/messages for get_samples method
 
 ## 0.2.1
 

lib/SampleService/SampleServiceImpl.py

@@ -404,8 +404,18 @@ def get_samples(self, ctx, params):
         # ctx is the context object
         # return variables are: samples
         #BEGIN get_samples
-        if not params.get('samples'):
-          raise ValueError(f"")
+        # if not params.get('samples'):
+        #   raise ValueError(f"")
+        if type(params.get('samples')) is not list:
+            raise ValueError(
+                'Missing or incorrect "samples" field - ' +
+                'must provide a list of samples to retrieve.'
+            )
+        if len(params.get('samples')) == 0:
+            raise ValueError(
+                'Cannot provide empty list of samples - ' +
+                'must provide at least one sample to retrieve.'
+            )
         ids_ = []
         for samp_obj in params['samples']:
           id_, ver = _get_sample_address_from_object(samp_obj)

lib/SampleService/core/storage/arango_sample_storage.py

@@ -250,7 +250,8 @@ def __init__(
             db, schema_collection, 'schema collection', 'schema_collection')
         self._ensure_indexes()
         self._check_schema()
-        self._deletion_delay = datetime.timedelta(hours=1)  # make configurable?
+        self._reaper_deletion_delay = datetime.timedelta(hours=1)  # make configurable?
+        self._reaper_update_delay = datetime.timedelta(minutes=5)  # make configurable?
         self._check_db_updated()
         self._scheduler = self._build_scheduler()
 
@@ -322,6 +323,10 @@ def _check_col_updated(self, col):
                     # the sample document was never saved for this version doc
                     self._delete_version_and_node_docs(uver, ts)
                 else:
+                    # Do not update this document if it was last updated less than _reaper_update_delay ago
+                    # this is to avoid writing to a document in the process of being created
+                    if self._now() - ts < self._reaper_update_delay:
+                        continue
                     version = self._get_int_version_from_sample_doc(sampledoc, str(uver))
                     if version:
                         self._update_version_and_node_docs_with_find(id_, uver, version)
@@ -338,8 +343,8 @@ def _get_int_version_from_sample_doc(self, sampledoc, uuidverstr):
         return None
 
     def _delete_version_and_node_docs(self, uuidver, savedate):
-        if self._now() - savedate > self._deletion_delay:
-            print('deleting docs', self._now(), savedate, self._deletion_delay)
+        if self._now() - savedate > self._reaper_deletion_delay:
+            print('deleting docs', self._now(), savedate, self._reaper_deletion_delay)
             try:
                 # TODO logging
                 # delete edge docs first to ensure we don't orphan them
@@ -901,7 +906,7 @@ def replace_sample_acls(self, id_: UUID, acls: SampleACL):
                 UPDATE s WITH {{{_FLD_ACLS}: MERGE(s.{_FLD_ACLS}, @acls),
                                 {_FLD_ACL_UPDATE_TIME}: @ts
                                 }} IN @@col
-                RETURN s
+                RETURN NEW
             '''
         bind_vars = {'@col': self._col_sample.name,
                      'id': str(id_),
@@ -1027,7 +1032,7 @@ def _update_sample_acls_pt2(self, id_, update, owner, update_time):
         aql += '''
                         }
                     } IN @@col
-                RETURN s
+                RETURN NEW
             '''
 
         try:

test/SampleService_test.py

@@ -880,6 +880,21 @@ def _create_sample_version_as_admin(sample_port, as_user, expected_user):
     }
 
 
+def test_get_samples_fail_no_samples(sample_port):
+    _test_get_samples_fail(sample_port, None,
+        'Missing or incorrect "samples" field - must provide a list of samples to retrieve.')
+
+    _test_get_samples_fail(sample_port, "im a random sample id string!",
+        'Missing or incorrect "samples" field - must provide a list of samples to retrieve.')
+
+    _test_get_samples_fail(sample_port, [],
+        'Cannot provide empty list of samples - must provide at least one sample to retrieve.')
+
+
+def _test_get_samples_fail(sample_port, samples, message):
+    params = {'samples': samples}
+    _request_fail(sample_port, 'get_samples', TOKEN1, params, message)
+
 def test_get_sample_public_read(sample_port):
     url = f'http://localhost:{sample_port}'
     id_ = _create_generic_sample(url, TOKEN1)