Skip to content

Move the bucket versioning request into S3Core - #1098

Merged
laughingman7743 merged 1 commit into
masterfrom
refactor/1086-bucket-versioning
Oct 5, 2026
Merged

laughingman7743 merged 1 commit into
masterfrom
refactor/1086-bucket-versioning

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

WHAT

This is the fourth and last implementation PR for #1086, following the addendum.
It moves the two pieces that #1084 added to S3FileSystem._move_pairs() after the #1086 inventory was recorded.

  • S3Core.get_bucket_versioning(bucket, **params) -> str | None sends one GetBucketVersioning request.
    • It returns "Enabled", "Suspended", or None for a bucket whose versioning has never been enabled.
    • A missing bucket raises FileNotFoundError.
  • The new public property S3Path.is_directory_bucket says whether the path's bucket is a directory bucket (a name ending with --x-s3).
    • It replaces the private S3Core._is_directory_bucket() staticmethod, which plan_multipart_copy() also used.
  • _move_pairs() uses both.
    • The requests, request counts and errors are unchanged: the same GetBucketVersioning per distinct bucket, sent through S3Core.call() as before.
  • docs/filesystem.md documents both.
  • The aio mv tests that mocked only _sync_fs._call now share the mock with _sync_fs._core.call, as the other tests do.
    • With the request sent by the core, the old wiring left test_mv_null_version_onto_key failing.
    • It would also have let test_mv_null_version_directory_bucket_does_not_read_bucket_state pass without observing anything.

WHY

Refs #1086.
#1084 merged after the inventory, so these were the last S3 request and S3 rule built in the adapter.

TEST

Tested commit: e12d5d5.

  • just format, just lint and just docs lint pass.
  • Offline: uv run --env-file <placeholder env> pytest --noconftest -p no:rerunfailures -n 8 tests/pyathena/filesystem/
    • This PR: 966 passed, 164 failed, 9 skipped.
    • Master 47fb6f31: 958 passed, 164 failed, 9 skipped.
    • The failing tests are identical on both, and all need AWS fixtures.
    • The 8 added tests are 4 for is_directory_bucket and 4 for get_bucket_versioning.
  • The new Stubber tests pin:
    • the request, with inherited RequestPayer filtered out and ExpectedBucketOwner passed through;
    • the three response states;
    • the NoSuchBucket → FileNotFoundError translation.
  • The existing sync and aio mv null-version tests pin the adapter's request sequence.
  • Live AWS: uv run --env-file .env pytest -n 8 tests/pyathena/filesystem/ tests/pyathena/s3fs/ tests/pyathena/aio/s3fs/ → 1235 passed, 10 skipped. No other Test run overlapped.

🤖 Generated with Claude Code

#1084 added a GetBucketVersioning request built in S3FileSystem and a
call to the private S3Core._is_directory_bucket(). As agreed on #1086,
add S3Core.get_bucket_versioning(), which returns the versioning status
or None, and the public S3Path.is_directory_bucket property, which also
replaces the private helper in plan_multipart_copy().

The aio mv tests that mocked only the sync filesystem's _call now share
the mock with its core, as the other tests do, so that they keep
observing the request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743 laughingman7743 added this to the 4.0.0 milestone Oct 4, 2026
Comment thread pyathena/filesystem/s3.py
for bucket in buckets
if self._call(self._client.get_bucket_versioning, Bucket=bucket).get("Status")
== "Enabled"
bucket for bucket in buckets if self.core.get_bucket_versioning(bucket) == "Enabled"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior): CLEAN

  • Base 47fb6f31749f505a723c314b03ebf1ca3c5b8114, head e12d5d584c0f5b6f6ff7944585ca66941be18e1a.
  • Requests: _move_pairs() sends the same GetBucketVersioning, once per distinct candidate bucket.
    • Before: self._call(self._client.get_bucket_versioning, Bucket=...), which delegated to S3Core.call().
    • Now: S3Core.get_bucket_versioning(), which sends self.call(self._client.get_bucket_versioning, Bucket=..., **params).
    • The client, the retry policy, the filtering of inherited request_kwargs and the error translation are therefore unchanged.
  • Result: the comparison with "Enabled" is unchanged; None and "Suspended" still mean the bucket is not versioning-enabled.
  • Directory buckets: S3Path.is_directory_bucket applies the same --x-s3 suffix rule to the same bucket name at all three former call sites. These are the adapter's candidate filter and the two checks in plan_multipart_copy().
  • Tests: the aio mv tests now set _sync_fs._call = _sync_fs._core.call, as the other tests do.
    • With the request sent by the core, the old wiring made test_mv_null_version_onto_key fail.
    • It also made the directory-bucket test pass without observing the request.
  • Callers: no other code used S3Core._is_directory_bucket() (repository grep).
  • Limitation: offline only at this head. A live run is in progress.

FileNotFoundError: If the bucket does not exist.
"""
response = self.call(self._client.get_bucket_versioning, Bucket=bucket, **params)
return cast(str | None, response.get("Status"))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers, operations): CLEAN

  • Base 47fb6f31749f505a723c314b03ebf1ca3c5b8114, head e12d5d584c0f5b6f6ff7944585ca66941be18e1a.
  • I checked these claims:
    • "None for a bucket whose versioning has never been enabled": GetBucketVersioning omits Status in that state, according to the botocore model docs and the S3 API reference. Stubber pins the empty response.
    • "A missing bucket raises FileNotFoundError": S3ClientError maps NoSuchBucket to it, and Stubber pins this.
    • "ExpectedBucketOwner is passed through and RequestPayer is filtered": the operation's input shape has only Bucket and ExpectedBucketOwner, measured with botocore 1.43.102.
    • "No change of requests": the existing sync mv tests assert fs._call.assert_called_once_with(fs._client.get_bucket_versioning, Bucket="bucket") through the shared core mock, and they pass unchanged.
  • The docs sentence matches the docstring.
  • Breaking-change check: the private staticmethod is removed. It was underscore-private and only the core used it, so no release note is needed.

@laughingman7743
laughingman7743 force-pushed the refactor/1086-bucket-versioning branch from 5c66f58 to e12d5d5 Compare October 4, 2026 16:33
return not self.key or not self.key.strip("/")

@property
def is_directory_bucket(self) -> bool:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review and /code-review at e12d5d584c0f5b6f6ff7944585ca66941be18e1a (base 47fb6f31749f505a723c314b03ebf1ca3c5b8114, detached snapshot, static only)

  • Codex (codex exec -s read-only): CLEAN. It covered the versioning request parameters, the status returns and error translation, the sync/aio move behavior and lookup counts, directory-bucket detection, the multipart-copy guards, the tests and the docs.
  • claude-fable-5-1 (max profile, effort high): CLEAN. It confirmed that the request, the request count, the retry policy, the error translation and the == "Enabled" result are unchanged. It also confirmed that the four aio tests are the only ones that mocked _call alone.
  • /code-review: no regression. Nine design, test-precision and docs notes, none changed:
    • The name is_directory_bucket is kept. "Directory bucket" is the AWS term, and the docstring already says it concerns the bucket of the path. The maintainer decided this.
    • Docstring and docs additions about directory-bucket support and paragraph placement were judged unnecessary and reverted. Other core operations do not list S3 feature support either.
    • The str | None return was agreed on Finish the public S3Core API extraction from filesystem adapters #1086.
    • Not changed because they are pre-existing behavior or out of scope: sequential GetBucketVersioning per bucket, --xa-s3 access point aliases, and test assertions through the shared _call alias.
  • Live AWS at e12d5d584c0f5b6f6ff7944585ca66941be18e1a: pytest -n 8 tests/pyathena/filesystem/ tests/pyathena/s3fs/ tests/pyathena/aio/s3fs/ → 1235 passed, 10 skipped.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 16:37
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 16:47
@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 18:47
@laughingman7743
laughingman7743 merged commit 23a54e1 into master Oct 5, 2026
37 of 38 checks passed
@laughingman7743
laughingman7743 deleted the refactor/1086-bucket-versioning branch October 5, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant