Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing non empty dirs to be deleted #3108

Merged
merged 13 commits into from
Dec 5, 2017
21 changes: 10 additions & 11 deletions notebook/services/contents/filemanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -489,17 +489,8 @@ def delete_file(self, path):
path = path.strip('/')
os_path = self._get_os_path(path)
rm = os.unlink
if os.path.isdir(os_path):
listing = os.listdir(os_path)
# Don't 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)
elif not os.path.isfile(os_path):
raise web.HTTPError(404, u'File does not exist: %s' % os_path)
if not os.path.exists(os_path):
raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path)

if self.delete_to_trash:
self.log.debug("Sending %s to trash", os_path)
Expand All @@ -510,6 +501,14 @@ def delete_file(self, path):
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)
Expand Down
7 changes: 4 additions & 3 deletions notebook/services/contents/tests/test_contents_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -523,9 +523,10 @@ def test_delete_dirs(self):
self.assertEqual(listing, [])

def test_delete_non_empty_dir(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test that deleting a non-empty directory does work?

"""delete non-empty dir raises 400"""
with assert_http_error(400):
self.api.delete(u'å b')
# Test that non empty directory can be deleted
self.api.delete(u'å b')
# Assertion will pass only if self.api.delete does not throw and error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test doesn't have to have an assert - you can leave this off, and it will still fail if there's an error. When a test just checks that something runs without an error, that's a 'smoketest' (as in 'does this make smoke come out').

However, it would be good to check that it has actually been removed as well. Maybe copy some code from test_list_nonexistant_dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay. Sure, I'll make these changes!

assert True

def test_rename(self):
resp = self.api.rename('foo/a.ipynb', 'foo/z.ipynb')
Expand Down