Merge branch 'master' into mp

Author: dimitri-yatsenko

.github/workflows/development.yaml

@@ -40,7 +40,7 @@ jobs:
           PY_VER: ${{matrix.py_ver}}
           MYSQL_VER: ${{matrix.mysql_ver}}
           ALPINE_VER: "3.10"
-          MINIO_VER: RELEASE.2019-09-26T19-42-35Z
+          MINIO_VER: RELEASE.2021-09-03T03-56-13Z
           COMPOSE_HTTP_TIMEOUT: "120"
           COVERALLS_SERVICE_NAME: travis-ci
           COVERALLS_REPO_TOKEN: fd0BoXG46TPReEem0uMy7BJO5j0w1MQiY

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### 0.13.3 -- TBD
 * Bugfix - Dependencies not properly loaded on populate. (#902) PR #919
 * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939
+* Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956
+* Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955
 
 ### 0.13.2 -- May 7, 2021
 * Update `setuptools_certificate` dependency to new name `otumat`

LNX-docker-compose.yml

@@ -32,7 +32,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.17
+    image: datajoint/nginx:v0.0.18
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306

README.md

@@ -108,7 +108,7 @@ A number of labs are currently adopting DataJoint and we are quickly getting the
 PY_VER=3.7
 ALPINE_VER=3.10
 MYSQL_VER=5.7
-MINIO_VER=RELEASE.2019-09-26T19-42-35Z
+MINIO_VER=RELEASE.2021-09-03T03-56-13Z
 UID=1000
 GID=1000
 ```
@@ -136,6 +136,8 @@ GID=1000
 * Add entry in `/etc/hosts` for `127.0.0.1 fakeservices.datajoint.io`
 
 
+
+
 ### Launch Jupyter Notebook for Interactive Use
 * Navigate to `localhost:8888`
 * Input Jupyter password

datajoint/external.py

@@ -127,7 +127,10 @@ def _remove_external_file(self, external_path):
         if self.spec['protocol'] == 's3':
             self.s3.remove_object(external_path)
         elif self.spec['protocol'] == 'file':
-            Path(external_path).unlink()
+            try:
+                Path(external_path).unlink()
+            except FileNotFoundError:
+                pass
 
     def exists(self, external_filepath):
         """
@@ -314,11 +317,12 @@ def used(self):
         return self & [FreeTable(self.connection, ref['referencing_table']).proj(hash=ref['column_name'])
                        for ref in self.references]
 
-    def delete(self, *, delete_external_files=None, limit=None, display_progress=True):
+    def delete(self, *, delete_external_files=None, limit=None, display_progress=True, errors_as_string=True):
         """
         :param delete_external_files: True or False. If False, only the tracking info is removed from the
         external store table but the external files remain intact. If True, then the external files
         themselves are deleted too.
+        :param errors_as_string: If True any errors returned when deleting from external files will be strings
         :param limit: (integer) limit the number of items to delete
         :param display_progress: if True, display progress as files are cleaned up
         :return: if deleting external files, returns errors
@@ -337,16 +341,20 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
             # delete items one by one, close to transaction-safe
             error_list = []
             for uuid, external_path in items:
-                try:
-                    count = (self & {'hash': uuid}).delete_quick(get_count=True)  # optimize
-                except Exception:
-                    pass   # if delete failed, do not remove the external file
-                else:
-                    assert count in (0, 1)
+                row = (self & {'hash': uuid}).fetch()
+                if row.size:
                     try:
-                        self._remove_external_file(external_path)
-                    except Exception as error:
-                        error_list.append((uuid, external_path, str(error)))
+                        (self & {'hash': uuid}).delete_quick()
+                    except Exception:
+                        pass   # if delete failed, do not remove the external file
+                    else:
+                        try:
+                            self._remove_external_file(external_path)
+                        except Exception as error:
+                            # adding row back into table after failed delete
+                            self.insert1(row[0], skip_duplicates=True)
+                            error_list.append((uuid, external_path,
+                                               str(error) if errors_as_string else error))
             return error_list
 
 

datajoint/s3.py

@@ -76,12 +76,11 @@ def get_size(self, name):
         except minio.error.S3Error as e:
             if e.code == 'NoSuchKey':
                 raise errors.MissingExternalFile
-            else:
-                raise e
+            raise e
 
     def remove_object(self, name):
         logger.debug('remove_object: {}:{}'.format(self.bucket, name))
         try:
             self.client.remove_object(self.bucket, str(name))
-        except minio.ResponseError:
-            return errors.DataJointError('Failed to delete %s from s3 storage' % name)
+        except minio.error.MinioException:
+            raise errors.DataJointError('Failed to delete %s from s3 storage' % name)

docs-parts/intro/Releases_lang1.rst

@@ -2,6 +2,8 @@
 ----------------------
 * Bugfix - Dependencies not properly loaded on populate. (#902) PR #919
 * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939
+* Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956
+* Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955
 
 0.13.2 -- May 7, 2021
 ----------------------

local-docker-compose.yml

@@ -34,7 +34,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.17
+    image: datajoint/nginx:v0.0.18
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306

tests/test_external.py

@@ -3,11 +3,10 @@
 from nose.tools import assert_true, assert_equal
 from datajoint.external import ExternalTable
 from datajoint.blob import pack, unpack
-
-from .schema_external import schema
 import datajoint as dj
-from .schema_external import stores_config
-from .schema_external import SimpleRemote
+from .schema_external import stores_config, SimpleRemote, Simple, schema
+import os
+
 current_location_s3 = dj.config['stores']['share']['location']
 current_location_local = dj.config['stores']['local']['location']
 
@@ -47,6 +46,9 @@ def test_s3_leading_slash(index=100, store='share'):
     """
     s3 external storage configured with leading slash
     """
+
+    oldConfig = dj.config['stores'][store]['location']
+
     value = np.array([1, 2, 3])
 
     id = index
@@ -97,9 +99,28 @@ def test_s3_leading_slash(index=100, store='share'):
     assert_true(np.array_equal(
         value, (SimpleRemote & 'simple={}'.format(id)).fetch1('item')))
 
+    dj.config['stores'][store]['location'] = oldConfig
+
 
 def test_file_leading_slash():
     """
     file external storage configured with leading slash
     """
     test_s3_leading_slash(index=200, store='local')
+
+
+def test_remove_fail():
+    # https://github.com/datajoint/datajoint-python/issues/953
+    data = dict(simple=2, item=[1, 2, 3])
+    Simple.insert1(data)
+    path1 = dj.config['stores']['local']['location'] + '/djtest_extern/4/c/'
+    currentMode = int(oct(os.stat(path1).st_mode), 8)
+    os.chmod(path1, 0o40555)
+    (Simple & 'simple=2').delete()
+    listOfErrors = schema.external['local'].delete(delete_external_files=True)
+    assert len(listOfErrors) == 1, 'unexpected number of errors'
+    assert len(schema.external['local'] & dict(hash=listOfErrors[0][0])) == 1, \
+        'unexpected number of rows in external table'
+    # ---------------------CLEAN UP--------------------
+    os.chmod(path1, currentMode)
+    listOfErrors = schema.external['local'].delete(delete_external_files=True)

tests/test_s3.py

@@ -1,9 +1,12 @@
 from . import S3_CONN_INFO
-
 from minio import Minio
 import urllib3
 import certifi
-from nose.tools import assert_true
+from nose.tools import assert_true, raises
+from .schema_external import schema, SimpleRemote
+from datajoint.errors import DataJointError
+from datajoint.hash import uuid_from_buffer
+from datajoint.blob import pack
 
 
 class TestS3:
@@ -51,3 +54,35 @@ def test_connection_secure():
             http_client=http_client)
 
         assert_true(minio_client.bucket_exists(S3_CONN_INFO['bucket']))
+
+    @staticmethod
+    @raises(DataJointError)
+    def test_remove_object_exception():
+        # https://github.com/datajoint/datajoint-python/issues/952
+
+        # Insert some test data and remove it so that the external table is populated
+        test = [1, [1, 2, 3]]
+        SimpleRemote.insert1(test)
+        SimpleRemote.delete()
+
+        # Save the old external table minio client
+        old_client = schema.external['share'].s3.client
+
+        # Apply our new minio client which has a user that does not exist
+        schema.external['share'].s3.client = Minio(
+            'minio:9000',
+            access_key='jeffjeff',
+            secret_key='jeffjeff',
+            secure=False)
+
+        # This method returns a list of errors
+        error_list = schema.external['share'].delete(delete_external_files=True,
+                                                     errors_as_string=False)
+
+        # Teardown
+        schema.external['share'].s3.client = old_client
+        schema.external['share'].delete(delete_external_files=True)
+
+        # Raise the error we want if the error matches the expected uuid
+        if str(error_list[0][0]) == str(uuid_from_buffer(pack(test[1]))):
+            raise error_list[0][2]