From 4c0a826eec51fa2fac890c0e513cd1a7b1fcb65f Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Fri, 19 Aug 2022 11:32:56 +0200 Subject: [PATCH 1/5] Make `FS.move` and `FS.movedir` raise `IllegalDestination` when moved to themselves --- fs/base.py | 4 ++-- fs/test.py | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/fs/base.py b/fs/base.py index d42997d4..df748f21 100644 --- a/fs/base.py +++ b/fs/base.py @@ -1099,7 +1099,7 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): _src_path = self.validatepath(src_path) _dst_path = self.validatepath(dst_path) if _src_path == _dst_path: - return + raise errors.IllegalDestination(dst_path) if isbase(_src_path, _dst_path): raise errors.IllegalDestination(dst_path) if not create and not self.exists(dst_path): @@ -1179,7 +1179,7 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): raise errors.FileExpected(src_path) if _src_path == _dst_path: # early exit when moving a file onto itself - return + raise errors.IllegalDestination(dst_path) if self.getmeta().get("supports_rename", False): try: src_sys_path = self.getsyspath(_src_path) diff --git a/fs/test.py b/fs/test.py index 32e6ea5c..16b3b178 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1813,20 +1813,20 @@ def test_move_file_temp(self): def test_move_file_onto_itself(self): self.fs.writetext("file.txt", "Hello") - self.fs.move("file.txt", "file.txt", overwrite=True) - self.assert_text("file.txt", "Hello") - + with self.assertRaises(errors.IllegalDestination): + self.fs.move("file.txt", "file.txt", overwrite=True) with self.assertRaises(errors.DestinationExists): self.fs.move("file.txt", "file.txt", overwrite=False) + self.assert_text("file.txt", "Hello") def test_move_file_onto_itself_relpath(self): subdir = self.fs.makedir("sub") subdir.writetext("file.txt", "Hello") - self.fs.move("sub/file.txt", "sub/../sub/file.txt", overwrite=True) - self.assert_text("sub/file.txt", "Hello") - + with self.assertRaises(errors.IllegalDestination): + self.fs.move("sub/file.txt", "sub/../sub/file.txt", overwrite=True) with self.assertRaises(errors.DestinationExists): self.fs.move("sub/file.txt", "sub/../sub/file.txt", overwrite=False) + self.assert_text("sub/file.txt", "Hello") def test_copy_file_onto_itself(self): self.fs.writetext("file.txt", "Hello") @@ -1911,8 +1911,8 @@ def test_movedir_onto_itself(self): folder.writetext("file1.txt", "Hello1") sub = folder.makedir("sub") sub.writetext("file2.txt", "Hello2") - - self.fs.movedir("folder", "folder") + with self.assertRaises(errors.IllegalDestination): + self.fs.movedir("folder", "folder") self.assert_text("folder/file1.txt", "Hello1") self.assert_text("folder/sub/file2.txt", "Hello2") From 0db2f29e1461e7850246fb4eff3c7bea0df9d92f Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Fri, 19 Aug 2022 11:35:23 +0200 Subject: [PATCH 2/5] Document the expected behaviour of `FS.move` and `FS.movedir` for self-moves --- fs/base.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fs/base.py b/fs/base.py index df748f21..2246e4d7 100644 --- a/fs/base.py +++ b/fs/base.py @@ -1091,6 +1091,8 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): and ``create`` is `False`. fs.errors.DirectoryExpected: if ``src_path`` or one of its ancestors is not a directory. + fs.errors.IllegalDestination: when attempting to move the + folder at ``src_path`` to itself or one of its subfolders. """ from .move import move_dir @@ -1169,6 +1171,8 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): and ``overwrite`` is `False`. fs.errors.ResourceNotFound: If a parent directory of ``dst_path`` does not exist. + fs.errors.IllegalDestination: If ``src_path`` and ``dst_path`` + refer to the same resource, and ``overwrite`` is `True`. """ _src_path = self.validatepath(src_path) @@ -1179,7 +1183,7 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): raise errors.FileExpected(src_path) if _src_path == _dst_path: # early exit when moving a file onto itself - raise errors.IllegalDestination(dst_path) + raise errors.IllegalDestination(dst_path) if self.getmeta().get("supports_rename", False): try: src_sys_path = self.getsyspath(_src_path) From 37c2b01514bc2c2cb2f1fa2de973e405083705d7 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Fri, 19 Aug 2022 11:45:45 +0200 Subject: [PATCH 3/5] Update `MemoryFS` to raise `IllegalDestination` on self-moves --- fs/memoryfs.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fs/memoryfs.py b/fs/memoryfs.py index 0ca5ce16..37264116 100644 --- a/fs/memoryfs.py +++ b/fs/memoryfs.py @@ -465,8 +465,9 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): # handle moving a file onto itself if src_dir == dst_dir and src_name == dst_name: if overwrite: - return - raise errors.DestinationExists(dst_path) + raise errors.IllegalDestination(dst_path) + else: + raise errors.DestinationExists(dst_path) # move the entry from the src folder to the dst folder dst_dir_entry.set_entry(dst_name, src_entry) @@ -483,11 +484,8 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): dst_dir, dst_name = split(_dst_path) src_dir, src_name = split(_src_path) - # move a dir onto itself - if _src_path == _dst_path: - return - # move a dir into itself - if isbase(_src_path, _dst_path): + # move a dir onto or into itself + if _src_path == _dst_path or isbase(_src_path, _dst_path): raise errors.IllegalDestination(dst_path) with self._lock: From 54f643d12f06d05d99899dff1eb1b3935afe23b5 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Fri, 19 Aug 2022 12:41:04 +0200 Subject: [PATCH 4/5] Update `test_move.py` to ensure `IllegalDestination` is raised on self-moves --- tests/test_move.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_move.py b/tests/test_move.py index 8eb1af75..b2f4a389 100644 --- a/tests/test_move.py +++ b/tests/test_move.py @@ -11,7 +11,7 @@ import fs.move from fs import open_fs -from fs.errors import FSError, ResourceReadOnly +from fs.errors import FSError, ResourceReadOnly, IllegalDestination from fs.path import join from fs.wrap import read_only @@ -175,7 +175,8 @@ def test_move_file_overwrite_itself(self, _, fs_url): # behaves like the regular one (TempFS tests the optmized code path). with open_fs(fs_url) as tmp: tmp.writetext("file.txt", "content") - fs.move.move_file(tmp, "file.txt", tmp, "file.txt") + with self.assertRaises(IllegalDestination): + fs.move.move_file(tmp, "file.txt", tmp, "file.txt") self.assertTrue(tmp.exists("file.txt")) self.assertEquals(tmp.readtext("file.txt"), "content") @@ -186,7 +187,8 @@ def test_move_file_overwrite_itself_relpath(self, _, fs_url): with open_fs(fs_url) as tmp: new_dir = tmp.makedir("dir") new_dir.writetext("file.txt", "content") - fs.move.move_file(tmp, "dir/../dir/file.txt", tmp, "dir/file.txt") + with self.assertRaises(IllegalDestination): + fs.move.move_file(tmp, "dir/../dir/file.txt", tmp, "dir/file.txt") self.assertTrue(tmp.exists("dir/file.txt")) self.assertEquals(tmp.readtext("dir/file.txt"), "content") From ef508c28df0fd718170c3b9b1597d743eac952b8 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Fri, 19 Aug 2022 15:23:59 +0200 Subject: [PATCH 5/5] Cleanup redundant code in `FS.move` and `MemoryFS.move` --- fs/base.py | 2 -- fs/memoryfs.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/base.py b/fs/base.py index 2246e4d7..d222831f 100644 --- a/fs/base.py +++ b/fs/base.py @@ -1100,8 +1100,6 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): with self._lock: _src_path = self.validatepath(src_path) _dst_path = self.validatepath(dst_path) - if _src_path == _dst_path: - raise errors.IllegalDestination(dst_path) if isbase(_src_path, _dst_path): raise errors.IllegalDestination(dst_path) if not create and not self.exists(dst_path): diff --git a/fs/memoryfs.py b/fs/memoryfs.py index 37264116..462cc485 100644 --- a/fs/memoryfs.py +++ b/fs/memoryfs.py @@ -485,7 +485,7 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): src_dir, src_name = split(_src_path) # move a dir onto or into itself - if _src_path == _dst_path or isbase(_src_path, _dst_path): + if isbase(_src_path, _dst_path): raise errors.IllegalDestination(dst_path) with self._lock: