Repository: jupyter-server/jupyter_server

Issue:
Allow non-empty directory deletion for windows through settings
Hey would it be ok to allow non-empty directory deletion for windows through settings?

https://github.com/jupyter-server/jupyter_server/blob/f914126d173dde7d2b2f6a6d474648060c823419/jupyter_server/services/contents/filemanager.py#L492

Discussion:
codecov-commenter: # [Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) Report
> Merging [#574](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) (f02cdb5) into [master](https://codecov.io/gh/jupyter-server/jupyter_server/commit/1444737514c3e8c6acbe3fc01f9420c8e078d3a0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) (1444737) will **decrease** coverage by `1.25%`.
> The diff coverage is `88.23%`.

[![Impacted file tree graph](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/graphs/tree.svg?width=650&height=150&src=pr&token=S9WiBg2iL0&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server)](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server)

```diff
@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
- Coverage   76.93%   75.68%   -1.26%     
==========================================
  Files         109      109              
  Lines        9951     9631     -320     
  Branches     1079     1077       -2     
==========================================
- Hits         7656     7289     -367     
- Misses       1912     1961      +49     
+ Partials      383      381       -2     
```


| [Impacted Files](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) | Coverage Δ | |
|---|---|---|
| [jupyter\_server/services/contents/filemanager.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMvY29udGVudHMvZmlsZW1hbmFnZXIucHk=) | `67.79% <60.00%> (+1.47%)` | :arrow_up: |
| [...ter\_server/tests/services/contents/test\_manager.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvdGVzdHMvc2VydmljZXMvY29udGVudHMvdGVzdF9tYW5hZ2VyLnB5) | `97.33% <100.00%> (+0.05%)` | :arrow_up: |
| [jupyter\_server/traittypes.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvdHJhaXR0eXBlcy5weQ==) | `47.02% <0.00%> (-16.67%)` | :arrow_down: |
| [jupyter\_server/extension/handler.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvZXh0ZW5zaW9uL2hhbmRsZXIucHk=) | `52.00% <0.00%> (-6.63%)` | :arrow_down: |
| [jupyter\_server/services/kernels/kernelmanager.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMva2VybmVscy9rZXJuZWxtYW5hZ2VyLnB5) | `76.01% <0.00%> (-4.86%)` | :arrow_down: |
| [...r/tests/unix\_sockets/test\_serverapp\_integration.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvdGVzdHMvdW5peF9zb2NrZXRzL3Rlc3Rfc2VydmVyYXBwX2ludGVncmF0aW9uLnB5) | `18.07% <0.00%> (-4.66%)` | :arrow_down: |
| [jupyter\_server/services/kernels/handlers.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMva2VybmVscy9oYW5kbGVycy5weQ==) | `59.07% <0.00%> (-3.89%)` | :arrow_down: |
| [jupyter\_server/gateway/gateway\_client.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvZ2F0ZXdheS9nYXRld2F5X2NsaWVudC5weQ==) | `71.69% <0.00%> (-3.58%)` | :arrow_down: |
| [jupyter\_server/services/shutdown.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMvc2h1dGRvd24ucHk=) | `71.42% <0.00%> (-3.58%)` | :arrow_down: |
| [...yter\_server/tests/services/kernelspecs/test\_api.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvdGVzdHMvc2VydmljZXMva2VybmVsc3BlY3MvdGVzdF9hcGkucHk=) | `96.49% <0.00%> (-3.51%)` | :arrow_down: |
| ... and [57 more](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server). Last update [1444737...f02cdb5](https://codecov.io/gh/jupyter-server/jupyter_server/pull/574?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server).

blink1073: FYI this now has a merge conflict from the formatting changes in #575 
fcollonval: @blink1073 CI tests failing are identical to the one on master. Is this one good to go?

Patch:
diff --git a/jupyter_server/services/contents/filemanager.py b/jupyter_server/services/contents/filemanager.py
index 376a8db62..54e2dca38 100644
--- a/jupyter_server/services/contents/filemanager.py
+++ b/jupyter_server/services/contents/filemanager.py
@@ -119,6 +119,16 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
         deleting files really deletes them.""",
     )
 
+    always_delete_dir = Bool(
+        False,
+        config=True,
+        help="""If True, deleting a non-empty directory will always be allowed.
+        WARNING this may result in files being permanently removed; e.g. on Windows,
+        if the data size is too big for the trash/recycle bin the directory will be permanently
+        deleted. If False (default), the non-empty directory will be sent to the trash only
+        if safe. And if ``delete_to_trash`` is True, the directory won't be deleted.""",
+    )
+
     @default("files_handler_class")
     def _files_handler_class_default(self):
         return AuthenticatedFileHandler
@@ -331,7 +341,10 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
         if content:
             content, format = self._read_file(os_path, format)
             if model["mimetype"] is None:
-                default_mime = {"text": "text/plain", "base64": "application/octet-stream"}[format]
+                default_mime = {
+                    "text": "text/plain",
+                    "base64": "application/octet-stream",
+                }[format]
                 model["mimetype"] = default_mime
 
             model.update(
@@ -391,7 +404,9 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
         if os.path.isdir(os_path):
             if type not in (None, "directory"):
                 raise web.HTTPError(
-                    400, u"%s is a directory, not a %s" % (path, type), reason="bad type"
+                    400,
+                    u"%s is a directory, not a %s" % (path, type),
+                    reason="bad type",
                 )
             model = self._dir_model(path, content=content)
         elif type == "notebook" or (type is None and path.endswith(".ipynb")):
@@ -494,7 +509,7 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
             return False
 
         if self.delete_to_trash:
-            if sys.platform == "win32" and is_non_empty_dir(os_path):
+            if not self.always_delete_dir and sys.platform == "win32" and is_non_empty_dir(os_path):
                 # send2trash can really delete files on Windows, so disallow
                 # deleting non-empty files. See Github issue 3631.
                 raise web.HTTPError(400, u"Directory %s not empty" % os_path)
@@ -507,12 +522,13 @@ class FileContentsManager(FileManagerMixin, ContentsManager):
                 return
             else:
                 self.log.warning(
-                    "Skipping trash for %s, on different device " "to home directory", os_path
+                    "Skipping trash for %s, on different device " "to home directory",
+                    os_path,
                 )
 
         if os.path.isdir(os_path):
             # Don't permanently delete non-empty directories.
-            if is_non_empty_dir(os_path):
+            if not self.always_delete_dir and is_non_empty_dir(os_path):
                 raise web.HTTPError(400, u"Directory %s not empty" % os_path)
             self.log.debug("Removing directory %s", os_path)
             with self.perm_to_403():
@@ -649,7 +665,10 @@ class AsyncFileContentsManager(FileContentsManager, AsyncFileManagerMixin, Async
         if content:
             content, format = await self._read_file(os_path, format)
             if model["mimetype"] is None:
-                default_mime = {"text": "text/plain", "base64": "application/octet-stream"}[format]
+                default_mime = {
+                    "text": "text/plain",
+                    "base64": "application/octet-stream",
+                }[format]
                 model["mimetype"] = default_mime
 
             model.update(
@@ -709,7 +728,9 @@ class AsyncFileContentsManager(FileContentsManager, AsyncFileManagerMixin, Async
         if os.path.isdir(os_path):
             if type not in (None, "directory"):
                 raise web.HTTPError(
-                    400, u"%s is a directory, not a %s" % (path, type), reason="bad type"
+                    400,
+                    u"%s is a directory, not a %s" % (path, type),
+                    reason="bad type",
                 )
             model = await self._dir_model(path, content=content)
         elif type == "notebook" or (type is None and path.endswith(".ipynb")):
@@ -813,7 +834,11 @@ class AsyncFileContentsManager(FileContentsManager, AsyncFileManagerMixin, Async
             return False
 
         if self.delete_to_trash:
-            if sys.platform == "win32" and await is_non_empty_dir(os_path):
+            if (
+                not self.always_delete_dir
+                and sys.platform == "win32"
+                and await is_non_empty_dir(os_path)
+            ):
                 # send2trash can really delete files on Windows, so disallow
                 # deleting non-empty files. See Github issue 3631.
                 raise web.HTTPError(400, u"Directory %s not empty" % os_path)
@@ -826,12 +851,13 @@ class AsyncFileContentsManager(FileContentsManager, AsyncFileManagerMixin, Async
                 return
             else:
                 self.log.warning(
-                    "Skipping trash for %s, on different device " "to home directory", os_path
+                    "Skipping trash for %s, on different device " "to home directory",
+                    os_path,
                 )
 
         if os.path.isdir(os_path):
             # Don't permanently delete non-empty directories.
-            if await is_non_empty_dir(os_path):
+            if not self.always_delete_dir and await is_non_empty_dir(os_path):
                 raise web.HTTPError(400, u"Directory %s not empty" % os_path)
             self.log.debug("Removing directory %s", os_path)
             with self.perm_to_403():


Test patch:
diff --git a/jupyter_server/tests/services/contents/test_manager.py b/jupyter_server/tests/services/contents/test_manager.py
index 9063d2249..386d3ed60 100644
--- a/jupyter_server/tests/services/contents/test_manager.py
+++ b/jupyter_server/tests/services/contents/test_manager.py
@@ -513,6 +513,47 @@ async def test_delete(jp_contents_manager):
         await ensure_async(cm.get(path))
 
 
+@pytest.mark.parametrize(
+    "delete_to_trash, always_delete, error",
+    (
+        [True, True, False],
+        # on linux test folder may not be on home folder drive
+        # => if this is the case, _check_trash will be False
+        [True, False, None],
+        [False, True, False],
+        [False, False, True],
+    ),
+)
+async def test_delete_non_empty_folder(delete_to_trash, always_delete, error, jp_contents_manager):
+    cm = jp_contents_manager
+    cm.delete_to_trash = delete_to_trash
+    cm.always_delete_dir = always_delete
+
+    dir = "to_delete"
+
+    await make_populated_dir(cm, dir)
+    await check_populated_dir_files(cm, dir)
+
+    if error is None:
+        error = False
+        if sys.platform == "win32":
+            error = True
+        elif sys.platform == "linux":
+            file_dev = os.stat(cm.root_dir).st_dev
+            home_dev = os.stat(os.path.expanduser("~")).st_dev
+            error = file_dev != home_dev
+
+    if error:
+        with pytest.raises(
+            HTTPError,
+            match=r"HTTP 400: Bad Request \(Directory .*?to_delete not empty\)",
+        ):
+            await ensure_async(cm.delete_file(dir))
+    else:
+        await ensure_async(cm.delete_file(dir))
+        assert cm.dir_exists(dir) == False
+
+
 async def test_rename(jp_contents_manager):
     cm = jp_contents_manager
     # Create a new notebook