diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 09c620f9d..1d2456db0 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -547,6 +547,11 @@ 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 directory, a path that some key + starts with followed by ``/``. + Example: >>> fs = S3FileSystem() >>> fs.ls("s3://my-bucket") # List objects in bucket @@ -560,14 +565,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] + 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_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..2bdef9302 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"), ], @@ -665,13 +671,24 @@ 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 + @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(path) == [] + 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 +917,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 +4132,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 +4882,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 directory. + 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")