Merge pull request datajoint#955 from jverswijver/external_remove_bug

Fix return instead of raise, added test to check for s3Error

Author: guzman-raphael

.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,7 @@
 ### 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 - 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
@@ -77,6 +77,9 @@ services:
       - -c
       - |
         set -e
+        wget -P /home/dja/.local/bin/ https://dl.min.io/client/mc/release/linux-amd64/mc
+        chmod +x /home/dja/.local/bin/mc
+        mc alias set myminio/ http://minio:9000 datajoint datajoint
         pip install --user -r test_requirements.txt
         pip install -e .
         pip freeze | grep datajoint

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
 ```

datajoint/external.py

@@ -314,11 +314,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
@@ -346,7 +347,8 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
                     try:
                         self._remove_external_file(external_path)
                     except Exception as error:
-                        error_list.append((uuid, external_path, str(error)))
+                        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,7 @@
 ----------------------
 * 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 - 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
@@ -82,6 +82,9 @@ services:
       - -c
       - |
         set -e
+        wget -P /home/dja/.local/bin/ https://dl.min.io/client/mc/release/linux-amd64/mc
+        chmod +x /home/dja/.local/bin/mc
+        mc alias set myminio/ http://minio:9000 datajoint datajoint
         pip install --user nose nose-cov coveralls flake8 ptvsd
         pip install -e .
         pip freeze | grep datajoint

tests/test_s3.py

@@ -1,9 +1,14 @@
 from . import S3_CONN_INFO
-
 from minio import Minio
+import json
 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
+import os
+from datajoint.hash import uuid_from_buffer
+from datajoint.blob import pack
 
 
 class TestS3:
@@ -51,3 +56,84 @@ 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
+
+        # Initialize minioClient with an endpoint and access/secret keys.
+        minio_client = Minio(
+            'minio:9000',
+            access_key='jeffjeff',
+            secret_key='jeffjeff',
+            secure=False)
+
+        # Create new user
+        os.system('mc admin user add myminio jeffjeff jeffjeff')
+        # json for test policy for permissionless user
+        testpolicy = {
+            "Version": "2012-10-17",
+            "Statement": [
+                {
+                    "Action": [
+                        "s3:GetBucketLocation",
+                        "s3:ListBucket",
+                        "s3:ListBucketMultipartUploads",
+                        "s3:ListAllMyBuckets"
+                    ],
+                    "Effect": "Allow",
+                    "Resource": [
+                        "arn:aws:s3:::datajoint.test",
+                        "arn:aws:s3:::datajoint.migrate"
+                    ],
+                    "Sid": ""
+                },
+                {
+                    "Action": [
+                        "s3:GetObject",
+                        "s3:ListMultipartUploadParts"
+                    ],
+                    "Effect": "Allow",
+                    "Resource": [
+                        "arn:aws:s3:::datajoint.test/*",
+                        "arn:aws:s3:::datajoint.migrate/*"
+                    ],
+                    "Sid": ""
+                }
+            ]
+        }
+
+        # Write test json to tmp directory so we can use it to create a new user policy
+        with open('/tmp/policy.json', 'w') as f:
+            f.write(json.dumps(testpolicy))
+
+        # Add the policy and apply it to the user
+        os.system('mc admin policy add myminio test /tmp/policy.json')
+        os.system('mc admin policy set myminio test user=jeffjeff')
+
+        # 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 to the external table that has permissions restrictions
+        schema.external['share'].s3.client = minio_client
+
+        # This method returns a list of errors
+        error_list = schema.external['share'].delete(delete_external_files=True,
+                                                     errors_as_string=False)
+
+        # Teardown
+        os.system('mc admin policy remove myminio test')
+        os.system('mc admin user remove myminio jeffjeff')
+        schema.external['share'].s3.client = old_client
+        schema.external['share'].delete(delete_external_files=True)
+        os.remove("/tmp/policy.json")
+
+        # 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]