Merge pull request #956 from zitrosolrac/external_removal_fail

Fix logic bug in external file clean up

Author: dimitri-yatsenko

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

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):
         """
@@ -338,17 +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) if errors_as_string else 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
 
 

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

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)