Route S3 request parameters to the operations that accept them

[laughingman7743]

WHAT

Route S3 request parameters to the operations that accept them, apply the parameters of a call consistently, and keep the open() parameters of put() and cp() away from S3.

• Inherited parameters are filtered per operation. A new S3FileSystem._get_operation_kwargs(method, kwargs) keeps the parameters that are members of the operation's botocore input shape (client.meta.service_model.operation_model(...).input_shape.members). It returns nothing for a method that is not an S3 operation, such as generate_presigned_url. It applies to the parameters that several requests inherit:
  • requester_pays=True: _call() adds RequestPayer only to the operations that accept it. A RequestPayer given to a call takes precedence instead of raising a duplicate-keyword TypeError (Per-file request parameters are not applied consistently to multipart upload requests #946).
  • The parameters of an S3File: each request (GetObject, PutObject, CreateMultipartUpload, UploadPart, UploadPartCopy, CompleteMultipartUpload, AbortMultipartUpload) receives those it accepts. _finish_multipart_upload() takes them as a request_kwargs mapping, so a parameter named like one of its arguments cannot collide with it. For example, ServerSideEncryption is no longer sent with GetObject, and RequestPayer, ExpectedBucketOwner and SSE-C now reach the part requests, the completion and the abort (Per-file request parameters are not applied consistently to multipart upload requests #946). This replaces the hand-written allowlist in discard().
  • The filesystem's s3_additional_kwargs on pipe_file()'s single PutObject.
  • The parameters of cp_file()'s multipart copy: CreateMultipartUpload still gets them all, as before; the part copies, the completion and the abort get those that they accept.
• Parameters given to a single request are not filtered. Examples are metadata(path, **kwargs), cp_file() on its CopyObject path, and pipe_file() on its single-request path. botocore still validates them, so a misspelled parameter still raises ParamValidationError there.
• The fields that a request sets itself win. The request helpers (_get_object, _put_object, _create_multipart_upload, _upload_part, _upload_part_copy, _complete_multipart_upload) and both aborts merge inherited parameters first and their own Bucket/Key/UploadId/PartNumber/Body/... last, so an inherited parameter of the same name cannot collide with them or leave an upload behind. A field the request does not set (e.g. VersionId without version_id) still applies.
• Precedence and no mutation. _open() (sync and async) and S3File.__init__ build new dictionaries. The parameters of the call take precedence over the filesystem's s3_additional_kwargs, and the caller's dictionary is no longer modified. Before, a read added IfMatch to it, and an append added the object's metadata.
• Keyword parameters of open() are added to the file's parameters and take precedence over its s3_additional_kwargs (S3File(**kwargs) was ignored). So pipe_file(..., ContentType=...) keeps the parameter on its buffered path (data larger than the block size, or inside a transaction).
• put_file() (sync, and AioS3FileSystem's in-transaction path, which keeps Check the part limit in AioS3FileSystem transaction writes #999's part-limit check) passes block_size and max_workers to open() and the other parameters to S3. It also accepts s3_additional_kwargs, as open() and pipe_file() do. ContentType is guessed from the file extension only when neither the call nor the filesystem's s3_additional_kwargs gives one, so a configured value is kept.
• cp_file() (sync and async) uses block_size and max_workers only for a multipart copy and never sends them to S3 (cp_file() sends block_size and max_workers to S3 for some object sizes #967). The async multipart copy now accepts max_workers and runs at most that many part copies at once; it defaults to the filesystem's max_workers.
• Docs: docs/filesystem.md describes how request parameters are given and routed, including the small pipe that sends its parameters as given.

Release-note items:

• requester_pays=True no longer breaks bucket operations (exists/info/isdir of a bucket, ls(""), mkdir/rmdir of a bucket, chmod of a bucket) or sign().
• A filesystem-level s3_additional_kwargs with write parameters (for example ServerSideEncryption) no longer breaks reads.
• The parameters given to open(), pipe() or put() take precedence over the filesystem's s3_additional_kwargs; before, the filesystem's won.
• Keyword S3 parameters of open() are applied; before, they were ignored.
• Parameters of a file (from open(), pipe(), put() or the filesystem's s3_additional_kwargs) that no request of the file accepts are no longer sent. Before, an unknown or misspelled one raised ParamValidationError at the first request.
• Multipart uploads and copies send RequestPayer, ExpectedBucketOwner and SSE-C parameters with their part, completion and abort requests.
• cp() with block_size or max_workers works for objects of any size, sync and async.
• put() passes max_workers (and, in an async transaction, block_size) to the file instead of S3, and accepts s3_additional_kwargs.
• The async multipart copy runs at most max_workers (the filesystem's by default) part copies at once; before, it scheduled them all, limited only by the event loop's default executor.

WHY

Closes #969, closes #946, closes #967.

The issues share one cause: parameters inherited by several requests were sent unchanged to every request, or not at all. The routing follows the agreed design: filter the inherited parameters by the botocore input shape of each operation, keep the parameters of a single request as they are, and add open()'s keyword parameters to the file's parameters.

The issue lists the guessed ContentType of put_file() being lost to the filesystem's as a defect. This PR instead keeps an explicitly configured filesystem ContentType over the guess (the independent review's finding): a guess is a fallback, and a configured value is the user's choice.

The put_file() part also addresses the #969 comment about max_workers (and, in the async transaction path from #988, block_size) reaching PutObject.

Not changed here:

• The append path still lets the existing object's attributes (to_api_repr()) override the parameters of the call, as before.
• S3File's lookups at open time (info(), exists(), and cat() of a small object to append to) do not receive the file's parameters, so, e.g., an uncached SSE-C object still cannot be opened for reading. The info() cache is not keyed by request parameters, so this needs its own change: S3File does not send its request parameters with the lookups made while opening #1004.
• The async multipart copy still does not abort on failure (Multipart copies drop the source metadata, and the async multipart copy does not abort on failure #973).
• A multipart copy (> 5 GiB) still passes all its parameters to CreateMultipartUpload, so CopyObject-only parameters such as CopySourceIfMatch or MetadataDirective fail validation there. Filtering them was tried (2d26f95) and reverted: it made MetadataDirective="COPY"/TaggingDirective="COPY" succeed without copying anything. How a multipart copy carries CopyObject's metadata, tags and conditions belongs to Multipart copies drop the source metadata, and the async multipart copy does not abort on failure #973.

TEST

Tested commit: 882ae2f (rebased onto 55af09a, which adds #986, #999 and #998; 2d26f95 and its revert cancel out).

• just format, just lint, just docs lint: pass.
• Offline unit tests (dummy region and credentials, --noconftest). They use a real botocore client, whose service model selects the parameters, and mocks or botocore's Stubber for the requests:
  • test_get_operation_kwargs: GetObject drops ServerSideEncryption, HeadBucket drops RequestPayer, UploadPart keeps SSE-C but not ContentType, and generate_presigned_url gets nothing.
  • test_requester_pays (Stubber): info() of a bucket sends HeadBucket without RequestPayer; metadata() sends it, also when given explicitly; sign() works.
  • test_open_s3_additional_kwargs: precedence, keyword parameters, per-operation selection for reads and writes, and the caller's dictionary unchanged.
  • test_pipe_file_buffered_s3_parameters (large data, and inside a transaction), test_put_file_open_parameters (Stubber), TestAioS3FileSystem::test_put_file_in_transaction_open_parameters.
  • test_finish_multipart_upload_request_parameters, TestS3File::test_multipart_write_request_parameters, test_copy_object_with_multipart_upload_request_parameters.
  • test_cp_file_multipart_parameters (sync and async, both size branches). The async case also checks that max_workers=1 runs one part copy at a time.
  • TestS3File::test_multipart_write_keyword_named_as_argument (a key= keyword of the file) and test_put_file_content_type (call, guessed, and filesystem-configured ContentType).
  • test_open_parameters_named_as_request_fields (a file with Key/UploadId/PartNumber parameters, through the real helpers, for a completed and an aborted upload).
  • With the source changes reverted, all of these fail, except the sync cp_file case above 5 GiB, which already passed block_size and max_workers to the multipart copy.
  • Updated tests: test_put_file_block_size and the async test_transaction_put_file_block_size (from Check the part limit in AioS3FileSystem transaction writes #999) now expect max_workers passed to open(). test_transaction_pipe_put_file uses a real client instead of a MagicMock connection, whose service model cannot select parameters. The mocked filesystems in TestS3FileSystem._make_fs and TestS3File use the real service model.
  • The whole tests/pyathena/filesystem/ directory offline has the same failures as the base (only the AWS integration tests that need credentials).
• Issue reproduction (real client, _call replaced): the caller's dictionary stays {'ExpectedBucketOwner': '111122223333'}; the later PutObject has no IfMatch; GetObject has no ServerSideEncryption; CreateMultipartUpload of the large pipe_file() has ContentType.
• AWS integration tests: uv run --env-file .env pytest -n 4 -p no:cacheprovider tests/pyathena/filesystem/ (local, against the CI account) at 882ae2f: 421 passed (earlier: 412 at 021ba88, 392 at 27112b3).
• Not covered against real S3: requester-pays buckets and SSE-C uploads (the CI bucket uses neither), and filtering, which uses the botocore model from uv.lock.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior) — CLEAN

Scope: git diff 92c9e3e67180eb3d52ef7b85579a24540a30fd64..57f7853ea41e72dc295467139e57a1f77be042c2 (base = merge-base with master, head = published head). Covered: _get_operation_kwargs() and _call() (requester_pays merge, explicit-wins, non-API methods), every S3File request (GetObject in both _fetch_range branches, PutObject, the empty-file touch(), CreateMultipartUpload, UploadPart, both UploadPartCopy branches of an append, the completion and abort through _finish_multipart_upload(), discard()), _open() sync/async, pipe_file() single and buffered paths, put_file() sync and the async in-transaction path, cp_file() sync/async in both size branches, the async multipart copy semaphore, and the docs.

Checked without findings:

• _call() resolves the operation from func.__name__ only when requester_pays is set; botocore client methods carry their Python operation name, and both string and bound-method callers resolve. Without requester_pays, the request is unchanged.
• A parameter given to a single request (e.g. metadata(path, **kwargs), CopyObject) is not filtered, so botocore still rejects a misspelled one; only inherited parameters are filtered.
• S3File keeps its own merged dict; read mode's IfMatch and append's to_api_repr() now update that copy only. Append keeps the existing object's attributes over the call's, as before (stated in the PR).
• put_file() passes block_size/max_workers to open(); max_workers defaults to the filesystem's, so open() never gets None.
• Tests with MagicMock clients cannot select parameters (the mock model has no members); _make_fs, TestS3File's mocked filesystems and test_transaction_pipe_put_file now use the real service model, and the offline failures equal the base's (only credential-less integration tests).
• Live: tests/pyathena/filesystem/ at 57f7853: 386 passed.

Reasoned non-change: the module-level S3_CLIENT in the tests is built with explicit dummy credentials and region, as test_get_client_compatible_with_s3fs already does per test.

[laughingman7743]

Self-review round 2 (claims, callers, operations) — FINDINGS (PR description corrected; no code change)

Scope: same base/head; the PR body, commit message, changed docstrings and docs/filesystem.md.

Claims checked against the botocore S3 model from uv.lock:

• HeadBucket, ListBuckets, CreateBucket, DeleteBucket and PutBucketAcl have no RequestPayer member, so the listed bucket operations failed with requester_pays=True and now omit it; HeadObject, GetObject, UploadPart, UploadPartCopy, CompleteMultipartUpload, AbortMultipartUpload, DeleteObjects and CopyObject accept it.
• UploadPart, UploadPartCopy and CompleteMultipartUpload accept SSE-C; AbortMultipartUpload does not, so the abort receives RequestPayer/ExpectedBucketOwner only.
• "before, an unknown parameter raised ParamValidationError at the first request": botocore validates every request's input against the same model.
• Corrected: "the async multipart copy ... before, it started them all" overstated the old behavior; asyncio.gather() of to_thread() calls was bounded by the event loop's default executor. The release note now says so.
• Docs: put accepts s3_additional_kwargs after this change, so the docs list open, pipe and put together; the example uses only documented arguments.
• Caller compatibility: precedence of call over filesystem parameters, applied open() keywords, and silently dropped unknown file parameters are listed as release-note behavior changes.
• Operations: no request is added; filtering removes parameters. Requester-pays and SSE-C against real S3 are not covered (stated in TEST).

[laughingman7743]

Independent review (relayed) — FINDINGS

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra (local Codex config), reasoning effort high, sandbox read-only; session 01a10119-d457-7c62-b419-30619b61ce72. Static review only: no tests, builds, edits, or network access. Snapshot: detached worktree at 57f7853ea41e72dc295467139e57a1f77be042c2, diff git diff 92c9e3e67180eb3d52ef7b85579a24540a30fd64..57f7853ea41e72dc295467139e57a1f77be042c2; the prompt omitted the PR number, description, commit messages, and prior findings. The snapshot and the PR worktree were unchanged afterwards.

Covered (reviewer): sync/async open, reads, writes, append, transactions, completion and abort; pipe, put, cp_file, inherited copy/mv, requester pays; precedence, dictionary ownership, helper-keyword collisions; installed fsspec callers and botocore mappings/input shapes, concurrency and model caching; tests, docs and comments.

Introduced:

1. P2 — s3.py:2712: a keyword parameter of a file named like a helper argument (e.g. open(p, "wb", key="x"), ignored before) is expanded unfiltered into _finish_multipart_upload(key=..., **parameters), raising a duplicate-keyword TypeError at completion; commit() then clears the upload ID without aborting it.
2. P2 — s3.py:1619, s3_async.py:228: with s3_additional_kwargs={"ContentType": "application/octet-stream"} on the filesystem, put() of data.txt now sends the guessed text/plain, overriding the configured value (sync, async transaction path).
3. P3 — docs/filesystem.md:106: the docs say every request receives only accepted parameters, but a small pipe sends its own parameters to PutObject unfiltered by design.

Pre-existing:
4. P2 — s3_async.py:412: the async multipart copy never aborts on a part or completion failure.
5. P2 — s3.py:2475: read/append initialization (HeadObject via info(), exists(), the small-object cat()) does not send the file's parameters, so an uncached SSE-C object cannot be opened, and a per-file RequestPayer is missing there.

Author verification: 1–3 confirmed. 4 is #973. 5 is pre-existing and outside this PR (the info() cache is not keyed by request parameters); listed in "Not changed here" and to be raised with the maintainer.

[laughingman7743]

Repair 14f4cfb for findings 1–3, with both self-review perspectives on the repair

Range: git range-diff 92c9e3e67180eb3d52ef7b85579a24540a30fd64..57f7853ea41e72dc295467139e57a1f77be042c2 92c9e3e67180eb3d52ef7b85579a24540a30fd64..14f4cfb475e1306fd605d99abcbc6e94e9cb8801 (same base; one new commit).

Repair:

1. _finish_multipart_upload() takes the upload's parameters as request_kwargs: Mapping | None instead of **kwargs; S3File.commit() and the sync multipart copy pass them as that mapping, so a file parameter named key, bucket, upload_id or futures cannot collide.
2. put_file() (sync) and the async in-transaction path guess ContentType only when neither the call nor the filesystem's s3_additional_kwargs gives one.
3. docs/filesystem.md says a file sends each request only the accepted parameters, a small pipe sends its parameters with a single PutObject as given, and put guesses ContentType only without a configured one.

Behavior (round-1 perspective): every _finish_multipart_upload() caller (S3File.commit(), the sync multipart copy) was updated; the async copy does not call it. Other helpers receive only operation-filtered (CamelCase) parameters, so no other collision is possible. The precedence for ContentType is call > filesystem > guess.

Claims (round-2 perspective): the PR body now states the ContentType decision against the issue's wording, the request_kwargs mapping, the put() release-note item, and the pre-existing open-time lookups (finding 5) in "Not changed here".

Tests: new TestS3File::test_multipart_write_keyword_named_as_argument and test_put_file_content_type (call / guessed / filesystem-configured) fail on 57f7853 and pass on 14f4cfb; the existing completion/copy tests assert request_kwargs. Offline tests/pyathena/filesystem/ failures equal the base's; just lint and just docs lint pass; live tests/pyathena/filesystem/ at 14f4cfb: 390 passed.

[laughingman7743]

Independent follow-up (relayed) on 14f4cfb — CLEAN for the repair; one further P2, and repair 27112b3

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only; session 01a10122-4bab-75f3-9217-9fc6f3125a98. Static review of git diff 57f7853e..14f4cfb4 (same base) at a detached snapshot of 14f4cfb, which stayed unchanged.

Reviewer result: the request_kwargs mapping resolves the lowercase collisions (including a parameter named request_kwargs) for both callers; put_file() and the async in-transaction path keep call > filesystem > guess; the docs' small-pipe exception is accurate; the new tests fail before the repair. Further P2 (s3.py:2300, s3.py:1502, also at 57f7853): open(..., "wb", UploadId="other") keeps UploadId for UploadPart and AbortMultipartUpload, which accept it, so it collides with the internal value; the parts fail, the abort raises before it is sent, and the upload is left behind.

Author verification: confirmed, and introduced by this PR's filtering: on the base, CreateMultipartUpload rejected UploadId before an upload existed. Bucket/Key/PartNumber collide the same way.

Repair 27112b3 (maintainer's choice: the request's own fields win): _get_object, _put_object, _create_multipart_upload, _upload_part, _upload_part_copy, _complete_multipart_upload merge {**kwargs, **request}, and both aborts (_finish_multipart_upload(), S3File.discard()) put Bucket/Key/UploadId after the inherited parameters. A field the request does not set (e.g. VersionId without version_id) still applies as before. Both self-review perspectives: explicit callers of these helpers no longer get a duplicate-keyword TypeError for a same-named parameter (the request's value is used); no caller relied on that error; no docs claim changes. New test_open_parameters_named_as_request_fields (completion and failing-part/abort cases, through the real helpers) fails on 14f4cfb and passes on 27112b3; offline tests/pyathena/filesystem/ failures equal the base's; just lint passes.

[laughingman7743]

Independent follow-up (relayed) on 27112b3 — CLEAN for the repair; folded one pre-existing finding in 2d26f95

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only; session 01a10128-2e0f-7590-8d26-c03032be8e33. Static review of git diff 14f4cfb4..27112b3a at a detached snapshot of 27112b3, which stayed unchanged.

Reviewer result: the six helpers and both aborts give their own fields precedence; unset fields (VersionId, Range, Body) remain available; _call's requester-pays merge has no collision; the new test exercises the real create/upload/complete helpers and _finish_multipart_upload()'s abort and fails without the repair. Pre-existing findings reported:

1. P2 — explicit per-call collisions, e.g. metadata(path, Key="other") or a small cp_file(..., Key="other"), raise TypeError before sending (also version listing, bulk delete, ACL calls).
2. P2 — s3.py:1306, s3_async.py:385: the multipart copy (> 5 GiB) passes all its parameters to CreateMultipartUpload, so a CopyObject parameter such as CopySourceIfMatch fails validation there.
3. P2 — s3_async.py:412: the async multipart copy never aborts on failure.

Author decisions: 1 deferred — a parameter given to that single request fails before anything is sent, and botocore's validation of explicit parameters is kept by design. 2 folded into this PR (same routing problem): 2d26f95 filters CreateMultipartUpload's parameters too (sync and async), so CopySourceIfMatch reaches the part copies; tests extended (fail on 27112b3, pass on 2d26f95); offline tests/pyathena/filesystem/ failures equal the base's; just lint passes. 3 is #973. Both self-review perspectives on 2d26f95: only the > 5 GiB copy path changes; a parameter no request of the copy accepts is no longer sent (release-note item added); the PR body now describes the copy's routing.

[laughingman7743]

Independent follow-up (relayed) on 2d26f95 — FINDINGS; reverted in 6979581

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only; session 01a1012c-1693-74b2-bf4c-c53a4c0312b6. Static review of git diff 27112b3a..2d26f95b at a detached snapshot of 2d26f95, which stayed unchanged.

Reviewer result: P2 (s3.py:1311, s3_async.py:390): with the creation filtered, a > 5 GiB copy with MetadataDirective="COPY" or TaggingDirective="COPY" no longer fails validation, but no multipart request receives the directive and nothing copies the source metadata or tags, so the copy succeeds without what was explicitly requested. The routing itself (ContentType/RequestPayer to the creation, CopySourceIfMatch to the part copies) worked, and the tests failed without the change.

Author verification: confirmed. Silently accepting an explicit directive is worse than the previous validation error, and directive/metadata semantics of the multipart copy belong to #973. 6979581 reverts 2d26f95; git diff 27112b3a 69795814 is empty, so the head's tree is the one the previous follow-up found CLEAN and that passed live (tests/pyathena/filesystem/: 392 passed). The PR body lists this under "Not changed here".

[laughingman7743]

Rebase onto 4950996 (#986, #999) → 021ba88, with an independent follow-up (relayed) — CLEAN

Range: git range-diff 92c9e3e67180eb3d52ef7b85579a24540a30fd64..6979581460db20593c04a0f22a840250d67dbe76 49509962..021ba880228dcbcf06658933589a72f034b4394f (both old objects verified with git cat-file -e); pushed with --force-with-lease against 6979581.

Resolutions: AioS3FileSystem._put_file_in_transaction() keeps #999's block_size default and _check_multipart_upload_size() and adds this PR's max_workers, s3_additional_kwargs and call > filesystem > guess ContentType; test conflicts were additions on both sides; #999's test_transaction_put_file_block_size now expects max_workers passed to open() (new commit 021ba88). Upstream #986's _delete_objects() sends explicit DeleteObjects parameters through _call() (requester pays filtered, DeleteObjects accepts it); no routing change is needed.

Validation at 021ba88: just lint pass; offline tests/pyathena/filesystem/ failures equal the new base's (only credential-less integration tests); live tests/pyathena/filesystem/: 412 passed.

Independent follow-up: OpenAI Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only; session 01a10131-73f3-7a01-b02b-757b8ea96cfb; static review of the range-diff and final tree at a detached snapshot of 021ba88, which stayed unchanged. Result: CLEAN — the async transaction put keeps the upstream block-size default and part-limit validation plus this change's forwarding and precedence; the async transaction pipe keeps its byte-based check and routes parameters through open(); bulk deletion needs no routing change; no reviewed change was lost.

[laughingman7743]

Rebase onto 55af09a (#998) → 882ae2f

Range: git range-diff 49509962..021ba880228dcbcf06658933589a72f034b4394f 55af09a2..882ae2f05b5272c679ba89569846fdaddaf27abd; pushed with --force-with-lease against 021ba88.

The only conflict was in docs/filesystem.md: #998 and this PR each added a paragraph after the multipart-limit paragraph; both are kept (#998's path-normalization paragraph first). No source or test conflict. #998 changes cat_file()/range handling and adds no request helper or parameter path, so the routing needs no change.

Validation at 882ae2f: just lint and just docs lint pass; offline tests/pyathena/filesystem/ failures equal the new base's (only credential-less integration tests); live tests/pyathena/filesystem/: 421 passed.

The pre-existing open-time lookup finding (finding 5 of the independent review) is filed as #1004.