Allow deleting non-empty directory when c.FileContentsManager.delete_to_trash = False

[albertmichaelj]

There has been a long standing issue at the Jupyter Lab repo around the inability to delete a non-empty directory (see comments jupyterlab/jupyterlab#835 (comment) and below for recent discussion). On some platforms (Mac OS for example), a non-empty directory can be deleted easily. However, on other platforms (including the official docker container jupyter/minimal-notebook), non-empty directories cannot be deleted. This appears to be an intentional design decision to not allow deletion of non-empty directories (see this code here).

However, there are lots of legitimate cases to delete a non-empty directory, so a much better solution would be to ask for confirmation before deleting the non-empty directory. Failing that, there should be a config parameter that allows for the deletion of non-empty directories.

Unfortunately, I'm not equipped to do development on the notebook server, so I cannot produce a pull request myself. Is this an issue that the maintainers are willing to consider?

[jasongrout]

One compelling usecase from jupyterlab/jupyterlab#835 (comment)

If the current design is intentional, one of the problems with it is when you have hidden files. For example, it's common to clone a git repo in various projects. All of the .git directory is hidden, and so you can never delete the repo without resorting to the command line. If this is a deliberate decision, I would encourage at least a config parameter in order to allow for the deletion of non-empty folders.

[jasongrout]

I can imagine one way to resolve this is to have a force flag in the rest api, like the -f in rm -rf, which will delete nonempty directories. Then jlab could call without the force flag, and if there is an error, could confirm with the user and call the rest api with the force flag.

[albertmichaelj]

I wanted to ping about this issue again. Is this something that maintainers might be willing to consider. This has ended up being a fairly significant problem for my use case (teaching non-sophisticated users in a JupyterHub deployment using git repositories for code distribution) due to hidden files that are very difficult to manually delete for non-sophisticated users. I'm in the process of reworking the JupyterHub deployment for the next academic year, and I would really like for the users to have an ability to delete a non-empty directory.

Happy to provide any more details if needed!

[hemangjoshi37a]

Has this been solved already??

[Dmitry1987]

Additional dialog box for confirmation, is what many other web UIs do. A flag for command line when we start the notebook server, would be a good solution as well. Would it be accepted if a PR is sent? It's a relatively simple patch for the filemanager.py code. I am actually surprised it is not a bug but a design decision.

[albertmichaelj]

@Dmitry1987 If you put together a pull request, I am happy to help with any testing. I think the problem is that if there is a way to send files to trash (which is what #3108 seemed to be allowing) then you can delete non-empty directories. However, there is a bug in using Docker containers that use the Overlay-FS (see #3130) that means that in a Docker container, you need to set c.FileContentsManager.delete_to_trash = False. I think a better solution would be just a dialog box that asks if you really want to delete the directory (with language like "This can not be undone.") would be a better approach instead of just refusing to delete non-empty directories.

Again, I'm very happy to help with any testing/feedback if you put together a pull request!

[Dmitry1987]

thanks for your suggestion @albertmichaelj I will try that setting to see if it works.
Could you point me to the function I can use to display a dialog box and collect the response with? I might be able to make the PR with that

[albertmichaelj]

Unfortunately, I have no idea what functions display dialogue boxes. If I had a better understanding of the notebook code base, I'd try a PR myself.

Like I said, if I can help with testing, I'd be happy to.

[Dmitry1987]

I tried the flag of c.FileContentsManager.delete_to_trash = False, added it to config and restarted everything, but looks like it had no effect :( . Seems like this is a default behavior of that function.. are you able to delete non-empty folders @albertmichaelj ? if so, mind sharing the config file sections that might be relevant as well, to that functionality? thanks for trying to help 👍

[W 2020-05-19 00:29:20.409 SingleUserNotebookApp handlers:236] delete /shared_disk/somefolder
[W 2020-05-19 00:29:20.410 SingleUserNotebookApp web:1786] 400 DELETE /user/dima1987/api/contents/shared_disk/somefolder (xxxxxx): Directory /home/ds-team/shared_disk/somefolder not empty
[W 2020-05-19 00:29:20.410 SingleUserNotebookApp handlers:620] Directory /home/ds-team/shared_disk/somefolder not empty
[W 2020-05-19 00:29:20.411 SingleUserNotebookApp log:174] 400 DELETE /user/dima1987/api/contents/shared_disk/somefolder (dima1987@xxxxxxx) 2.31ms

[Dmitry1987]

I guess the solution in my case would be just to patch the relevant file, and rebuild the notebook, each time in our CI, before it gets deployed. We run a CI that rebuilds kernels (the data team keeps a list of kernel names and the packages they'd like to have, so when they modify these files, we rebuild and redeploy it), so it can also patch the code after "pip install ..." of the notebook. But it'll need to be maintained alongside newer versions, and it's a pain to do every time. Maybe I can use the repo as a source instead of pip package, so it'll be patched by git command, can be a longer term solution (the patch will work for newer versions until there's a conflict in same file)

[albertmichaelj]

@Dmitry1987 You need to set c.FileContentsManager.delete_to_trash = True to get it to delete to trash. The default behavior is True, but I'm guessing that somehow it is sest to False in a config. Are you using the Jupyter docker notebooks by any chance? If so, it is set here.

Hope that helps, but as for this issue, I still think that we should be able to delete folders that aren't empty with a dialogue box.

[Dmitry1987]

thanks @albertmichaelj , I do use the docker versions, but I am running it on linux, if I set to "True" doesn't it look for windows trash bin only in windows related parts of the code?

I'll try the "True" setting today and will post results, thanks :)

[Dmitry1987]

No it doesn't work :( both True/False of this setting have no effect. I'll try to patch the relevant piece of code, will share results if I have a working solution.

[albertmichaelj]

@dmikushin To be honest, I'm not sure what you're talking about. I can confirm that the following Dockerfile works fine (as in I can delete non-empty directories):

ARG BASE_CONTAINER=jupyter/base-notebook:8e8cbd0a0af7
FROM $BASE_CONTAINER

RUN sed -i 's/c.FileContentsManager.delete_to_trash = False/c.FileContentsManager.delete_to_trash = True/g' /etc/jupyter/jupyter_notebook_config.py

While the following does not allow me to delete non-empty directories:

ARG BASE_CONTAINER=jupyter/base-notebook:8e8cbd0a0af7
FROM $BASE_CONTAINER

# RUN sed -i 's/c.FileContentsManager.delete_to_trash = False/c.FileContentsManager.delete_to_trash = True/g' /etc/jupyter/jupyter_notebook_config.py

I build the containers as test_docker, and I run them with the following command:

docker run -it --rm -e JUPYTER_ENABLE_LAB=yes -p 10000:8888 test_docker start-notebook.sh --NotebookApp.token=''

or

docker run -it --rm -p 10000:8888 test_docker start-notebook.sh --NotebookApp.token=''

This will allow you to delete folders. What issue are you running into?

[dmikushin]

@albertmichaelj Wrong Dmitry @Dmitry1987

[albertmichaelj]

Sorry @dmikushin!

[Dmitry1987]

Thanks for sharing @albertmichaelj .
I have a custom image, it's basically the same Dockerfile as the "jupyter/base-notebook" is based on, but uses a different python version because we wanted Python 3.7 to be the default in the first kernel that loads (the default environment). Maybe it was a bad idea. But the installation of notebook is the same.

The relevant part of the dockerfile is this for example

# Create NB_USER wtih name ds-team user with UID=1000 and in the 'users' group
# and make sure these dirs are writable by the `users` group.
RUN echo "auth requisite pam_deny.so" >> /etc/pam.d/su && \
    sed -i.bak -e 's/^%admin/#%admin/' /etc/sudoers && \
    sed -i.bak -e 's/^%sudo/#%sudo/' /etc/sudoers && \
    useradd -m -s /bin/bash -N -u $NB_UID $NB_USER && \
    mkdir -p /usr/local/bin/before-notebook.d && \
    mkdir -p $PYTHON_DIR && \
    chown $NB_USER:$NB_GID $PYTHON_DIR && \
    chmod g+w /etc/passwd && \
    fix-permissions $HOME && \
    fix-permissions "$(dirname $PYTHON_DIR)" && \
    fix-permissions /usr/local/bin/before-notebook.d


RUN HOME=/root pip install \
    'notebook==6.0.0' \
    'jupyterhub==1.0.0' \
    'jupyterlab==1.2.1'

USER $NB_UID
WORKDIR $HOME

# Setup work directory for backward-compatibility, and the .jupyter folder for custom settings.
RUN mkdir /home/$NB_USER/temp && \
    mkdir /home/$NB_USER/.jupyter

# Copy custom settings for frequent autosave in UI
COPY custom.js /home/$NB_USER/.jupyter

RUN fix-permissions /home/$NB_USER

EXPOSE 8888

# Configure container startup
ENTRYPOINT ["tini", "-g", "--"]
CMD ["start-singleuser.sh"]

So what runs inside is a vanilla 'notebook==6.0.0'. But the dialogue of "directory not empty" error gets displayed in both cases, with True/False of 'c.FileContentsManager.delete_to_trash' :(

[Dmitry1987]

I inspected the image "jupyter/base-notebook:8e8cbd0a0af7" and saw my versions of all components are a bit older but not much. Also the fact it works in the original image with that config file, gives me a place to start investigation why it doesn't work in my custom image :) .
That's very helpful, thanks!

[Dmitry1987]

I ended up patching the filemanager.py :) and it works great now. Just removed the send2trash and everything windows related, it solved the issue. Maybe a bad solution but couldn't find what else is different in my docker images, it was all same python content from "pip install notebook=6.0.3" 🤷

[Dmitry1987]

just in case someone wants to patch it same way:

    def delete_file(self, path):
        """Delete file at path."""
        path = path.strip('/')
        os_path = self._get_os_path(path)
        rm = os.unlink
        if not os.path.exists(os_path):
            raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path)

        if os.path.isdir(os_path):
            self.log.debug("Removing directory %s", os_path)
            with self.perm_to_403():
                shutil.rmtree(os_path)
        else:
            self.log.debug("Unlinking file %s", os_path)
            with self.perm_to_403():
                rm(os_path)

this will do it (UI confirmation box still exists, asks whether you want to delete the folder, seems good enough to me).

[albertmichaelj]

@Dmitry1987 I am in the process of updating my JupyterHub configuration, and I discovered that my fix that I gave you wasn't actually working when deployed. The docker container that I showed in #4916 (comment) works fine for deleting folders when I'm using docker on my local machine (a Mac), but when I deployed it to a kubernetes cluster (for JupyterHub on Kubernetes), I couldn't delete folders. For some reason, the config c.FileContentsManager.delete_to_trash = True was not being respected.

I ended up using the notebook argument --FileContentsManager.delete_to_trash = True at launch time. For the Jupyter Docker Stacks images, you can set this with the environmental variable NOTEBOOK_ARGS. When I did that it worked fine. I cannot figure out why their is a difference between whether or not that config gets set based on whether or not I'm launching the docker container locally or on a JupyterHub kubernetes based deployment, but there it is.

This might be an easier path forward for you instead of patching the notebook files.

[Dmitry1987]

the CLI flag is a good option, thanks for sharing 👍

[perllaghu]

@Dmitry1987 - I ended up writing this to get round the problem... https://github.com/edina/nb_empty_trash

Set c.FileContentsManager.delete_to_trash = True and install the extension.

seems to be working for me (docker images in a k8s cluster)

(but note, this is for classic notebook, not jupyterlab - but I'm sure someone could fork & adapt :) )

[Dmitry1987]

nice work @perllaghu , I hope it helps others who find this thread, our hacks and plugins :)))

[albertmichaelj]

@Dmitry1987 Just an FYI, I ended up using your patch technique because I ran into another problem with having symlinks in the home folder to other mount locations. Thanks for the patch!

[tlvu]

just in case someone wants to patch it same way:

    def delete_file(self, path):
        """Delete file at path."""
        path = path.strip('/')
        os_path = self._get_os_path(path)
        rm = os.unlink
        if not os.path.exists(os_path):
            raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path)

        if os.path.isdir(os_path):
            self.log.debug("Removing directory %s", os_path)
            with self.perm_to_403():
                shutil.rmtree(os_path)
        else:
            self.log.debug("Unlinking file %s", os_path)
            with self.perm_to_403():
                rm(os_path)

this will do it (UI confirmation box still exists, asks whether you want to delete the folder, seems good enough to me).

@Dmitry1987 Thanks for the patch, I am interested as well. This is the patch for which file?

[thakkarparth007]

Pinging on this issue again. Can someone please fix this? The original issue started in the Jupyter Lab repo 4 years ago (jupyterlab/jupyterlab#835)

[FernandezMathieu]

I got the same need here, i'll look if i can find something to fix that.
Meanwhile, anyone has an update on this issue ? Look like everyone dropped the deal.

[gauravcdac]

just in case someone wants to patch it same way:

    def delete_file(self, path):
        """Delete file at path."""
        path = path.strip('/')
        os_path = self._get_os_path(path)
        rm = os.unlink
        if not os.path.exists(os_path):
            raise web.HTTPError(404, u'File or directory does not exist: %s' % os_path)

        if os.path.isdir(os_path):
            self.log.debug("Removing directory %s", os_path)
            with self.perm_to_403():
                shutil.rmtree(os_path)
        else:
            self.log.debug("Unlinking file %s", os_path)
            with self.perm_to_403():
                rm(os_path)

this will do it (UI confirmation box still exists, asks whether you want to delete the folder, seems good enough to me).

@Dmitry1987 Thanks for the patch... Just want to know in which file we can add this patch...

[meeseeksmachine]

This issue has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterhub-not-allowing-to-delete-non-empty-directory/8539/1

[romeokienzler]

Any updates on this from anybody? Would it help to drop the undo - support and just delete the folder recursively after user confirmation? this solutions would be better that the current state where recursive deletion is impossible

[meeseeksmachine]

This issue has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterhub-singleuser-and-jupyter-server/10942/1

[ktaletsk]

It looks like the issue can now be solved by setting a new parameter in Jupyter Server: FileContentsManager.always_delete_dir = True (see reference)

Following the @albertmichaelj's example, I used a latest jupyter/base-notebook image with the above parameter and was able to delete the non-empty folders

ARG BASE_CONTAINER=jupyter/base-notebook:latest@sha256:9b8c1a9d7c8281704bbae9858793c5fd56dbb65c07812bd4411a6100d734dd3e
FROM $BASE_CONTAINER

RUN sed -i 's/c.FileContentsManager.delete_to_trash = False/c.FileContentsManager.always_delete_dir = True/g' /etc/jupyter/jupyter_server_config.py

Deletion in action:

[albertmichaelj]

@ktaletsk is right, as long as you are using the Server backend, which I think people should be doing now. So, I'm going to go ahead and close this issue.

[perllaghu]

Question: are notebooks still using os.rename to move the deleted contents to ~/.local/share/Trash/files/ ?

This solution if fine for most cases, however has a problem when one starts doing clever things with mounts... os.rename will not rename across file-system mounts (see https://stackoverflow.com/questions/26765130/how-can-i-move-files-between-mounts-using-python amongst others)

[albertmichaelj]

@perllaghu I think that it is using the send2trash python module (relevant code is here). I'm not sure what exactly that is using (I'm not as familiar with that code base), but from a quick glance, there does seem to be certain situations under which it uses os.rename (again relevant code is here).

[albertmichaelj]

Actually, it looks like this behavior may have been fixed in a pre-release beta of send2trash (relevant link is here).