Move tagging, ACL, metadata and presign requests into S3Core

[laughingman7743]

WHAT

This PR is the second implementation step of #1086. It covers sections 2, 3 and 5 of the agreed design.

It adds these public, synchronous S3Core operations:

• get_object_tagging(path, **params) -> dict[str, str]
• put_object_tagging(path, tags, **params)
• put_object_acl(path, acl, **params) and put_bucket_acl(bucket, acl, **params), which validate the canned ACL against S3Core.OBJECT_ACLS and S3Core.BUCKET_ACLS.
• replace_object_metadata(path, head, metadata, **params): one CopyObject request that copies the object onto itself with the REPLACE metadata directive. It retains the content headers, Expires, WebsiteRedirectLocation and StorageClass of head. Unless params set an encryption parameter, it also retains ServerSideEncryption, SSEKMSKeyId and BucketKeyEnabled. params take precedence over the retained fields, and a field that the copy itself sets raises TypeError.
• generate_presigned_url(path, client_method="get_object", expires_in=3600, **params) -> str: params take precedence over the path's bucket, key and version ID. request_kwargs are not signed.

The tagging and object ACL operations take the version ID from the path, including null.

S3FileSystem.get_tags(), put_tags(), chmod(), setxattr() and sign() now delegate to these operations. They keep their path checks and messages, the tag merge mode, recursive chmod() with up-front ACL validation, the HEAD in setxattr(), and cache invalidation. The requests they send are unchanged.

plan_multipart_copy() reads the source's tags with get_object_tagging().

The private S3FileSystem._SSE_COPY_PARAMS moves to S3Core.

The aio filesystem delegates these methods to the sync filesystem and is unchanged.

The "Typed S3 operations" section of docs/filesystem.md documents the contracts of the new operations, with examples.

Release note (4.0.0, breaking)

• S3FileSystem.OBJECT_ACLS and S3FileSystem.BUCKET_ACLS are removed. Use S3Core.OBJECT_ACLS and S3Core.BUCKET_ACLS, for example fs.core.OBJECT_ACLS. The MULTIPART_UPLOAD_* limits were removed the same way.

WHY

Refs #1086: finishing the public S3Core API extraction of #1053. The other inventory items are handled in separate PRs.

TEST

Tested code commit: 962ed09. The later commits 09e4bce and 3b445a1 change only docs/filesystem.md.

• just format, just lint, just docs lint: passed. just docs lint was rerun on 3b445a1.
• The new docs examples were run against Stubber offline. They send the documented requests, and an explicit RequestPayer is signed when it is passed to the call.
• New Stubber tests in tests/pyathena/filesystem/test_s3_core.py pin the requests independently of the implementation. They cover:
  • version IDs, including null
  • inherited and per-call parameters
  • tag order
  • ACL validation
  • retained metadata and encryption, and TypeError on fields of the copy
  • presigned URL precedence and local signing; request_kwargs are not signed
  • an S3Metadata passed as the replacement metadata
• New adapter tests test_put_tags_merge and test_put_tags_invalid_mode in test_s3.py also pass unchanged on master 855d4a7, which shows that the requests are unchanged.
• Offline run: uv run --env-file .env pytest --noconftest -p no:rerunfailures -q -n 8 tests/pyathena/filesystem/ with a placeholder .env and dummy keys. Master 855d4a7: 164 failed, 859 passed. This branch: 164 failed, 887 passed. The failing tests are identical on both; they need AWS fixtures.
• Live AWS run at 962ed09, with no overlapping Test workflow runs, using the main checkout's AWS .env: uv run --env-file <main checkout .env> pytest -n 8 tests/pyathena/filesystem/ tests/pyathena/s3fs/ tests/pyathena/aio/s3fs/ → 1156 passed, 1 skipped. This includes the integration tests of sign, setxattr, tags and chmod validation, and their aio versions.
• Not run: the full just test pyathena suite and just docs build, which builds committed refs. AWS CI runs when the PR is marked Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior): base 855d4a7, head c3a4e4f (first pass on 04f1cf8; repairs in c3a4e4f).

Covered: every changed file (s3_core.py, s3.py, docs/filesystem.md, test_s3_core.py, test_s3.py). For each adapter method (get_tags, put_tags o/m/invalid, chmod object/bucket/recursive, setxattr, sign, mkdir ACL check) and for the plan_multipart_copy GetObjectTagging, I compared the old and new request construction field by field. Precedence and duplicate-field TypeError are the same: fixed fields plus **params for tags, ACLs and copy, and params win over the path for presign. Validation order is the same (adapter key/version checks before the HEAD in setxattr; up-front ACL check before the recursive chmod). The aio adapter delegates these methods to the sync filesystem. No callers outside pyathena/filesystem/ use the removed names.

Result: FINDINGS, all repaired in c3a4e4f (from this pass and /code-review):

• This docstring claimed unset fields are not sent, but S3Metadata.storage_class reports STANDARD when HeadObject omits it, so StorageClass is always sent (as on master). Corrected the docstring.
• put_object_acl did not document FileNotFoundError. Added.
• OBJECT_ACLS/BUCKET_ACLS now use ClassVar, like the sibling constants.
• urlencode(tags) replaces urlencode(list(tags.items())), with the same output, and put_tags(mode="o") no longer copies the mapping.
• test_generate_presigned_url_signs_locally now asserts the versionId query instead of an endpoint host, which ambient AWS configuration could change.

Rejected or deferred (pre-existing or contrary to the agreed design):

• Pinning the self-copy with CopySourceIfMatch: master sends no condition, so adding one would change requests. It is a pre-existing race that is out of scope.
• Signing request_kwargs such as RequestPayer: master does not sign them, and the agreed design keeps that.
• Keeping S3FileSystem.OBJECT_ACLS/BUCKET_ACLS aliases: the maintainer approved their removal.
• Sharing the retained-header mapping with plan_multipart_copy: the field sets differ on purpose.
• Accepted deviation: a copy_kwargs key equal to a Python parameter name (path, head, metadata, source, destination) now raises TypeError instead of botocore's ParamValidationError for an unknown parameter. Both reject the input.

Validation: just lint passed. Offline pytest --noconftest tests/pyathena/filesystem/ failed the same 164 AWS-dependent tests on master and on this branch (859 → 887 passed).

[laughingman7743]

Self-review round 2 (claims, callers, operations): base 855d4a7, head c3a4e4f.

Claims checked:

• "The requests they send are unchanged": traced each operation. test_put_tags_merge and test_put_tags_invalid_mode also pass on master 855d4a7, unmodified, and the existing Stubber tests for setxattr, the mock tests for chmod, and the url() request test pass unchanged.
• Tag order and the merge semantics of mode m: dict order is preserved, and the merge yields the same TagSet as existing.update(tags).
• The core class docstring and docs "each operation sends one request" with the named exceptions: generate_presigned_url sends none (it is local and listed as an exception). Each new operation otherwise sends one request.
• Release note for the removed attributes: git grep finds no other users in the repository or docs.
• The S3Metadata STANDARD note: matches test_setxattr_omits_unset_system_metadata on master.

Existing callers:

• The public signatures and return shapes of get_tags/put_tags/chmod/setxattr/sign are unchanged.
• A subclass or test that overrides only S3FileSystem._call no longer intercepts these requests, which now go through core.call. This is the same trade-off as the earlier Separate an S3 core from the fsspec adapter in pyathena.filesystem #1053 steps, and _call stays a delegate.

AWS operator: the same retry layer (S3Core.call) and the same request count.

Documentation reader: docs/filesystem.md lists the new request kinds. The setxattr note is still accurate.

Evidence limits: static plus offline Stubber/mock evidence only. The live AWS tests (test_sign, test_metadata_getxattr_setxattr, test_get_and_put_tags, test_chmod_validation, and the aio equivalents) have not run on this head; they run when the PR is marked Ready. just docs build builds committed refs and was not used as evidence.

Result: CLEAN (no corrections needed beyond round one's repairs).

[laughingman7743]

Docs follow-up (09e4bce and 3b445a1, docs/filesystem.md only)

At the maintainer's request, the "Typed S3 operations" section now states the contracts of the new operations:

• put_object_tagging() replaces all tags. Tagging and object ACLs act on the version ID of the path, including null.
• ACLs are canned only and are checked against S3Core.OBJECT_ACLS and S3Core.BUCKET_ACLS; another value raises ValueError.
• replace_object_metadata() takes the head_object() result of the same path. It rewrites the object, or creates a new version in a versioned bucket. The section lists the retained fields, says that parameters take precedence over them, and says that any encryption parameter replaces the retained encryption.
• generate_presigned_url() signs locally, and its parameters take precedence over Bucket/Key/VersionId. request_kwargs such as RequestPayer are not signed unless they are passed as parameters.

The section also has two small examples.

Self-check:

• Documentation-reader and claim perspectives: every statement was checked against s3_core.py.
• The examples ran offline against Stubber with this code. They send the documented requests, and an explicit RequestPayer appears in the signed URL.
• just docs lint: 0 errors.

Independent follow-up:

• On 962ed09e..09e4bceb: Codex (codex exec -s read-only) CLEAN, static review. claude-fable-5-1 (profile max, effort high, plan mode) CLEAN, static review, with one optional note: "pass them to the call" could be read as S3Core.call().
• That wording was fixed in 3b445a1 ("pass them as parameters of generate_presigned_url()"). On 09e4bceb..3b445a18, Codex and Fable were both CLEAN.

The live AWS run by the coordinator at 962ed09 passed: pytest -n 8 tests/pyathena/filesystem/ tests/pyathena/s3fs/ tests/pyathena/aio/s3fs/ → 1156 passed, 1 skipped. The later commits change only the docs.

[laughingman7743]

Rebase onto master 5a0a5cb2e4894e1bb49edfc9bd587859dc97bf1d (after #1084 and #1091): 3b445a1841cd872069caa3c71bc6a484bd672212 → 5c233f8e04c8ba08328ee87aa2454d7fcfd88d20, force-pushed with an explicit lease.

• Range-diff (855d4a7b..3b445a18 vs 5a0a5cb2..5c233f8e): the five commits are unchanged except for the conflict in the intro paragraph of docs/filesystem.md.
  • The resolution keeps Move object GET and PUT requests into S3Core #1091's "read, write" and this PR's "tagging, ACL and metadata replacement".
  • It also keeps this PR's generate_presigned_url() sentence.
  • The source and test patches applied without conflicts.
• Upstream contracts checked:
  • Move object GET and PUT requests into S3Core #1091 changed cat_file, touch, pipe_file and the S3File GET/PUT paths. This PR does not touch those, and plan_multipart_copy() still reads tags through get_object_tagging().
  • Fix null-version moves in versioning-enabled S3 buckets #1084 changed mv() pairing. It does not use tags, ACLs, setxattr or sign.
  • No S3FileSystem.OBJECT_ACLS/BUCKET_ACLS references remain in pyathena/, tests/ or docs/.
• Validation at 5c233f8e:
  • just lint and just docs lint pass.
  • Offline pytest --noconftest -n 8 tests/pyathena/filesystem/ gives 927 passed, 164 failed and 9 skipped, against master 5a0a5cb2 with 899 passed, 164 failed and 9 skipped. The failing tests are identical on both and need AWS.
  • The full AWS suite runs in CI when the PR is marked Ready.
• Out of scope for this PR: Fix null-version moves in versioning-enabled S3 buckets #1084 added a get_bucket_versioning request and a call to the private S3Core._is_directory_bucket() in the adapter's _move_pairs (s3.py:1464). The final Finish the public S3Core API extraction from filesystem adapters #1086 SDK-call audit will cover them.

[laughingman7743]

Independent review: Codex (relayed)

Reviewer: Codex CLI, codex exec -s read-only, run on a detached snapshot of c3a4e4f. Scope: git diff 855d4a7b652d6dad106c27ee7112264ca45b923e..c3a4e4fa2588ce63b88612a22c93f95253446c85 and the touched sources, tests and docs. The prompt gave the intended behavior and no PR text. This was a static review; Codex ran nothing.

Codex covered all six new core methods, the filesystem adapters, the async and file wrappers, the tag read in the multipart copy, both test files, and the docs.

Result: FINDINGS (1). Codex found no adapter regressions.

• replace_object_metadata accepts a Mapping[str, str] but forwarded it unchanged as Metadata. botocore validates that map as a dict, so core.replace_object_metadata(path, head, head), with head an S3Metadata, raised ParamValidationError before sending CopyObject. The added tests covered only dicts.

Verified: confirmed. The new test fails with the old line and passes with the fix. This is a new-API defect only: setxattr() always passed a dict, so its requests are unchanged.

Repair in 962ed09: Metadata=dict(metadata) (this line), plus a Stubber case that passes an S3Metadata. The offline filesystem suite still has the same 164 AWS-dependent failures as master (887 passed).

[laughingman7743]

Repair follow-up (962ed09)

Self-review of the repair:

• Behavior: setxattr() already passed a dict, so dict(metadata) copies an equal dict and its CopyObject request is unchanged. The existing Stubber tests for setxattr still pass.
• Claims: the docstring's Mapping[str, str] contract now holds for an S3Metadata. A Metadata key in params still raises TypeError.

Independent follow-up on git diff c3a4e4fa2588ce63b88612a22c93f95253446c85..962ed09e9ed098a09d7833982bb9e7788908281c, with its callers and tests, run on a detached snapshot of 962ed09:

• Codex (codex exec -s read-only): CLEAN. "dict(metadata) supports the declared Mapping contract and preserves setxattr request contents." Static review only.
• claude-fable-5-1 (profile max, effort high, plan mode): CLEAN on this repair. See the other thread for its test note.

[laughingman7743]

Independent review: claude-fable-5-1 (relayed)

Reviewer: Claude Code with model claude-fable-5-1, profile max (claude auth status: claude.ai, firstParty), effort high, permission mode plan (read-only). It ran on the same detached snapshot of c3a4e4f with the same scope and prompt as the Codex review. This was a static review; nothing was executed.

Fable covered:

• the six new core methods and the moved constants
• the tag read of plan_multipart_copy
• the get_tags/put_tags/chmod/setxattr/sign/mkdir adapters: request equality, precedence, TypeError on duplicate fields, VersionId including null
• the aio delegation
• leftover references to the removed attributes
• the tests and docs

Result: CLEAN. Its non-actionable notes and their disposition:

1. put_tags(path, [(k, v)], mode="m") used to succeed through dict.update and now raises TypeError after the GET. This is outside the dict[str, str] contract, so it was accepted.
2. Python-name collisions now raise TypeError instead of botocore's ParamValidationError. Examples: copy_kwargs keys path/head/metadata/source/destination, sign(..., expires_in=...), chmod(..., bucket=...). Both reject the input before any write, so this was accepted. It was also noted in round 1.
3. The merge mode reads the tags through core.get_object_tagging, not an overridable get_tags(). The requests are identical, so this was accepted.
4. The claim "request_kwargs are not signed" was only checked with a mocked call. Repaired in 962ed09: test_generate_presigned_url_signs_locally now signs with request_kwargs={"RequestPayer": "requester"} on a real client and asserts that the URL has no x-amz-request-payer. I checked separately that an explicit RequestPayer does add that query parameter, so the assertion can fail.
5. The debug message changed from "Get tags to copy" to "Get object tagging". Accepted as harmless.

Fable found no pre-existing issues in the changed area.

[laughingman7743]

Repair follow-up (962ed09)

test_generate_presigned_url_signs_locally now signs on a real client that has request_kwargs={"RequestPayer": "requester"}. The test asserts that the URL has no x-amz-request-payer query key and still carries versionId=v1. The production code is unchanged.

Independent follow-up on git diff c3a4e4fa2588ce63b88612a22c93f95253446c85..962ed09e9ed098a09d7833982bb9e7788908281c:

• claude-fable-5-1 (profile max, effort high, plan mode): CLEAN. Contrary to the read-only instruction, it ran two offline boto3 signing snippets to check the URL shape, and it ran no tests.
• Codex (codex exec -s read-only): CLEAN.

Non-blocking note from Fable, deferred: the new assertion depends on the signature version. If an ambient ~/.aws/config selected signature_version = s3v4 for S3, a signed RequestPayer would appear in X-Amz-SignedHeaders instead of as a query key. The default client uses SigV2 query signing, where the key does appear; I checked this by signing an explicit RequestPayer. test_generate_presigned_url independently pins the exact Params that are signed, whatever the signature version.