Skip to content

bpo-45234: Fix FileNotFound exception raised instead of IsADirectoryError in shutil.copyfile() #28421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ambv merged 4 commits into from
Sep 21, 2021
Merged

bpo-45234: Fix FileNotFound exception raised instead of IsADirectoryError in shutil.copyfile() #28421

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9fd9d7
fix copyfile() logic; add a unit test
akulakov Sep 17, 2021
5e6dad6
move dst handling into a separate "with" block
akulakov Sep 17, 2021
f19df1b
fix expected error on windows
akulakov Sep 18, 2021
b154282
extract copy and copy2 tests into separate methods
akulakov Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 30 additions & 29 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,36 +253,37 @@ def copyfile(src, dst, *, follow_symlinks=True):
if not follow_symlinks and _islink(src):
os.symlink(os.readlink(src), dst)
else:
try:
with open(src, 'rb') as fsrc, open(dst, 'wb') as fdst:
# macOS
if _HAS_FCOPYFILE:
try:
_fastcopy_fcopyfile(fsrc, fdst, posix._COPYFILE_DATA)
return dst
except _GiveupOnFastCopy:
pass
# Linux
elif _USE_CP_SENDFILE:
try:
_fastcopy_sendfile(fsrc, fdst)
with open(src, 'rb') as fsrc:
try:
with open(dst, 'wb') as fdst:
# macOS
if _HAS_FCOPYFILE:
try:
_fastcopy_fcopyfile(fsrc, fdst, posix._COPYFILE_DATA)
return dst
except _GiveupOnFastCopy:
pass
# Linux
elif _USE_CP_SENDFILE:
try:
_fastcopy_sendfile(fsrc, fdst)
return dst
except _GiveupOnFastCopy:
pass
# Windows, see:
# https://github.com/python/cpython/pull/7160#discussion_r195405230
elif _WINDOWS and file_size > 0:
_copyfileobj_readinto(fsrc, fdst, min(file_size, COPY_BUFSIZE))
return dst
except _GiveupOnFastCopy:
pass
# Windows, see:
# https://github.com/python/cpython/pull/7160#discussion_r195405230
elif _WINDOWS and file_size > 0:
_copyfileobj_readinto(fsrc, fdst, min(file_size, COPY_BUFSIZE))
return dst

copyfileobj(fsrc, fdst)

# Issue 43219, raise a less confusing exception
except IsADirectoryError as e:
if os.path.exists(dst):
raise
else:
raise FileNotFoundError(f'Directory does not exist: {dst}') from e

copyfileobj(fsrc, fdst)

# Issue 43219, raise a less confusing exception
except IsADirectoryError as e:
if not os.path.exists(dst):
raise FileNotFoundError(f'Directory does not exist: {dst}') from e
else:
raise

return dst

Expand Down
24 changes: 24 additions & 0 deletions Lib/test/test_shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1259,6 +1259,30 @@ def test_copyfile_nonexistent_dir(self):
write_file(src_file, 'foo')
self.assertRaises(FileNotFoundError, shutil.copyfile, src_file, dst)

def test_copyfile_copy_dir(self):
# Issue 45234
# test copy() and copyfile() raising proper exceptions when src and/or
# dst are directories
src_dir = self.mkdtemp()
src_file = os.path.join(src_dir, 'foo')
dir2 = self.mkdtemp()
dst = os.path.join(src_dir, 'does_not_exist/')
write_file(src_file, 'foo')
if sys.platform == "win32":
err = PermissionError
else:
err = IsADirectoryError

self.assertRaises(err, shutil.copyfile, src_dir, dst)
self.assertRaises(err, shutil.copyfile, src_file, src_dir)
self.assertRaises(err, shutil.copyfile, dir2, src_dir)
self.assertRaises(err, shutil.copy, dir2, src_dir)
self.assertRaises(err, shutil.copy2, dir2, src_dir)
Copy link
Contributor

@eryksun eryksun Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this just test copyfile()?

Note that for Windows there's been some discussion about changing copy2(), and maybe copy(), to call the system CopyFileExW() function, which also copies alternate data streams, extended attributes, and security resource attributes. In that case the high-level copy functions will no longer use copyfile(), copymode(), and copystat() in Windows.

Copy link
Contributor Author

@akulakov akulakov Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth testing copy and copy2 for consistency of handling a dir arg, because they already do some dir-related logic (for dst arg), but this may be done here or in separate tests. Looking at other tests, to follow the same naming pattern, we can do test_copy_dirs() and test_copy2_dirs().

But it makes sense to do this testing in the same function for all three because we're ensuring consistency and also it's convenient because the set up of dirs and expected errors is the same.

I can remove these lines though if you think it's not worth testing them here at this point in view of the other work you mentioned.

Copy link
Contributor

@eryksun eryksun Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently what copy() and copy2() do that's related to src being a directory is implemented in copyfile(), so only the latter really needs to be tested. I see the point about checking consistency, but would separate tests maybe be more obvious for someone who's modifying copy() or copy2()?

Copy link
Contributor Author

@akulakov akulakov Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I will extract them into separate tests a bit later today.

Copy link
Contributor Author

@akulakov akulakov Sep 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the tests and they're passing, please take a look..


# raise *err* because of src rather than FileNotFoundError because of dst
self.assertRaises(err, shutil.copy, dir2, dst)
shutil.copy(src_file, dir2) # should not raise exceptions


class TestArchives(BaseTest, unittest.TestCase):

Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2021-09-17-11-20-55.bpo-45234.qUcTVt.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fixed a regression in :func:`~shutil.copyfile`, :func:`~shutil.copy`,
:func:`~shutil.copy2` raising :exc:`FileNotFoundError` when source is a
directory, which should raise :exc:`IsADirectoryError`