From c280956c9c2e2789041e81e51cb1f9c4529340cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=2EBurak=20Ya=C5=9Far?= <91782773+4o4x@users.noreply.github.com> Date: Fri, 26 Jul 2024 16:36:16 +0300 Subject: [PATCH] fix: cp command S3toS3 --content-type flag (#739) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In s5cmd, the --content-type flag does not work when performing an S3 to S3 copy operation with the cp command. Added tests addressing this issue. resolves #738 --------- Co-authored-by: Deniz Surmeli <52484739+denizsurmeli@users.noreply.github.com> Co-authored-by: İbrahim Güngör --- CHANGELOG.md | 3 + command/cp.go | 13 ++++- e2e/cp_test.go | 145 ++++++++++++++++++++++++++++++++++++++++++++++++- storage/s3.go | 4 ++ 4 files changed, 162 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80a4e04e2..59404e59f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - Added prefix and wildcard support to `cat` command. ([#716](https://github.com/peak/s5cmd/issues/716)) - Added `head` command. ([#730](https://github.com/peak/s5cmd/pull/730)) +#### Bugfixes +- Fixed the `cp` command to work with the `--content-type` flag when performing a copy operation from S3 to S3. ([#738](https://github.com/peak/s5cmd/issues/738)) + ## v2.2.2 - 13 Sep 2023 #### Bugfixes diff --git a/command/cp.go b/command/cp.go index e50985e20..7ed06bfee 100644 --- 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,7 +534,11 @@ 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" + 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) case srcurl.IsRemote(): // remote->local diff --git a/e2e/cp_test.go b/e2e/cp_test.go index 2f0f69821..a13b2ceda 100644 --- a/e2e/cp_test.go +++ b/e2e/cp_test.go @@ -866,6 +866,11 @@ func TestCopyS3ToS3WithArbitraryMetadataWithDefaultDirective(t *testing.T) { "Key2": aws.String("value2"), } + dstmetadata := 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(dstmetadata))) } // cp s3://bucket2/obj2 s3://bucket1/obj1 --metadata-directive REPLACE --metadata key1=val1 --metadata key2=val2 ... @@ -4596,3 +4601,141 @@ func TestCopyS3ObjectsWithIncludeExcludeFilter2(t *testing.T) { expected := fs.Expected(t, expectedFileSystem...) assert.Assert(t, fs.Equal(cmd.Dir, expected)) } + +// cp --content-type "video/mp4" file s3://bucket/ +func TestCopySingleLocalFileToS3WithContentType(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + const ( + filename = "testfile.txt" + content = "this is a test file" + ) + + workdir := fs.NewDir(t, t.Name(), fs.WithFile(filename, content)) + defer workdir.Remove() + + srcpath := workdir.Join(filename) + dstpath := fmt.Sprintf("s3://%v/", bucket) + + cmd := s5cmd("cp", "--content-type", "video/mp4", srcpath, dstpath) + + result := icmd.RunCmd(cmd) + result.Assert(t, icmd.Success) + + expected := fs.Expected(t, fs.WithFile(filename, content)) + assert.Assert(t, fs.Equal(workdir.Path(), expected)) + + assert.Assert(t, ensureS3Object(s3client, bucket, filename, content, ensureContentType("video/mp4"))) +} + +// cp --content-type "video/avi" dir s3://bucket/ +func TestCopyMultipleFilesToS3BucketWithContentType(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Files in Windows cannot contain glob(*) characters") + } + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + folderLayout := []fs.PathOp{ + fs.WithFile("file1.html", "

file1

"), + fs.WithDir( + "a", + fs.WithFile("another_test_file.txt", "yet another txt file. yatf."), + ), + } + + workdir := fs.NewDir(t, "somedir", folderLayout...) + defer workdir.Remove() + + src := fmt.Sprintf("%v/*", workdir.Path()) + dst := fmt.Sprintf("s3://%v/", bucket) + + cmd := s5cmd("cp", "--content-type", "video/avi", src, dst) + + result := icmd.RunCmd(cmd) + result.Assert(t, icmd.Success) + + expected := fs.Expected(t, folderLayout...) + assert.Assert(t, fs.Equal(workdir.Path(), expected)) + + 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.Assert(t, ensureS3Object(s3client, bucket, "file1.html", "

file1

", ensureContentType("video/avi"))) + assert.Assert(t, ensureS3Object(s3client, bucket, "a/another_test_file.txt", "yet another txt file. yatf.", ensureContentType("video/avi"))) +} + +// cp --content-type "video/avi" s3://srcbucket/object s3://dstbucket/ +func TestCopySingleS3ObjectToAnotherBucketWithContentType(t *testing.T) { + t.Parallel() + + srcbucket := s3BucketFromTestNameWithPrefix(t, "src") + dstbucket := s3BucketFromTestNameWithPrefix(t, "dst") + + s3client, s5cmd := setup(t) + + createBucket(t, s3client, srcbucket) + createBucket(t, s3client, dstbucket) + + const ( + filename = "testfile.txt" + content = "this is a test file" + ) + + putFile(t, s3client, srcbucket, filename, content) + + src := fmt.Sprintf("s3://%v/%v", srcbucket, filename) + dst := fmt.Sprintf("s3://%v/", dstbucket) + + cmd := s5cmd("cp", "--content-type", "video/avi", src, dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assert.Assert(t, ensureS3Object(s3client, dstbucket, filename, content, ensureContentType("video/avi"))) +} + +// cp --content-type "video/avi" s3://srcbucket/* s3://dstbucket/ +func TestCopyMultipleS3ObjectsToAnotherBucketWithContentType(t *testing.T) { + t.Parallel() + + srcbucket := s3BucketFromTestNameWithPrefix(t, "src") + dstbucket := s3BucketFromTestNameWithPrefix(t, "dst") + + s3client, s5cmd := setup(t) + + createBucket(t, s3client, srcbucket) + createBucket(t, s3client, dstbucket) + + fileAndContent := map[string]string{ + "file1.txt": "this is a test file 1", + "a/another_test_file.txt": "yet another txt file. yatf.", + } + + for filename, content := range fileAndContent { + putFile(t, s3client, srcbucket, filename, content) + } + + src := fmt.Sprintf("s3://%v/*", srcbucket) + dst := fmt.Sprintf("s3://%v/", dstbucket) + + cmd := s5cmd("cp", "--content-type", "video/avi", src, dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + 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 index c49de30f5..7c85468ca 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -552,6 +552,10 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata Metadata) err input.MetadataDirective = aws.String(metadata.Directive) } + if metadata.ContentType != "" { + input.ContentType = aws.String(metadata.ContentType) + } + if len(metadata.UserDefined) != 0 { m := make(map[string]*string) for k, v := range metadata.UserDefined {