Skip to content

Commit 23a54e1

Browse files
Merge pull request #1098 from pyathena-dev/refactor/1086-bucket-versioning
Move the bucket versioning request into S3Core
2 parents 47fb6f3 + e12d5d5 commit 23a54e1

7 files changed

Lines changed: 80 additions & 27 deletions

File tree

‎docs/filesystem.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,9 @@ upload_url = core.generate_presigned_url(path, "put_object", ContentType="text/c
401401
`create_bucket()` and `delete_bucket()` create and delete a bucket when called. The
402402
`allow_bucket_creation` and `allow_bucket_deletion` options apply only to the
403403
filesystem's `mkdir`/`makedirs` and `rmdir`, not to calls through the core.
404+
`get_bucket_versioning()` returns the versioning state of a bucket, `Enabled` or
405+
`Suspended`, or `None` if versioning has never been enabled on it.
406+
`S3Path.is_directory_bucket` tells whether the bucket of a path is a directory bucket.
404407

405408
`create_bucket()` sends a `LocationConstraint` for the `region_name` argument, or for
406409
the client's region by default, except in `us-east-1`. The fields of a

‎pyathena/filesystem/s3.py‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1438,15 +1438,10 @@ def _move_pairs(
14381438
buckets = {
14391439
p.bucket
14401440
for p in paths.values()
1441-
if p.version_id == "null"
1442-
and p.name in unversioned
1443-
and not self.core._is_directory_bucket(p.bucket)
1441+
if p.version_id == "null" and p.name in unversioned and not p.is_directory_bucket
14441442
}
14451443
versioned_buckets = {
1446-
bucket
1447-
for bucket in buckets
1448-
if self._call(self._client.get_bucket_versioning, Bucket=bucket).get("Status")
1449-
== "Enabled"
1444+
bucket for bucket in buckets if self.core.get_bucket_versioning(bucket) == "Enabled"
14501445
}
14511446
missing = {
14521447
source

‎pyathena/filesystem/s3_core.py‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -835,6 +835,23 @@ def delete_bucket(self, bucket: str, **params) -> None:
835835
_logger.debug(f"Delete bucket: s3://{bucket}")
836836
self.call(self._client.delete_bucket, Bucket=bucket, **params)
837837

838+
def get_bucket_versioning(self, bucket: str, **params) -> str | None:
839+
"""Get the versioning state of a bucket with GetBucketVersioning.
840+
841+
Args:
842+
bucket: The name of the bucket.
843+
**params: Additional request parameters, sent as given.
844+
845+
Returns:
846+
``Enabled`` or ``Suspended``, or None if versioning has never
847+
been enabled on the bucket.
848+
849+
Raises:
850+
FileNotFoundError: If the bucket does not exist.
851+
"""
852+
response = self.call(self._client.get_bucket_versioning, Bucket=bucket, **params)
853+
return cast(str | None, response.get("Status"))
854+
838855
def delete_object(self, path: S3Path, **params) -> None:
839856
"""Delete an object, or a version of it, with DeleteObject.
840857
@@ -1266,7 +1283,7 @@ def plan_multipart_copy(
12661283
request.pop("Tagging", None)
12671284
# Directory buckets do not support GetObjectTagging, and their
12681285
# objects have no tags.
1269-
if not self._is_directory_bucket(source.bucket):
1286+
if not source.is_directory_bucket:
12701287
tags = self.get_object_tagging(
12711288
source, **self.operation_params("get_object_tagging", source_params)
12721289
)
@@ -1290,7 +1307,7 @@ def plan_multipart_copy(
12901307
)
12911308
if annotation_directive == "COPY"
12921309
and "CopySourceSSECustomerAlgorithm" not in params
1293-
and not self._is_directory_bucket(source.bucket)
1310+
and not source.is_directory_bucket
12941311
else ()
12951312
)
12961313
return S3MultipartCopyPlan(
@@ -1604,20 +1621,6 @@ def generate_presigned_url(
16041621
),
16051622
)
16061623

1607-
@staticmethod
1608-
def _is_directory_bucket(bucket: str) -> bool:
1609-
"""Return whether the bucket is a directory bucket (S3 Express One Zone).
1610-
1611-
Directory bucket names end with ``--x-s3``.
1612-
1613-
Args:
1614-
bucket: S3 bucket name.
1615-
1616-
Returns:
1617-
True if the bucket is a directory bucket.
1618-
"""
1619-
return bucket.endswith("--x-s3")
1620-
16211624
@staticmethod
16221625
def _copy_source_params(params: Mapping[str, Any]) -> dict[str, Any]:
16231626
"""Map the parameters of a copy to those of the requests that read its source.

‎pyathena/filesystem/s3_path.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,14 @@ def is_bucket(self) -> bool:
123123
"""Whether the path names the bucket: it has no key, or a key of only slashes."""
124124
return not self.key or not self.key.strip("/")
125125

126+
@property
127+
def is_directory_bucket(self) -> bool:
128+
"""Whether the bucket is a directory bucket (S3 Express One Zone).
129+
130+
The names of directory buckets end with ``--x-s3``.
131+
"""
132+
return self.bucket.endswith("--x-s3")
133+
126134
@property
127135
def name(self) -> str:
128136
"""The path without a scheme or version, in ``bucket/key`` form, or the bucket."""

‎tests/pyathena/filesystem/test_s3_async.py‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,7 +1036,7 @@ async def copy_file(path1, path2, **kwargs):
10361036
@pytest.mark.asyncio
10371037
async def test_mv_null_version_onto_key(self, status):
10381038
fs = AioS3FileSystem(connection=mock.MagicMock(), skip_instance_cache=True)
1039-
fs._sync_fs._call = mock.MagicMock(
1039+
fs._sync_fs._call = fs._sync_fs._core.call = mock.MagicMock(
10401040
return_value={} if status is None else {"Status": status}
10411041
)
10421042
fs._copy_file = mock.AsyncMock(return_value=True)
@@ -1063,7 +1063,7 @@ async def test_mv_null_version_onto_key(self, status):
10631063
@pytest.mark.asyncio
10641064
async def test_mv_null_version_conflicts_depend_on_bucket_state(self, status):
10651065
fs = AioS3FileSystem(connection=mock.MagicMock(), skip_instance_cache=True)
1066-
fs._sync_fs._call = mock.MagicMock(
1066+
fs._sync_fs._call = fs._sync_fs._core.call = mock.MagicMock(
10671067
return_value={} if status is None else {"Status": status}
10681068
)
10691069
fs._copy_file = mock.AsyncMock(return_value=True)
@@ -1090,7 +1090,7 @@ async def test_mv_null_version_conflicts_depend_on_bucket_state(self, status):
10901090
@pytest.mark.asyncio
10911091
async def test_mv_null_version_directory_bucket_does_not_read_bucket_state(self):
10921092
fs = AioS3FileSystem(connection=mock.MagicMock(), skip_instance_cache=True)
1093-
fs._sync_fs._call = mock.MagicMock()
1093+
fs._sync_fs._call = fs._sync_fs._core.call = mock.MagicMock()
10941094
fs._copy_file = mock.AsyncMock(return_value=True)
10951095
fs._delete_objects = mock.AsyncMock()
10961096
key = "s3://example--usw2-az1--x-s3/key"
@@ -1105,7 +1105,9 @@ async def test_mv_null_version_directory_bucket_does_not_read_bucket_state(self)
11051105
@pytest.mark.asyncio
11061106
async def test_mv_null_version_failure_does_not_delete(self, stage):
11071107
fs = AioS3FileSystem(connection=mock.MagicMock(), skip_instance_cache=True)
1108-
fs._sync_fs._call = mock.MagicMock(return_value={"Status": "Enabled"})
1108+
fs._sync_fs._call = fs._sync_fs._core.call = mock.MagicMock(
1109+
return_value={"Status": "Enabled"}
1110+
)
11091111
fs._copy_file = mock.AsyncMock(return_value=True)
11101112
fs._delete_objects = mock.AsyncMock()
11111113
if stage == "lookup":

‎tests/pyathena/filesystem/test_s3_core.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,36 @@ def test_delete_bucket(self):
425425
core.delete_bucket("bucket")
426426
stubber.assert_no_pending_responses()
427427

428+
@pytest.mark.parametrize(
429+
("response", "expected"),
430+
[
431+
({"Status": "Enabled"}, "Enabled"),
432+
({"Status": "Suspended", "MFADelete": "Disabled"}, "Suspended"),
433+
# A bucket whose versioning has never been enabled.
434+
({}, None),
435+
],
436+
)
437+
def test_get_bucket_versioning(self, response, expected):
438+
core, stubber = _make_core(request_kwargs={"RequestPayer": "requester"})
439+
# RequestPayer, which GetBucketVersioning does not accept, is not sent.
440+
stubber.add_response(
441+
"get_bucket_versioning",
442+
response,
443+
{"Bucket": "bucket", "ExpectedBucketOwner": "123456789012"},
444+
)
445+
with stubber:
446+
status = core.get_bucket_versioning("bucket", ExpectedBucketOwner="123456789012")
447+
stubber.assert_no_pending_responses()
448+
assert status == expected
449+
450+
def test_get_bucket_versioning_translates_errors(self):
451+
core, stubber = _make_core()
452+
stubber.add_client_error(
453+
"get_bucket_versioning", service_error_code="NoSuchBucket", http_status_code=404
454+
)
455+
with stubber, pytest.raises(FileNotFoundError):
456+
core.get_bucket_versioning("bucket")
457+
428458
def test_list_objects(self):
429459
core, stubber = _make_core()
430460
stubber.add_response(

‎tests/pyathena/filesystem/test_s3_path.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,18 @@ def test_parse_invalid(self, path):
6565
def test_is_bucket(self, path, expected):
6666
assert path.is_bucket is expected
6767

68+
@pytest.mark.parametrize(
69+
("path", "expected"),
70+
[
71+
(S3Path("bucket--usw2-az1--x-s3", "key"), True),
72+
(S3Path("bucket--usw2-az1--x-s3"), True),
73+
(S3Path("bucket", "key--x-s3"), False),
74+
(S3Path("bucket--x-s3-other"), False),
75+
],
76+
)
77+
def test_is_directory_bucket(self, path, expected):
78+
assert path.is_directory_bucket is expected
79+
6880
@pytest.mark.parametrize(
6981
("path", "name", "string", "uri"),
7082
[

0 commit comments

Comments
 (0)