diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index fb473705..8a330ed7 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -1,3 +1,7 @@ # Unreleased ## Summary + +## Bugfixes + +* #322: Fixed `BucketPath` and `MountedBucket` using backslash separators on Windows; BucketFS paths are now always POSIX paths diff --git a/doc/user_guide/basics.rst b/doc/user_guide/basics.rst index b8af8468..dd52a51d 100644 --- a/doc/user_guide/basics.rst +++ b/doc/user_guide/basics.rst @@ -89,6 +89,15 @@ PathLike interface ------------------ A PathLike is an interface similar to the pathlib.Path and should feel familiar to most users. +.. note:: + + BucketFS paths are always POSIX paths, regardless of the client OS. + A path given as a string must use ``/`` as separator. A backslash is treated + as a regular character of a file name, e.g. ``my_model\v1.pkl`` is a single + file. On Windows, either pass a ``PurePath`` or ``PureWindowsPath`` object, + which is converted to a POSIX path, or convert a string first, e.g. + ``PureWindowsPath(s).as_posix()``. + Using the PathLike interface ++++++++++++++++++++++++++++ diff --git a/exasol/bucketfs/_buckets.py b/exasol/bucketfs/_buckets.py index 0c02c991..018ecad6 100644 --- a/exasol/bucketfs/_buckets.py +++ b/exasol/bucketfs/_buckets.py @@ -438,7 +438,7 @@ def udf_path(self) -> str: @property def files(self) -> list[str]: - return [str(pth.relative_to(self.root)) for pth in self.root.rglob("*.*")] + return [pth.relative_to(self.root).as_posix() for pth in self.root.rglob("*.*")] def delete(self, path: str) -> None: try: diff --git a/exasol/bucketfs/_path.py b/exasol/bucketfs/_path.py index 060385f4..77718529 100644 --- a/exasol/bucketfs/_path.py +++ b/exasol/bucketfs/_path.py @@ -14,6 +14,7 @@ ) from pathlib import ( PurePath, + PurePosixPath, PureWindowsPath, ) from typing import ( @@ -85,7 +86,7 @@ def relative_to(self, other: PathLike) -> PurePath: Example: a = build_path(..., path="parent") b = build_path(..., path="parent/child") - assert b.relative(a) == PurePath("child") + assert b.relative_to(a) == PurePath("child") """ def as_uri(self) -> str: @@ -215,6 +216,18 @@ def __truediv__(self, other): """ +def _to_posix(path): + """ + Converts a PureWindowsPath to a POSIX path string, e.g. + 'C:\\dir\\a.txt' -> 'C:/dir/a.txt'. Leaves other paths as they are, + including strings. Constructing a PurePosixPath directly from an absolute + PureWindowsPath gives this result only in Python >= 3.12. + """ + if isinstance(path, PureWindowsPath): + return path.as_posix() + return path + + def _remove_archive_suffix(path: PurePath) -> PurePath: while path.suffix in ARCHIVE_SUFFIXES: path = path.with_suffix("") @@ -277,15 +290,21 @@ def __init__(self, path: str | PurePath, bucket_api: BucketLike): """ :param path: A pure path of a file or directory. The path is assumed to be relative to the bucket. It is also permissible to have - this path in an absolute form, e.g. '/dir1/...' - or '\\\\abc\\...\\'. + this path in an absolute form, e.g. '/dir1/...'. + + BucketFS paths are always POSIX paths, regardless + of the client OS. A string is interpreted as a POSIX + path, i.e. only '/' separates the path segments and + a backslash is a regular character of a file name, + e.g. 'my_model\\v1.pkl'. A PureWindowsPath is + converted to a POSIX path. All Pure Path methods of the PathLike protocol will be delegated to this object. :param bucket_api: An object supporting the Bucket API protocol. """ - self._path = PurePath(path) + self._path = PurePosixPath(_to_posix(path)) self._bucket_api = bucket_api @property @@ -302,8 +321,6 @@ def _get_relative_posix(self) -> str: file: 'dir/subdir/.../filename'. """ path_str = str(self._path)[len(self._path.anchor) :] - if isinstance(self._path, PureWindowsPath): - path_str = path_str.replace("\\", "/") if path_str == ".": path_str = "" return path_str @@ -360,7 +377,8 @@ def relative_to(self, other: PathLike) -> PurePath: f" from a foreign bucket {other_bucket_path._bucket_api}." ) try: - return self._path.relative_to(other_bucket_path._path) + # Returns a path in the native format of the client OS, as before. + return PurePath(self._path.relative_to(other_bucket_path._path)) except ValueError as ex: raise BucketFsError(repr(ex)) from ex @@ -368,9 +386,13 @@ def as_uri(self) -> str: return self._path.as_uri() def as_udf_path(self) -> str: - return str( - PurePath(self._bucket_api.udf_path) / _remove_archive_suffix(self._path) - ) + udf_root = self._bucket_api.udf_path + relative_path = _remove_archive_suffix(self._path) + if udf_root.startswith("/"): + return str(PurePosixPath(udf_root) / relative_path) + # The udf_path of a MountedBucket on Windows is a local path in the native + # format, e.g. 'C:\\tmp\\bucket'. + return str(PurePath(udf_root) / relative_path) def exists(self) -> bool: return self._navigate() is not None @@ -433,7 +455,8 @@ def joinpath(self, *path_segments) -> PathLike: # The path segments can be of either this type or an os.PathLike. cls = type(self) seg_paths = [ - seg._path if isinstance(seg, cls) else seg for seg in path_segments + seg._path if isinstance(seg, cls) else _to_posix(seg) + for seg in path_segments ] new_path = self._path.joinpath(*seg_paths) return cls(new_path, self._bucket_api) @@ -490,9 +513,7 @@ def iterdir(self) -> Generator[PathLike]: def __truediv__(self, other): # The other object can be of either this type or an os.PathLike. - cls = type(self) - new_path = self._path / (other._path if isinstance(other, cls) else other) - return cls(new_path, self._bucket_api) + return self.joinpath(other) def __eq__(self, other) -> bool: if not isinstance(other, BucketPath): @@ -585,6 +606,12 @@ def build_path(**kwargs) -> PathLike: path = build_path(...) / "the_desired_path" + BucketFS paths are always POSIX paths, regardless of the client OS. + A string path must use '/' as separator; a backslash is treated as a + regular character of a file name. On Windows, either pass a + PurePath / PureWindowsPath object, which gets converted, or convert + a string to POSIX format first, e.g. PureWindowsPath(s).as_posix(). + The rest of the arguments are backend specific. Additional On-prem arguments: diff --git a/test/unit/test_bucket_path.py b/test/unit/test_bucket_path.py index 17427e03..492a2e8b 100644 --- a/test/unit/test_bucket_path.py +++ b/test/unit/test_bucket_path.py @@ -2,6 +2,8 @@ from pathlib import ( Path, PurePath, + PurePosixPath, + PureWindowsPath, ) import pytest @@ -281,3 +283,142 @@ def test_relative_to(path, expected_relative, bucket_fake): root = bfs.path.BucketPath("root", bucket_api=bucket_fake) testee = root / path assert testee.relative_to(root) == PurePath(expected_relative) + + +def test_relative_to_is_native(monkeypatch): + """ + relative_to returns a path in the native format of the client OS, as before. + Windows is emulated by patching PurePath. + """ + monkeypatch.setattr(bfs._path, "PurePath", PureWindowsPath) + api = _RecordingBucket(files=[]) + root = bfs.path.BucketPath("root", bucket_api=api) + assert (root / "a/b").relative_to(root) == PureWindowsPath("a\\b") + + +class _RecordingBucket: + """A bucket API fake recording the paths passed to it.""" + + udf_path = "/buckets/bfsdefault/default" + + def __init__(self, files): + self.files = files + self.calls = [] + + def upload(self, path, data): + self.calls.append(("upload", path)) + + def download(self, path, chunk_size=8192): + self.calls.append(("download", path)) + return [] + + def delete(self, path): + self.calls.append(("delete", path)) + + +@pytest.mark.parametrize( + "segment", + [ + "dir/sub/a.txt", + PurePosixPath("dir/sub/a.txt"), + PureWindowsPath("dir\\sub\\a.txt"), + ], +) +def test_paths_are_posix(segment, monkeypatch): + """ + Paths sent to BucketFS must use POSIX separators, even if the path is + passed as a PureWindowsPath, see issue #322. Windows is emulated by + patching PurePath, which BucketPath uses only where the native path format + of the client OS is expected. + """ + monkeypatch.setattr(bfs._path, "PurePath", PureWindowsPath) + api = _RecordingBucket(files=["dir/sub/a.txt"]) + root = bfs.path.BucketPath("", bucket_api=api) + for path in [root.joinpath(segment), root / segment]: + api.calls.clear() + path.write(b"x") + list(path.read()) + path.rm() + assert api.calls == [ + ("upload", "dir/sub/a.txt"), + ("download", "dir/sub/a.txt"), + ("delete", "dir/sub/a.txt"), + ] + assert str(path) == "dir/sub/a.txt" + assert path.as_udf_path() == "/buckets/bfsdefault/default/dir/sub/a.txt" + children = [str(p) for p in root.joinpath(PureWindowsPath("dir")).iterdir()] + assert children == ["dir/sub"] + + +def test_backslash_is_part_of_file_name(): + """ + A string path is a POSIX path, a backslash is a regular character. + """ + api = _RecordingBucket(files=[]) + path = bfs.path.BucketPath("dir", bucket_api=api) / "my_model\\v1.pkl" + path.write(b"x") + assert path.name == "my_model\\v1.pkl" + assert api.calls == [("upload", "dir/my_model\\v1.pkl")] + + +def test_as_udf_path_with_windows_udf_root(monkeypatch): + """ + On Windows, the udf_path of a MountedBucket is a native path. The UDF path + stays in the native format, as before. Windows is emulated by patching PurePath. + """ + monkeypatch.setattr(bfs._path, "PurePath", PureWindowsPath) + api = _RecordingBucket(files=[]) + api.udf_path = "C:\\tmp\\bucket" + path = bfs.path.BucketPath("dir/a.txt", bucket_api=api) + assert path.as_udf_path() == "C:\\tmp\\bucket\\dir\\a.txt" + + +def test_mounted_bucket_files_are_posix(bucket_fake): + assert "dir1/dir11/file110.dat" in bucket_fake.files + + +def test_absolute_windows_path_is_posix(): + """ + An absolute PureWindowsPath is converted the same way in all Python versions. + """ + api = _RecordingBucket(files=[]) + win_path = PureWindowsPath("\\dir\\a.txt") + root = bfs.path.BucketPath("root", bucket_api=api) + assert str(bfs.path.BucketPath(win_path, bucket_api=api)) == "/dir/a.txt" + assert str(root / win_path) == "/dir/a.txt" + assert str(root.joinpath(win_path)) == "/dir/a.txt" + + +@pytest.mark.parametrize( + "win_path, expected, expected_joined", + [ + ("C:\\models\\m.pkl", "C:/models/m.pkl", "root/C:/models/m.pkl"), + ("C:models\\m.pkl", "C:models/m.pkl", "root/C:models/m.pkl"), + ( + "\\\\server\\share\\models\\m.pkl", + "//server/share/models/m.pkl", + "//server/share/models/m.pkl", + ), + ], +) +def test_windows_path_with_drive(win_path, expected, expected_joined): + """ + The drive is kept, the result is the same as in Python >= 3.12 without + the conversion, see _to_posix(). + """ + api = _RecordingBucket(files=[]) + root = bfs.path.BucketPath("root", bucket_api=api) + win_path = PureWindowsPath(win_path) + assert str(bfs.path.BucketPath(win_path, bucket_api=api)) == expected + assert str(root.joinpath(win_path)) == expected_joined + assert str(root / win_path) == expected_joined + + +def test_string_with_drive_is_kept(): + """ + A string is taken as a POSIX path as it is, even if it looks like a Windows path. + """ + api = _RecordingBucket(files=[]) + path = bfs.path.BucketPath("C:\\models\\m.pkl", bucket_api=api) + path.write(b"x") + assert api.calls == [("upload", "C:\\models\\m.pkl")]