From e10a0d259f31a14269ad751746bc90d0a41286d3 Mon Sep 17 00:00:00 2001 From: sbiscigl Date: Wed, 11 Nov 2026 16:15:57 -0500 Subject: [PATCH] Let S3 copy annotations on single-part S3CrtClient CopyObject --- .../bugfix-s3crt-small-copy-annotations.json | 6 + .../aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp | 15 +- .../BucketAndObjectOperationTest.cpp | 152 +++++++++++++++++- .../cpp/s3/s3-crt/S3CrtSpecificOperations.vm | 17 +- .../s3-crt/SmithyS3CrtSpecificOperations.vm | 17 +- 5 files changed, 175 insertions(+), 32 deletions(-) create mode 100644 .changelog/bugfix-s3crt-small-copy-annotations.json diff --git a/.changelog/bugfix-s3crt-small-copy-annotations.json b/.changelog/bugfix-s3crt-small-copy-annotations.json new file mode 100644 index 00000000000..213545249e5 --- /dev/null +++ b/.changelog/bugfix-s3crt-small-copy-annotations.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "aws-cpp-sdk-s3-crt", + "contributor": "sbiscigl", + "description": "S3CrtClient::CopyObject no longer reads and rewrites annotations itself for objects under 1 GiB, because S3's single-part CopyObject already copies them. Small copies with AnnotationDirective::COPY no longer make extra annotation calls or need annotation permissions." +} diff --git a/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp b/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp index c7c22e6872a..376db919de4 100644 --- a/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp +++ b/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp @@ -880,10 +880,13 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti httpRequest->SetHeaderValue("x-amz-copy-source-if-match", sourceETag); } + // Below the multipart threshold S3's single-part CopyObject honors the metadata, tagging and + // annotation directives natively, so skip copying them here. + if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) { + return annotations; + } + if (copyAnnotations) { - // Neither the multipart copy nor S3's single-part CopyObject carries annotations, so always - // read them from the source here (regardless of size) and return them to be written to the - // destination after the copy completes (see the wrapped completion handler). Aws::String continuationToken; do { Model::ListObjectAnnotationsRequest listRequest; @@ -922,12 +925,6 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti } while (!continuationToken.empty()); } - // Below the multipart threshold S3's single-part CopyObject honors the metadata/tagging directives - // natively, so skip header injection (annotations are handled above regardless of size). - if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) { - return annotations; - } - if (copyMetadata) { if (!head.GetContentType().empty()) { httpRequest->SetHeaderValue("content-type", head.GetContentType()); diff --git a/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp b/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp index 82d35db0022..18130217bea 100644 --- a/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp +++ b/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -47,6 +48,11 @@ #include #include #include +#include +#include +#include +#include +#include #include #include #include @@ -55,8 +61,11 @@ #include #include #include +#include #include +#include #include +#include #ifdef _WIN32 #pragma warning(disable: 4127) @@ -95,6 +104,7 @@ namespace static std::string BASE_CHECKSUMS_BUCKET_NAME = "checksums-crt"; static std::string BASE_CRT_CREDENTIALS_TEST_BUCKET_NAME = "crt-credentials-test"; static std::string BASE_VERSIONED_COPY_BUCKET_NAME = "versionedcopytest"; + static std::string BASE_ANNOTATIONS_COPY_BUCKET_NAME = "annotationcopy"; static const char* ALLOCATION_TAG = "BucketAndObjectOperationTest"; static const char* TEST_OBJ_KEY = "TestObjectKey"; static const char* TEST_NOT_MODIFIED_OBJ_KEY = "TestNotModifiedObjectKey"; @@ -136,7 +146,8 @@ namespace std::ref(BASE_EVENT_STREAM_ERRORS_IN_EVENT_TEST_BUCKET_NAME), std::ref(BASE_CHECKSUMS_BUCKET_NAME), std::ref(BASE_CRT_CREDENTIALS_TEST_BUCKET_NAME), - std::ref(BASE_VERSIONED_COPY_BUCKET_NAME) + std::ref(BASE_VERSIONED_COPY_BUCKET_NAME), + std::ref(BASE_ANNOTATIONS_COPY_BUCKET_NAME) }; for (auto& testBucketName : TEST_BUCKETS) @@ -153,6 +164,26 @@ namespace long CalculateDelayBeforeNextRetry(const AWSError&, long) const override { return 0; } }; + class AnnotationRequestCountingRetryStrategy: public Aws::Client::DefaultRetryStrategy + { + public: + void RequestBookkeeping(const HttpResponseOutcome& outcome) override { Count(outcome); } + void RequestBookkeeping(const HttpResponseOutcome& outcome, const AWSError&) override { Count(outcome); } + size_t GetAnnotationRequestCount() const { return m_annotationRequestCount; } + + private: + void Count(const HttpResponseOutcome& outcome) + { + if (outcome.IsSuccess() && + outcome.GetResult()->GetOriginatingRequest().GetUri().GetQueryString().find("annotation") != Aws::String::npos) + { + ++m_annotationRequestCount; + } + } + + std::atomic m_annotationRequestCount{0}; + }; + class BucketAndObjectOperationTest : public ::testing::Test { public: @@ -160,8 +191,6 @@ namespace static std::shared_ptr oregonClient; static std::shared_ptr m_HttpClient; - protected: - static void SetUpTestCase() { EnsureUniqueBucketNames(); @@ -185,6 +214,7 @@ namespace DeleteBucket(CalculateBucketName(BASE_EVENT_STREAM_ERRORS_IN_EVENT_TEST_BUCKET_NAME.c_str())); DeleteBucket(CalculateBucketName(BASE_CHECKSUMS_BUCKET_NAME.c_str())); DeleteBucket(CalculateBucketName(BASE_CRT_CREDENTIALS_TEST_BUCKET_NAME.c_str())); + DeleteBucket(CalculateBucketName(BASE_ANNOTATIONS_COPY_BUCKET_NAME.c_str())); Client = nullptr; oregonClient = nullptr; @@ -1152,6 +1182,122 @@ namespace Client->DeleteBucket(deleteBucketRequest); } + void CopyObjectWithAnnotations(S3CrtClient& copyClient, + const std::function& createSource) + { + const auto& client = BucketAndObjectOperationTest::Client; + const Aws::String fullBucketName = BucketAndObjectOperationTest::CalculateBucketName(BASE_ANNOTATIONS_COPY_BUCKET_NAME.c_str()); + SCOPED_TRACE(Aws::String("FullBucketName ") + fullBucketName); + CreateBucketRequest createBucketRequest; + createBucketRequest.SetBucket(fullBucketName); + createBucketRequest.SetACL(BucketCannedACL::private_); + CreateBucketOutcome createBucketOutcome = client->CreateBucket(createBucketRequest); + AWS_ASSERT_SUCCESS(createBucketOutcome); + ASSERT_TRUE(BucketAndObjectOperationTest::WaitForBucketToPropagate(fullBucketName)); + BucketAndObjectOperationTest::TagTestBucket(fullBucketName, client); + + const Aws::String testName = ::testing::UnitTest::GetInstance()->current_test_info()->name(); + const Aws::String sourceKey = testName + "-source"; + const Aws::String destKey = testName + "-destination"; + ASSERT_NO_FATAL_FAILURE(createSource(fullBucketName, sourceKey)); + + const Aws::Map annotations{{"classification", "internal"}, {"owner", "sdk-team"}}; + for (const auto& annotation : annotations) + { + PutObjectAnnotationRequest putAnnotationRequest; + putAnnotationRequest.WithBucket(fullBucketName).WithKey(sourceKey).WithAnnotationName(annotation.first); + putAnnotationRequest.SetBody(Aws::MakeShared(ALLOCATION_TAG, annotation.second)); + auto putAnnotationOutcome = client->PutObjectAnnotation(putAnnotationRequest); + AWS_ASSERT_SUCCESS(putAnnotationOutcome); + } + + CopyObjectRequest copyRequest; + copyRequest.WithBucket(fullBucketName) + .WithKey(destKey) + .WithCopySource(fullBucketName + "/" + sourceKey) + .WithAnnotationDirective(AnnotationDirective::COPY); + auto copyOutcome = copyClient.CopyObject(copyRequest); + AWS_ASSERT_SUCCESS(copyOutcome); + + ListObjectAnnotationsRequest listAnnotationsRequest; + listAnnotationsRequest.WithBucket(fullBucketName).WithKey(destKey); + auto listAnnotationsOutcome = client->ListObjectAnnotations(listAnnotationsRequest); + AWS_ASSERT_SUCCESS(listAnnotationsOutcome); + Aws::Map copiedAnnotations; + for (const auto& entry : listAnnotationsOutcome.GetResult().GetAnnotations()) + { + GetObjectAnnotationRequest getAnnotationRequest; + getAnnotationRequest.WithBucket(fullBucketName).WithKey(destKey).WithAnnotationName(entry.GetAnnotationName()); + auto getAnnotationOutcome = client->GetObjectAnnotation(getAnnotationRequest); + AWS_ASSERT_SUCCESS(getAnnotationOutcome); + auto& payload = getAnnotationOutcome.GetResult().GetAnnotationPayload(); + copiedAnnotations.emplace(entry.GetAnnotationName(), Aws::String(std::istreambuf_iterator(payload), {})); + } + ASSERT_EQ(annotations, copiedAnnotations); + } + + TEST_F(BucketAndObjectOperationTest, TestSinglePartCopyObjectLeavesAnnotationsToS3) + { + auto retryStrategy = Aws::MakeShared(ALLOCATION_TAG); + Aws::S3Crt::ClientConfiguration countingClientConfig; + countingClientConfig.region = Aws::Region::US_EAST_1; + countingClientConfig.retryStrategy = retryStrategy; + S3CrtClient countingClient(countingClientConfig); + + ASSERT_NO_FATAL_FAILURE(CopyObjectWithAnnotations(countingClient, [](const Aws::String& bucketName, const Aws::String& sourceKey) + { + PutObjectRequest putObjectRequest; + putObjectRequest.WithBucket(bucketName).WithKey(sourceKey); + putObjectRequest.SetBody(Aws::MakeShared(ALLOCATION_TAG, "single part annotations copy test payload")); + auto putObjectOutcome = Client->PutObject(putObjectRequest); + AWS_ASSERT_SUCCESS(putObjectOutcome); + })); + ASSERT_EQ(0u, retryStrategy->GetAnnotationRequestCount()); + } + + TEST_F(BucketAndObjectOperationTest, TestMultipartCopyObjectCopiesAnnotations) + { + ASSERT_NO_FATAL_FAILURE(CopyObjectWithAnnotations(*Client, [](const Aws::String& bucketName, const Aws::String& sourceKey) + { + const Aws::String partSourceKey = sourceKey + "-part"; + PutObjectRequest putPartSourceRequest; + putPartSourceRequest.WithBucket(bucketName).WithKey(partSourceKey); + putPartSourceRequest.SetBody(Aws::MakeShared(ALLOCATION_TAG, Aws::String(8 * 1024 * 1024, 'x'))); + auto putPartSourceOutcome = Client->PutObject(putPartSourceRequest); + AWS_ASSERT_SUCCESS(putPartSourceOutcome); + + CreateMultipartUploadRequest createMultipartUploadRequest; + createMultipartUploadRequest.WithBucket(bucketName).WithKey(sourceKey); + auto createMultipartUploadOutcome = Client->CreateMultipartUpload(createMultipartUploadRequest); + AWS_ASSERT_SUCCESS(createMultipartUploadOutcome); + const Aws::String uploadId = createMultipartUploadOutcome.GetResult().GetUploadId(); + + CompletedMultipartUpload completedMultipartUpload; + for (int partNumber = 1; partNumber <= 128; ++partNumber) + { + UploadPartCopyRequest uploadPartCopyRequest; + uploadPartCopyRequest.WithBucket(bucketName) + .WithKey(sourceKey) + .WithUploadId(uploadId) + .WithPartNumber(partNumber) + .WithCopySource(bucketName + "/" + partSourceKey); + auto uploadPartCopyOutcome = Client->UploadPartCopy(uploadPartCopyRequest); + AWS_ASSERT_SUCCESS(uploadPartCopyOutcome); + completedMultipartUpload.AddParts(CompletedPart() + .WithPartNumber(partNumber) + .WithETag(uploadPartCopyOutcome.GetResult().GetCopyPartResult().GetETag())); + } + + CompleteMultipartUploadRequest completeMultipartUploadRequest; + completeMultipartUploadRequest.WithBucket(bucketName) + .WithKey(sourceKey) + .WithUploadId(uploadId) + .WithMultipartUpload(completedMultipartUpload); + auto completeMultipartUploadOutcome = Client->CompleteMultipartUpload(completeMultipartUploadRequest); + AWS_ASSERT_SUCCESS(completeMultipartUploadOutcome); + })); + } + TEST_F(BucketAndObjectOperationTest, TestObjectOperationWithEventStream) { GTEST_SKIP() << "Select objects is not supported on new AWS accounts"; diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm index dc34976e850..12bc1768a21 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm @@ -374,11 +374,15 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti httpRequest->SetHeaderValue("x-amz-copy-source-if-match", sourceETag); } + // Below the multipart threshold S3's single-part CopyObject honors the metadata, tagging and + // annotation directives natively, so skip copying them here. + if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) + { + return annotations; + } + if (copyAnnotations) { - // Neither the multipart copy nor S3's single-part CopyObject carries annotations, so always - // read them from the source here (regardless of size) and return them to be written to the - // destination after the copy completes (see the wrapped completion handler). Aws::String continuationToken; do { @@ -424,13 +428,6 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti } while (!continuationToken.empty()); } - // Below the multipart threshold S3's single-part CopyObject honors the metadata/tagging directives - // natively, so skip header injection (annotations are handled above regardless of size). - if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) - { - return annotations; - } - if (copyMetadata) { if (!head.GetContentType().empty()) { httpRequest->SetHeaderValue("content-type", head.GetContentType()); } diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm index 84fb3e62aa9..b0e11469835 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm @@ -374,11 +374,15 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti httpRequest->SetHeaderValue("x-amz-copy-source-if-match", sourceETag); } + // Below the multipart threshold S3's single-part CopyObject honors the metadata, tagging and + // annotation directives natively, so skip copying them here. + if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) + { + return annotations; + } + if (copyAnnotations) { - // Neither the multipart copy nor S3's single-part CopyObject carries annotations, so always - // read them from the source here (regardless of size) and return them to be written to the - // destination after the copy completes (see the wrapped completion handler). Aws::String continuationToken; do { @@ -424,13 +428,6 @@ S3CrtClient::CopyObjectPropertiesOutcome S3CrtClient::PopulateCopyObjectProperti } while (!continuationToken.empty()); } - // Below the multipart threshold S3's single-part CopyObject honors the metadata/tagging directives - // natively, so skip header injection (annotations are handled above regardless of size). - if (head.GetContentLength() < (1LL * 1024 * 1024 * 1024)) - { - return annotations; - } - if (copyMetadata) { if (!head.GetContentType().empty()) { httpRequest->SetHeaderValue("content-type", head.GetContentType()); }