Merge pull request #781 from guzman-raphael/filepath-delete

Fix S3 remove_object and add external location checks

Author: eywalker

LNX-docker-compose.yml

@@ -19,27 +19,27 @@ services:
       - MINIO_ACCESS_KEY=datajoint
       - MINIO_SECRET_KEY=datajoint
     # ports:
-    #   - "9000:9000"
+    #   - "80:80"
     # volumes:
     #   - ./minio/config:/root/.minio
     #   - ./minio/data:/data
-    command: server /data
+    command: server --address ":80" /data
     healthcheck:
-      test: ["CMD", "curl", "--fail", "http://minio:9000/minio/health/live"]
+      test: ["CMD", "curl", "--fail", "http://minio:80/minio/health/live"]
       timeout: 5s
       retries: 60
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: raphaelguzman/nginx:v0.0.4
+    image: raphaelguzman/nginx:v0.0.5
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306
     - ADD_minio_TYPE=MINIO
-    - ADD_minio_ENDPOINT=minio:9000
+    - ADD_minio_ENDPOINT=minio:80
     - ADD_minio_PREFIX=/
     # ports:
-    #   - "9000:9000"
+    #   - "80:80"
     #   - "443:443"
     #   - "3306:3306"
     depends_on:
@@ -60,7 +60,7 @@ services:
       - DJ_TEST_HOST=fakeservices.datajoint.io
       - DJ_TEST_USER=datajoint
       - DJ_TEST_PASSWORD=datajoint
-      - S3_ENDPOINT=fakeservices.datajoint.io:9000
+      - S3_ENDPOINT=fakeservices.datajoint.io
       - S3_ACCESS_KEY=datajoint
       - S3_SECRET_KEY=datajoint
       - S3_BUCKET=datajoint-test

datajoint/errors.py

@@ -81,6 +81,12 @@ class MissingExternalFile(DataJointError):
     """
 
 
+class BucketInaccessible(DataJointError):
+    """
+    Error raised when a S3 bucket is inaccessible
+    """
+
+
 # environment variables to control availability of experimental features
 
 ADAPTED_TYPE_SWITCH = "DJ_SUPPORT_ADAPTED_TYPES"

datajoint/external.py

@@ -46,6 +46,9 @@ def __init__(self, connection, store=None, database=None):
         if not self.is_declared:
             self.declare()
         self._s3 = None
+        if self.spec['protocol'] == 'file' and not Path(self.spec['location']).is_dir():
+            raise FileNotFoundError('Inaccessible local directory %s' %
+                                    self.spec['location']) from None
 
     @property
     def definition(self):

datajoint/s3.py

@@ -14,11 +14,11 @@ class Folder:
     A Folder instance manipulates a flat folder of objects within an S3-compatible object store
     """
     def __init__(self, endpoint, bucket, access_key, secret_key, *, secure=False, **_):
-        self.client = minio.Minio(endpoint, access_key=access_key, secret_key=secret_key, secure=secure)
+        self.client = minio.Minio(endpoint, access_key=access_key, secret_key=secret_key,
+                                  secure=secure)
         self.bucket = bucket
         if not self.client.bucket_exists(bucket):
-            warnings.warn('Creating bucket "%s"' % self.bucket)
-            self.client.make_bucket(self.bucket)
+            raise errors.BucketInaccessible('Inaccessible s3 bucket %s' % bucket) from None
 
     def put(self, name, buffer):
         return self.client.put_object(
@@ -63,6 +63,6 @@ def get_size(self, name):
 
     def remove_object(self, name):
         try:
-            self.client.remove_objects(self.bucket, str(name))
+            self.client.remove_object(self.bucket, str(name))
         except minio.ResponseError:
             return errors.DataJointError('Failed to delete %s from s3 storage' % name)

local-docker-compose.yml

@@ -20,28 +20,28 @@ services:
       - MINIO_ACCESS_KEY=datajoint
       - MINIO_SECRET_KEY=datajoint
     # ports:
-    #   - "9000:9000"
+    #   - "80:80"
     # To persist MinIO data and config
     # volumes:
     #   - ./minio/data:/data
     #   - ./minio/config:/root/.minio
-    command: server /data
+    command: server --address ":80" /data
     healthcheck:
-      test: ["CMD", "curl", "--fail", "http://minio:9000/minio/health/live"]
+      test: ["CMD", "curl", "--fail", "http://minio:80/minio/health/live"]
       timeout: 5s
       retries: 60
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: raphaelguzman/nginx:v0.0.4
+    image: raphaelguzman/nginx:v0.0.5
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306
     - ADD_minio_TYPE=MINIO
-    - ADD_minio_ENDPOINT=minio:9000
+    - ADD_minio_ENDPOINT=minio:80
     - ADD_minio_PREFIX=/
     ports:
-      - "9000:9000"
+      - "80:80"
       - "443:443"
       - "3306:3306"
     depends_on:
@@ -63,7 +63,7 @@ services:
       - DJ_TEST_USER=datajoint
       - DJ_TEST_PASSWORD=datajoint
       # If running tests locally, make sure to add entry in /etc/hosts for 127.0.0.1 fakeservices.datajoint.io
-      - S3_ENDPOINT=fakeservices.datajoint.io:9000
+      - S3_ENDPOINT=fakeservices.datajoint.io
       - S3_ACCESS_KEY=datajoint
       - S3_SECRET_KEY=datajoint
       - S3_BUCKET=datajoint-test

tests/__init__.py

@@ -41,6 +41,10 @@
     secret_key=environ.get('S3_SECRET_KEY', 'datajoint'),
     bucket=environ.get('S3_BUCKET', 'datajoint-test'))
 
+S3_MIGRATE_BUCKET = [path.stem for path in Path(
+        Path(__file__).resolve().parent,
+        'external-legacy-data', 's3').iterdir()][0]
+
 # Prefix for all databases used during testing
 PREFIX = environ.get('DJ_TEST_DB_PREFIX', 'djtest')
 conn_root = dj.conn(**CONN_INFO_ROOT)
@@ -129,23 +133,22 @@ def setup_package():
     source = Path(
         Path(__file__).resolve().parent,
         'external-legacy-data','s3')
-    bucket = "migrate-test"
     region = "us-east-1"
     try:
-        minioClient.make_bucket(bucket, location=region)
+        minioClient.make_bucket(S3_MIGRATE_BUCKET, location=region)
     except minio.error.BucketAlreadyOwnedByYou:
         pass
 
     pathlist = Path(source).glob('**/*')
     for path in pathlist:
         if os.path.isfile(str(path)) and ".sql" not in str(path):
             minioClient.fput_object(
-                    bucket, str(Path(
-                        os.path.relpath(str(path),str(Path(source,bucket))))
+                    S3_MIGRATE_BUCKET, str(Path(
+                        os.path.relpath(str(path),str(Path(source,S3_MIGRATE_BUCKET))))
                                 .as_posix()), str(path))
     # Add S3
     try:
-        minioClient.make_bucket("datajoint-test", location=region)
+        minioClient.make_bucket(S3_CONN_INFO['bucket'], location=region)
     except minio.error.BucketAlreadyOwnedByYou:
         pass
 
@@ -176,19 +179,17 @@ def teardown_package():
         remove("dj_local_conf.json")
 
     # Remove old S3
-    bucket = "migrate-test"
     objs = list(minioClient.list_objects_v2(
-            bucket, recursive=True))
-    objs = [minioClient.remove_object(bucket,
+            S3_MIGRATE_BUCKET, recursive=True))
+    objs = [minioClient.remove_object(S3_MIGRATE_BUCKET,
             o.object_name.encode('utf-8')) for o in objs]
-    minioClient.remove_bucket(bucket)
+    minioClient.remove_bucket(S3_MIGRATE_BUCKET)
 
     # Remove S3
-    bucket = "datajoint-test"
-    objs = list(minioClient.list_objects_v2(bucket, recursive=True))
-    objs = [minioClient.remove_object(bucket,
+    objs = list(minioClient.list_objects_v2(S3_CONN_INFO['bucket'], recursive=True))
+    objs = [minioClient.remove_object(S3_CONN_INFO['bucket'],
             o.object_name.encode('utf-8')) for o in objs]
-    minioClient.remove_bucket(bucket)
+    minioClient.remove_bucket(S3_CONN_INFO['bucket'])
 
     # Remove old File Content
     shutil.rmtree(str(Path(os.path.expanduser('~'),'temp')))