Use separate XML writer for S3 operations

.changes/next-release/bugfix-AmazonS3-8749050.json

@@ -0,0 +1,6 @@
+{
+    "category": "Amazon S3", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Fixes a bug in DeleteObjects to properly encode the key in the request."
+}

core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsS3ProtocolFactory.java

@@ -22,7 +22,9 @@
 import software.amazon.awssdk.core.Response;
 import software.amazon.awssdk.core.SdkPojo;
 import software.amazon.awssdk.core.http.HttpResponseHandler;
+import software.amazon.awssdk.protocols.core.OperationInfo;
 import software.amazon.awssdk.protocols.query.unmarshall.XmlElement;
+import software.amazon.awssdk.protocols.xml.internal.marshall.XmlGenerator;
 import software.amazon.awssdk.protocols.xml.internal.unmarshall.AwsXmlPredicatedResponseHandler;
 import software.amazon.awssdk.protocols.xml.internal.unmarshall.DecorateErrorFromResponseBodyUnmarshaller;
 
@@ -72,6 +74,16 @@ public <T extends AwsResponse> HttpResponseHandler<Response<T>> createCombinedRe
         return createErrorCouldBeInBodyResponseHandler(pojoSupplier, staxOperationMetadata);
     }
 
+    /**
+     * Creates a {@link XmlGenerator} with a S3XmlWriter.
+     */
+    @Override
+    protected XmlGenerator createGenerator(OperationInfo operationInfo) {
+        return operationInfo.hasPayloadMembers() ?
+               XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE), true) :
+               null;
+    }
+
     private <T extends AwsResponse> HttpResponseHandler<Response<T>> createErrorCouldBeInBodyResponseHandler(
         Supplier<SdkPojo> pojoSupplier, XmlOperationMetadata staxOperationMetadata) {
 

core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java

@@ -161,9 +161,9 @@ Optional<XmlElement> getErrorRoot(XmlElement document) {
         return document.getOptionalElementByName("Error");
     }
 
-    private XmlGenerator createGenerator(OperationInfo operationInfo) {
+    protected XmlGenerator createGenerator(OperationInfo operationInfo) {
         return operationInfo.hasPayloadMembers() ?
-               XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)) :
+               XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE), false) :
                null;
     }
 

core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.protocols.xml.internal.marshall;
+
+import java.io.Writer;
+import software.amazon.awssdk.annotations.SdkInternalApi;
+import software.amazon.awssdk.utils.StringUtils;
+
+/**
+ * Utility for creating easily creating XML documents, one element at a time, for S3 operations. Compared to
+ * {@link XmlWriter}, {@link S3XmlWriter} does not unescape escaped characters. This is because S3 expects payloads to
+ * be encoded and will decode the received payload.
+ */
+@SdkInternalApi
+final class S3XmlWriter extends XmlWriter {
+
+    /**
+     * Creates a new S3XmlWriter, ready to write an XML document to the specified writer. The root element in the XML
+     * document will specify an xmlns attribute with the specified namespace parameter.
+     *
+     * @param w     The writer this S3XmlWriter will write to.
+     * @param xmlns The XML namespace to include in the xmlns attribute of the root element.
+     */
+    S3XmlWriter(Writer w, String xmlns) {
+        super(w, xmlns);
+    }
+
+    @Override
+    protected String escapeXmlEntities(String s) {
+        return StringUtils.replaceEach(s, ESCAPE_SEARCHES, ESCAPE_REPLACEMENTS);
+    }
+}

core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java

@@ -33,9 +33,10 @@ private XmlGenerator(StringWriter stringWriter, XmlWriter xmlWriter) {
         this.xmlWriter = xmlWriter;
     }
 
-    public static XmlGenerator create(String xmlns) {
+    public static XmlGenerator create(String xmlns, boolean isS3) {
         StringWriter stringWriter = new StringWriter();
-        return new XmlGenerator(stringWriter, new XmlWriter(stringWriter, xmlns));
+        XmlWriter xmlWriter = isS3 ? new S3XmlWriter(stringWriter, xmlns) : new XmlWriter(stringWriter, xmlns);
+        return new XmlGenerator(stringWriter, xmlWriter);
     }
 
     public XmlWriter xmlWriter() {

core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java

@@ -31,7 +31,16 @@
  * Utility for creating easily creating XML documents, one element at a time.
  */
 @SdkInternalApi
-final class XmlWriter {
+class XmlWriter {
+
+    static final String[] ESCAPE_SEARCHES = {
+        // Ampersands should always be the first to escape
+        "&", "\"", "'", "<", ">", "\r", "\n"
+    };
+
+    static final String[] ESCAPE_REPLACEMENTS = {
+        "&amp;", "&quot;", "&apos;", "&lt;", "&gt;", "&#x0D;", "&#x0A;"
+    };
 
     /** Standard XML prolog to add to the beginning of each XML document. */
     private static final String PROLOG = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>";
@@ -45,15 +54,6 @@ final class XmlWriter {
         "\"", "'", "<", ">", "\r", "\n", "&"
     };
 
-    private static final String[] ESCAPE_SEARCHES = {
-        // Ampersands should always be the first to escape
-        "&", "\"", "'", "<", ">", "\r", "\n"
-    };
-
-    private static final String[] ESCAPE_REPLACEMENTS = {
-        "&amp;", "&quot;", "&apos;", "&lt;", "&gt;", "&#x0D;", "&#x0A;"
-    };
-
     /** The writer to which the XML document created by this writer will be written. */
     private final Writer writer;
 
@@ -205,7 +205,7 @@ private void append(String s) {
         }
     }
 
-    private String escapeXmlEntities(String s) {
+    protected String escapeXmlEntities(String s) {
         // Unescape any escaped characters.
         if (s.contains("&")) {
             s = StringUtils.replaceEach(s, UNESCAPE_SEARCHES, UNESCAPE_REPLACEMENTS);

core/protocols/aws-xml-protocol/src/test/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriterTest.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.protocols.xml.internal.marshall;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.StringWriter;
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+public class S3XmlWriterTest {
+
+    private static final S3XmlWriter s3XmlWriter = new S3XmlWriter(new StringWriter(), "namespace");
+
+    private static Stream<Arguments> testKeys() {
+        return Stream.of(
+            Arguments.of("<Key>objectId</Key>", "&lt;Key&gt;objectId&lt;/Key&gt;"),
+            Arguments.of("&lt;Key&gt;objectId&lt;/Key&gt;", "&amp;lt;Key&amp;gt;objectId&amp;lt;/Key&amp;gt;"),
+            Arguments.of("&lt;<", "&amp;lt;&lt;")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("testKeys")
+    public void escapeXmlEntities_properlyEncodesKey(String key, String encodedKey) {
+        String escaped = s3XmlWriter.escapeXmlEntities(key);
+        assertThat(escaped).isEqualTo(encodedKey);
+    }
+}

services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/DeleteObjectsFunctionalTest.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.services.s3.functionaltests;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.any;
+import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
+import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import java.io.InputStream;
+import java.net.URI;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import software.amazon.awssdk.core.exception.SdkClientException;
+import software.amazon.awssdk.core.interceptor.Context;
+import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
+import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
+import software.amazon.awssdk.http.SdkHttpFullRequest;
+import software.amazon.awssdk.regions.Region;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.model.Delete;
+import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest;
+
+@WireMockTest
+public class DeleteObjectsFunctionalTest {
+
+    private static S3AsyncClient s3Client;
+    private static final CapturingInterceptor CAPTURING_INTERCEPTOR = new CapturingInterceptor();
+
+    @BeforeEach
+    public void init(WireMockRuntimeInfo wm) {
+        s3Client = S3AsyncClient.builder()
+                                .region(Region.US_EAST_1)
+                                .endpointOverride(URI.create(wm.getHttpBaseUrl()))
+                                .overrideConfiguration(c -> c.addExecutionInterceptor(CAPTURING_INTERCEPTOR))
+                                .credentialsProvider(
+                                    StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret")))
+                                .build();
+    }
+
+    private static Stream<Arguments> testKeys() {
+        return Stream.of(
+            Arguments.of("<Key>objectId</Key>", "&lt;Key&gt;objectId&lt;/Key&gt;"),
+            Arguments.of("&lt;Key&gt;objectId&lt;/Key&gt;", "&amp;lt;Key&amp;gt;objectId&amp;lt;/Key&amp;gt;"),
+            Arguments.of("&lt;<", "&amp;lt;&lt;")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("testKeys")
+    public void deleteObjects_shouldProperlyEncodeKeysInPayload(String key, String encodedKey) {
+        stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200)));
+
+        Delete delete = Delete.builder().objects(o -> o.key(key)).build();
+        DeleteObjectsRequest request = DeleteObjectsRequest.builder().bucket("bucket").delete(delete).build();
+        s3Client.deleteObjects(request);
+
+        verifyPayload(encodedKey);
+    }
+
+    private void verifyPayload(String encodedKey) {
+        String expectedPayload = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><Delete xmlns=\"http://s3.amazonaws.com/doc/2006-03-01/\"><Object><Key>"
+                                 + encodedKey
+                                 + "</Key></Object></Delete>";
+
+        assertThat(CAPTURING_INTERCEPTOR.payload).contains(expectedPayload);
+    }
+
+    private static final class CapturingInterceptor implements ExecutionInterceptor {
+        private String payload;
+
+        @Override
+        public void beforeTransmission(Context.BeforeTransmission context, ExecutionAttributes executionAttributes) {
+            SdkHttpFullRequest request = (SdkHttpFullRequest) context.httpRequest();
+            InputStream is = request.contentStreamProvider().get().newStream();
+            byte[] buf = new byte[200];
+            try {
+                is.read(buf);
+            } catch (Exception e) {
+                throw SdkClientException.create(e.getMessage(), e);
+            }
+            payload = new String(buf);
+        }
+    }
+}