nastra commented on code in PR #8365:
URL: https://github.com/apache/iceberg/pull/8365#discussion_r1301299261
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -177,6 +181,22 @@ public void validatePutObject() {
PutObjectRequest.builder().bucket(BUCKET).key("some/key").build(),
Paths.get("/etc/hosts"));
}
+ @Test
+ public void validateDeleteObjects() {
+ Path sourcePath = Paths.get("/etc/hosts");
+
s3.putObject(PutObjectRequest.builder().bucket(BUCKET).key("some/key1").build(),
sourcePath);
+
s3.putObject(PutObjectRequest.builder().bucket(BUCKET).key("some/key2").build(),
sourcePath);
+
+ Delete objectsToDelete =
+ Delete.builder()
+ .objects(
+ ObjectIdentifier.builder().key("some/key1").build(),
+ ObjectIdentifier.builder().key("some/key2").build())
+ .build();
+
+
s3.deleteObjects(DeleteObjectsRequest.builder().bucket(BUCKET).delete(objectsToDelete).build());
Review Comment:
yeah this is a little bit difficult to test. One option I can think of is
adding a spy onto `S3SignerServlet` and then verifying that certain
server-side code was called. We do something similar in `TestRESTCatalog`
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -328,6 +334,27 @@ public SdkHttpFullRequest sign(
return mutableRequest.build();
}
+ private String body(SdkHttpFullRequest request) {
+ if (shouldAddBody(request) && request.contentStreamProvider().isPresent())
{
+ try (InputStream is = request.contentStreamProvider().get().newStream())
{
+ return IoUtils.toUtf8String(is);
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to set body for S3 sign
request", e);
Review Comment:
I think we wouldn't want to fail signing, given that the body is optional.
Maybe issue a WARN here instead?
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -328,6 +334,27 @@ public SdkHttpFullRequest sign(
return mutableRequest.build();
}
+ private String body(SdkHttpFullRequest request) {
+ if (shouldAddBody(request) && request.contentStreamProvider().isPresent())
{
+ try (InputStream is = request.contentStreamProvider().get().newStream())
{
+ return IoUtils.toUtf8String(is);
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to set body for S3 sign
request", e);
+ }
+ }
+
+ return null;
+ }
+
+ /**
+ * Only add body for DeleteObjectsRequest. Refer to
+ *
https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html#API_DeleteObjects_RequestSyntax
+ */
+ private boolean shouldAddBody(SdkHttpFullRequest request) {
Review Comment:
I'd either inline this check or rename the method to
`isDeleteObjectsRequest(..)`
--
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]
