Skip to content

Commit

Permalink
remove fspath from StagingRegistry
Browse files Browse the repository at this point in the history
This is to avoid likely footguns related to using os.path.join. Includes
test of error on os.path.join.
  • Loading branch information
dwhswenson committed Dec 22, 2023
1 parent cf60d1b commit da9955e
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
5 changes: 1 addition & 4 deletions gufe/storage/stagingregistry.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def _safe_to_delete_file(


class StagingRegistry:
"""PathLike local representation of an :class:`.ExternalStorage`.
"""Local representation of an :class:`.ExternalStorage`.
This connects objects on a local filesystem to the key-value store of a
(possibly remote) :class:`.ExternalStorage`. It presents a FileLike
Expand Down Expand Up @@ -202,9 +202,6 @@ def _load_file_from_external(self, external: ExternalStorage,
def __truediv__(self, path: Union[PathLike, str]):
return StagingPath(root=self, path=path)

def __fspath__(self):
return str(self.staging_dir)

def __repr__(self):
return (
f"{self.__class__.__name__}('{self.scratch}', {self.external})"
Expand Down
15 changes: 10 additions & 5 deletions gufe/tests/storage/test_stagingregistry.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def read_only_with_overwritten(root_with_contents):
delete_staging=root_with_contents.delete_staging,
read_only=True
)
filename = pathlib.Path(read_only) / "old_unit/data.txt"
filename = (read_only / "old_unit/data.txt").as_path()
assert not filename.exists()
staged = read_only / "old_unit/data.txt"
assert not filename.exists()
Expand Down Expand Up @@ -140,6 +140,12 @@ def test_repr(self, root):
assert r.startswith("SharedStaging")
assert "MemoryStorage" in r

def test_fspath_fail(self, root):
# ensure that we get an error on os.path.join (or really, anything
# that hits os.fspath)
with pytest.raises(TypeError):
os.path.join(root, "filename.txt")

@pytest.mark.parametrize('pathlist', [
['file.txt'], ['dir', 'file.txt']
])
Expand Down Expand Up @@ -266,8 +272,7 @@ def test_transfer_read_only(self, read_only_with_overwritten, caplog):

def test_cleanup(self, root_with_contents):
root_with_contents.delete_staging = True # slightly naughty
root_path = pathlib.Path(root_with_contents.__fspath__())
path = root_path / "new_unit/data.txt"
path = (root_with_contents / "new_unit/data.txt").as_path()
assert path.exists()
root_with_contents.cleanup()
assert not path.exists()
Expand Down Expand Up @@ -303,7 +308,7 @@ def test_cleanup_directory(self, root, caplog):
assert "During staging cleanup, the directory" in caplog.text

def test_register_cleanup_preexisting_file(self, root):
filename = pathlib.Path(root.__fspath__()) / "new_unit/foo.txt"
filename = (root / "new_unit/foo.txt").as_path()
filename.parent.mkdir(parents=True, exist_ok=True)
filename.touch()
root.external.store_bytes("new_unit/foo.txt", b"")
Expand Down Expand Up @@ -350,7 +355,7 @@ def test_delete_file_safe(self, tmp_path, is_safe):
assert permanent._delete_file_safe(my_file) is is_safe

def test_load_missing_for_transfer(self, permanent):
fname = pathlib.Path(permanent) / "old_unit/data.txt"
fname = (permanent / "old_unit/data.txt").as_path()
assert not fname.exists()
staging = permanent / "old_unit/data.txt"
staging.__fspath__()
Expand Down

0 comments on commit da9955e

Please sign in to comment.