Skip to content
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.

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

Transport: Bug fix in rename method #6735

Merged
merged 17 commits into from
Feb 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions src/aiida/transports/plugins/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
:param str oldpath: existing name of the file or folder
:param str newpath: new name for the file or folder

:raises OSError: if src/dst is not found
:raises OSError: if oldpath is not found or newpath already exists
:raises ValueError: if src/dst is not a valid string
"""
oldpath = str(oldpath)
Expand All @@ -877,8 +877,8 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
raise ValueError(f'Destination {newpath} is not a valid string')
if not os.path.exists(oldpath):
raise OSError(f'Source {oldpath} does not exist')
if not os.path.exists(newpath):
raise OSError(f'Destination {newpath} does not exist')
if os.path.exists(newpath):
raise OSError(f'Destination {newpath} already exists.')

shutil.move(oldpath, newpath)

Expand Down
14 changes: 5 additions & 9 deletions src/aiida/transports/plugins/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -1357,8 +1357,8 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
:param str oldpath: existing name of the file or folder
:param str newpath: new name for the file or folder

:raises OSError: if oldpath/newpath is not found
:raises ValueError: if sroldpathc/newpath is not a valid path
:raises OSError: if oldpath is not found or newpath already exists
:raises ValueError: if oldpath/newpath is not a valid path
"""
if not oldpath:
raise ValueError(f'Source {oldpath} is not a valid path')
Expand All @@ -1371,13 +1371,9 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
if not self.isfile(oldpath):
if not self.isdir(oldpath):
raise OSError(f'Source {oldpath} does not exist')
# TODO: this seems to be a bug (?)
# why to raise an OSError if the newpath does not exist?
# ofcourse newpath shouldn't exist, that's why we are renaming it!
# issue opened here: https://github.com/aiidateam/aiida-core/issues/6725
if not self.isfile(newpath):
if not self.isdir(newpath):
raise OSError(f'Destination {newpath} does not exist')

if self.path_exists(newpath):
raise OSError(f'Destination {newpath} already exist')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstring on line 1360 needs fixing. (github does not allow me to comment on that line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the docstring on line 1360, it states that OSError is used for both cases—when oldpath and newpath both are not found—which needs to be fixed. However, is it okay to use specific errors (FileNotFoundError and FileExistsError) instead of OSError?

Copy link
Contributor

Choose a reason for hiding this comment

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

                   :raises OSError: if oldpath is not found

Copy link
Contributor

Choose a reason for hiding this comment

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

In general. you are right FileNotFoundError might be a better fit for such cases, as with OSError is not immediately clear what's the error case.

However, please note OSError is the parent of FileNotFoundError

 └── Exception
      └── OSError
           ├── FileNotFoundError
           ├── PermissionError
           ├── IsADirectoryError
           ├── NotADirectoryError

For the sake of consistency --I assume-- OSError was chose to raise from our Transport class with proper message. This way it's easier to handle, when adopting any of the Transport plugins.


return self.sftp.rename(oldpath, newpath)

Expand Down
25 changes: 25 additions & 0 deletions tests/transports/test_all_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -1232,3 +1232,28 @@ def test_asynchronous_execution(custom_transport, tmp_path):
except ProcessLookupError:
# If the process is already dead (or has never run), I just ignore the error
pass


def test_rename(custom_transport, tmp_path_remote):
"""Test the rename function of the transport plugin."""
with custom_transport as transport:
old_file = tmp_path_remote / 'oldfile.txt'
new_file = tmp_path_remote / 'newfile.txt'
another_file = tmp_path_remote / 'anotherfile.txt'

# Create a test file to rename
old_file.touch()
another_file.touch()
assert old_file.exists()
assert another_file.exists()

# Perform rename operation
transport.rename(old_file, new_file)

# Verify rename was successful
assert not old_file.exists()
assert new_file.exists()

# Perform rename operation if new file already exists
with pytest.raises(OSError, match='already exist|destination exists'):
transport.rename(new_file, another_file)