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.

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

fix: expand ~ to user home dir #557

Open
wants to merge 1 commit into
base: mainline
Choose a base branch
from
Open

fix: expand ~ to user home dir #557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions src/deadline/job_attachments/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,13 @@ def set_root_path(self, original_root: str, new_root: str) -> None:
"""
Changes the root path for downloading output files, (which is the root path
saved in the S3 metadata for the output manifest by default,) with a custom path.
(It will store the new root path as an absolute path.)
The `new_root` path will have the following transformations applied:
- Normalizes the path with `os.path.normpath` (e.g. removes unnecessary relative paths `..`)
- Expands leading tilde (~) character to corresponding user home directory with pathlib.Path.expanduser
- Makes the path absolute if a relative path is given (prepends current working directory) with pathlib.Path.absolute
"""
# Need to use absolute to not resolve symlinks, but need normpath to get rid of relative paths, i.e. '..'
new_root = str(os.path.normpath(Path(new_root).absolute()))
new_root = str(os.path.normpath(Path(new_root).expanduser().absolute()))
Copy link
Contributor

Choose a reason for hiding this comment

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

This place will call expanduser() on all calls to set_root_path, but I doubt that's what we want to do. The original issue is about interactive user input, I think it's better to put this call immediately after that input is collected instead of inside this setter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we do want it in here since we make a call to pathlib.Path.absolute() which will prepend the current working directory to the path in cases where the path starts with ~, resulting in a root path like /home/jericht/~/tmp/123, which is something we don't want in my opinion.

We are applying os.normpath and pathlib.Path.absolute to the path, so we're already not setting the new_root parameter as is to the self.outputs_by_root dict. To me, this means the code is intended to "make sure the path is a valid root path". However, transforming a path like ~/tmp/123 into something like /the/current/working/dir/~/tmp/123, for example, seems like a bug. If the caller really wants to set a root path like /the/current/working/dir/~/tmp/123 by just passing in ~/tmp/123 while the cwd is /the/current/working/dir/, I think it's reasonable to have the caller pass in the absolute path in this case.

I've updated the function docstring to make it clear that this happens along with the other transformations it applies to new_root.

I tested this locally to make sure expanduser handles some edge cases I could think of correctly. If the path is already has the ~ expanded, it won't attempt to expand it again creating an invalid path. Here are a few tests I ran locally.

>>> def test_expanduser(path: str):
...     print(f"os.path.expanduser: {os.path.expanduser(path)}")
...     print(f"pathlib.Path.expanduser: {pathlib.Path(path).expanduser()}")
... 

>>> test_expanduser("~/tmp/123")
os.path.expanduser: /home/jericht/tmp/123
pathlib.Path.expanduser: /home/jericht/tmp/123

>>> test_expanduser("~/~/tmp/123")
os.path.expanduser: /home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/~/tmp/123

>>> test_expanduser("/tmp/~/123")
os.path.expanduser: /tmp/~/123
pathlib.Path.expanduser: /tmp/~/123

>>> test_expanduser("/home/jericht/~/tmp/123")
os.path.expanduser: /home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/~/tmp/123

>>> test_expanduser("~/home/jericht/~/tmp/123")
os.path.expanduser: /home/jericht/home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/home/jericht/~/tmp/123

Copy link
Contributor

Choose a reason for hiding this comment

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

side comment: Does this change also work for deadline attachment download ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do want this expansion to happen always in the setter, I think we should look more closely at where the value comes from currently, and where it could come from. Will this be surprising in any case?


if original_root not in self.outputs_by_root:
raise ValueError(
Expand Down
21 changes: 21 additions & 0 deletions test/unit/deadline_job_attachments/test_download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1304,6 +1304,27 @@ def test_OutputDownloader_set_root_path_wrong_root_throws_exception(
with pytest.raises(ValueError):
output_downloader.set_root_path(original_root="/wrong_root", new_root="/new_root_path")

@mock_aws
def test_OutputDownloader_set_root_path_expanduser(self) -> None:
# GIVEN
mock_outputdownloader = MagicMock()
original_root = os.path.join("/tmp", "original_root")
new_root = os.path.join("~", "asset_root")
mock_outputdownloader.outputs_by_root = {
original_root: "groot",
}

# WHEN
OutputDownloader.set_root_path(
self=mock_outputdownloader,
original_root=original_root,
new_root=new_root,
)

# THEN
assert len(mock_outputdownloader.outputs_by_root) == 1
assert os.path.expanduser(new_root) in mock_outputdownloader.outputs_by_root

@mock_aws
def test_OutputDownloader_download_job_output_when_skip(
self, farm_id, queue_id, tmp_path: Path
Expand Down
Loading