[GitHub] [iceberg] nastra commented on a diff in pull request #8365: AWS: Update S3V4RestSignerClient to send body for DeleteObjects requests

["nastra (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org