-
Notifications
You must be signed in to change notification settings - Fork 541
IO: Fix Windows drive letters misidentified as URI schemes #3721
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
|
|
||
| import importlib | ||
| import logging | ||
| import sys | ||
| import warnings | ||
| from abc import ABC, abstractmethod | ||
| from io import SEEK_SET | ||
|
|
@@ -41,6 +42,17 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _is_windows_drive_letter(scheme: str) -> bool: | ||
| r"""Check if a parsed URL scheme is actually a Windows drive letter. | ||
|
|
||
| On Windows, Python's urlparse treats paths like 'C:\\Users\\...' as having | ||
| scheme='c'. This detects that case so callers can route to the local filesystem. | ||
| Only returns True on Windows — no behavior change on other platforms. | ||
| """ | ||
| return sys.platform == "win32" and len(scheme) == 1 and scheme.isalpha() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to replace with something around
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @jayceslesar, interesting suggestion, and I think it's reasonable. We could restructure to check the original path with However, refactoring for the |
||
|
|
||
|
|
||
| AWS_PROFILE_NAME = "client.profile-name" | ||
| AWS_REGION = "client.region" | ||
| AWS_ACCESS_KEY_ID = "client.access-key-id" | ||
|
|
@@ -336,13 +348,19 @@ def _import_file_io(io_impl: str, properties: Properties) -> FileIO | None: | |
|
|
||
| def _infer_file_io_from_scheme(path: str, properties: Properties) -> FileIO | None: | ||
| parsed_url = urlparse(path) | ||
| if parsed_url.scheme: | ||
| if file_ios := SCHEMA_TO_FILE_IO.get(parsed_url.scheme): | ||
| scheme = parsed_url.scheme | ||
|
|
||
| if scheme and _is_windows_drive_letter(scheme): | ||
| # Windows drive letter (e.g. 'C:\...') misidentified as a URL scheme | ||
| scheme = "file" | ||
|
|
||
| if scheme: | ||
| if file_ios := SCHEMA_TO_FILE_IO.get(scheme): | ||
| for file_io_path in file_ios: | ||
| if file_io := _import_file_io(file_io_path, properties): | ||
| return file_io | ||
| else: | ||
| warnings.warn(f"No preferred file implementation for scheme: {parsed_url.scheme}", stacklevel=2) | ||
| warnings.warn(f"No preferred file implementation for scheme: {scheme}", stacklevel=2) | ||
| return None | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows newb here:
I'm looking at https://stackoverflow.com/questions/446209/possible-values-from-sys-platform. There's a couple other Windows derivatives mentioned. Should we do that for these too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! My understanding is that
"win32"is sufficient here, the drive letter parsing bug only exists on native Windows where paths look likeC:\....The other
sys.platformvalues from that SO thread don't need coverage because they use POSIX paths, sourlparsenever produces a single-letter scheme on them:sys.platform == "cygwin") translatesC:\Users\...to/cygdrive/c/Users/...at the filesystem layer (Cygwin docs)sys.platform == "msys") similarly mapsC:\Usersto/c/Users(MSYS2 docs)sys.platform == "win32"is also the standard CPython idiom for this, it's how thestdlibdistinguishes Windows from POSIX behavior (sys.platformdocs), and what pytest recommends forskipifmarkers.