Expand ?/[] globs for a single-string path in read mode - #2080
Expand ?/[] globs for a single-string path in read mode#2080chuenchen309 wants to merge 2 commits into
Conversation
`get_fs_token_paths` expanded a single-string read-mode path only when it
contained `*`, so `open_files("data?.txt")` and `open_files("data[0-9].txt")`
returned the literal path instead of the matching files — while `data*.txt`,
and every form given as a list, expanded correctly.
The list branch already routes through `expand_paths_if_needed`, whose
read-mode docstring promises expansion of "any of *?[]"; use `glob.has_magic`
here too so the single-string branch agrees. The directory filtering is left
unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| if ("w" in mode or "x" in mode) and expand: | ||
| paths = _expand_paths(paths, name_function, num) | ||
| elif "*" in paths: | ||
| elif has_magic(paths): |
There was a problem hiding this comment.
? is also the query delimiter in HTTP URLs, and HTTPFileSystem._strip_protocol deliberately keeps the full URL. This makes a normal URL like https://host/file?download=1 go through fs.glob() instead of being opened literally; can the magic check be limited to the path component?
"?" is a glob wildcard but also the query delimiter in URLs, and
HTTPFileSystem's own magic_check ("[*[]") deliberately excludes it. Using
glob.has_magic() here routed a normal URL like "https://host/f?x=1"
through fs.glob(), which for HTTP means an extra _exists() round-trip and
an empty result (so the open silently yields nothing) when existence
checks are unreliable. Detect only "*" and "[" instead, matching the HTTP
filesystem, and add a test that a "?" path is passed through literally.
|
Good catch — you're right, and I verified the mechanism: Fixed in bcebd8b by detecting only |
|
Stepping back — @martindurant, this needs your call, because @Sanjays2402's point conflicts with the original request here. The tension:
So I pushed the conservative
Happy to implement whichever you prefer. |
|
It's not just local Vs HTTP: the special glob characters can be included in the legitimate names of files on many backends, including local. Actually HTTP is the only true outlier, because we don't expect "?" in the filename except as the query marker (and other ?s should be url-encoded). So, I suggest that glob in general be made fully capable as intended in this PR, but HTTP(S) (alone) have special treatment. Documentation should make clear, that if there are URLs containing special characters that users don't want to pass through glob, they should pass a length-one list for the URL instead. Perhaps this deserves a section in the documentation. |
Fixes #957.
Problem
open_files/get_fs_token_pathsexpand a single-string read-mode path only when it contains*. So?and[...]globs are returned literally:The same patterns work when passed as a list, because the list branch routes through
expand_paths_if_needed, whose read-mode docstring promises expansion of "any of*?[]" and usesglob.has_magic. Only the single-string branch inget_fs_token_pathsstill checks"*" in paths.This is the fix @martindurant suggested on #957 ("that line could probably use a
glob.has_magicfunction call... please feel free to make a PR"). #961 appliedhas_magicto the list branch; this applies it to the single-string branch so the two agree.Change
One line:
elif "*" in paths:→elif has_magic(paths):(has_magicis already imported in this module). Thenot fs.isdir(f)directory filtering on that branch is left exactly as-is — this PR only changes which patterns count as a glob, not the filtering that #964 got tangled up in.Tests
test_get_fs_token_paths_single_string_read_mode_globsmirrors the existingtest_expand_paths_if_needed_in_read_modebut drives a single string throughget_fs_token_paths, over*,?, and[12]. It fails on?/[12]before the change and passes after.fsspec/tests/test_core.pystays green (59 passed, 6 skipped);ruff checkandruff format --checkpass on both files.Disclosure: this contribution is fully AI-authored and autonomous (Claude Code, acting on this account). An AI found the issue, wrote and ran the repro and the test, and wrote this description; the human account holder reviews every change and is accountable for it. The verification above is real and re-runnable from the diff. If this isn't the kind of contribution you want, say so and I'll close it.