smaheshwar-pltr commented on code in PR #4039:
URL: https://github.com/apache/iceberg-python/pull/4039#discussion_r4151681116
##########
pyiceberg/io/fsspec.py:
##########
@@ -148,6 +148,9 @@ def __call__(self, request: "AWSRequest", **_: Any) -> None:
"uri": request.url,
"headers": {key: [val] for key, val in request.headers.items()},
}
+ if request.method == "POST" and "delete" in
parse_qs(urlparse(request.url).query, keep_blank_values=True):
Review Comment:
This matches the [Java client's existing predicate and body
handling](https://github.com/apache/iceberg/blob/d008ad230bed30383a667af8151573e48de7624c/aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java#L349-L367):
POST with a `delete` query key. The [REST
contract](https://github.com/apache/iceberg/blob/d008ad230bed30383a667af8151573e48de7624c/open-api/rest-catalog-open-api.yaml#L5834-L5838)
calls out DeleteObjects because its body contains the keys that the signing
service must validate; the URL identifies only the bucket.
`keep_blank_values=True` makes both `?delete` and `?delete=` recognizable.
Other POST operations, including multipart completion, keep their existing
behavior.
##########
pyiceberg/io/fsspec.py:
##########
@@ -148,6 +148,9 @@ def __call__(self, request: "AWSRequest", **_: Any) -> None:
"uri": request.url,
"headers": {key: [val] for key, val in request.headers.items()},
}
+ if request.method == "POST" and "delete" in
parse_qs(urlparse(request.url).query, keep_blank_values=True):
+ if body := request.body:
+ signer_body["body"] = body.decode("utf-8")
Review Comment:
The bytes assumption comes from [Botocore's REST XML
serializer](https://github.com/boto/botocore/9fda087a19c80ad04bff404fc463571ec8a9ec3f/botocore/serialize.py#L1107-L1115),
which serializes the request using its [UTF-8
default](https://github.com/boto/botocore/9fda087a19c80ad04bff404fc463571ec8a9ec3f/botocore/serialize.py#L106-L110).
[AWSRequest.body](https://github.com/boto/botocore/9fda087a19c80ad04bff404fc463571ec8a9ec3f/botocore/awsrequest.py#L483-L488)
exposes that prepared body. Decoding creates the string expected by the
signing API; it does not replace the original XML bytes or checksum headers
sent to S3. The guards above prevent accessing unrelated upload streams.
##########
tests/io/test_fsspec.py:
##########
@@ -995,6 +996,49 @@ def test_s3v4_rest_signer(requests_mock: Mocker) -> None:
}
[email protected]("query", ["delete", "delete=",
"delete=&x-id=DeleteObjects"])
+def test_s3v4_rest_signer_sends_delete_objects_body(requests_mock: Mocker,
query: str) -> None:
Review Comment:
This is reachable even when deleting one file: [FsspecFileIO.delete calls
fs.rm](https://github.com/apache/iceberg-python/blob/5d96c0e910abbb14ebc15deeb0ffe1b2814e038e/pyiceberg/io/fsspec.py#L479-L495),
and [s3fs batches those paths through
_bulk_delete](https://github.com/fsspec/s3fs/blob/65f394575b9667f33b59473dc28a8f1cf6708745/s3fs/core.py#L2099-L2116),
which [calls
DeleteObjects](https://github.com/fsspec/s3fs/blob/65f394575b9667f33b59473dc28a8f1cf6708745/s3fs/core.py#L2077-L2085).
All three cases here failed before the fix because the captured signing JSON
had no `body`. The six cases below separately guard against forwarding or
reading unrelated request bodies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
