s3fs rm with recursive set as true doesn't work as one would expected

[rjhawar]

Using the s3fs rm with recursive set as true doesn't work as one would expected
When trying to delete any "directory" on s3 using the rm command the files instead the folder will be deleted, but the s3 directory will not be deleted.

Example:
using fs.rm(uri, recursive = True)
If I provide the s3 uri path root/dir_1/dir_2 --> only deletes the file and not the folders.
root/dir_1/dir_2/file_1 ---> only file_1 is deleted

Version Info:
Python: 3.11.1
s3fs = 2011.11.0

[martindurant]

S3 does not have folders. You probably have a file with the same name as the original "folder", which it is reasonable not t have deleted.

[rjhawar]

@martindurant here is a similar example for rm code.

import s3fs
def main(s3_uri):

    fs = s3fs.S3FileSystem(anon=False)

    # print(fs.info(s3_uri))
    fs.rm(s3_uri)

# -----------------------------------------------------------------------------
if __name__ == '__main__':
    s3_uri = "s3://bucket-test-gh/test1/"
    main(s3_uri)
    
# -----------------------------------------------------------------------------

The command executes, but when i check the s3 the file still exists

[martindurant]

@ianthomas23 , this could be seen as a follow-up to your work. The likely issue is the directory as a prefix versus a zero-length key.

• if a prefix only, deleting that exact path will cause an error
• if a zero key, the delete should be done, but right now rmdir is a no-op

So probably we should always attempt to delete directories as if they were keys, but allow it to fail silently. The exact same situation exists in gcsfs.

[ianthomas23]

@martindurant Yes, there are a few issues that are in the same areas of recursive actions with and without directories and files existing that I will work through.

[ianthomas23]

I think this is working as expected. If I add the following test

def test_issue_688(s3):
    dir = test_bucket_name + "/dir"
    file = dir + "/file"
    s3.touch(file)

    assert s3.exists(dir)
    assert s3.exists(file)

    s3.rm(dir, recursive=True)

    assert not s3.exists(dir)
    assert not s3.exists(file)

then it passes.

Without the recursive kwarg, i.e. s3.rm(dir) then it is a no-op, again as expected.

Unless I have misunderstood something here.

[ianthomas23]

I am going to close this as I don't think any action is required. Feel free to reopen if there is still an issue.

[dhirschfeld]

Without the recursive kwarg, i.e. s3.rm(dir) then it is a no-op, again as expected.

I'm poking about here investigating silent errors and this statement rings huge warning bells! 🚨

If you call rm without the recursive flag on a directory which isn't empty I would absolutely expect/want a loud error.

From the Zen of Python:

Errors should never pass silently.

When I call rm on a directory my expectation is that the directory is removed or an error is raised, not that the call silently does nothing and the directory which I've requested to be removed is still there.

[dhirschfeld]

xref: #838

[martindurant]

When I call rm on a directory my expectation is that the directory is removed or an error is raised

Agreed, rm on an existing (non-empty) directory should raise IsADirectoryError.