Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
import software.amazon.awssdk.services.s3.model.ListPartsRequest;
import software.amazon.awssdk.services.s3.model.MetadataDirective;
import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
import software.amazon.awssdk.services.s3.model.ObjectIdentifier;
import software.amazon.awssdk.services.s3.model.PutBucketAclRequest;
Expand All @@ -151,6 +152,7 @@
import software.amazon.awssdk.services.s3.model.S3Object;
import software.amazon.awssdk.services.s3.model.Tag;
import software.amazon.awssdk.services.s3.model.Tagging;
import software.amazon.awssdk.services.s3.model.TaggingDirective;
import software.amazon.awssdk.services.s3.model.UploadPartCopyRequest;
import software.amazon.awssdk.services.s3.model.UploadPartRequest;
import software.amazon.awssdk.services.s3.model.UploadPartResponse;
Expand Down Expand Up @@ -1204,6 +1206,69 @@ public void testCopyObject() {
assertEquals("\"37b51d194a7513e45b56f6524f2d51f2\"", copyObjectResponse.copyObjectResult().eTag());
}

@Test
public void testCopyObjectToSelfWithMetadataReplace() {
final String bucketName = getBucketName();
final String key = getKeyName();
final String content = "bar";
s3Client.createBucket(b -> b.bucket(bucketName));
s3Client.putObject(b -> b.bucket(bucketName).key(key).metadata(Collections.singletonMap("meta1", "v1")),
RequestBody.fromString(content));

// Copying an object onto itself is allowed when the metadata is replaced.
CopyObjectRequest copyReq = CopyObjectRequest.builder()
.sourceBucket(bucketName)
.sourceKey(key)
.destinationBucket(bucketName)
.destinationKey(key)
.metadataDirective(MetadataDirective.REPLACE)
.metadata(Collections.singletonMap("meta2", "v2"))
.build();

CopyObjectResponse copyObjectResponse = assertDoesNotThrow(() -> s3Client.copyObject(copyReq));
assertNotNull(copyObjectResponse.copyObjectResult().eTag());

// The metadata was replaced in place: the new entry is present and the old one is gone.
HeadObjectResponse head = s3Client.headObject(b -> b.bucket(bucketName).key(key));
assertThat(head.metadata())
.containsEntry("meta2", "v2")
.doesNotContainKey("meta1");

// The object is still readable with its original content after the in-place copy.
ResponseBytes<GetObjectResponse> objectBytes = s3Client.getObjectAsBytes(
b -> b.bucket(bucketName).key(key));
assertEquals(content, objectBytes.asUtf8String());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also verify the resulting metadata through HeadObject here?

This test currently confirms that the self-copy succeeds and that the object content is preserved, but it would still pass if Ozone accepted the request without replacing the metadata. Since metadata replacement is the behavior this PR fixes, it would be useful to assert both that meta2=v2 is present and that meta1 is absent.

For example:

    HeadObjectResponse head = s3Client.headObject(
        b -> b.bucket(bucketName).key(key));
    assertEquals("v2", head.metadata().get("meta2"));
    assertFalse(head.metadata().containsKey("meta1"));

The unit test verifies the internal OzoneKeyDetails; this assertion would additionally cover the complete AWS SDK → S3 Gateway → OM → HeadObject path. Thanks!

}

@Test
public void testCopyObjectToSelfWithTaggingReplace() {
final String bucketName = getBucketName();
final String key = getKeyName();
final String content = "bar";
s3Client.createBucket(b -> b.bucket(bucketName));
s3Client.putObject(b -> b.bucket(bucketName).key(key).tagging("tag1=value1"),
RequestBody.fromString(content));

// Copying an object onto itself is allowed when the tag set is replaced.
CopyObjectRequest copyReq = CopyObjectRequest.builder()
.sourceBucket(bucketName)
.sourceKey(key)
.destinationBucket(bucketName)
.destinationKey(key)
.taggingDirective(TaggingDirective.REPLACE)
.tagging("tag2=value2")
.build();

CopyObjectResponse copyObjectResponse = assertDoesNotThrow(() -> s3Client.copyObject(copyReq));
assertNotNull(copyObjectResponse.copyObjectResult().eTag());

// The tag set was replaced in place.
GetObjectTaggingResponse tagging = s3Client.getObjectTagging(b -> b.bucket(bucketName).key(key));
assertEquals(1, tagging.tagSet().size());
assertEquals("tag2", tagging.tagSet().get(0).key());
assertEquals("value2", tagging.tagSet().get(0).value());
}

@Test
public void testCopyObjectWithSourceIfMatch() {
final String sourceBucketName = getBucketName("source");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1069,13 +1069,19 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
try {
OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails(
volume.getName(), sourceBucket, sourceKey);
// The tagging and metadata directives are read up front because a
// self-copy is only legal when at least one attribute is being changed.
String tagCopyDirective = getHeaders().getHeaderString(TAG_DIRECTIVE_HEADER);
String metadataCopyDirective = getHeaders().getHeaderString(CUSTOM_METADATA_COPY_DIRECTIVE_HEADER);
boolean replacingTags = CopyDirective.REPLACE.name().equals(tagCopyDirective);
boolean replacingMetadata = CopyDirective.REPLACE.name().equals(metadataCopyDirective);

// Checking whether we trying to copying to it self.
if (sourceBucket.equals(destBucket) && sourceKey
.equals(destkey)) {
// When copying to same storage type when storage type is provided,
// we should not throw exception, as aws cli checks if any of the
// options like storage type are provided or not when source and
// dest are given same
if (sourceBucket.equals(destBucket) && sourceKey.equals(destkey)
&& !replacingTags && !replacingMetadata) {
// Self-copy without a metadata or tag replacement. AWS still allows it
// when a storage class is provided (aws cli passes storage type), so
// only the default-storage-type case is rejected.
if (storageTypeDefault) {
OS3Exception ex = newError(S3ErrorTable.INVALID_REQUEST, copyHeader);
ex.setErrorMessage("This copy request is illegal because it is " +
Expand Down Expand Up @@ -1106,7 +1112,6 @@ private CopyObjectResponse copyObject(OzoneVolume volume,

// Object tagging in copyObject with tagging directive
Map<String, String> tags;
String tagCopyDirective = getHeaders().getHeaderString(TAG_DIRECTIVE_HEADER);
if (StringUtils.isEmpty(tagCopyDirective) || tagCopyDirective.equals(CopyDirective.COPY.name())) {
// Tag-set will be copied from the source directly
tags = sourceKeyDetails.getTags();
Expand All @@ -1123,7 +1128,6 @@ private CopyObjectResponse copyObject(OzoneVolume volume,

// Custom metadata in copyObject with metadata directive
Map<String, String> customMetadata;
String metadataCopyDirective = getHeaders().getHeaderString(CUSTOM_METADATA_COPY_DIRECTIVE_HEADER);
if (StringUtils.isEmpty(metadataCopyDirective) || metadataCopyDirective.equals(CopyDirective.COPY.name())) {
// The custom metadata will be copied from the source key
customMetadata = sourceKeyDetails.getMetadata();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,57 @@ void testCopyObject() throws Exception {
() -> put(objectEndpoint, "nonexistent", KEY_NAME, CONTENT));
}

@Test
void testCopyObjectToSelfWithMetadataReplace() throws Exception {
// Put the source object with some custom metadata.
Map<String, String> sourceMetadata = ImmutableMap.of(
"custom-key-1", "custom-value-1",
"custom-key-2", "custom-value-2");
MultivaluedMap<String, String> metadataHeaders = new MultivaluedHashMap<>();
sourceMetadata.forEach((k, v) -> metadataHeaders.putSingle(CUSTOM_METADATA_HEADER_PREFIX + k, v));
when(headers.getRequestHeaders()).thenReturn(metadataHeaders);
when(headers.getHeaderString(CUSTOM_METADATA_COPY_DIRECTIVE_HEADER)).thenReturn("COPY");
assertSucceeds(() -> putObject(CONTENT));
assertThat(bucket.getKey(KEY_NAME).getMetadata()).containsAllEntriesOf(sourceMetadata);

// Copy the object onto itself with x-amz-metadata-directive: REPLACE and a
// new metadata set. AWS allows this as an in-place metadata update.
when(headers.getHeaderString(COPY_SOURCE_HEADER)).thenReturn(
BUCKET_NAME + "/" + urlEncode(KEY_NAME));
when(headers.getHeaderString(CUSTOM_METADATA_COPY_DIRECTIVE_HEADER)).thenReturn("REPLACE");
metadataHeaders.clear();
metadataHeaders.putSingle(CUSTOM_METADATA_HEADER_PREFIX + "custom-key-3", "custom-value-3");

assertSucceeds(() -> putObject(CONTENT));

OzoneKeyDetails keyDetails = assertKeyContent(bucket, KEY_NAME, CONTENT);
assertThat(keyDetails.getMetadata())
.containsEntry("custom-key-3", "custom-value-3")
.doesNotContainKeys("custom-key-1", "custom-key-2");
}

@Test
void testCopyObjectToSelfWithTaggingReplace() throws Exception {
// Put the source object with some tags.
when(headers.getHeaderString(TAG_HEADER)).thenReturn("tag1=value1&tag2=value2");
assertSucceeds(() -> putObject(CONTENT));
assertThat(bucket.getKey(KEY_NAME).getTags()).hasSize(2);

// Copy the object onto itself with x-amz-tagging-directive: REPLACE and a
// new tag set. AWS allows this as an in-place tag update.
when(headers.getHeaderString(COPY_SOURCE_HEADER)).thenReturn(
BUCKET_NAME + "/" + urlEncode(KEY_NAME));
when(headers.getHeaderString(TAG_DIRECTIVE_HEADER)).thenReturn("REPLACE");
when(headers.getHeaderString(TAG_HEADER)).thenReturn("tag3=value3");

assertSucceeds(() -> putObject(CONTENT));

Map<String, String> tags = assertKeyContent(bucket, KEY_NAME, CONTENT).getTags();
assertThat(tags)
.containsEntry("tag3", "value3")
.doesNotContainKeys("tag1", "tag2");
}

@Test
void testContentTypeStoredAndCopied() throws Exception {
// PUT with an explicit Content-Type (preserved by HeaderPreprocessor).
Expand Down
Loading