From 0c1a10aeee9dbd4c8420eefef0cc367bf0d3e97e Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Tue, 20 Jul 2021 19:50:01 -0700 Subject: [PATCH 1/5] [S3] Add support for more user-friendly CopyObject source parameters ## Motivation and Context The current S3Client interface has a cumbersome API for invoking CopyObjectRequests. We require users to define the bucket name, key name, and version ID in a raw string format. We require that the string conform to the S3 API, which forces users to know the intricate details for how to join these values together. Additionally, portions (but not all) of the value must be URL encoded, further increasing the burden. https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#API_CopyObject_RequestParameters In the Java SDK v1, users are given explicit parameters for the different copy source attributes. But in v2, parity for this support is clearly lacking. E.g., v1: ``` s3.copyObject(new CopyObjectRequest() .withSourceBucketName(SOURCE_BUCKET) .withSourceKey(key) .withSourceVersionId(versionId) .withDestinationBucketName(DESTINATION_BUCKET) .withDestinationKey(key)); ``` v2: ``` s3.copyObject(CopyObjectRequest.builder() .copySource(SOURCE_BUCKET + "/" + key + "?versionId=" + versionId) .destinationBucket(DESTINATION_BUCKET) .destinationKey(key) .build()); ``` The v1 SDK will also URL encode on the user's behalf, allowing users to use the same input values as they would for a PutObjectRequest. The v2 code snippet above may appear to work for most users until they run into unexpected source keys that require URL encoding, at which point they will typically be given `NoSuchKey` errors. This API deficiency has been called out by users in at least the following issues: * https://github.com/aws/aws-sdk-java-v2/issues/1313 * https://github.com/aws/aws-sdk-java-v2/issues/1452 * https://github.com/aws/aws-sdk-java-v2/issues/1656 * https://github.com/awsdocs/aws-doc-sdk-examples/issues/740 ## Description * For both CopyObjectRequest and UploadPartCopyRequest, add explicit parameters for: SourceBucket, SourceKey, SourceVersionId * If specified, these values will be used to construct a CopySource on the user's behalf, including URL encoding the relevant portions. * These values are introduced in a backwards compatible fashion. Users who are already using CopySource today will see no change in behavior, but these new fields may not be used in conjunction with CopySource. * A follow-up PR will be submitted to propose deprecating the current CopySource parameter. It is excluded from this PR since our current code gen configuration lacks appropriate support. * Add support for "DestinationBucket" & "DestinationKey" to UploadPartCopyRequest (this support already existed for CopyObjectRequest) * Utility function added to detect if an ARN is for a particular S3 resource type. This is to conform with the S3 API requirements of inserting "/object" in the path of these requests. ## Testing * New unit tests added * New integration tests added --- .../s3/CopySourceIntegrationTest.java | 148 +++++++++++++++ .../handlers/CopySourceInterceptor.java | 127 +++++++++++++ .../s3/internal/resource/S3ArnUtils.java | 10 + .../s3/reflect-config.json | 9 + .../codegen-resources/customization.config | 46 +++++ .../awssdk/services/s3/execution.interceptors | 3 +- .../handlers/CopySourceInterceptorTest.java | 173 ++++++++++++++++++ .../s3/internal/resource/S3ArnUtilsTest.java | 19 ++ 8 files changed, 534 insertions(+), 1 deletion(-) create mode 100644 services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java create mode 100644 services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java new file mode 100644 index 000000000000..8efec01ef82a --- /dev/null +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java @@ -0,0 +1,148 @@ +/* + * 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; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName; + +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.services.s3.internal.handlers.CopySourceInterceptor; +import software.amazon.awssdk.services.s3.model.BucketVersioningStatus; +import software.amazon.awssdk.services.s3.model.CopyObjectRequest; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.PutObjectResponse; + +/** + * Integration tests for the {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters for + * {@link CopyObjectRequest}. Specifically, we ensure that users are able to seamlessly use the same input for both the + * {@link PutObjectRequest} key and the {@link CopyObjectRequest} source key (and not be required to manually URL encode the + * COPY source key). This also effectively tests for parity with the SDK v1 behavior. + * + * @see CopySourceInterceptor + */ +@RunWith(Parameterized.class) +public class CopySourceIntegrationTest extends S3IntegrationTestBase { + + private static final String SOURCE_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-src"); + private static final String DESTINATION_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-dest"); + + @Before + public void initializeTestData() throws Exception { + createBucket(SOURCE_BUCKET_NAME); + createBucket(DESTINATION_BUCKET_NAME); + } + + @After + public void tearDown() { + deleteBucketAndAllContents(SOURCE_BUCKET_NAME); + deleteBucketAndAllContents(DESTINATION_BUCKET_NAME); + } + + @Parameters + public static Collection parameters() throws Exception { + return Arrays.asList( + "simpleKey", + "key/with/slashes", + "\uD83E\uDEA3", + "specialChars/ +!#$&'()*,:;=?@\"", + "%20" + ); + } + + private final String key; + + public CopySourceIntegrationTest(String key) { + this.key = key; + } + + @Test + public void copyObject_WithoutExplicitVersion_AcceptsSameKeyAsPut() throws Exception { + String originalContent = UUID.randomUUID().toString(); + + s3.putObject(PutObjectRequest.builder() + .bucket(SOURCE_BUCKET_NAME) + .key(key) + .build(), RequestBody.fromString(originalContent, StandardCharsets.UTF_8)); + + s3.copyObject(CopyObjectRequest.builder() + .sourceBucket(SOURCE_BUCKET_NAME) + .sourceKey(key) + .destinationBucket(DESTINATION_BUCKET_NAME) + .destinationKey(key) + .build()); + + String copiedContent = s3.getObjectAsBytes(GetObjectRequest.builder() + .bucket(DESTINATION_BUCKET_NAME) + .key(key) + .build()).asUtf8String(); + + assertThat(copiedContent, is(originalContent)); + } + + /** + * Test that we can correctly copy versioned source objects. + *

+ * Motivated by: https://github.com/aws/aws-sdk-js/issues/727 + */ + @Test + public void copyObject_WithVersioning_AcceptsSameKeyAsPut() throws Exception { + s3.putBucketVersioning(r -> r + .bucket(SOURCE_BUCKET_NAME) + .versioningConfiguration(v -> v.status(BucketVersioningStatus.ENABLED))); + + Map versionToContentMap = new HashMap<>(); + int numVersionsToCreate = 3; + for (int i = 0; i < numVersionsToCreate; i++) { + String originalContent = UUID.randomUUID().toString(); + PutObjectResponse response = s3.putObject(PutObjectRequest.builder() + .bucket(SOURCE_BUCKET_NAME) + .key(key) + .build(), + RequestBody.fromString(originalContent, StandardCharsets.UTF_8)); + versionToContentMap.put(response.versionId(), originalContent); + } + + versionToContentMap.forEach((versionId, originalContent) -> { + s3.copyObject(CopyObjectRequest.builder() + .sourceBucket(SOURCE_BUCKET_NAME) + .sourceKey(key) + .sourceVersionId(versionId) + .destinationBucket(DESTINATION_BUCKET_NAME) + .destinationKey(key) + .build()); + + String copiedContent = s3.getObjectAsBytes(GetObjectRequest.builder() + .bucket(DESTINATION_BUCKET_NAME) + .key(key) + .build()).asUtf8String(); + assertThat(copiedContent, is(originalContent)); + }); + } +} diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java new file mode 100644 index 000000000000..b101fca97cb8 --- /dev/null +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java @@ -0,0 +1,127 @@ +/* + * 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.internal.handlers; + +import static software.amazon.awssdk.services.s3.internal.resource.S3ArnUtils.isArnFor; +import static software.amazon.awssdk.utils.http.SdkHttpUtils.urlEncodeIgnoreSlashes; + +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.core.SdkRequest; +import software.amazon.awssdk.core.interceptor.Context.ModifyRequest; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.services.s3.internal.resource.S3ResourceType; +import software.amazon.awssdk.services.s3.model.CopyObjectRequest; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.UploadPartCopyRequest; + +/** + * This interceptor transforms the {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters for + * {@link CopyObjectRequest} and {@link UploadPartCopyRequest} into a {@code copySource} parameter. The logic needed to + * construct a {@code copySource} can be considered non-trivial, so this interceptor facilitates allowing users to + * use higher-level constructs that more closely match other APIs, like {@link PutObjectRequest}. Additionally, this + * interceptor is responsible for URL encoding the relevant portions of the {@code copySource} value. + *

+ * API_CopyObject_RequestParameters + *

+ * API_UploadPartCopy_RequestParameters + */ +@SdkInternalApi +public final class CopySourceInterceptor implements ExecutionInterceptor { + + @Override + public SdkRequest modifyRequest(ModifyRequest context, ExecutionAttributes executionAttributes) { + SdkRequest request = context.request(); + if (request instanceof CopyObjectRequest) { + return modifyCopyObjectRequest((CopyObjectRequest) request); + } + if (request instanceof UploadPartCopyRequest) { + return modifyUploadPartCopyRequest((UploadPartCopyRequest) request); + } + return request; + } + + private static SdkRequest modifyCopyObjectRequest(CopyObjectRequest request) { + if (request.copySource() != null) { + requireNotSet(request.sourceBucket(), "sourceBucket"); + requireNotSet(request.sourceKey(), "sourceKey"); + requireNotSet(request.sourceVersionId(), "sourceVersionId"); + return request; + } + String copySource = constructCopySource( + requireSet(request.sourceBucket(), "sourceBucket"), + requireSet(request.sourceKey(), "sourceKey"), + request.sourceVersionId() + ); + return request.toBuilder() + .sourceBucket(null) + .sourceKey(null) + .sourceVersionId(null) + .copySource(copySource) + .build(); + } + + private static SdkRequest modifyUploadPartCopyRequest(UploadPartCopyRequest request) { + if (request.copySource() != null) { + requireNotSet(request.sourceBucket(), "sourceBucket"); + requireNotSet(request.sourceKey(), "sourceKey"); + requireNotSet(request.sourceVersionId(), "sourceVersionId"); + return request; + } + String copySource = constructCopySource( + requireSet(request.sourceBucket(), "sourceBucket"), + requireSet(request.sourceKey(), "sourceKey"), + request.sourceVersionId() + ); + return request.toBuilder() + .sourceBucket(null) + .sourceKey(null) + .sourceVersionId(null) + .copySource(copySource) + .build(); + } + + private static String constructCopySource(String sourceBucket, String sourceKey, String sourceVersionId) { + StringBuilder copySource = new StringBuilder(); + copySource.append("/"); + copySource.append(urlEncodeIgnoreSlashes(sourceBucket)); + if (isArnFor(S3ResourceType.ACCESS_POINT, sourceBucket) || isArnFor(S3ResourceType.OUTPOST, sourceBucket)) { + copySource.append("/object"); + } + copySource.append("/"); + copySource.append(urlEncodeIgnoreSlashes(sourceKey)); + if (sourceVersionId != null) { + copySource.append("?versionId="); + copySource.append(urlEncodeIgnoreSlashes(sourceVersionId)); + } + return copySource.toString(); + } + + private static void requireNotSet(Object value, String paramName) { + if (value != null) { + throw new IllegalArgumentException(String.format("Parameter 'copySource' must not be used in conjunction with '%s'", + paramName)); + } + } + + private static T requireSet(T value, String paramName) { + if (value == null) { + throw new IllegalArgumentException(String.format("Parameter '%s' must not be null", + paramName)); + } + return value; + } +} diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java index ec5262419c5a..f77976f97b31 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java @@ -72,4 +72,14 @@ public static IntermediateOutpostResource parseOutpostArn(Arn arn) { .outpostSubresource(ArnResource.fromString(subresource)) .build(); } + + public static boolean isArnFor(S3ResourceType s3ResourceType, String arnString) { + try { + Arn arn = Arn.fromString(arnString); + String parsedResourceType = arn.resource().resourceType().get(); + return S3ResourceType.fromValue(parsedResourceType) == s3ResourceType; + } catch (Exception ignored) { + return false; + } + } } diff --git a/services/s3/src/main/resources/META-INF/native-image/software.amazon.awssdk/s3/reflect-config.json b/services/s3/src/main/resources/META-INF/native-image/software.amazon.awssdk/s3/reflect-config.json index 27a7bbe640b9..a8cb0f7a641a 100644 --- a/services/s3/src/main/resources/META-INF/native-image/software.amazon.awssdk/s3/reflect-config.json +++ b/services/s3/src/main/resources/META-INF/native-image/software.amazon.awssdk/s3/reflect-config.json @@ -115,5 +115,14 @@ "parameterTypes": [] } ] + }, + { + "name": "software.amazon.awssdk.services.s3.internal.handlers.CopySourceInterceptor", + "methods": [ + { + "name": "", + "parameterTypes": [] + } + ] } ] \ No newline at end of file diff --git a/services/s3/src/main/resources/codegen-resources/customization.config b/services/s3/src/main/resources/codegen-resources/customization.config index e284b3aa21cf..1f2e50aa08b4 100644 --- a/services/s3/src/main/resources/codegen-resources/customization.config +++ b/services/s3/src/main/resources/codegen-resources/customization.config @@ -17,6 +17,52 @@ ] }, "CopyObjectRequest": { + "inject": [ + { + "SourceBucket": { + "shape": "BucketName", + "documentation": "The name of the bucket containing the object to copy. The provided input will be URL encoded. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + }, + "SourceKey": { + "shape": "ObjectKey", + "documentation": "The key of the object to copy. The provided input will be URL encoded. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + }, + "SourceVersionId": { + "shape": "ObjectVersionId", + "documentation": "Specifies a particular version of the source object to copy. By default the latest version is copied. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + } + } + ], + "modify": [ + { + "Bucket": { + "emitPropertyName": "DestinationBucket", + "existingNameDeprecated": true + }, + "Key": { + "emitPropertyName": "DestinationKey", + "existingNameDeprecated": true + } + } + ] + }, + "UploadPartCopyRequest": { + "inject": [ + { + "SourceBucket": { + "shape": "BucketName", + "documentation": "The name of the bucket containing the object to copy. The provided input will be URL encoded. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + }, + "SourceKey": { + "shape": "ObjectKey", + "documentation": "The key of the object to copy. The provided input will be URL encoded. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + }, + "SourceVersionId": { + "shape": "ObjectVersionId", + "documentation": "Specifies a particular version of the source object to copy. By default the latest version is copied. The {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters must not be used in conjunction with the {@code copySource} parameter." + } + } + ], "modify": [ { "Bucket": { diff --git a/services/s3/src/main/resources/software/amazon/awssdk/services/s3/execution.interceptors b/services/s3/src/main/resources/software/amazon/awssdk/services/s3/execution.interceptors index d9d447cb955d..a0cad21e1180 100644 --- a/services/s3/src/main/resources/software/amazon/awssdk/services/s3/execution.interceptors +++ b/services/s3/src/main/resources/software/amazon/awssdk/services/s3/execution.interceptors @@ -10,4 +10,5 @@ software.amazon.awssdk.services.s3.internal.handlers.AsyncChecksumValidationInte software.amazon.awssdk.services.s3.internal.handlers.SyncChecksumValidationInterceptor software.amazon.awssdk.services.s3.internal.handlers.EnableTrailingChecksumInterceptor software.amazon.awssdk.services.s3.internal.handlers.ExceptionTranslationInterceptor -software.amazon.awssdk.services.s3.internal.handlers.GetObjectInterceptor \ No newline at end of file +software.amazon.awssdk.services.s3.internal.handlers.GetObjectInterceptor +software.amazon.awssdk.services.s3.internal.handlers.CopySourceInterceptor \ No newline at end of file diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java new file mode 100644 index 000000000000..ad38f53cf1f9 --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java @@ -0,0 +1,173 @@ +/* + * 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.internal.handlers; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; + +import java.util.Arrays; +import java.util.Collection; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.services.s3.model.CopyObjectRequest; +import software.amazon.awssdk.services.s3.model.UploadPartCopyRequest; + +@RunWith(Parameterized.class) +public class CopySourceInterceptorTest { + private final CopySourceInterceptor interceptor = new CopySourceInterceptor(); + + @Parameters + public static Collection parameters() throws Exception { + return Arrays.asList(new String[][] { + {"bucket", "simpleKey", null, + "/bucket/simpleKey"}, + + {"bucket", "key/with/slashes", null, + "/bucket/key/with/slashes"}, + + {"bucket", "\uD83E\uDEA3", null, + "/bucket/%F0%9F%AA%A3"}, + + {"bucket", "specialChars._ +!#$&'()*,:;=?@\"", null, + "/bucket/specialChars._%20%2B%21%23%24%26%27%28%29%2A%2C%3A%3B%3D%3F%40%22"}, + + {"bucket", "%20", null, + "/bucket/%2520"}, + + {"bucket", "key/with/version", "ZJlqdTGGfnWjRWjm.WtQc5XRTNJn3sz_", + "/bucket/key/with/version?versionId=ZJlqdTGGfnWjRWjm.WtQc5XRTNJn3sz_"} + }); + } + + private final String sourceBucket; + private final String sourceKey; + private final String sourceVersionId; + private final String expectedCopySource; + + public CopySourceInterceptorTest(String sourceBucket, String sourceKey, String sourceVersionId, String expectedCopySource) { + this.sourceBucket = sourceBucket; + this.sourceKey = sourceKey; + this.sourceVersionId = sourceVersionId; + this.expectedCopySource = expectedCopySource; + } + + @Test + public void modifyRequest_ConstructsUrlEncodedCopySource_whenCopyObjectRequest() { + CopyObjectRequest originalRequest = CopyObjectRequest.builder() + .sourceBucket(sourceBucket) + .sourceKey(sourceKey) + .sourceVersionId(sourceVersionId) + .build(); + CopyObjectRequest modifiedRequest = (CopyObjectRequest) interceptor + .modifyRequest(() -> originalRequest, new ExecutionAttributes()); + + assertThat(modifiedRequest.copySource()).isEqualTo(expectedCopySource); + } + + @Test + public void modifyRequest_ConstructsUrlEncodedCopySource_whenUploadPartCopyRequest() { + UploadPartCopyRequest originalRequest = UploadPartCopyRequest.builder() + .sourceBucket(sourceBucket) + .sourceKey(sourceKey) + .sourceVersionId(sourceVersionId) + .build(); + UploadPartCopyRequest modifiedRequest = (UploadPartCopyRequest) interceptor + .modifyRequest(() -> originalRequest, new ExecutionAttributes()); + + assertThat(modifiedRequest.copySource()).isEqualTo(expectedCopySource); + } + + @Test + public void modifyRequest_Throws_whenCopySourceUsedWithSourceBucket_withCopyObjectRequest() { + CopyObjectRequest originalRequest = CopyObjectRequest.builder() + .sourceBucket(sourceBucket) + .sourceKey(sourceKey) + .sourceVersionId(sourceVersionId) + .copySource("copySource") + .build(); + + assertThatThrownBy(() -> { + interceptor.modifyRequest(() -> originalRequest, new ExecutionAttributes()); + }).isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'copySource' must not be used in conjunction with 'sourceBucket'"); + } + + @Test + public void modifyRequest_Throws_whenCopySourceUsedWithSourceBucket_withUploadPartCopyRequest() { + UploadPartCopyRequest originalRequest = UploadPartCopyRequest.builder() + .sourceBucket(sourceBucket) + .sourceKey(sourceKey) + .sourceVersionId(sourceVersionId) + .copySource("copySource") + .build(); + + assertThatThrownBy(() -> { + interceptor.modifyRequest(() -> originalRequest, new ExecutionAttributes()); + }).isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'copySource' must not be used in conjunction with 'sourceBucket'"); + } + + @Test + public void modifyRequest_Throws_whenSourceBucketNotSpecified_withCopyObjectRequest() { + CopyObjectRequest originalRequest = CopyObjectRequest.builder().build(); + + assertThatThrownBy(() -> { + interceptor.modifyRequest(() -> originalRequest, new ExecutionAttributes()); + }).isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'sourceBucket' must not be null"); + } + + @Test + public void modifyRequest_Throws_whenSourceBucketNotSpecified_withUploadPartCopyRequest() { + UploadPartCopyRequest originalRequest = UploadPartCopyRequest.builder().build(); + + assertThatThrownBy(() -> { + interceptor.modifyRequest(() -> originalRequest, new ExecutionAttributes()); + }).isInstanceOf(IllegalArgumentException.class) + .hasMessage("Parameter 'sourceBucket' must not be null"); + } + + @Test + public void modifyRequest_insertsSlashObject_whenAccessPointArn() { + String accessPointArn = "arn:aws:s3:us-west-2:123456789012:accesspoint/my-access-point"; + CopyObjectRequest originalRequest = CopyObjectRequest.builder() + .sourceBucket(accessPointArn) + .sourceKey("my-key") + .build(); + CopyObjectRequest modifiedRequest = (CopyObjectRequest) interceptor + .modifyRequest(() -> originalRequest, new ExecutionAttributes()); + + assertThat(modifiedRequest.copySource()).isEqualTo( + "/arn%3Aaws%3As3%3Aus-west-2%3A123456789012%3Aaccesspoint/my-access-point/object/my-key"); + } + + @Test + public void modifyRequest_insertsSlashObject_whenOutpostArn() { + String outpostBucketArn = "arn:aws:s3-outposts:us-west-2:123456789012:outpost/my-outpost/bucket/my-bucket"; + CopyObjectRequest originalRequest = CopyObjectRequest.builder() + .sourceBucket(outpostBucketArn) + .sourceKey("my-key") + .build(); + CopyObjectRequest modifiedRequest = (CopyObjectRequest) interceptor + .modifyRequest(() -> originalRequest, new ExecutionAttributes()); + + assertThat(modifiedRequest.copySource()).isEqualTo( + "/arn%3Aaws%3As3-outposts%3Aus-west-2%3A123456789012%3Aoutpost/my-outpost/bucket/my-bucket/object/my-key"); + } +} diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java index 1bd47ba02294..b4a7834b9bba 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java @@ -20,6 +20,7 @@ import static org.hamcrest.Matchers.is; import java.util.Optional; +import java.util.UUID; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -131,4 +132,22 @@ public void parseOutpostArn_malformedArnEmptyOutpostId_shouldThrowException() { .resource("outpost::accesspoint:name") .build()); } + + @Test + public void isArnFor_shouldRecognizeAccessPointArn() { + String arnString = "arn:aws:s3:us-west-2:123456789012:accesspoint/my-access-point"; + assertThat(S3ArnUtils.isArnFor(S3ResourceType.ACCESS_POINT, arnString), is(true)); + } + + @Test + public void isArnFor_shouldRecognizeOutpostArn() { + String arnString = "arn:aws:s3-outposts:us-west-2:123456789012:outpost/my-outpost/bucket/my-bucket"; + assertThat(S3ArnUtils.isArnFor(S3ResourceType.OUTPOST, arnString), is(true)); + } + + @Test + public void isArnFor_shouldNotThrow_onRandomInput() { + String arnString = UUID.randomUUID().toString(); + assertThat(S3ArnUtils.isArnFor(S3ResourceType.BUCKET, arnString), is(false)); + } } From a1513c00c947c9a7497e2a50cbf487ee90d78717 Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Tue, 20 Jul 2021 19:57:06 -0700 Subject: [PATCH 2/5] [S3] Add support for more user-friendly CopyObject source parameters ## Motivation and Context The current S3Client interface has a cumbersome API for invoking CopyObjectRequests. We require users to define the bucket name, key name, and version ID in a raw string format. We require that the string conform to the S3 API, which forces users to know the intricate details for how to join these values together. Additionally, portions (but not all) of the value must be URL encoded, further increasing the burden. https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#API_CopyObject_RequestParameters In the Java SDK v1, users are given explicit parameters for the different copy source attributes. But in v2, parity for this support is clearly lacking. E.g., v1: ``` s3.copyObject(new CopyObjectRequest() .withSourceBucketName(SOURCE_BUCKET) .withSourceKey(key) .withSourceVersionId(versionId) .withDestinationBucketName(DESTINATION_BUCKET) .withDestinationKey(key)); ``` v2: ``` s3.copyObject(CopyObjectRequest.builder() .copySource(SOURCE_BUCKET + "/" + key + "?versionId=" + versionId) .destinationBucket(DESTINATION_BUCKET) .destinationKey(key) .build()); ``` The v1 SDK will also URL encode on the user's behalf, allowing users to use the same input values as they would for a PutObjectRequest. The v2 code snippet above may appear to work for most users until they run into unexpected source keys that require URL encoding, at which point they will typically be given `NoSuchKey` errors. This API deficiency has been called out by users in at least the following issues: * https://github.com/aws/aws-sdk-java-v2/issues/1313 * https://github.com/aws/aws-sdk-java-v2/issues/1452 * https://github.com/aws/aws-sdk-java-v2/issues/1656 * https://github.com/awsdocs/aws-doc-sdk-examples/issues/740 ## Description * For both CopyObjectRequest and UploadPartCopyRequest, add explicit parameters for: SourceBucket, SourceKey, SourceVersionId * If specified, these values will be used to construct a CopySource on the user's behalf, including URL encoding the relevant portions. * These values are introduced in a backwards compatible fashion. Users who are already using CopySource today will see no change in behavior, but these new fields may not be used in conjunction with CopySource. * A follow-up PR will be submitted to propose deprecating the current CopySource parameter. It is excluded from this PR since our current code gen configuration lacks appropriate support. * Add support for "DestinationBucket" & "DestinationKey" to UploadPartCopyRequest (this support already existed for CopyObjectRequest) * Utility function added to detect if an ARN is for a particular S3 resource type. This is to conform with the S3 API requirements of inserting "/object" in the path of these requests. ## Testing * New unit tests added * New integration tests added --- .changes/next-release/feature-AmazonS3-9b198d0.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/feature-AmazonS3-9b198d0.json diff --git a/.changes/next-release/feature-AmazonS3-9b198d0.json b/.changes/next-release/feature-AmazonS3-9b198d0.json new file mode 100644 index 000000000000..7bc4aa7703b0 --- /dev/null +++ b/.changes/next-release/feature-AmazonS3-9b198d0.json @@ -0,0 +1,6 @@ +{ + "category": "Amazon S3", + "contributor": "", + "type": "feature", + "description": "Add support for more user-friendly CopyObject source parameters" +} From cf7f797e34827c82ffb98dacb8d285f57e47b328 Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 21 Jul 2021 12:18:51 -0700 Subject: [PATCH 3/5] [S3] Add support for more user-friendly CopyObject source parameters ## Motivation and Context The current S3Client interface has a cumbersome API for invoking CopyObjectRequests. We require users to define the bucket name, key name, and version ID in a raw string format. We require that the string conform to the S3 API, which forces users to know the intricate details for how to join these values together. Additionally, portions (but not all) of the value must be URL encoded, further increasing the burden. https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#API_CopyObject_RequestParameters In the Java SDK v1, users are given explicit parameters for the different copy source attributes. But in v2, parity for this support is clearly lacking. E.g., v1: ``` s3.copyObject(new CopyObjectRequest() .withSourceBucketName(SOURCE_BUCKET) .withSourceKey(key) .withSourceVersionId(versionId) .withDestinationBucketName(DESTINATION_BUCKET) .withDestinationKey(key)); ``` v2: ``` s3.copyObject(CopyObjectRequest.builder() .copySource(SOURCE_BUCKET + "/" + key + "?versionId=" + versionId) .destinationBucket(DESTINATION_BUCKET) .destinationKey(key) .build()); ``` The v1 SDK will also URL encode on the user's behalf, allowing users to use the same input values as they would for a PutObjectRequest. The v2 code snippet above may appear to work for most users until they run into unexpected source keys that require URL encoding, at which point they will typically be given `NoSuchKey` errors. This API deficiency has been called out by users in at least the following issues: * https://github.com/aws/aws-sdk-java-v2/issues/1313 * https://github.com/aws/aws-sdk-java-v2/issues/1452 * https://github.com/aws/aws-sdk-java-v2/issues/1656 * https://github.com/awsdocs/aws-doc-sdk-examples/issues/740 ## Description * For both CopyObjectRequest and UploadPartCopyRequest, add explicit parameters for: SourceBucket, SourceKey, SourceVersionId * If specified, these values will be used to construct a CopySource on the user's behalf, including URL encoding the relevant portions. * These values are introduced in a backwards compatible fashion. Users who are already using CopySource today will see no change in behavior, but these new fields may not be used in conjunction with CopySource. * A follow-up PR will be submitted to propose deprecating the current CopySource parameter. It is excluded from this PR since our current code gen configuration lacks appropriate support. * Add support for "DestinationBucket" & "DestinationKey" to UploadPartCopyRequest (this support already existed for CopyObjectRequest) * Utility function added to detect if an ARN is for a particular S3 resource type. This is to conform with the S3 API requirements of inserting "/object" in the path of these requests. ## Testing * New unit tests added * New integration tests added --- .../s3/CopySourceIntegrationTest.java | 6 ++--- .../handlers/CopySourceInterceptor.java | 23 +++++++++---------- .../s3/internal/resource/S3ArnUtils.java | 10 ++++---- .../handlers/CopySourceInterceptorTest.java | 2 +- .../s3/internal/resource/S3ArnUtilsTest.java | 17 +++++++++----- 5 files changed, 32 insertions(+), 26 deletions(-) diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java index 8efec01ef82a..8bc395a7ce24 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java @@ -66,7 +66,7 @@ public void tearDown() { } @Parameters - public static Collection parameters() throws Exception { + public static Collection parameters() throws Exception { return Arrays.asList( "simpleKey", "key/with/slashes", @@ -83,7 +83,7 @@ public CopySourceIntegrationTest(String key) { } @Test - public void copyObject_WithoutExplicitVersion_AcceptsSameKeyAsPut() throws Exception { + public void copyObject_WithoutVersion_AcceptsSameKeyAsPut() throws Exception { String originalContent = UUID.randomUUID().toString(); s3.putObject(PutObjectRequest.builder() @@ -112,7 +112,7 @@ public void copyObject_WithoutExplicitVersion_AcceptsSameKeyAsPut() throws Excep * Motivated by: https://github.com/aws/aws-sdk-js/issues/727 */ @Test - public void copyObject_WithVersioning_AcceptsSameKeyAsPut() throws Exception { + public void copyObject_WithVersion_AcceptsSameKeyAsPut() throws Exception { s3.putBucketVersioning(r -> r .bucket(SOURCE_BUCKET_NAME) .versioningConfiguration(v -> v.status(BucketVersioningStatus.ENABLED))); diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java index b101fca97cb8..81816bfd01af 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptor.java @@ -15,7 +15,6 @@ package software.amazon.awssdk.services.s3.internal.handlers; -import static software.amazon.awssdk.services.s3.internal.resource.S3ArnUtils.isArnFor; import static software.amazon.awssdk.utils.http.SdkHttpUtils.urlEncodeIgnoreSlashes; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -23,10 +22,12 @@ import software.amazon.awssdk.core.interceptor.Context.ModifyRequest; import software.amazon.awssdk.core.interceptor.ExecutionAttributes; import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.services.s3.internal.resource.S3ArnUtils; import software.amazon.awssdk.services.s3.internal.resource.S3ResourceType; import software.amazon.awssdk.services.s3.model.CopyObjectRequest; import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.UploadPartCopyRequest; +import software.amazon.awssdk.utils.Validate; /** * This interceptor transforms the {@code sourceBucket}, {@code sourceKey}, and {@code sourceVersionId} parameters for @@ -98,9 +99,11 @@ private static String constructCopySource(String sourceBucket, String sourceKey, StringBuilder copySource = new StringBuilder(); copySource.append("/"); copySource.append(urlEncodeIgnoreSlashes(sourceBucket)); - if (isArnFor(S3ResourceType.ACCESS_POINT, sourceBucket) || isArnFor(S3ResourceType.OUTPOST, sourceBucket)) { - copySource.append("/object"); - } + S3ArnUtils.getArnType(sourceBucket).ifPresent(arnType -> { + if (arnType == S3ResourceType.ACCESS_POINT || arnType == S3ResourceType.OUTPOST) { + copySource.append("/object"); + } + }); copySource.append("/"); copySource.append(urlEncodeIgnoreSlashes(sourceKey)); if (sourceVersionId != null) { @@ -111,17 +114,13 @@ private static String constructCopySource(String sourceBucket, String sourceKey, } private static void requireNotSet(Object value, String paramName) { - if (value != null) { - throw new IllegalArgumentException(String.format("Parameter 'copySource' must not be used in conjunction with '%s'", - paramName)); - } + Validate.isTrue(value == null, "Parameter 'copySource' must not be used in conjunction with '%s'", + paramName); } private static T requireSet(T value, String paramName) { - if (value == null) { - throw new IllegalArgumentException(String.format("Parameter '%s' must not be null", - paramName)); - } + Validate.isTrue(value != null, "Parameter '%s' must not be null", + paramName); return value; } } diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java index f77976f97b31..4aa5bbeb10e5 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtils.java @@ -16,6 +16,7 @@ package software.amazon.awssdk.services.s3.internal.resource; +import java.util.Optional; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.arns.Arn; import software.amazon.awssdk.arns.ArnResource; @@ -73,13 +74,14 @@ public static IntermediateOutpostResource parseOutpostArn(Arn arn) { .build(); } - public static boolean isArnFor(S3ResourceType s3ResourceType, String arnString) { + public static Optional getArnType(String arnString) { try { Arn arn = Arn.fromString(arnString); - String parsedResourceType = arn.resource().resourceType().get(); - return S3ResourceType.fromValue(parsedResourceType) == s3ResourceType; + String resourceType = arn.resource().resourceType().get(); + S3ResourceType s3ResourceType = S3ResourceType.fromValue(resourceType); + return Optional.of(s3ResourceType); } catch (Exception ignored) { - return false; + return Optional.empty(); } } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java index ad38f53cf1f9..5bbe9b025adf 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/CopySourceInterceptorTest.java @@ -33,7 +33,7 @@ public class CopySourceInterceptorTest { private final CopySourceInterceptor interceptor = new CopySourceInterceptor(); @Parameters - public static Collection parameters() throws Exception { + public static Collection parameters() throws Exception { return Arrays.asList(new String[][] { {"bucket", "simpleKey", null, "/bucket/simpleKey"}, diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java index b4a7834b9bba..dc0ed4142eac 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java @@ -134,20 +134,25 @@ public void parseOutpostArn_malformedArnEmptyOutpostId_shouldThrowException() { } @Test - public void isArnFor_shouldRecognizeAccessPointArn() { + public void getArnType_shouldRecognizeAccessPointArn() { String arnString = "arn:aws:s3:us-west-2:123456789012:accesspoint/my-access-point"; - assertThat(S3ArnUtils.isArnFor(S3ResourceType.ACCESS_POINT, arnString), is(true)); + Optional arnType = S3ArnUtils.getArnType(arnString); + assertThat(arnType.isPresent(), is(true)); + assertThat(arnType.get(), is(S3ResourceType.ACCESS_POINT)); } @Test - public void isArnFor_shouldRecognizeOutpostArn() { + public void getArnType_shouldRecognizeOutpostArn() { String arnString = "arn:aws:s3-outposts:us-west-2:123456789012:outpost/my-outpost/bucket/my-bucket"; - assertThat(S3ArnUtils.isArnFor(S3ResourceType.OUTPOST, arnString), is(true)); + Optional arnType = S3ArnUtils.getArnType(arnString); + assertThat(arnType.isPresent(), is(true)); + assertThat(arnType.get(), is(S3ResourceType.OUTPOST)); } @Test - public void isArnFor_shouldNotThrow_onRandomInput() { + public void getArnType_shouldNotThrow_onRandomInput() { String arnString = UUID.randomUUID().toString(); - assertThat(S3ArnUtils.isArnFor(S3ResourceType.BUCKET, arnString), is(false)); + Optional arnType = S3ArnUtils.getArnType(arnString); + assertThat(arnType.isPresent(), is(false)); } } From 0f2e5757d97cd46d7044fc71d4f989b47954928b Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 21 Jul 2021 12:28:56 -0700 Subject: [PATCH 4/5] [S3] Add support for more user-friendly CopyObject source parameters ## Motivation and Context The current S3Client interface has a cumbersome API for invoking CopyObjectRequests. We require users to define the bucket name, key name, and version ID in a raw string format. We require that the string conform to the S3 API, which forces users to know the intricate details for how to join these values together. Additionally, portions (but not all) of the value must be URL encoded, further increasing the burden. https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#API_CopyObject_RequestParameters In the Java SDK v1, users are given explicit parameters for the different copy source attributes. But in v2, parity for this support is clearly lacking. E.g., v1: ``` s3.copyObject(new CopyObjectRequest() .withSourceBucketName(SOURCE_BUCKET) .withSourceKey(key) .withSourceVersionId(versionId) .withDestinationBucketName(DESTINATION_BUCKET) .withDestinationKey(key)); ``` v2: ``` s3.copyObject(CopyObjectRequest.builder() .copySource(SOURCE_BUCKET + "/" + key + "?versionId=" + versionId) .destinationBucket(DESTINATION_BUCKET) .destinationKey(key) .build()); ``` The v1 SDK will also URL encode on the user's behalf, allowing users to use the same input values as they would for a PutObjectRequest. The v2 code snippet above may appear to work for most users until they run into unexpected source keys that require URL encoding, at which point they will typically be given `NoSuchKey` errors. This API deficiency has been called out by users in at least the following issues: * https://github.com/aws/aws-sdk-java-v2/issues/1313 * https://github.com/aws/aws-sdk-java-v2/issues/1452 * https://github.com/aws/aws-sdk-java-v2/issues/1656 * https://github.com/awsdocs/aws-doc-sdk-examples/issues/740 ## Description * For both CopyObjectRequest and UploadPartCopyRequest, add explicit parameters for: SourceBucket, SourceKey, SourceVersionId * If specified, these values will be used to construct a CopySource on the user's behalf, including URL encoding the relevant portions. * These values are introduced in a backwards compatible fashion. Users who are already using CopySource today will see no change in behavior, but these new fields may not be used in conjunction with CopySource. * A follow-up PR will be submitted to propose deprecating the current CopySource parameter. It is excluded from this PR since our current code gen configuration lacks appropriate support. * Add support for "DestinationBucket" & "DestinationKey" to UploadPartCopyRequest (this support already existed for CopyObjectRequest) * Utility function added to detect if an ARN is for a particular S3 resource type. This is to conform with the S3 API requirements of inserting "/object" in the path of these requests. ## Testing * New unit tests added * New integration tests added --- .../services/s3/internal/resource/S3ArnUtilsTest.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java index dc0ed4142eac..b5159aa8bd35 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/resource/S3ArnUtilsTest.java @@ -136,23 +136,18 @@ public void parseOutpostArn_malformedArnEmptyOutpostId_shouldThrowException() { @Test public void getArnType_shouldRecognizeAccessPointArn() { String arnString = "arn:aws:s3:us-west-2:123456789012:accesspoint/my-access-point"; - Optional arnType = S3ArnUtils.getArnType(arnString); - assertThat(arnType.isPresent(), is(true)); - assertThat(arnType.get(), is(S3ResourceType.ACCESS_POINT)); + assertThat(S3ArnUtils.getArnType(arnString), is(Optional.of(S3ResourceType.ACCESS_POINT))); } @Test public void getArnType_shouldRecognizeOutpostArn() { String arnString = "arn:aws:s3-outposts:us-west-2:123456789012:outpost/my-outpost/bucket/my-bucket"; - Optional arnType = S3ArnUtils.getArnType(arnString); - assertThat(arnType.isPresent(), is(true)); - assertThat(arnType.get(), is(S3ResourceType.OUTPOST)); + assertThat(S3ArnUtils.getArnType(arnString), is(Optional.of(S3ResourceType.OUTPOST))); } @Test public void getArnType_shouldNotThrow_onRandomInput() { String arnString = UUID.randomUUID().toString(); - Optional arnType = S3ArnUtils.getArnType(arnString); - assertThat(arnType.isPresent(), is(false)); + assertThat(S3ArnUtils.getArnType(arnString), is(Optional.empty())); } } From ff4ac00fbfa263a17f6342c3c3ab08ac69752ba9 Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 21 Jul 2021 12:43:14 -0700 Subject: [PATCH 5/5] [S3] Add support for more user-friendly CopyObject source parameters ## Motivation and Context The current S3Client interface has a cumbersome API for invoking CopyObjectRequests. We require users to define the bucket name, key name, and version ID in a raw string format. We require that the string conform to the S3 API, which forces users to know the intricate details for how to join these values together. Additionally, portions (but not all) of the value must be URL encoded, further increasing the burden. https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#API_CopyObject_RequestParameters In the Java SDK v1, users are given explicit parameters for the different copy source attributes. But in v2, parity for this support is clearly lacking. E.g., v1: ``` s3.copyObject(new CopyObjectRequest() .withSourceBucketName(SOURCE_BUCKET) .withSourceKey(key) .withSourceVersionId(versionId) .withDestinationBucketName(DESTINATION_BUCKET) .withDestinationKey(key)); ``` v2: ``` s3.copyObject(CopyObjectRequest.builder() .copySource(SOURCE_BUCKET + "/" + key + "?versionId=" + versionId) .destinationBucket(DESTINATION_BUCKET) .destinationKey(key) .build()); ``` The v1 SDK will also URL encode on the user's behalf, allowing users to use the same input values as they would for a PutObjectRequest. The v2 code snippet above may appear to work for most users until they run into unexpected source keys that require URL encoding, at which point they will typically be given `NoSuchKey` errors. This API deficiency has been called out by users in at least the following issues: * https://github.com/aws/aws-sdk-java-v2/issues/1313 * https://github.com/aws/aws-sdk-java-v2/issues/1452 * https://github.com/aws/aws-sdk-java-v2/issues/1656 * https://github.com/awsdocs/aws-doc-sdk-examples/issues/740 ## Description * For both CopyObjectRequest and UploadPartCopyRequest, add explicit parameters for: SourceBucket, SourceKey, SourceVersionId * If specified, these values will be used to construct a CopySource on the user's behalf, including URL encoding the relevant portions. * These values are introduced in a backwards compatible fashion. Users who are already using CopySource today will see no change in behavior, but these new fields may not be used in conjunction with CopySource. * A follow-up PR will be submitted to propose deprecating the current CopySource parameter. It is excluded from this PR since our current code gen configuration lacks appropriate support. * Add support for "DestinationBucket" & "DestinationKey" to UploadPartCopyRequest (this support already existed for CopyObjectRequest) * Utility function added to detect if an ARN is for a particular S3 resource type. This is to conform with the S3 API requirements of inserting "/object" in the path of these requests. ## Testing * New unit tests added * New integration tests added --- .../s3/CopySourceIntegrationTest.java | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java index 8bc395a7ce24..1b47754040d4 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/CopySourceIntegrationTest.java @@ -25,8 +25,8 @@ import java.util.HashMap; import java.util.Map; import java.util.UUID; -import org.junit.After; -import org.junit.Before; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -50,18 +50,24 @@ @RunWith(Parameterized.class) public class CopySourceIntegrationTest extends S3IntegrationTestBase { - private static final String SOURCE_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-src"); + private static final String SOURCE_UNVERSIONED_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-src"); + private static final String SOURCE_VERSIONED_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-versioned-src"); private static final String DESTINATION_BUCKET_NAME = temporaryBucketName("copy-source-integ-test-dest"); - @Before - public void initializeTestData() throws Exception { - createBucket(SOURCE_BUCKET_NAME); + @BeforeClass + public static void initializeTestData() throws Exception { + createBucket(SOURCE_UNVERSIONED_BUCKET_NAME); + createBucket(SOURCE_VERSIONED_BUCKET_NAME); + s3.putBucketVersioning(r -> r + .bucket(SOURCE_VERSIONED_BUCKET_NAME) + .versioningConfiguration(v -> v.status(BucketVersioningStatus.ENABLED))); createBucket(DESTINATION_BUCKET_NAME); } - @After - public void tearDown() { - deleteBucketAndAllContents(SOURCE_BUCKET_NAME); + @AfterClass + public static void tearDown() { + deleteBucketAndAllContents(SOURCE_UNVERSIONED_BUCKET_NAME); + deleteBucketAndAllContents(SOURCE_VERSIONED_BUCKET_NAME); deleteBucketAndAllContents(DESTINATION_BUCKET_NAME); } @@ -87,12 +93,12 @@ public void copyObject_WithoutVersion_AcceptsSameKeyAsPut() throws Exception { String originalContent = UUID.randomUUID().toString(); s3.putObject(PutObjectRequest.builder() - .bucket(SOURCE_BUCKET_NAME) + .bucket(SOURCE_UNVERSIONED_BUCKET_NAME) .key(key) .build(), RequestBody.fromString(originalContent, StandardCharsets.UTF_8)); s3.copyObject(CopyObjectRequest.builder() - .sourceBucket(SOURCE_BUCKET_NAME) + .sourceBucket(SOURCE_UNVERSIONED_BUCKET_NAME) .sourceKey(key) .destinationBucket(DESTINATION_BUCKET_NAME) .destinationKey(key) @@ -113,16 +119,12 @@ public void copyObject_WithoutVersion_AcceptsSameKeyAsPut() throws Exception { */ @Test public void copyObject_WithVersion_AcceptsSameKeyAsPut() throws Exception { - s3.putBucketVersioning(r -> r - .bucket(SOURCE_BUCKET_NAME) - .versioningConfiguration(v -> v.status(BucketVersioningStatus.ENABLED))); - Map versionToContentMap = new HashMap<>(); int numVersionsToCreate = 3; for (int i = 0; i < numVersionsToCreate; i++) { String originalContent = UUID.randomUUID().toString(); PutObjectResponse response = s3.putObject(PutObjectRequest.builder() - .bucket(SOURCE_BUCKET_NAME) + .bucket(SOURCE_VERSIONED_BUCKET_NAME) .key(key) .build(), RequestBody.fromString(originalContent, StandardCharsets.UTF_8)); @@ -131,7 +133,7 @@ public void copyObject_WithVersion_AcceptsSameKeyAsPut() throws Exception { versionToContentMap.forEach((versionId, originalContent) -> { s3.copyObject(CopyObjectRequest.builder() - .sourceBucket(SOURCE_BUCKET_NAME) + .sourceBucket(SOURCE_VERSIONED_BUCKET_NAME) .sourceKey(key) .sourceVersionId(versionId) .destinationBucket(DESTINATION_BUCKET_NAME)