From 03d5d900bc935dbfaa352c092ebdb9463a8118b5 Mon Sep 17 00:00:00 2001 From: Kirit Thadaka Date: Tue, 5 Dec 2017 16:18:06 +0530 Subject: [PATCH 1/3] Changed order of checks in delete_file --- notebook/services/contents/filemanager.py | 17 ++++++++--------- .../contents/tests/test_contents_api.py | 3 +-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/notebook/services/contents/filemanager.py b/notebook/services/contents/filemanager.py index 19cdc8f4f..7d0c7be5d 100644 --- a/notebook/services/contents/filemanager.py +++ b/notebook/services/contents/filemanager.py @@ -493,15 +493,6 @@ class FileContentsManager(FileManagerMixin, ContentsManager): raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path) if self.delete_to_trash: - if os.path.isdir(os_path): - listing = os.listdir(os_path) - # Don't send non-empty directories to trash. - # A directory containing only leftover checkpoints is - # considered empty. - cp_dir = getattr(self.checkpoints, 'checkpoint_dir', None) - for entry in listing: - if entry != cp_dir: - raise web.HTTPError(400, u'Directory %s not empty' % os_path) self.log.debug("Sending %s to trash", os_path) # Looking at the code in send2trash, I don't think the errors it # raises let us distinguish permission errors from other errors in @@ -510,6 +501,14 @@ class FileContentsManager(FileManagerMixin, ContentsManager): return if os.path.isdir(os_path): + listing = os.listdir(os_path) + # Don't permanently delete non-empty directories. + # A directory containing only leftover checkpoints is + # considered empty. + cp_dir = getattr(self.checkpoints, 'checkpoint_dir', None) + for entry in listing: + if entry != cp_dir: + raise web.HTTPError(400, u'Directory %s not empty' % os_path) self.log.debug("Removing directory %s", os_path) with self.perm_to_403(): shutil.rmtree(os_path) diff --git a/notebook/services/contents/tests/test_contents_api.py b/notebook/services/contents/tests/test_contents_api.py index 9b880f539..990b6ca51 100644 --- a/notebook/services/contents/tests/test_contents_api.py +++ b/notebook/services/contents/tests/test_contents_api.py @@ -518,8 +518,7 @@ class APITest(NotebookTestBase): for name in sorted(self.dirs + ['/'], key=len, reverse=True): listing = self.api.list(name).json()['content'] for model in listing: - if os.path.exists(model['path']): - self.api.delete(model['path']) + self.api.delete(model['path']) listing = self.api.list('/').json()['content'] self.assertEqual(listing, []) From 7f23ad65a62f704b5573b4b81e83d04c696303f8 Mon Sep 17 00:00:00 2001 From: Kirit Thadaka Date: Tue, 5 Dec 2017 16:30:10 +0530 Subject: [PATCH 2/3] modified test for deleting non empty directory --- notebook/services/contents/tests/test_contents_api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/notebook/services/contents/tests/test_contents_api.py b/notebook/services/contents/tests/test_contents_api.py index 990b6ca51..12049fc4b 100644 --- a/notebook/services/contents/tests/test_contents_api.py +++ b/notebook/services/contents/tests/test_contents_api.py @@ -525,7 +525,8 @@ class APITest(NotebookTestBase): def test_delete_non_empty_dir(self): """delete non-empty dir raises 400""" with assert_http_error(400): - self.api.delete(u'å b') + shutil.rmtree((u'å b', ignore_errors=True) + # self.api.delete(u'å b') def test_rename(self): resp = self.api.rename('foo/a.ipynb', 'foo/z.ipynb') From 773e55cb1329f38b29235dbf514e1aa5256568b6 Mon Sep 17 00:00:00 2001 From: Kirit Thadaka Date: Tue, 5 Dec 2017 16:30:13 +0530 Subject: [PATCH 3/3] modified test for deleting non empty directory --- notebook/services/contents/tests/test_contents_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/services/contents/tests/test_contents_api.py b/notebook/services/contents/tests/test_contents_api.py index 12049fc4b..a37b99d13 100644 --- a/notebook/services/contents/tests/test_contents_api.py +++ b/notebook/services/contents/tests/test_contents_api.py @@ -523,7 +523,7 @@ class APITest(NotebookTestBase): self.assertEqual(listing, []) def test_delete_non_empty_dir(self): - """delete non-empty dir raises 400""" + """permanentaly deleting non-empty dir raises 400""" with assert_http_error(400): shutil.rmtree((u'å b', ignore_errors=True) # self.api.delete(u'å b')