Repository navigation
Raise FileNotFoundError from ls() of a missing key path (GH-1330) - #1347
Conversation
ls() of a key path that is neither an object nor a key prefix of one returned an empty list, also with versions=True. fsspec's filesystems and s3fs raise FileNotFoundError for a missing path, and fsspec's conformance tests expect it, so ls() now raises. A bucket path keeps listing an empty bucket as []. The four conformance tests marked as failing on GH-1330 now pass, so their xfail marks are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| file = self._head_object(path, refresh=refresh) | ||
| if file: | ||
| files = [file] | ||
| key = S3Path.parse(path).key |
There was a problem hiding this comment.
Self-review round one (behavior and implementation), finding 1 of 2.
Base 1a6a0d6, reviewed head 3fe812a. Covered: ls() for bucket/key/version paths, _ls_dirs/_head_object/_ls_object_versions cache eviction, AioS3FileSystem._ls (delegates to the sync ls()), fsspec callers of ls (walk, _walk, listdir, tree; find/glob/info/exists do not call it), the updated and new tests, and the removed xfail marks. Passes: my own read, /code-review (high), /simplify (reuse, simplification, efficiency, altitude).
FINDING: the new raise decided "key path" with "/" in path. ls("s3://bucket/?versionId=v1") keeps bucket/?versionId=v1 after rstrip("/"), so an existing empty bucket raised FileNotFoundError instead of returning [] (and the pre-existing fallback sent a HeadObject without a key). Repaired in 0de3190: the key is taken from S3Path.parse(path).key, as the other lookups do; test_ls_empty_bucket gained the version-query case, which fails with "/" in path.
There was a problem hiding this comment.
Correction from self-review round two: at 3fe812a, ls("s3://bucket/?versionId=v1") of an empty bucket did not raise FileNotFoundError and did not send a HeadObject. The "/" in path fallback called _head_object(), which raises ValueError("The path has no key") before any request (checked with a stubbed client; the same held before this PR). The repair stands; its commit was reworded from 0de3190 to aaff8b1 (same tree) to state this, and the PR body now lists the ValueError → [] change.
| pytest.param(False, lambda fs: fs.ls("s3://bucket/d/key", refresh=True), id="ls"), | ||
| pytest.param( | ||
| False, | ||
| lambda fs: pytest.raises( |
There was a problem hiding this comment.
Self-review round one, finding 2 of 2.
FINDING: test_missing_object_drops_cached_parent_listing wrapped every lookup in contextlib.suppress(FileNotFoundError), so the ls case no longer asserted the raise and an unexpected FileNotFoundError from exists/isfile would have been hidden. Repaired in 0de3190: only the ls parameter expects the error with pytest.raises.
| # An empty bucket lists nothing, while a missing bucket raises | ||
| # from the listing. | ||
| if not files and key: | ||
| raise FileNotFoundError(path) |
There was a problem hiding this comment.
Self-review round one, deferrals.
- The empty-files check appears twice (HeadObject fallback and raise). Kept: one raise covers both the
versionsand listing branches. - A missing key whose cached parent listing lacks it still costs ListObjectsV2 + HeadObject before raising. The same two requests were sent before this PR to return
[]; answeringls()from the parent listing changes cache semantics and is out of scope. - The breaking change is recorded as a release note in the PR body;
docs/filesystem.mddoes not describels()of a missing path, so no docs change.
After the repair: just lint passed; offline test_s3.py 464 passed / 3 skipped; live test_ls_dirs + six *_directory_to_new_directory conformance tests 7 passed.
…GH-1330) A bucket path with a version query, such as bucket/?versionId=v1, keeps a "/" after the trailing slash is stripped, so ls() of such a path to an empty bucket took the HeadObject fallback, which raises ValueError for a path without a key before sending a request. ls() now decides whether the path has a key with S3Path.parse(), as the other lookups do, so that path lists an empty bucket as [] like the bucket path. test_missing_object_drops_cached_parent_listing expects the raise only in its ls() case instead of suppressing FileNotFoundError for every lookup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0de3190 to
aaff8b1
Compare
| # An empty bucket lists nothing, while a missing bucket raises | ||
| # from the listing. | ||
| if not files and key: | ||
| raise FileNotFoundError(path) |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations). Base 1a6a0d6, head aaff8b1 (tree identical to 0de3190). FINDINGS (claims only; no code change).
Claims checked with a stubbed client against this head (_serve_keys with dir/file, path bucket/dir/missing):
walk()(default) yields nothing;walk(on_error="raise"),listdir(),tree()raiseFileNotFoundError— as the PR body states.find→[],glob→[],du→ 0,exists/isdir→ False,info→FileNotFoundError,expand_path→['bucket/dir/missing'], with zerols()calls from these — confirms "do not callls()".AioS3FileSystem.walk/listdir/treeare fsspec'sAbstractFileSystemmethods (identity checked), so they follow the sync results; the async_walk()claim is from fsspec'sasyn.pysource (yields(path, [], [])on an omitted error), not run.- No request is added: the raise reads only the existing listing/HeadObject result.
- Docstring
Raises: a missing bucket raises from the listing (pre-existing translation ofNoSuchBucket). No docs page describesls()of a missing path (searcheddocs/filesystem.md,docs/s3fs.md,docs/aio.md).
CORRECTED: the PR body and the 0de3190 commit message said a bucket path with a version query "sends a HeadObject without a key" / was unchanged. Before this PR, ls("s3://bucket/?versionId=v1") of an empty bucket raised ValueError from _head_object() without a request; it now returns []. The commit was reworded (now aaff8b1, same tree, --force-with-lease), and the PR body lists this as a change.
Evidence limits: live S3 runs were local (test_ls_dirs + six conformance tests at 0de3190; the full conformance module at 3fe812a); the rest of just test pyathena runs in CI after Ready.
…H-1330) A key path such as dir/test_ is a key prefix of dir/test_0 in S3's sense, yet ls() raises for it, because only a directory, a path that some key starts with followed by "/", is listed. The docstring and the test comment now say directory instead of key prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| List of S3Object instances (if detail=True) or paths as strings (if detail=False). | ||
|
|
||
| Raises: | ||
| FileNotFoundError: If the bucket does not exist, or if a key path |
There was a problem hiding this comment.
Independent review (relayed, static). Reviewer: Codex CLI 0.164.0, model gpt-6-astra, codex exec -s read-only, session 01a178cf-4e17-7cf3-b831-2ea730a8f4a3. Base 1a6a0d6, head aaff8b1, run in a detached snapshot of that head; the prompt held the diff, source, and repository conventions, without the PR text, commits, or prior findings. No edits, tests, builds, or network access. The snapshot and PR worktree were unchanged afterwards.
Covered: the exact diff; bucket, object, prefix and version-qualified listings; versions, refresh, detail; cache lookup and eviction; async delegation, info, find/_find, internal callers; fsspec's walk, _walk, listdir, tree and the conformance fixtures; added and rewritten tests and the removed xfails ("The added tests protect distinct contracts; rewritten tests retain eviction coverage; conformance methods remain inherited with their fixtures and cleanup").
Result: FINDINGS.
Introduced:
- P3
pyathena/filesystem/s3.py:551: the newRaisestext conflates an S3 key prefix with a directory. With onlybucket/p/test_0,ls("bucket/p/test_")raises althoughp/test_is a key prefix of an existing key; the test comment attests/pyathena/filesystem/test_s3.py:4885says the same.
Pre-existing, not introduced by the diff (both reproduced by the author with a stubbed client):
- P2
pyathena/filesystem/s3.py:508: a missing-bucket listing error leaves a cached positive listing. Cache a populated bucket, delete it,ls(bucket, refresh=True)raisesFileNotFoundError, and the nextls(bucket)returns the deleted entries. - P2
pyathena/filesystem/s3.py:572: withbucket/key/childpresent and no versionv1ofbucket/key,ls("bucket/key?versionId=v1", refresh=True)returns the child listing without checking the version;_head_object()sees the version only when the listing is empty.
There was a problem hiding this comment.
Repair of the P3 finding in a8a020a: the Raises text now says a key path that "is neither an object nor a directory, a path that some key starts with followed by /", and the test_ls_dirs comment says "directory". The PR body uses the same wording.
Self-review of the repair. Round one: docstring and comment only, no executable code or assertion changed; just lint passed. Round two: the wording matches _ls_dirs() (lists key/), the exact-object HeadObject fallback, _ls_object_versions() (same / boundary, then exact-key versions), and a dir/ marker (a key that starts with dir/). CLEAN.
Independent follow-up (relayed, static): Codex CLI 0.164.0, gpt-6-astra, codex exec -s read-only, session 01a178d8-a1c9-7f70-a80b-e1e17ad932ac, range aaff8b1..a8a020a in a detached snapshot of a8a020a. Result: CLEAN — "Ordinary listing searches key/, then falls back to the exact object. Thus p/test_0 alone does not make p/test_ listable; the revised wording and fixture comment correctly describe this"; versions=True, a dir/-marker-only directory, empty and missing buckets were also checked.
The two pre-existing P2 findings are not changed by this PR; they are left for a separate issue.
WHAT
S3FileSystem.ls()andAioS3FileSystem._ls()now raiseFileNotFoundErrorfor a key path that is neither an object nor a directory (a path that some key starts with followed by/), instead of returning[]. This includes the start of an object's key (bucket/dir/miforbucket/dir/missing.txt) andls(..., versions=True)of a missing path. A bucket path is unchanged: an existing empty bucket lists as[], and a missing bucket raisesFileNotFoundErroras before. A bucket path that carries only a version query (bucket/?versionId=...) now also lists an empty bucket as[]; it raisedValueError("The path has no key") from the HeadObject fallback before. Paths that exist (a directory, adir/marker, an object) list as before.Whether the path has a key is decided with
S3Path.parse(), as the other lookups do. The check runs after the listing and the HeadObject lookup thatls()already made, so it adds no request. The cache handling is unchanged: an empty listing still evicts the cached listing of the path, and a missing object still evicts its cached entries and the parent listing that names it.The four fsspec conformance tests marked
xfail(strict=True)on GH-1330 (test_copy_directory_to_new_directoryandtest_put_directory_to_new_directory, for both filesystems) now pass, so the marks and their helper are removed fromtests/pyathena/filesystem/test_fsspec_conformance.py.Compatibility (release note)
A caller that lists a key path that may not exist and expects
[]now getsFileNotFoundError. Through the methods inherited from fsspec, this also changes:walk()of a missing path yields nothing instead of(path, [], [])with the defaulton_error="omit", and raises withon_error="raise". fsspec's async_walk()ofAioS3FileSystemstill yields(path, [], [])withon_error="omit".listdir()andtree()of a missing path raiseFileNotFoundError.find(),glob(),du(),info(),exists(),isdir(), andexpand_path()do not callls()and are unchanged;find()of a missing path still returns[].WHY
Closes #1330. Part of #1326.
fsspec's filesystems and s3fs raise
FileNotFoundErrorfor a missing path, and fsspec's conformance tests (#1329) depend on it.TEST
Tested at 0de3190 (on 1a6a0d6); aaff8b1 rewords its commit message only, with the same tree, and a8a020a changes only a docstring and a test comment.
just lint: passed.uv run --env-file .env pytest -n 8 tests/pyathena/filesystem/test_s3.py -k "TestS3FileSystem and not (test_ls_dirs or test_ls_buckets)": 464 passed, 3 skipped (offline).uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/test_s3.py::TestS3FileSystem::test_ls_dirs tests/pyathena/filesystem/test_fsspec_conformance.py -k "test_ls_dirs or directory_to_new_directory"on real S3: 7 passed (test_ls_dirsand the six*_directory_to_new_directorytests, including the four former xfails).test_fsspec_conformance.pyplustest_ls_dirson real S3 at the first commit 3fe812a (on ad48ad5): 273 passed. The second commit changes only how a bucket path is told from a key path, which none of these paths exercise.test_ls_versions_missing_path(new),test_ls_empty_refresh_evicts_cached_listing,test_refresh_evicts_cached_object_and_bucket_not_found, and thelscase oftest_missing_object_drops_cached_parent_listing(now expectFileNotFoundError) fail with thes3.pychange reverted.test_ls_empty_bucket(new) pins[]for an empty bucket, with and without a version query; its version-query case fails if the key path is decided by"/" in path.test_ls_dirsnow expectsFileNotFoundErrorfor the start of an object's key.just test pyathena); it runs in CI once the PR is Ready.🤖 Generated with Claude Code