From 7f0ff5a0b8008fb483e668ff3880d8d11c0939c2 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 20 Dec 2023 13:04:48 -0800 Subject: [PATCH 1/8] Use separate XML writer for S3 operations --- .../protocols/xml/AwsXmlProtocolFactory.java | 7 +- .../xml/internal/marshall/S3XmlWriter.java | 43 +++++++++++ .../xml/internal/marshall/XmlGenerator.java | 5 +- .../xml/internal/marshall/XmlWriter.java | 22 +++--- .../s3/DeleteObjectsIntegrationTest.java | 71 +++++++++++++++++++ 5 files changed, 134 insertions(+), 14 deletions(-) create mode 100644 core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java create mode 100644 services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java index 44b222959a9d..fe384bb3f444 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java @@ -163,10 +163,15 @@ Optional getErrorRoot(XmlElement document) { private XmlGenerator createGenerator(OperationInfo operationInfo) { return operationInfo.hasPayloadMembers() ? - XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)) : + XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE), isS3(operationInfo)) : null; } + private boolean isS3(OperationInfo operationInfo) { + return "http://s3.amazonaws.com/doc/2006-03-01/" + .equals(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)); + } + public static Builder builder() { return new Builder(); } diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java new file mode 100644 index 000000000000..fe3fb9881ad9 --- /dev/null +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java @@ -0,0 +1,43 @@ +/* + * 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. + */ +@SdkInternalApi +public 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); + } +} diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java index 3ce773f633f0..5b627da89a1d 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java +++ b/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() { diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java index e212c164855a..cc7e274226cd 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java +++ b/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 { +public class XmlWriter { + + static final String[] ESCAPE_SEARCHES = { + // Ampersands should always be the first to escape + "&", "\"", "'", "<", ">", "\r", "\n" + }; + + static final String[] ESCAPE_REPLACEMENTS = { + "&", """, "'", "<", ">", " ", " " + }; /** Standard XML prolog to add to the beginning of each XML document. */ private static final String PROLOG = ""; @@ -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 = { - "&", """, "'", "<", ">", " ", " " - }; - /** 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); diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java new file mode 100644 index 000000000000..137986a44fcc --- /dev/null +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java @@ -0,0 +1,71 @@ +package software.amazon.awssdk.services.s3; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName; + +import java.util.Arrays; +import java.util.Collection; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.services.s3.model.Delete; +import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest; +import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; +import software.amazon.awssdk.services.s3.model.NoSuchKeyException; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; + +@RunWith(Parameterized.class) +public class DeleteObjectsIntegrationTest extends S3IntegrationTestBase { + + private static final String BUCKET = temporaryBucketName(DeleteObjectsIntegrationTest.class); + private static final String XML_KEY = "objectId"; + private static final String ENCODED_KEY = "<Key>objectId</Key>"; + private static final String MIXED_KEY = "<<"; + + @BeforeClass + public static void init() { + createBucket(BUCKET); + } + + @AfterClass + public static void tearDownFixture() { + deleteBucketAndAllContents(BUCKET); + } + + @Parameterized.Parameters + public static Collection data() { + return Arrays.asList(new Object[][] { + { XML_KEY }, + { ENCODED_KEY }, + { MIXED_KEY } + }); + } + + private final String key; + + public DeleteObjectsIntegrationTest(String key) { + this.key = key; + } + + @Test + public void deleteObjects_shouldCorrectlyDeleteObjectsWithSpecifiedKeys() { + s3.putObject(PutObjectRequest.builder() + .bucket(BUCKET) + .key(key) + .build(), RequestBody.fromString("contents")); + + // Does not throw exception + s3.headObject(r -> r.bucket(BUCKET).key(key)); + + Delete delete = Delete.builder().objects(o -> o.key(key)).build(); + DeleteObjectsRequest request = DeleteObjectsRequest.builder().bucket(BUCKET).delete(delete).build(); + DeleteObjectsResponse response = s3.deleteObjects(request); + + assertThat(response.deleted().get(0).key()).isEqualTo(key); + assertThrows(NoSuchKeyException.class, () -> s3.headObject(r -> r.bucket(BUCKET).key(key))); + } +} From f9c1bf4f1ad66da3839e8fb141bca169f75cf1a5 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 20 Dec 2023 13:12:26 -0800 Subject: [PATCH 2/8] Make S3XmlWriter class final --- .../awssdk/protocols/xml/internal/marshall/S3XmlWriter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java index fe3fb9881ad9..1ef132f0ed1b 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java @@ -23,7 +23,7 @@ * Utility for creating easily creating XML documents, one element at a time, for S3 operations. */ @SdkInternalApi -public class S3XmlWriter extends XmlWriter { +public 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 From 29aec35f5e7efe1a5edea7a8a2cc5e13204f4b4c Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 20 Dec 2023 13:53:16 -0800 Subject: [PATCH 3/8] Add changelog --- .changes/next-release/bugfix-AmazonS3-8749050.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/bugfix-AmazonS3-8749050.json diff --git a/.changes/next-release/bugfix-AmazonS3-8749050.json b/.changes/next-release/bugfix-AmazonS3-8749050.json new file mode 100644 index 000000000000..13543f9d9f46 --- /dev/null +++ b/.changes/next-release/bugfix-AmazonS3-8749050.json @@ -0,0 +1,6 @@ +{ + "category": "Amazon S3", + "contributor": "", + "type": "bugfix", + "description": "Fixes a bug in DeleteObjects where specifying a key that contains characters that need to be escaped in XML and that is encoded, would delete the object with the decoded key instead." +} From 78db4368e438344c6dfcd9ff792046ce0acdb033 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 21 Dec 2023 10:00:09 -0800 Subject: [PATCH 4/8] Check for prefix instead of full S3 namespace --- .../amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java index fe384bb3f444..b5e9c44cda10 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java @@ -74,6 +74,8 @@ public class AwsXmlProtocolFactory { private static final XmlProtocolUnmarshaller XML_PROTOCOL_UNMARSHALLER = XmlProtocolUnmarshaller.create(); + private static final String S3_NAMESPACE_PREFIX = "http://s3.amazonaws.com"; + private final List modeledExceptions; private final Supplier defaultServiceExceptionSupplier; private final HttpResponseHandler errorUnmarshaller; @@ -168,8 +170,7 @@ private XmlGenerator createGenerator(OperationInfo operationInfo) { } private boolean isS3(OperationInfo operationInfo) { - return "http://s3.amazonaws.com/doc/2006-03-01/" - .equals(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)); + return operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE).startsWith(S3_NAMESPACE_PREFIX); } public static Builder builder() { From 62d91e3be3f19322e188c1be110c5e890e54770e Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 21 Dec 2023 14:08:19 -0800 Subject: [PATCH 5/8] Add namespace attribute null check --- .../amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java index b5e9c44cda10..b11fd6882877 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java @@ -170,7 +170,8 @@ private XmlGenerator createGenerator(OperationInfo operationInfo) { } private boolean isS3(OperationInfo operationInfo) { - return operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE).startsWith(S3_NAMESPACE_PREFIX); + String namespace = operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE); + return namespace != null && namespace.startsWith(S3_NAMESPACE_PREFIX); } public static Builder builder() { From 0064354f4403ed688a31701bb4583d5bc0ad5a92 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Tue, 2 Jan 2024 12:18:20 -0800 Subject: [PATCH 6/8] Address comments --- .../protocols/xml/AwsS3ProtocolFactory.java | 12 +++++ .../protocols/xml/AwsXmlProtocolFactory.java | 11 +---- .../xml/internal/marshall/S3XmlWriter.java | 6 ++- .../xml/internal/marshall/XmlGenerator.java | 4 ++ .../xml/internal/marshall/XmlWriter.java | 2 +- .../internal/marshall/S3XmlWriterTest.java | 44 +++++++++++++++++++ 6 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 core/protocols/aws-xml-protocol/src/test/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriterTest.java diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsS3ProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsS3ProtocolFactory.java index f2a798476f82..10a29d81283a 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsS3ProtocolFactory.java +++ b/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 HttpResponseHandler> 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 HttpResponseHandler> createErrorCouldBeInBodyResponseHandler( Supplier pojoSupplier, XmlOperationMetadata staxOperationMetadata) { diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java index b11fd6882877..4c5fba6e4498 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java @@ -74,8 +74,6 @@ public class AwsXmlProtocolFactory { private static final XmlProtocolUnmarshaller XML_PROTOCOL_UNMARSHALLER = XmlProtocolUnmarshaller.create(); - private static final String S3_NAMESPACE_PREFIX = "http://s3.amazonaws.com"; - private final List modeledExceptions; private final Supplier defaultServiceExceptionSupplier; private final HttpResponseHandler errorUnmarshaller; @@ -163,17 +161,12 @@ Optional 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), isS3(operationInfo)) : + XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)) : null; } - private boolean isS3(OperationInfo operationInfo) { - String namespace = operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE); - return namespace != null && namespace.startsWith(S3_NAMESPACE_PREFIX); - } - public static Builder builder() { return new Builder(); } diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java index 1ef132f0ed1b..66eed1bb57d7 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriter.java @@ -20,10 +20,12 @@ import software.amazon.awssdk.utils.StringUtils; /** - * Utility for creating easily creating XML documents, one element at a time, for S3 operations. + * 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 -public final class S3XmlWriter extends XmlWriter { +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 diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java index 5b627da89a1d..9c1044f09846 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java @@ -33,6 +33,10 @@ private XmlGenerator(StringWriter stringWriter, XmlWriter xmlWriter) { this.xmlWriter = xmlWriter; } + public static XmlGenerator create(String xmlnx) { + return create(xmlnx, false); + } + public static XmlGenerator create(String xmlns, boolean isS3) { StringWriter stringWriter = new StringWriter(); XmlWriter xmlWriter = isS3 ? new S3XmlWriter(stringWriter, xmlns) : new XmlWriter(stringWriter, xmlns); diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java index cc7e274226cd..824e3ee3ea1d 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlWriter.java @@ -31,7 +31,7 @@ * Utility for creating easily creating XML documents, one element at a time. */ @SdkInternalApi -public class XmlWriter { +class XmlWriter { static final String[] ESCAPE_SEARCHES = { // Ampersands should always be the first to escape diff --git a/core/protocols/aws-xml-protocol/src/test/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriterTest.java b/core/protocols/aws-xml-protocol/src/test/java/software/amazon/awssdk/protocols/xml/internal/marshall/S3XmlWriterTest.java new file mode 100644 index 000000000000..d82a212f7c10 --- /dev/null +++ b/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 testKeys() { + return Stream.of( + Arguments.of("objectId", "<Key>objectId</Key>"), + Arguments.of("<Key>objectId</Key>", "&lt;Key&gt;objectId&lt;/Key&gt;"), + Arguments.of("<<", "&lt;<") + ); + } + + @ParameterizedTest + @MethodSource("testKeys") + public void escapeXmlEntities_properlyEncodesKey(String key, String encodedKey) { + String escaped = s3XmlWriter.escapeXmlEntities(key); + assertThat(escaped).isEqualTo(encodedKey); + } +} From fcee861f0843c26baddc4d02dae51a5d0e346dae Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 3 Jan 2024 16:57:49 -0800 Subject: [PATCH 7/8] Address comments --- .../protocols/xml/AwsXmlProtocolFactory.java | 2 +- .../xml/internal/marshall/XmlGenerator.java | 4 - .../s3/DeleteObjectsIntegrationTest.java | 71 ------------ .../DeleteObjectsFunctionalTest.java | 106 ++++++++++++++++++ 4 files changed, 107 insertions(+), 76 deletions(-) delete mode 100644 services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/DeleteObjectsFunctionalTest.java diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java index 4c5fba6e4498..4fa73a1aeabd 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/AwsXmlProtocolFactory.java @@ -163,7 +163,7 @@ Optional getErrorRoot(XmlElement document) { protected XmlGenerator createGenerator(OperationInfo operationInfo) { return operationInfo.hasPayloadMembers() ? - XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE)) : + XmlGenerator.create(operationInfo.addtionalMetadata(XML_NAMESPACE_ATTRIBUTE), false) : null; } diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java index 9c1044f09846..5b627da89a1d 100644 --- a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java +++ b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlGenerator.java @@ -33,10 +33,6 @@ private XmlGenerator(StringWriter stringWriter, XmlWriter xmlWriter) { this.xmlWriter = xmlWriter; } - public static XmlGenerator create(String xmlnx) { - return create(xmlnx, false); - } - public static XmlGenerator create(String xmlns, boolean isS3) { StringWriter stringWriter = new StringWriter(); XmlWriter xmlWriter = isS3 ? new S3XmlWriter(stringWriter, xmlns) : new XmlWriter(stringWriter, xmlns); diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java deleted file mode 100644 index 137986a44fcc..000000000000 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/DeleteObjectsIntegrationTest.java +++ /dev/null @@ -1,71 +0,0 @@ -package software.amazon.awssdk.services.s3; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName; - -import java.util.Arrays; -import java.util.Collection; -import org.junit.AfterClass; -import org.junit.BeforeClass; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import software.amazon.awssdk.core.sync.RequestBody; -import software.amazon.awssdk.services.s3.model.Delete; -import software.amazon.awssdk.services.s3.model.DeleteObjectsRequest; -import software.amazon.awssdk.services.s3.model.DeleteObjectsResponse; -import software.amazon.awssdk.services.s3.model.NoSuchKeyException; -import software.amazon.awssdk.services.s3.model.PutObjectRequest; - -@RunWith(Parameterized.class) -public class DeleteObjectsIntegrationTest extends S3IntegrationTestBase { - - private static final String BUCKET = temporaryBucketName(DeleteObjectsIntegrationTest.class); - private static final String XML_KEY = "objectId"; - private static final String ENCODED_KEY = "<Key>objectId</Key>"; - private static final String MIXED_KEY = "<<"; - - @BeforeClass - public static void init() { - createBucket(BUCKET); - } - - @AfterClass - public static void tearDownFixture() { - deleteBucketAndAllContents(BUCKET); - } - - @Parameterized.Parameters - public static Collection data() { - return Arrays.asList(new Object[][] { - { XML_KEY }, - { ENCODED_KEY }, - { MIXED_KEY } - }); - } - - private final String key; - - public DeleteObjectsIntegrationTest(String key) { - this.key = key; - } - - @Test - public void deleteObjects_shouldCorrectlyDeleteObjectsWithSpecifiedKeys() { - s3.putObject(PutObjectRequest.builder() - .bucket(BUCKET) - .key(key) - .build(), RequestBody.fromString("contents")); - - // Does not throw exception - s3.headObject(r -> r.bucket(BUCKET).key(key)); - - Delete delete = Delete.builder().objects(o -> o.key(key)).build(); - DeleteObjectsRequest request = DeleteObjectsRequest.builder().bucket(BUCKET).delete(delete).build(); - DeleteObjectsResponse response = s3.deleteObjects(request); - - assertThat(response.deleted().get(0).key()).isEqualTo(key); - assertThrows(NoSuchKeyException.class, () -> s3.headObject(r -> r.bucket(BUCKET).key(key))); - } -} diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/DeleteObjectsFunctionalTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/DeleteObjectsFunctionalTest.java new file mode 100644 index 000000000000..afb96f29ec78 --- /dev/null +++ b/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 testKeys() { + return Stream.of( + Arguments.of("objectId", "<Key>objectId</Key>"), + Arguments.of("<Key>objectId</Key>", "&lt;Key&gt;objectId&lt;/Key&gt;"), + Arguments.of("<<", "&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 = "" + + encodedKey + + ""; + + 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); + } + } +} From e6fb8b41e40fb22e1bceebb2b46eb7e64cb4aa31 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 3 Jan 2024 17:12:24 -0800 Subject: [PATCH 8/8] Update changelog --- .changes/next-release/bugfix-AmazonS3-8749050.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/next-release/bugfix-AmazonS3-8749050.json b/.changes/next-release/bugfix-AmazonS3-8749050.json index 13543f9d9f46..2380d16f55ef 100644 --- a/.changes/next-release/bugfix-AmazonS3-8749050.json +++ b/.changes/next-release/bugfix-AmazonS3-8749050.json @@ -2,5 +2,5 @@ "category": "Amazon S3", "contributor": "", "type": "bugfix", - "description": "Fixes a bug in DeleteObjects where specifying a key that contains characters that need to be escaped in XML and that is encoded, would delete the object with the decoded key instead." + "description": "Fixes a bug in DeleteObjects to properly encode the key in the request." }