From ec7c68d19cfee4bb037302911538989f5e9b08d5 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 11:26:29 +0200 Subject: [PATCH 01/22] add failing tests --- fs/test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/fs/test.py b/fs/test.py index 232666da..c15658ec 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1811,6 +1811,28 @@ def test_move_file_mem(self): def test_move_file_temp(self): self._test_move_file("temp://") + 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") + + 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") + + def test_copy_file_onto_itself(self): + self.fs.writetext("file.txt", "Hello") + self.fs.copy("file.txt", "file.txt", overwrite=True) + self.assert_text("file.txt", "Hello") + + def test_copy_file_onto_itself_relpath(self): + subdir = self.fs.makedir("sub") + subdir.writetext("file.txt", "Hello") + self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=True) + self.assert_text("sub/file.txt", "Hello") + def test_copydir(self): self.fs.makedirs("foo/bar/baz/egg") self.fs.writetext("foo/bar/foofoo.txt", "Hello") From 56bed8364013ede9d10035ab7def5d35898163da Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 12:42:36 +0200 Subject: [PATCH 02/22] fix OSFS copy --- fs/osfs.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/osfs.py b/fs/osfs.py index 0add3d97..dc4e852f 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -409,7 +409,7 @@ def _check_copy(self, src_path, dst_path, overwrite=False): if self.gettype(src_path) is not ResourceType.file: raise errors.FileExpected(src_path) # check dst_path does not exist if we are not overwriting - if not overwrite and self.exists(_dst_path): + if not overwrite and _src_path != _dst_path and self.exists(_dst_path): raise errors.DestinationExists(dst_path) # check parent dir of _dst_path exists and is a directory if self.gettype(dirname(dst_path)) is not ResourceType.directory: @@ -440,6 +440,9 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): self.getsyspath(_src_path), self.getsyspath(_dst_path), ) + # exit early if we copy the file onto itself + if overwrite and _src_sys == _dst_sys: + return # attempt using sendfile try: # initialise variables to pass to sendfile @@ -467,7 +470,14 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None with self._lock: _src_path, _dst_path = self._check_copy(src_path, dst_path, overwrite) - shutil.copy2(self.getsyspath(_src_path), self.getsyspath(_dst_path)) + _src_sys, _dst_sys = ( + self.getsyspath(_src_path), + self.getsyspath(_dst_path), + ) + # exit early if we copy the file onto itself + if overwrite and _src_sys == _dst_sys: + return + shutil.copy2(_src_sys, _dst_sys) # --- Backport of os.scandir for Python < 3.5 ------------ From 0e3ed7214234f8fc7caca4bcdc13db7e78d231b2 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 12:53:05 +0200 Subject: [PATCH 03/22] assert DestinationExists when using overwrite=False --- fs/base.py | 3 +++ fs/osfs.py | 2 +- fs/test.py | 12 ++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/fs/base.py b/fs/base.py index 56e5cf99..5d100676 100644 --- a/fs/base.py +++ b/fs/base.py @@ -1161,6 +1161,9 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): raise errors.DestinationExists(dst_path) if self.getinfo(src_path).is_dir: raise errors.FileExpected(src_path) + if normpath(src_path) == normpath(dst_path): + # early exit when moving a file onto itself + return if self.getmeta().get("supports_rename", False): try: src_sys_path = self.getsyspath(src_path) diff --git a/fs/osfs.py b/fs/osfs.py index dc4e852f..ed6b0953 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -409,7 +409,7 @@ def _check_copy(self, src_path, dst_path, overwrite=False): if self.gettype(src_path) is not ResourceType.file: raise errors.FileExpected(src_path) # check dst_path does not exist if we are not overwriting - if not overwrite and _src_path != _dst_path and self.exists(_dst_path): + if not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) # check parent dir of _dst_path exists and is a directory if self.gettype(dirname(dst_path)) is not ResourceType.directory: diff --git a/fs/test.py b/fs/test.py index c15658ec..108239b9 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1816,23 +1816,35 @@ def test_move_file_onto_itself(self): self.fs.move("file.txt", "file.txt", overwrite=True) self.assert_text("file.txt", "Hello") + with self.assertRaises(errors.DestinationExists): + self.fs.move("file.txt", "file.txt", overwrite=False) + 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.DestinationExists): + self.fs.move("sub/file.txt", "sub/../sub/file.txt", overwrite=False) + def test_copy_file_onto_itself(self): self.fs.writetext("file.txt", "Hello") self.fs.copy("file.txt", "file.txt", overwrite=True) self.assert_text("file.txt", "Hello") + with self.assertRaises(errors.DestinationExists): + self.fs.copy("file.txt", "file.txt", overwrite=False) + def test_copy_file_onto_itself_relpath(self): subdir = self.fs.makedir("sub") subdir.writetext("file.txt", "Hello") self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=True) self.assert_text("sub/file.txt", "Hello") + with self.assertRaises(errors.DestinationExists): + self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=False) + def test_copydir(self): self.fs.makedirs("foo/bar/baz/egg") self.fs.writetext("foo/bar/foofoo.txt", "Hello") From b3d2d733302c08bc81771959268a0bc0a436a805 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 13:08:37 +0200 Subject: [PATCH 04/22] fix memoryfs --- fs/base.py | 13 +++++++++---- fs/memoryfs.py | 6 ++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/fs/base.py b/fs/base.py index 5d100676..6ef1b5bd 100644 --- a/fs/base.py +++ b/fs/base.py @@ -423,13 +423,18 @@ def copy( """ with self._lock: - if not overwrite and self.exists(dst_path): + _src_path = self.validatepath(src_path) + _dst_path = self.validatepath(dst_path) + if not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) - with closing(self.open(src_path, "rb")) as read_file: + if overwrite and _src_path == _dst_path: + # exit early when copying a file onto itself + return + with closing(self.open(_src_path, "rb")) as read_file: # FIXME(@althonos): typing complains because open return IO - self.upload(dst_path, read_file) # type: ignore + self.upload(_dst_path, read_file) # type: ignore if preserve_time: - copy_modified_time(self, src_path, self, dst_path) + copy_modified_time(self, _src_path, self, _dst_path) def copydir( self, diff --git a/fs/memoryfs.py b/fs/memoryfs.py index 8efdc139..8c428d35 100644 --- a/fs/memoryfs.py +++ b/fs/memoryfs.py @@ -462,6 +462,12 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): elif not overwrite and dst_name in dst_dir_entry: raise errors.DestinationExists(dst_path) + # handle moving a file onto itself + if src_dir == dst_dir and src_name == dst_name: + if overwrite: + return + 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) src_dir_entry.remove_entry(src_name) From f1a7bfb85cc9bd75f68078591990b6f7ca9e9882 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 13:17:50 +0200 Subject: [PATCH 05/22] update changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 837f4947..4ffe9392 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Elaborated documentation of `filter_dirs` and `exclude_dirs` in `fs.walk.Walker`. Closes [#371](https://github.com/PyFilesystem/pyfilesystem2/issues/371). - +- Fixed a bug where files could be truncated or deleted when moved / copied onto itself. + Closes [#546](https://github.com/PyFilesystem/pyfilesystem2/issues/546) ## [2.4.16] - 2022-05-02 From cec3e06611feda7ee40cc6dce8544f43ec0f01a4 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 17:13:37 +0200 Subject: [PATCH 06/22] add movedir tests --- fs/errors.py | 7 +++++++ fs/test.py | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/fs/errors.py b/fs/errors.py index 32c795d9..d986f179 100644 --- a/fs/errors.py +++ b/fs/errors.py @@ -34,6 +34,7 @@ "IllegalBackReference", "InsufficientStorage", "InvalidCharsInPath", + "InvalidMoveOperation", "InvalidPath", "MissingInfoNamespace", "NoSysPath", @@ -303,6 +304,12 @@ class DirectoryNotEmpty(ResourceError): default_message = "directory '{path}' is not empty" +class InvalidMoveOperation(ResourceError): + """Attempt to move a folder into its own subfolder.""" + + default_message = "you cannot move '{path}' into its own subfolder" + + class ResourceLocked(ResourceError): """Attempt to use a locked resource.""" diff --git a/fs/test.py b/fs/test.py index 108239b9..a4e27d9c 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1885,6 +1885,25 @@ def test_movedir(self): with self.assertRaises(errors.DirectoryExpected): self.fs.movedir("foo2/foofoo.txt", "foo2/baz/egg") + def test_movedir_onto_itself(self): + folder = self.fs.makedir("folder") + folder.writetext("file1.txt", "Hello1") + sub = folder.makedir("sub") + sub.writetext("file2.txt", "Hello2") + + self.fs.movedir("folder", "folder") + self.assert_text("folder/file1.txt", "Hello1") + self.assert_text("folder/sub/file2.txt", "Hello2") + + def test_movedir_into_its_own_subfolder(self): + folder = self.fs.makedir("folder") + folder.writetext("file1.txt", "Hello1") + sub = folder.makedir("sub") + sub.writetext("file2.txt", "Hello2") + + with self.assertRaises(errors.InvalidMoveOperation): + self.fs.movedir("folder", "folder/sub/") + def test_match(self): self.assertTrue(self.fs.match(["*.py"], "foo.py")) self.assertEqual( From 2b0cbac410faf482095db83f5e86aa977dc79ae3 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 17:20:28 +0200 Subject: [PATCH 07/22] add copydir tests --- fs/test.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/fs/test.py b/fs/test.py index a4e27d9c..870e4171 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1862,6 +1862,29 @@ def test_copydir(self): with self.assertRaises(errors.DirectoryExpected): self.fs.copydir("foo2/foofoo.txt", "foofoo.txt", create=True) + def test_copydir_onto_itself(self): + folder = self.fs.makedir("folder") + folder.writetext("file1.txt", "Hello1") + sub = folder.makedir("sub") + sub.writetext("file2.txt", "Hello2") + + self.fs.copydir("folder", "folder") + self.assert_text("folder/file1.txt", "Hello1") + self.assert_text("folder/sub/file2.txt", "Hello2") + + def test_copydir_into_its_own_subfolder(self): + # TODO: This test hangs forever at the moment. + # + # folder = self.fs.makedir("folder") + # folder.writetext("file1.txt", "Hello1") + # sub = folder.makedir("sub") + # sub.writetext("file2.txt", "Hello2") + # self.fs.copydir("folder", "folder/sub/") + # self.assert_text("folder/file1.txt", "Hello1") + # self.assert_text("folder/sub/file1.txt", "Hello1") + # self.assert_not_exists("folder/sub/file2.txt") + pass + def test_movedir(self): self.fs.makedirs("foo/bar/baz/egg") self.fs.writetext("foo/bar/foofoo.txt", "Hello") From 9a7fe1ad21b92d7cfa9d83c9457e0159d129ab28 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 22 Jul 2022 17:34:44 +0200 Subject: [PATCH 08/22] new error names --- fs/errors.py | 18 ++++++++++++------ fs/test.py | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/fs/errors.py b/fs/errors.py index d986f179..56c392db 100644 --- a/fs/errors.py +++ b/fs/errors.py @@ -241,6 +241,18 @@ class RemoveRootError(OperationFailed): default_message = "root directory may not be removed" +class IllegalMoveDestination(OperationFailed): + """Attempt to move a folder into its own subfolder.""" + + default_message = "'{path}' cannot be moved into itself" + + +class IllegalCopyDestination(OperationFailed): + """Attempt to copy a folder into its own subfolder.""" + + default_message = "'{path}' cannot be copied into itself" + + class ResourceError(FSError): """Base exception class for error associated with a specific resource.""" @@ -304,12 +316,6 @@ class DirectoryNotEmpty(ResourceError): default_message = "directory '{path}' is not empty" -class InvalidMoveOperation(ResourceError): - """Attempt to move a folder into its own subfolder.""" - - default_message = "you cannot move '{path}' into its own subfolder" - - class ResourceLocked(ResourceError): """Attempt to use a locked resource.""" diff --git a/fs/test.py b/fs/test.py index 870e4171..b68396d6 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1924,7 +1924,7 @@ def test_movedir_into_its_own_subfolder(self): sub = folder.makedir("sub") sub.writetext("file2.txt", "Hello2") - with self.assertRaises(errors.InvalidMoveOperation): + with self.assertRaises(errors.IllegalMoveDestination): self.fs.movedir("folder", "folder/sub/") def test_match(self): From 2a9330456cbafb2cbf74bd62f15f7d5939f2b156 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:05:48 +0200 Subject: [PATCH 09/22] establish new errors.IllegalDestination --- fs/base.py | 15 ++++++++++----- fs/errors.py | 16 +++++++--------- fs/memoryfs.py | 15 ++++++++++++--- fs/test.py | 29 +++++++++++++---------------- 4 files changed, 42 insertions(+), 33 deletions(-) diff --git a/fs/base.py b/fs/base.py index 6ef1b5bd..a1cfdef0 100644 --- a/fs/base.py +++ b/fs/base.py @@ -21,11 +21,11 @@ from contextlib import closing from functools import partial, wraps -from . import copy, errors, fsencode, iotools, tools, walk, wildcard, glob +from . import copy, errors, fsencode, glob, iotools, tools, walk, wildcard from .copy import copy_modified_time from .glob import BoundGlobber from .mode import validate_open_mode -from .path import abspath, join, normpath +from .path import abspath, isbase, join, normpath from .time import datetime_to_epoch from .walk import Walker @@ -425,11 +425,10 @@ def copy( 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 not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) - if overwrite and _src_path == _dst_path: - # exit early when copying a file onto itself - return with closing(self.open(_src_path, "rb")) as read_file: # FIXME(@althonos): typing complains because open return IO self.upload(_dst_path, read_file) # type: ignore @@ -1093,6 +1092,12 @@ def movedir(self, src_path, dst_path, create=False, preserve_time=False): from .move import move_dir with self._lock: + _src_path = self.validatepath(src_path) + _dst_path = self.validatepath(dst_path) + if _src_path == _dst_path: + return + if isbase(_src_path, _dst_path): + raise errors.IllegalDestination(dst_path) if not create and not self.exists(dst_path): raise errors.ResourceNotFound(dst_path) move_dir(self, src_path, self, dst_path, preserve_time=preserve_time) diff --git a/fs/errors.py b/fs/errors.py index 56c392db..adc9afa9 100644 --- a/fs/errors.py +++ b/fs/errors.py @@ -32,9 +32,9 @@ "FilesystemClosed", "FSError", "IllegalBackReference", + "IllegalDestination", "InsufficientStorage", "InvalidCharsInPath", - "InvalidMoveOperation", "InvalidPath", "MissingInfoNamespace", "NoSysPath", @@ -241,16 +241,14 @@ class RemoveRootError(OperationFailed): default_message = "root directory may not be removed" -class IllegalMoveDestination(OperationFailed): - """Attempt to move a folder into its own subfolder.""" +class IllegalDestination(OperationFailed): + """The given destination cannot be used for the operation. - default_message = "'{path}' cannot be moved into itself" - - -class IllegalCopyDestination(OperationFailed): - """Attempt to copy a folder into its own subfolder.""" + This error will occur when attempting to move / copy a folder into itself or copying + a file onto itself. + """ - default_message = "'{path}' cannot be copied into itself" + default_message = "'{path}' is not a legal destination" class ResourceError(FSError): diff --git a/fs/memoryfs.py b/fs/memoryfs.py index 8c428d35..0ca5ce16 100644 --- a/fs/memoryfs.py +++ b/fs/memoryfs.py @@ -19,7 +19,7 @@ from .enums import ResourceType, Seek from .info import Info from .mode import Mode -from .path import iteratepath, normpath, split +from .path import isbase, iteratepath, normpath, split if typing.TYPE_CHECKING: from typing import ( @@ -478,8 +478,17 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): copy_modified_time(self, src_path, self, dst_path) def movedir(self, src_path, dst_path, create=False, preserve_time=False): - src_dir, src_name = split(self.validatepath(src_path)) - dst_dir, dst_name = split(self.validatepath(dst_path)) + _src_path = self.validatepath(src_path) + _dst_path = self.validatepath(dst_path) + 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): + raise errors.IllegalDestination(dst_path) with self._lock: src_dir_entry = self._get_dir_entry(src_dir) diff --git a/fs/test.py b/fs/test.py index b68396d6..715e6d20 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1830,20 +1830,20 @@ def test_move_file_onto_itself_relpath(self): def test_copy_file_onto_itself(self): self.fs.writetext("file.txt", "Hello") - self.fs.copy("file.txt", "file.txt", overwrite=True) - self.assert_text("file.txt", "Hello") - - with self.assertRaises(errors.DestinationExists): + with self.assertRaises(errors.IllegalDestination): + self.fs.copy("file.txt", "file.txt", overwrite=True) + with self.assertRaises(errors.IllegalDestination): self.fs.copy("file.txt", "file.txt", overwrite=False) + self.assert_text("file.txt", "Hello") def test_copy_file_onto_itself_relpath(self): subdir = self.fs.makedir("sub") subdir.writetext("file.txt", "Hello") - self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=True) - self.assert_text("sub/file.txt", "Hello") - - with self.assertRaises(errors.DestinationExists): + with self.assertRaises(errors.IllegalDestination): + self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=True) + with self.assertRaises(errors.IllegalDestination): self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=False) + self.assert_text("sub/file.txt", "Hello") def test_copydir(self): self.fs.makedirs("foo/bar/baz/egg") @@ -1868,21 +1868,18 @@ def test_copydir_onto_itself(self): sub = folder.makedir("sub") sub.writetext("file2.txt", "Hello2") - self.fs.copydir("folder", "folder") + with self.assertRaises(errors.IllegalDestination): + self.fs.copydir("folder", "folder") self.assert_text("folder/file1.txt", "Hello1") self.assert_text("folder/sub/file2.txt", "Hello2") def test_copydir_into_its_own_subfolder(self): - # TODO: This test hangs forever at the moment. - # # folder = self.fs.makedir("folder") # folder.writetext("file1.txt", "Hello1") # sub = folder.makedir("sub") # sub.writetext("file2.txt", "Hello2") - # self.fs.copydir("folder", "folder/sub/") - # self.assert_text("folder/file1.txt", "Hello1") - # self.assert_text("folder/sub/file1.txt", "Hello1") - # self.assert_not_exists("folder/sub/file2.txt") + # with self.assertRaises(errors.IllegalDestination): + # self.fs.copydir("folder", "folder/sub/") pass def test_movedir(self): @@ -1924,7 +1921,7 @@ def test_movedir_into_its_own_subfolder(self): sub = folder.makedir("sub") sub.writetext("file2.txt", "Hello2") - with self.assertRaises(errors.IllegalMoveDestination): + with self.assertRaises(errors.IllegalDestination): self.fs.movedir("folder", "folder/sub/") def test_match(self): From 0f097317b763b6deb975c506a21a69de7bd0a538 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:10:25 +0200 Subject: [PATCH 10/22] fix osfs --- fs/osfs.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/fs/osfs.py b/fs/osfs.py index ed6b0953..e56bde03 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -408,6 +408,9 @@ def _check_copy(self, src_path, dst_path, overwrite=False): # check src_path exists and is a file if self.gettype(src_path) is not ResourceType.file: raise errors.FileExpected(src_path) + # it's not allowed to copy a file onto itself + if _src_path == _dst_path: + raise errors.IllegalDestination(dst_path) # check dst_path does not exist if we are not overwriting if not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) @@ -440,9 +443,6 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): self.getsyspath(_src_path), self.getsyspath(_dst_path), ) - # exit early if we copy the file onto itself - if overwrite and _src_sys == _dst_sys: - return # attempt using sendfile try: # initialise variables to pass to sendfile @@ -474,9 +474,6 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): self.getsyspath(_src_path), self.getsyspath(_dst_path), ) - # exit early if we copy the file onto itself - if overwrite and _src_sys == _dst_sys: - return shutil.copy2(_src_sys, _dst_sys) # --- Backport of os.scandir for Python < 3.5 ------------ From 2e0238af8104d3b10d1e6a0abf5d1fff3583c4a7 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:12:23 +0200 Subject: [PATCH 11/22] fix copydir hangup --- fs/base.py | 4 ++++ fs/test.py | 13 ++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/fs/base.py b/fs/base.py index a1cfdef0..a794bf48 100644 --- a/fs/base.py +++ b/fs/base.py @@ -461,6 +461,10 @@ def copydir( """ with self._lock: + _src_path = self.validatepath(src_path) + _dst_path = self.validatepath(dst_path) + if isbase(_src_path, _dst_path): + raise errors.IllegalDestination(dst_path) if not create and not self.exists(dst_path): raise errors.ResourceNotFound(dst_path) if not self.getinfo(src_path).is_dir: diff --git a/fs/test.py b/fs/test.py index 715e6d20..2b5bd343 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1874,13 +1874,12 @@ def test_copydir_onto_itself(self): self.assert_text("folder/sub/file2.txt", "Hello2") def test_copydir_into_its_own_subfolder(self): - # folder = self.fs.makedir("folder") - # folder.writetext("file1.txt", "Hello1") - # sub = folder.makedir("sub") - # sub.writetext("file2.txt", "Hello2") - # with self.assertRaises(errors.IllegalDestination): - # self.fs.copydir("folder", "folder/sub/") - pass + folder = self.fs.makedir("folder") + folder.writetext("file1.txt", "Hello1") + sub = folder.makedir("sub") + sub.writetext("file2.txt", "Hello2") + with self.assertRaises(errors.IllegalDestination): + self.fs.copydir("folder", "folder/sub/") def test_movedir(self): self.fs.makedirs("foo/bar/baz/egg") From e157d936cdd947c9b2508851508a436177a8a41d Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:16:10 +0200 Subject: [PATCH 12/22] uses validated paths in copydir --- fs/base.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/base.py b/fs/base.py index a794bf48..217cbb22 100644 --- a/fs/base.py +++ b/fs/base.py @@ -465,11 +465,11 @@ def copydir( _dst_path = self.validatepath(dst_path) if isbase(_src_path, _dst_path): raise errors.IllegalDestination(dst_path) - if not create and not self.exists(dst_path): + if not create and not self.exists(_dst_path): raise errors.ResourceNotFound(dst_path) - if not self.getinfo(src_path).is_dir: + if not self.getinfo(_src_path).is_dir: raise errors.DirectoryExpected(src_path) - copy.copy_dir(self, src_path, self, dst_path, preserve_time=preserve_time) + copy.copy_dir(self, _src_path, self, _dst_path, preserve_time=preserve_time) def create(self, path, wipe=False): # type: (Text, bool) -> bool From a039bb687519f1090bc19dd4b5e80c35891d53c1 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:16:36 +0200 Subject: [PATCH 13/22] assert no files were touched --- fs/test.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/test.py b/fs/test.py index 2b5bd343..154eb32e 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1880,6 +1880,8 @@ def test_copydir_into_its_own_subfolder(self): sub.writetext("file2.txt", "Hello2") with self.assertRaises(errors.IllegalDestination): self.fs.copydir("folder", "folder/sub/") + self.assert_text("folder/file1.txt", "Hello1") + self.assert_text("folder/sub/file2.txt", "Hello2") def test_movedir(self): self.fs.makedirs("foo/bar/baz/egg") @@ -1922,6 +1924,8 @@ def test_movedir_into_its_own_subfolder(self): with self.assertRaises(errors.IllegalDestination): self.fs.movedir("folder", "folder/sub/") + self.assert_text("folder/file1.txt", "Hello1") + self.assert_text("folder/sub/file2.txt", "Hello2") def test_match(self): self.assertTrue(self.fs.match(["*.py"], "foo.py")) From f9cf49f03260322fb8672ee2d99be5d7e7862cbd Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Mon, 25 Jul 2022 11:43:19 +0200 Subject: [PATCH 14/22] fix wrapfs --- fs/wrapfs.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/wrapfs.py b/fs/wrapfs.py index abbbe4e3..2fbb7050 100644 --- a/fs/wrapfs.py +++ b/fs/wrapfs.py @@ -12,7 +12,7 @@ from .copy import copy_dir, copy_file from .error_tools import unwrap_errors from .info import Info -from .path import abspath, join, normpath +from .path import abspath, isbase, join, normpath if typing.TYPE_CHECKING: from typing import ( @@ -267,7 +267,11 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None src_fs, _src_path = self.delegate_path(src_path) dst_fs, _dst_path = self.delegate_path(dst_path) + _val_src_path = self.validatepath(_src_path) + _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): + if src_fs == dst_fs and _val_src_path == _val_dst_path: + raise errors.IllegalDestination(_dst_path) if not overwrite and dst_fs.exists(_dst_path): raise errors.DestinationExists(_dst_path) copy_file(src_fs, _src_path, dst_fs, _dst_path, preserve_time=preserve_time) @@ -276,7 +280,11 @@ def copydir(self, src_path, dst_path, create=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None src_fs, _src_path = self.delegate_path(src_path) dst_fs, _dst_path = self.delegate_path(dst_path) + _val_src_path = self.validatepath(_src_path) + _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): + if src_fs == dst_fs and isbase(_val_src_path, _val_dst_path): + raise errors.IllegalDestination(_dst_path) if not create and not dst_fs.exists(_dst_path): raise errors.ResourceNotFound(dst_path) if not src_fs.getinfo(_src_path).is_dir: From b4f2dc430efaebc1cb4773887bd0216cf5f04b3c Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 10:41:27 +0200 Subject: [PATCH 15/22] wip copy --- fs/copy.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/copy.py b/fs/copy.py index 90848c76..ae3acd8b 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -7,9 +7,9 @@ import warnings -from .errors import ResourceNotFound +from .errors import IllegalDestination, ResourceNotFound from .opener import manage_fs -from .path import abspath, combine, frombase, normpath +from .path import abspath, combine, frombase, isbase, normpath from .tools import is_thread_safe from .walk import Walker @@ -438,6 +438,8 @@ def copy_dir_if( dst_fs, create=True ) as _dst_fs: with _src_fs.lock(), _dst_fs.lock(): + if src_fs == dst_fs and isbase(_src_path, _dst_path): + raise IllegalDestination(dst_path) _thread_safe = is_thread_safe(_src_fs, _dst_fs) with Copier( num_workers=workers if _thread_safe else 0, preserve_time=preserve_time From 2668bfcb3141e2923a3be60c35c8bc70785f41c7 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 11:20:45 +0200 Subject: [PATCH 16/22] raise DestinationExists if overwrite is not set --- fs/base.py | 4 ++-- fs/copy.py | 10 ++++++---- fs/osfs.py | 6 +++--- fs/test.py | 4 ++-- fs/wrapfs.py | 2 -- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/fs/base.py b/fs/base.py index 217cbb22..189d45ab 100644 --- a/fs/base.py +++ b/fs/base.py @@ -425,10 +425,10 @@ def copy( 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 not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) + if _src_path == _dst_path: + raise errors.IllegalDestination(dst_path) with closing(self.open(_src_path, "rb")) as read_file: # FIXME(@althonos): typing complains because open return IO self.upload(_dst_path, read_file) # type: ignore diff --git a/fs/copy.py b/fs/copy.py index ae3acd8b..334c7ea9 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -257,9 +257,13 @@ def copy_file_internal( lock (bool): Lock both filesystems before copying. """ + _src_path = src_fs.validatepath(src_path) + _dst_path = dst_fs.validatepath(dst_path) if src_fs is dst_fs: - # Same filesystem, so we can do a potentially optimized - # copy + # It's not allowed to copy a file onto itself + if _src_path == _dst_path: + raise IllegalDestination(dst_path) + # Same filesystem, so we can do a potentially optimized copy src_fs.copy(src_path, dst_path, overwrite=True, preserve_time=preserve_time) return @@ -438,8 +442,6 @@ def copy_dir_if( dst_fs, create=True ) as _dst_fs: with _src_fs.lock(), _dst_fs.lock(): - if src_fs == dst_fs and isbase(_src_path, _dst_path): - raise IllegalDestination(dst_path) _thread_safe = is_thread_safe(_src_fs, _dst_fs) with Copier( num_workers=workers if _thread_safe else 0, preserve_time=preserve_time diff --git a/fs/osfs.py b/fs/osfs.py index e56bde03..94d16844 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -408,12 +408,12 @@ def _check_copy(self, src_path, dst_path, overwrite=False): # check src_path exists and is a file if self.gettype(src_path) is not ResourceType.file: raise errors.FileExpected(src_path) - # it's not allowed to copy a file onto itself - if _src_path == _dst_path: - raise errors.IllegalDestination(dst_path) # check dst_path does not exist if we are not overwriting if not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) + # it's not allowed to copy a file onto itself + if _src_path == _dst_path: + raise errors.IllegalDestination(dst_path) # check parent dir of _dst_path exists and is a directory if self.gettype(dirname(dst_path)) is not ResourceType.directory: raise errors.DirectoryExpected(dirname(dst_path)) diff --git a/fs/test.py b/fs/test.py index 154eb32e..32e6ea5c 100644 --- a/fs/test.py +++ b/fs/test.py @@ -1832,7 +1832,7 @@ def test_copy_file_onto_itself(self): self.fs.writetext("file.txt", "Hello") with self.assertRaises(errors.IllegalDestination): self.fs.copy("file.txt", "file.txt", overwrite=True) - with self.assertRaises(errors.IllegalDestination): + with self.assertRaises(errors.DestinationExists): self.fs.copy("file.txt", "file.txt", overwrite=False) self.assert_text("file.txt", "Hello") @@ -1841,7 +1841,7 @@ def test_copy_file_onto_itself_relpath(self): subdir.writetext("file.txt", "Hello") with self.assertRaises(errors.IllegalDestination): self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=True) - with self.assertRaises(errors.IllegalDestination): + with self.assertRaises(errors.DestinationExists): self.fs.copy("sub/file.txt", "sub/../sub/file.txt", overwrite=False) self.assert_text("sub/file.txt", "Hello") diff --git a/fs/wrapfs.py b/fs/wrapfs.py index 2fbb7050..9e382031 100644 --- a/fs/wrapfs.py +++ b/fs/wrapfs.py @@ -270,8 +270,6 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): _val_src_path = self.validatepath(_src_path) _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): - if src_fs == dst_fs and _val_src_path == _val_dst_path: - raise errors.IllegalDestination(_dst_path) if not overwrite and dst_fs.exists(_dst_path): raise errors.DestinationExists(_dst_path) copy_file(src_fs, _src_path, dst_fs, _dst_path, preserve_time=preserve_time) From 6ef99f672a1c22d950ead87f837651f2002681b8 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 11:29:00 +0200 Subject: [PATCH 17/22] wrapfs does not need checks sprinkled everywhere anymore --- fs/copy.py | 11 ++++++++--- fs/wrapfs.py | 2 -- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fs/copy.py b/fs/copy.py index 334c7ea9..e70aafa6 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -306,14 +306,19 @@ def copy_structure( dst_root (str): Path to the target root of the tree structure. """ + _src_root = abspath(normpath(src_root)) + _dst_root = abspath(normpath(dst_root)) + # It's not allowed to copy a structure into itself + if src_fs == dst_fs and isbase(_src_root, _dst_root): + raise IllegalDestination(dst_root) walker = walker or Walker() with manage_fs(src_fs) as _src_fs: with manage_fs(dst_fs, create=True) as _dst_fs: with _src_fs.lock(), _dst_fs.lock(): - _dst_fs.makedirs(dst_root, recreate=True) - for dir_path in walker.dirs(_src_fs, src_root): + _dst_fs.makedirs(_dst_root, recreate=True) + for dir_path in walker.dirs(_src_fs, _src_root): _dst_fs.makedir( - combine(dst_root, frombase(src_root, dir_path)), recreate=True + combine(_dst_root, frombase(_src_root, dir_path)), recreate=True ) diff --git a/fs/wrapfs.py b/fs/wrapfs.py index 9e382031..e7f8f848 100644 --- a/fs/wrapfs.py +++ b/fs/wrapfs.py @@ -281,8 +281,6 @@ def copydir(self, src_path, dst_path, create=False, preserve_time=False): _val_src_path = self.validatepath(_src_path) _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): - if src_fs == dst_fs and isbase(_val_src_path, _val_dst_path): - raise errors.IllegalDestination(_dst_path) if not create and not dst_fs.exists(_dst_path): raise errors.ResourceNotFound(dst_path) if not src_fs.getinfo(_src_path).is_dir: From c703c99fe2ff1ac2ab98a8936d8535899eb1fc57 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 11:30:27 +0200 Subject: [PATCH 18/22] remove unneeded code --- fs/wrapfs.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/fs/wrapfs.py b/fs/wrapfs.py index e7f8f848..2d166bc7 100644 --- a/fs/wrapfs.py +++ b/fs/wrapfs.py @@ -267,8 +267,6 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None src_fs, _src_path = self.delegate_path(src_path) dst_fs, _dst_path = self.delegate_path(dst_path) - _val_src_path = self.validatepath(_src_path) - _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): if not overwrite and dst_fs.exists(_dst_path): raise errors.DestinationExists(_dst_path) @@ -278,8 +276,6 @@ def copydir(self, src_path, dst_path, create=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None src_fs, _src_path = self.delegate_path(src_path) dst_fs, _dst_path = self.delegate_path(dst_path) - _val_src_path = self.validatepath(_src_path) - _val_dst_path = self.validatepath(_dst_path) with unwrap_errors({_src_path: src_path, _dst_path: dst_path}): if not create and not dst_fs.exists(_dst_path): raise errors.ResourceNotFound(dst_path) From 300fe893a8a0cce445af6aee3fdf4a51805f16e1 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 11:35:49 +0200 Subject: [PATCH 19/22] remove unused import --- fs/wrapfs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/wrapfs.py b/fs/wrapfs.py index 2d166bc7..abbbe4e3 100644 --- a/fs/wrapfs.py +++ b/fs/wrapfs.py @@ -12,7 +12,7 @@ from .copy import copy_dir, copy_file from .error_tools import unwrap_errors from .info import Info -from .path import abspath, isbase, join, normpath +from .path import abspath, join, normpath if typing.TYPE_CHECKING: from typing import ( From 9d9dcd675ba15821b37d827e07f56625ddf9e23d Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Wed, 27 Jul 2022 11:38:18 +0200 Subject: [PATCH 20/22] revert shutil formatting --- fs/osfs.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/fs/osfs.py b/fs/osfs.py index 94d16844..7a095f7f 100644 --- a/fs/osfs.py +++ b/fs/osfs.py @@ -470,11 +470,7 @@ def copy(self, src_path, dst_path, overwrite=False, preserve_time=False): # type: (Text, Text, bool, bool) -> None with self._lock: _src_path, _dst_path = self._check_copy(src_path, dst_path, overwrite) - _src_sys, _dst_sys = ( - self.getsyspath(_src_path), - self.getsyspath(_dst_path), - ) - shutil.copy2(_src_sys, _dst_sys) + shutil.copy2(self.getsyspath(_src_path), self.getsyspath(_dst_path)) # --- Backport of os.scandir for Python < 3.5 ------------ From 3bc8691ba07a031626c492de8ded014bd538ea1a Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Tue, 2 Aug 2022 11:35:20 +0200 Subject: [PATCH 21/22] use validatepath --- fs/base.py | 22 ++++++++++++---------- fs/copy.py | 4 ++-- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/fs/base.py b/fs/base.py index 189d45ab..d42997d4 100644 --- a/fs/base.py +++ b/fs/base.py @@ -1171,17 +1171,19 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): ``dst_path`` does not exist. """ - if not overwrite and self.exists(dst_path): + _src_path = self.validatepath(src_path) + _dst_path = self.validatepath(dst_path) + if not overwrite and self.exists(_dst_path): raise errors.DestinationExists(dst_path) - if self.getinfo(src_path).is_dir: + if self.getinfo(_src_path).is_dir: raise errors.FileExpected(src_path) - if normpath(src_path) == normpath(dst_path): + if _src_path == _dst_path: # early exit when moving a file onto itself return if self.getmeta().get("supports_rename", False): try: - src_sys_path = self.getsyspath(src_path) - dst_sys_path = self.getsyspath(dst_path) + src_sys_path = self.getsyspath(_src_path) + dst_sys_path = self.getsyspath(_dst_path) except errors.NoSysPath: # pragma: no cover pass else: @@ -1191,15 +1193,15 @@ def move(self, src_path, dst_path, overwrite=False, preserve_time=False): pass else: if preserve_time: - copy_modified_time(self, src_path, self, dst_path) + copy_modified_time(self, _src_path, self, _dst_path) return with self._lock: - with self.open(src_path, "rb") as read_file: + with self.open(_src_path, "rb") as read_file: # FIXME(@althonos): typing complains because open return IO - self.upload(dst_path, read_file) # type: ignore + self.upload(_dst_path, read_file) # type: ignore if preserve_time: - copy_modified_time(self, src_path, self, dst_path) - self.remove(src_path) + copy_modified_time(self, _src_path, self, _dst_path) + self.remove(_src_path) def open( self, diff --git a/fs/copy.py b/fs/copy.py index e70aafa6..5f593571 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -306,8 +306,8 @@ def copy_structure( dst_root (str): Path to the target root of the tree structure. """ - _src_root = abspath(normpath(src_root)) - _dst_root = abspath(normpath(dst_root)) + _src_root = src_fs.validatepath(src_root) + _dst_root = dst_fs.validatepath(dst_root) # It's not allowed to copy a structure into itself if src_fs == dst_fs and isbase(_src_root, _dst_root): raise IllegalDestination(dst_root) From 420998dba31c25a1db642f161fb9a1efc4097a8f Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Tue, 2 Aug 2022 11:46:42 +0200 Subject: [PATCH 22/22] copy_structure FS can be of type str --- fs/copy.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/fs/copy.py b/fs/copy.py index 5f593571..154fe715 100644 --- a/fs/copy.py +++ b/fs/copy.py @@ -306,14 +306,16 @@ def copy_structure( dst_root (str): Path to the target root of the tree structure. """ - _src_root = src_fs.validatepath(src_root) - _dst_root = dst_fs.validatepath(dst_root) - # It's not allowed to copy a structure into itself - if src_fs == dst_fs and isbase(_src_root, _dst_root): - raise IllegalDestination(dst_root) walker = walker or Walker() with manage_fs(src_fs) as _src_fs: with manage_fs(dst_fs, create=True) as _dst_fs: + _src_root = _src_fs.validatepath(src_root) + _dst_root = _dst_fs.validatepath(dst_root) + + # It's not allowed to copy a structure into itself + if _src_fs == _dst_fs and isbase(_src_root, _dst_root): + raise IllegalDestination(dst_root) + with _src_fs.lock(), _dst_fs.lock(): _dst_fs.makedirs(_dst_root, recreate=True) for dir_path in walker.dirs(_src_fs, _src_root):