Repository navigation
Fixed BucketPath using backslash separators on Windows - #325
Ramnath0521 wants to merge 2 commits into
Conversation
BucketPath stored its path as PurePath, which is a PureWindowsPath on Windows, so read(), write(), rm(), iterdir() and as_udf_path() sent backslash paths to BucketFS. Store a PurePosixPath instead, converting PurePath arguments with as_posix(). Closes exasol#322 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ahsimb
left a comment
There was a problem hiding this comment.
Thanks for tackling this! I found a few issues; inline comments cover the changed lines, and these points are about code outside the diff:
- Root cause in
MountedBucketis still there (exasol/bucketfs/_buckets.py,MountedBucket.files). On Windows it still returns backslash paths (dir\a.txt), while_navigate()now compares against a strictly POSIXpath_str. Soexists(),iterdir(),rm()andwalk()still fail with a mounted bucket on Windows. Returningpth.relative_to(self.root).as_posix()there (and a POSIXudf_path) would fix it at the source. relative_to()return type changes on Windows (_path.py,relative_to). It now returnsPurePosixPathinstead ofPureWindowsPath, so caller code likebp.relative_to(root) == PurePath('child')is always False on Windows. That is a public API change (the existing unit test had to be changed for it), so it should probably be mentioned in the changelog rather than only listed as a bug fix.- Lookup and I/O use different strings (
_path.py,read/write/rm). They sendstr(self._path), which includes the root, while_navigate()uses_get_relative_posix()without the anchor. For example,BucketPath('/dir/a.txt', api).exists()findsdir/a.txt, butrm()callsdelete('/dir/a.txt'). - Outdated docstring (
_path.py,__init__). It still says Windows forms like\\abc\...\are accepted, which is no longer true for plain strings.
| :param bucket_api: An object supporting the Bucket API protocol. | ||
| """ | ||
| self._path = PurePath(path) | ||
| self._path = PurePosixPath( |
There was a problem hiding this comment.
Regression for backslash strings on Windows. A plain str is now parsed by PurePosixPath, so os.path.join('dir', 'a.txt') or str(WindowsPath(...)) ('dir\a.txt') becomes a single component with a literal backslash. Previously _get_relative_posix replaced \ with /, so exists() worked; now exists() returns False, .name is 'dir\a.txt', .parent is '.', and write() uploads a file whose name contains a backslash. Consider normalizing strings too (e.g. PureWindowsPath(path).as_posix() on Windows, or always replacing \ with /).
| """ | ||
| self._path = PurePath(path) | ||
| self._path = PurePosixPath( | ||
| path.as_posix() if isinstance(path, PurePath) else path |
There was a problem hiding this comment.
as_posix() keeps the drive / UNC anchor. BucketPath(PureWindowsPath(r'C:\data\a.txt'), api) becomes C:/data/a.txt, so lookup uses C:/data/a.txt instead of data/a.txt (the old code stripped the anchor). A UNC path \\srv\share\x becomes //srv/share/x, which write()/rm() send to BucketFS with a leading //. Should the anchor be dropped (e.g. via path.relative_to(path.anchor)) before converting?
Also: joinpath() and __truediv__ don't go through this normalization. On Python 3.10/3.11 (both supported), BucketPath('a', api) / PureWindowsPath('C:/x/y') yields a/C:\/x/y, and .joinpath(PureWindowsPath(r'\x\y')) yields a/\/x/y. They should apply the same conversion.
| def as_udf_path(self) -> str: | ||
| return str( | ||
| PurePath(self._bucket_api.udf_path) / _remove_archive_suffix(self._path) | ||
| PurePosixPath(self._bucket_api.udf_path) |
There was a problem hiding this comment.
MountedBucket.udf_path returns str(Path(...)), which uses backslashes on Windows, so wrapping it in PurePosixPath produces mixed separators, e.g. \buckets\bfsdefault\default/dir/a.txt. Either MountedBucket.udf_path should return a POSIX string, or this should convert it.
| assert path.exists() | ||
|
|
||
|
|
||
| def test_joinpath_with_string_uses_posix_separators(bucket_api): |
There was a problem hiding this comment.
This test uses only forward slashes, so it also passes on the unfixed code and doesn't exercise the bug. Missing coverage: backslash strings, absolute / drive-letter / UNC PureWindowsPath, and / / joinpath with Windows path objects. Those would catch the regressions noted on _path.py.
Convert every path that enters BucketPath through one helper, so a Windows string, a Windows path object, joinpath() and "/" all end up as the same POSIX path: separators become "/", a drive or UNC share is dropped and a root is kept. Make MountedBucket.files and udf_path POSIX as well, which is the root cause the reviewer pointed out and also makes the existing unit tests pass on Windows. Document the relative_to() return type change under Breaking Changes and cover backslash strings, drive and UNC anchors, joinpath() and "/" with Windows path objects and MountedBucket in the unit tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the careful review, all of this was right. I pushed a follow-up commit (764ec93) that addresses it:
One point I left alone on purpose: |
Closes #322
Cause
BucketPathstored its path asPurePath(path), which is aPureWindowsPathon Windows. Only_get_relative_posix()converted it back to forward slashes.read(),write(),rm(),__str__,iterdir()andas_udf_path()usedstr(self._path)directly and so sent backslash paths to BucketFS, as described in the issue.Change
Following the suggestion in the issue,
BucketPathnow stores aPurePosixPath:__init__builds aPurePosixPath. APurePathargument (for example aPureWindowsPathorWindowsPath) is converted withas_posix()first, sojoinpath()with Windows path objects also works._get_relative_posix()no longer needs itsPureWindowsPathspecial case.as_udf_path()builds the result withPurePosixPath, so the UDF path uses/on every platform.Behaviour on Linux and macOS is unchanged, because
PurePathis alreadyPurePosixPaththere.Two things on Windows are different now and worth a look:
PureWindowsPath/WindowsPathobjects are still accepted and converted.relative_to()returns aPurePosixPathinstead of aPureWindowsPath. I updated the one existing assertion that compared againstPurePath(...)accordingly.Tests
New
test/unit/test_bucket_path_separators.py(7 tests) uses a recording fake bucket API andPureWindowsPathinputs, so it runs the same on every OS and does not need to patchPurePaththe way the issue's Linux emulation does. It coversstr(),write()/read()/rm(),joinpath(),iterdir()andas_udf_path().On the unfixed code (Windows 11, Python 3.12) 5 of the new tests fail, with the backslash paths from the issue (
'dir\sub\a.txt' == 'dir/sub/a.txt'); with the fix all 7 pass.I also ran the whole
test/unitfolder on Windows. It does not pass on Windows even before this change (17 failures). With this change it is 14 failures, none new, andtest_joinpath,test_truedivandtest_archive_as_udf_pathnow pass. The remaining 14 come from thebucket_fakefixture:MountedBucket.fileson a local directory returns backslash paths on Windows. I left that out because it is a separate code path (a locally mounted bucket) from the one in this issue, but I'm happy to look at it in a follow-up if you want it.black --check,isort --checkandmypyare clean on the changed files. I did not run the integration tests or the fullnoxsessions.Checklist
doc/changes/unreleased.mdWritten with AI assistance (Claude Code). I reproduced the failure on my own Windows machine, read the fix, and verified the added tests fail before it and pass after. Errors are mine.
🤖 Generated with Claude Code