Issue in fs.move.move_file overwrite logic in fs==2.4.16

[AleksMat]

Today's release of fs==2.4.16 introduced an code-breaking change that isn't really documented in the release notes and might be a bug.

Here is the code sample that works for fs<=2.4.15 but doesn't work for fs==2.4.16:

import fs.move
from fs.osfs import OSFS


source_fs = OSFS("source-folder")
target_fs = OSFS("target-folder")


FILENAME = "data.txt"
for filesystem in [source_fs, target_fs]:
    with filesystem.open(FILENAME, "w") as fp:
        fp.write("")

fs.move.move_file(source_fs, FILENAME, target_fs, FILENAME)

Basically, we have 2 different OSFS filesystem objects, a file in both, and we want to move a version of the file from filesystem one to another. For fs==2.4.16 this raises an overwrite error permission:

---------------------------------------------------------------------------
DestinationExists                         Traceback (most recent call last)
Input In [1], in <module>
     11     with filesystem.open(FILENAME, "w") as fp:
     12         fp.write("")
---> 14 fs.move.move_file(source_fs, FILENAME, target_fs, FILENAME)

File ~/.local/share/virtualenvs/xxx/lib/python3.8/site-packages/fs/move.py:85, in move_file(src_fs, src_path, dst_fs, dst_path, preserve_time, cleanup_dst_on_error)
     83         with _src_fs.lock(), _dst_fs.lock():
     84             with OSFS(common) as base:
---> 85                 base.move(rel_src, rel_dst, preserve_time=preserve_time)
     86                 return  # optimization worked, exit early
     87 except ValueError:
     88     # This is raised if we cannot find a common base folder.
     89     # In this case just fall through to the standard method.

File ~/.local/share/virtualenvs/xxx/lib/python3.8/site-packages/fs/base.py:1161, in FS.move(self, src_path, dst_path, overwrite, preserve_time)
   1140 """Move a file from ``src_path`` to ``dst_path``.
   1141 
   1142 Arguments:
   (...)
   1158 
   1159 """
   1160 if not overwrite and self.exists(dst_path):
-> 1161     raise errors.DestinationExists(dst_path)
   1162 if self.getinfo(src_path).is_dir:
   1163     raise errors.FileExpected(src_path)

DestinationExists: destination '/target-folder/data.txt' exists

I don't know if this change was intentional but there is at least an inconsistency with the following example that works for both fs<=2.4.15 and fs==2.4.16

import fs.move
from fs.osfs import OSFS


filesystem = OSFS(".")

FILENAME = "data.txt"
source_path = fs.path.join("source-folder", FILENAME)
target_path = fs.path.join("target-folder", FILENAME)

for path in [source_path, target_path]:
    with filesystem.open(path, "w") as fp:
        fp.write("")

fs.move.move_file(filesystem, source_path, filesystem, target_path)

In this example we have 1 filesystem object, a file in both folders, and we again copy the file from one folder to another.

This inconsistency comes down to the following lines of code:

• here overwrite flag is set to True,
• here it is not - the source of the problem and potentially a bug?

So I would suggest that:

• If this was an unintentional change then the missing flag overwrite=True is added.
• If this was an intentional change then an overwrite flag parameter is added to move_file function so that users can specify themselves which option they prefer. For my use case I would really like to specify overwrite=True so that I don't have to write additional checks.

Let me know if you would need any help with this issue.

[althonos]

Hi @AleksMat ,

thanks for the very prompt issue ticket. The change was likely introduced as a side-effect of #523. I think for backward compatibility we should at least add overwrite=True, but it may be worth thinking about whether adding a parameter to move_file would be worth it.

@tfeldmann, care to have a look?

[tfeldmann]

Sorry I'm on vacation and cannot get to this until next month

[althonos]

No problem, i'll have a look first if you can't 😉

[tfeldmann]

I submitted a PR for this issue.

I think having an overwrite argument in move_file would be nice. The only thing I don't like is that in FS.move(...) overwrite defaults to False and in move.move_file we would have to default to True for backwards compatibility reasons?

[lurch]

Yeah that's unfortunate, but I think that maintaining backwards-compatibility has to be the primary concern here? We can always attempt to make API-defaults more consistent when we move to PyFS v3?

[tfeldmann]

Is v3 already planned or in development somewhere?

[althonos]

We have a conversation in the Pyfilesystem maintainers organization listing some ideas and changes, but nothing has started yet, would be good to have a branch going at some point I agree.