fix: Fix meta-directive default value in copy command when s3tos3

peak · Jul 23, 2024 · 4785b7e · 4785b7e
1 parent 6bbf9f9
commit 4785b7e
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 28 deletions.
diff --git a/command/cp.go b/command/cp.go
@@ -32,6 +32,11 @@ const (
 	kilobytes              = 1024
 )
 
+const (
+	metadataDirectiveCopy    = "COPY"
+	metadataDirectiveReplace = "REPLACE"
+)
+
 var copyHelpTemplate = `Name:
 	{{.HelpName}} - {{.Usage}}
 
@@ -145,7 +150,7 @@ func NewSharedFlags() []cli.Flag {
 			Name:  "metadata-directive",
 			Usage: "set metadata directive for the object: COPY or REPLACE",
 			Value: &EnumValue{
-				Enum:    []string{"COPY", "REPLACE", ""},
+				Enum:    []string{metadataDirectiveCopy, metadataDirectiveReplace, ""},
 				Default: "",
 				ConditionFunction: func(str, target string) bool {
 					return strings.EqualFold(target, str)
@@ -529,10 +534,10 @@ func (c Copy) Run(ctx context.Context) error {
 		case srcurl.Type == c.dst.Type: // local->local or remote->remote
 			if c.metadataDirective == "" {
 				// default to COPY
-				c.metadataDirective = "COPY"
-				if c.src.IsRemote() && c.dst.IsRemote() && c.contentType != "" {
-					// default to REPLACE for content type change
-					c.metadataDirective = "REPLACE"
+				c.metadataDirective = metadataDirectiveCopy
+				if c.src.IsRemote() && c.dst.IsRemote() {
+					// default to REPLACE when copying between remote storages
+					c.metadataDirective = metadataDirectiveReplace
 				}
 			}
 			task = c.prepareCopyTask(ctx, srcurl, c.dst, isBatch, c.metadata)

diff --git a/e2e/cp_test.go b/e2e/cp_test.go
@@ -866,6 +866,11 @@ func TestCopyS3ToS3WithArbitraryMetadataWithDefaultDirective(t *testing.T) {
 		"Key2": aws.String("value2"),
 	}
 
+	destmetadata := map[string]*string{
+		"Key1": aws.String("foo"),
+		"Key2": aws.String("bar"),
+	}
+
 	srcpath := fmt.Sprintf("s3://%v/%v", bucket, filename)
 	dstpath := fmt.Sprintf("s3://%v/%v_cp", bucket, filename)
 
@@ -876,7 +881,7 @@ func TestCopyS3ToS3WithArbitraryMetadataWithDefaultDirective(t *testing.T) {
 	result.Assert(t, icmd.Success)
 
 	// assert S3
-	assert.Assert(t, ensureS3Object(s3client, bucket, fmt.Sprintf("%s_cp", filename), content, ensureArbitraryMetadata(srcmetadata)))
+	assert.Assert(t, ensureS3Object(s3client, bucket, fmt.Sprintf("%s_cp", filename), content, ensureArbitraryMetadata(destmetadata)))
 }
 
 // cp s3://bucket2/obj2 s3://bucket1/obj1 --metadata-directive REPLACE --metadata key1=val1 --metadata key2=val2 ...
@@ -4622,11 +4627,9 @@ func TestCopySingleLocalFileToS3WithContentType(t *testing.T) {
 	result := icmd.RunCmd(cmd)
 	result.Assert(t, icmd.Success)
 
-	// assert local filesystem
 	expected := fs.Expected(t, fs.WithFile(filename, content))
 	assert.Assert(t, fs.Equal(workdir.Path(), expected))
 
-	// assert s3
 	assert.Assert(t, ensureS3Object(s3client, bucket, filename, content, ensureContentType("video/mp4")))
 }
 
@@ -4642,8 +4645,6 @@ func TestCopyMultipleFilesToS3BucketWithContentType(t *testing.T) {
 	bucket := s3BucketFromTestName(t)
 	createBucket(t, s3client, bucket)
 
-	// create files with different content types
-
 	folderLayout := []fs.PathOp{
 		fs.WithFile("file1.html", "<html><body><h1>file1</h1></body></html>"),
 		fs.WithDir(
@@ -4663,17 +4664,14 @@ func TestCopyMultipleFilesToS3BucketWithContentType(t *testing.T) {
 	result := icmd.RunCmd(cmd)
 	result.Assert(t, icmd.Success)
 
-	// assert local filesystem
 	expected := fs.Expected(t, folderLayout...)
 	assert.Assert(t, fs.Equal(workdir.Path(), expected))
 
-	// assert lines
 	assertLines(t, result.Stdout(), map[int]compareFunc{
 		0: equals("cp %v/a/another_test_file.txt %sa/another_test_file.txt", workdir.Path(), dst),
 		1: equals("cp %v/file1.html %sfile1.html", workdir.Path(), dst),
 	}, sortInput(true))
 
-	// assert s3
 	assert.Assert(t, ensureS3Object(s3client, bucket, "file1.html", "<html><body><h1>file1</h1></body></html>", ensureContentType("video/avi")))
 	assert.Assert(t, ensureS3Object(s3client, bucket, "a/another_test_file.txt", "yet another txt file. yatf.", ensureContentType("video/avi")))
 }
@@ -4705,8 +4703,6 @@ func TestCopySingleS3ObjectToAnotherBucketWithContentType(t *testing.T) {
 
 	result.Assert(t, icmd.Success)
 
-	// assert s3
-	assert.Assert(t, ensureS3Object(s3client, srcbucket, filename, content))
 	assert.Assert(t, ensureS3Object(s3client, dstbucket, filename, content, ensureContentType("video/avi")))
 }
 
@@ -4739,16 +4735,6 @@ func TestCopyMultipleS3ObjectsToAnotherBucketWithContentType(t *testing.T) {
 
 	result.Assert(t, icmd.Success)
 
-	// assert s3
-	// these seperation is intentional to make it easier to debug.
-	for filename, content := range fileAndContent {
-		assert.Assert(t, ensureS3Object(s3client, srcbucket, filename, content))
-	}
-
-	for filename, content := range fileAndContent {
-		assert.Assert(t, ensureS3Object(s3client, dstbucket, filename, content))
-	}
-
 	for filename, content := range fileAndContent {
 		assert.Assert(t, ensureS3Object(s3client, dstbucket, filename, content, ensureContentType("video/avi")))
 	}

diff --git a/storage/s3.go b/storage/s3.go
@@ -557,9 +557,6 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata Metadata) err
 
 	if metadata.ContentType != "" {
 		input.ContentType = aws.String(metadata.ContentType)
-		if metadata.Directive == "" {
-			input.MetadataDirective = aws.String("REPLACE")
-		}
 	}
 
 	if len(metadata.UserDefined) != 0 {