From 3fe812a290c1582bdb0d03fa6438f54e23ceca8f Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Tue, 27 Oct 2026 00:10:13 +0900 Subject: [PATCH 1/3] Raise FileNotFoundError from ls() of a missing key path (GH-1330) 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 --- pyathena/filesystem/s3.py | 8 ++++ .../filesystem/test_fsspec_conformance.py | 44 ++----------------- tests/pyathena/filesystem/test_s3.py | 36 ++++++++++++--- 3 files changed, 42 insertions(+), 46 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 09c620f9d..a107defe0 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -547,6 +547,10 @@ def ls( Returns: 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 + is neither an object nor a key prefix of one. + Example: >>> fs = S3FileSystem() >>> fs.ls("s3://my-bucket") # List objects in bucket @@ -568,6 +572,10 @@ def ls( file = self._head_object(path, refresh=refresh) if file: files = [file] + # An empty bucket lists nothing, while a missing bucket raises from + # the listing. + if not files and "/" in path: + raise FileNotFoundError(path) return list(files) if detail else [f.name for f in files] def _ls_object_versions(self, path: str) -> list[S3Object]: diff --git a/tests/pyathena/filesystem/test_fsspec_conformance.py b/tests/pyathena/filesystem/test_fsspec_conformance.py index 2960174cd..93106b04d 100644 --- a/tests/pyathena/filesystem/test_fsspec_conformance.py +++ b/tests/pyathena/filesystem/test_fsspec_conformance.py @@ -8,7 +8,6 @@ """fsspec's conformance tests, run against the S3 filesystems on real S3.""" import contextlib -import functools import posixpath import uuid @@ -21,33 +20,6 @@ from tests.pyathena.conftest import connect -def _xfail_missing_ls(test): - """Mark an inherited conformance test as failing on GH-1330. - - The test is expected to fail with pytest's ``Failed``, as the - ``pytest.raises(FileNotFoundError)`` around ``ls()`` does when nothing is - raised, so other errors still fail it. The mark goes on a wrapper, so the - test function of fsspec's class stays unmarked. - - Args: - test: The test function of fsspec's abstract class. - - Returns: - The wrapped test, expected to fail. - """ - - @functools.wraps(test) - def wrapper(*args, **kwargs): - return test(*args, **kwargs) - - return pytest.mark.xfail( - reason="GH-1330: ls() of a missing key prefix returns [] instead of raising " - "FileNotFoundError", - raises=pytest.fail.Exception, - strict=True, - )(wrapper) - - class S3Fixtures(abstract.AbstractFixtures): @pytest.fixture(scope="class") def fs(self): @@ -88,9 +60,7 @@ def fs(self): class TestS3FileSystemCopy(abstract.AbstractCopyTests, S3Fixtures): - test_copy_directory_to_new_directory = _xfail_missing_ls( - abstract.AbstractCopyTests.test_copy_directory_to_new_directory - ) + pass class TestS3FileSystemGet(abstract.AbstractGetTests, S3Fixtures): @@ -98,9 +68,7 @@ class TestS3FileSystemGet(abstract.AbstractGetTests, S3Fixtures): class TestS3FileSystemPut(abstract.AbstractPutTests, S3Fixtures): - test_put_directory_to_new_directory = _xfail_missing_ls( - abstract.AbstractPutTests.test_put_directory_to_new_directory - ) + pass class TestS3FileSystemPipe(abstract.AbstractPipeTests, S3Fixtures): @@ -112,9 +80,7 @@ class TestS3FileSystemOpen(abstract.AbstractOpenTests, S3Fixtures): class TestAioS3FileSystemCopy(abstract.AbstractCopyTests, AioS3Fixtures): - test_copy_directory_to_new_directory = _xfail_missing_ls( - abstract.AbstractCopyTests.test_copy_directory_to_new_directory - ) + pass class TestAioS3FileSystemGet(abstract.AbstractGetTests, AioS3Fixtures): @@ -122,6 +88,4 @@ class TestAioS3FileSystemGet(abstract.AbstractGetTests, AioS3Fixtures): class TestAioS3FileSystemPut(abstract.AbstractPutTests, AioS3Fixtures): - test_put_directory_to_new_directory = _xfail_missing_ls( - abstract.AbstractPutTests.test_put_directory_to_new_directory - ) + pass diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index d6c6bd059..9d9d9b0c7 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -256,7 +256,9 @@ def call(method, **kwargs): fs._call.side_effect = call - lookup(fs) + # ls() raises FileNotFoundError for the missing path. + with contextlib.suppress(FileNotFoundError): + lookup(fs) assert ("bucket/d", "/") not in fs.dircache assert not fs.exists("s3://bucket/d/key") @@ -665,13 +667,23 @@ def test_ls_empty_refresh_evicts_cached_listing(self): self._serve_keys(fs, ()) - assert fs.ls("s3://bucket/dir", refresh=True) == [] + with pytest.raises(FileNotFoundError): + fs.ls("s3://bucket/dir", refresh=True) # The next listing is requested again instead of returning the # deleted object from the cache. fs._call.reset_mock() - assert fs.ls("s3://bucket/dir") == [] + with pytest.raises(FileNotFoundError): + fs.ls("s3://bucket/dir") assert fs._call.call_args_list[0].args[0] is fs._client.list_objects_v2 + def test_ls_empty_bucket(self): + # An existing bucket without objects is listed as empty, unlike a key + # path that matches nothing. + fs = self._make_fs() + self._serve_keys(fs, ()) + + assert fs.ls("s3://bucket") == [] + def test_find_withdirs_does_not_modify_cached_listing(self): fs = self._make_fs() fs.dircache[("bucket/dir", "")] = [self._file_object("dir/sub/file")] @@ -900,9 +912,11 @@ def call(method, **kwargs): fs._call.side_effect = call - assert fs.ls("s3://bucket/key", refresh=True) == [] + with pytest.raises(FileNotFoundError): + fs.ls("s3://bucket/key", refresh=True) # The next lookups must not return the deleted object and bucket from the cache. - assert fs.ls("s3://bucket/key") == [] + with pytest.raises(FileNotFoundError): + fs.ls("s3://bucket/key") assert not fs.exists("s3://bucket/key") with pytest.raises(FileNotFoundError): fs.info("s3://bucket", refresh=True) @@ -4113,6 +4127,14 @@ def test_ls_versions(self): assert actual[2].size == 2 assert fs.ls("s3://bucket/path", versions=True) == [f.name for f in actual] + def test_ls_versions_missing_path(self): + fs = self._make_fs() + fs.version_aware = True + fs._call.return_value = {"IsTruncated": False} + + with pytest.raises(FileNotFoundError): + fs.ls("s3://bucket/path/missing", versions=True) + def test_ls_versions_object_path_falls_back_to_the_key(self): fs = self._make_fs() fs.version_aware = True @@ -4855,7 +4877,9 @@ def test_ls_dirs(self, fs): assert len(fs.ls(f"{dir_}/prefix")) == 5 assert len(fs.ls(f"{dir_}/prefix/")) == 5 - assert len(fs.ls(f"{dir_}/prefix/test_")) == 0 + # The start of an object's key is neither an object nor a key prefix. + with pytest.raises(FileNotFoundError): + fs.ls(f"{dir_}/prefix/test_") assert len(fs.ls(f"{dir_}/prefix2")) == 1 test_1 = fs.ls(f"{dir_}/prefix/test_1") From aaff8b145387900481dd1a235ef95db42b54d38c Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Tue, 27 Oct 2026 00:20:45 +0900 Subject: [PATCH 2/3] Tell key paths by the parsed key and assert the ls() raise per lookup (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 --- pyathena/filesystem/s3.py | 24 +++++++++++++----------- tests/pyathena/filesystem/test_s3.py | 17 +++++++++++------ 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index a107defe0..26a56ab5f 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -564,18 +564,20 @@ def ls( path = self._strip_protocol(path).rstrip("/") if path in ["", "/"]: files = self._ls_buckets(refresh) - elif versions: - files = self._ls_object_versions(path) else: - files = self._ls_dirs(path, refresh=refresh) - if not files and "/" in path: - file = self._head_object(path, refresh=refresh) - if file: - files = [file] - # An empty bucket lists nothing, while a missing bucket raises from - # the listing. - if not files and "/" in path: - raise FileNotFoundError(path) + key = S3Path.parse(path).key + if versions: + files = self._ls_object_versions(path) + else: + files = self._ls_dirs(path, refresh=refresh) + if not files and key: + file = self._head_object(path, refresh=refresh) + if file: + files = [file] + # An empty bucket lists nothing, while a missing bucket raises + # from the listing. + if not files and key: + raise FileNotFoundError(path) return list(files) if detail else [f.name for f in files] def _ls_object_versions(self, path: str) -> list[S3Object]: diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 9d9d9b0c7..549119414 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -237,7 +237,13 @@ def test_info_does_not_use_listing_of_path(self): pytest.param( False, lambda fs: fs.exists("s3://bucket/d/key", refresh=True), id="exists" ), - pytest.param(False, lambda fs: fs.ls("s3://bucket/d/key", refresh=True), id="ls"), + pytest.param( + False, + lambda fs: pytest.raises( + FileNotFoundError, fs.ls, "s3://bucket/d/key", refresh=True + ), + id="ls", + ), # The listed entry has no version, so it is looked up again. pytest.param(True, lambda fs: fs.isfile("s3://bucket/d/key"), id="version_aware"), ], @@ -256,9 +262,7 @@ def call(method, **kwargs): fs._call.side_effect = call - # ls() raises FileNotFoundError for the missing path. - with contextlib.suppress(FileNotFoundError): - lookup(fs) + lookup(fs) assert ("bucket/d", "/") not in fs.dircache assert not fs.exists("s3://bucket/d/key") @@ -676,13 +680,14 @@ def test_ls_empty_refresh_evicts_cached_listing(self): fs.ls("s3://bucket/dir") assert fs._call.call_args_list[0].args[0] is fs._client.list_objects_v2 - def test_ls_empty_bucket(self): + @pytest.mark.parametrize("path", ["s3://bucket", "s3://bucket/?versionId=v1"]) + def test_ls_empty_bucket(self, path): # An existing bucket without objects is listed as empty, unlike a key # path that matches nothing. fs = self._make_fs() self._serve_keys(fs, ()) - assert fs.ls("s3://bucket") == [] + assert fs.ls(path) == [] def test_find_withdirs_does_not_modify_cached_listing(self): fs = self._make_fs() From a8a020a92fde131617f5ed2c612d426803c0639d Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Tue, 27 Oct 2026 00:33:01 +0900 Subject: [PATCH 3/3] Describe the missing ls() path as neither an object nor a directory (GH-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 --- pyathena/filesystem/s3.py | 3 ++- tests/pyathena/filesystem/test_s3.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 26a56ab5f..1d2456db0 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -549,7 +549,8 @@ def ls( Raises: FileNotFoundError: If the bucket does not exist, or if a key path - is neither an object nor a key prefix of one. + is neither an object nor a directory, a path that some key + starts with followed by ``/``. Example: >>> fs = S3FileSystem() diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 549119414..2bdef9302 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -4882,7 +4882,7 @@ def test_ls_dirs(self, fs): assert len(fs.ls(f"{dir_}/prefix")) == 5 assert len(fs.ls(f"{dir_}/prefix/")) == 5 - # The start of an object's key is neither an object nor a key prefix. + # The start of an object's key is neither an object nor a directory. with pytest.raises(FileNotFoundError): fs.ls(f"{dir_}/prefix/test_") assert len(fs.ls(f"{dir_}/prefix2")) == 1