From a2e86d1a45a6cea5a30a7d7405f5f42453cb15af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tar=C4=B1k=20Bu=C4=9Fra=20=C3=96zyurt?= <74174778+tarikozyurtt@users.noreply.github.com> Date: Mon, 8 Jul 2024 16:14:48 +0300 Subject: [PATCH 1/8] feat: add wildcard and prefix support to cat --- CHANGELOG.md | 8 ++ command/cat.go | 62 ++++++++++---- e2e/cat_test.go | 205 +++++++++++++++++++++++++++++++++++++++------ e2e/du_test.go | 18 ++++ e2e/ls_test.go | 16 ++++ e2e/select_test.go | 12 +++ storage/s3.go | 6 +- 7 files changed, 283 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56043dab0..ec044142b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,12 @@ # Changelog +## v2.3.0 + +#### Breaking changes +- Changed the exit code from 1 to 0 for `ls` when used with an empty bucket. Exits with 1 if the bucket is non-existent. ([#722](https://github.com/peak/s5cmd/issues/722)) + +#### Features +- Added prefix and wildcard support to `cat` command. ([#716](https://github.com/peak/s5cmd/issues/716)) + ## v2.2.2 - 13 Sep 2023 #### Bugfixes diff --git a/command/cat.go b/command/cat.go index d16a11600..2751676eb 100644 --- a/command/cat.go +++ b/command/cat.go @@ -28,6 +28,9 @@ Examples: 2. Print specific version of a remote object's content to stdout > s5cmd {{.HelpName}} --version-id VERSION_ID s3://bucket/prefix/object + + 3. Concatenate multiple objects matching a prefix or wildcard and print to stdout + > s5cmd {{.HelpName}} s3://bucket/prefix/* ` func NewCatCommand() *cli.Command { @@ -111,16 +114,43 @@ func (c Cat) Run(ctx context.Context) error { printError(c.fullCommand, c.op, err) return err } - _, err = client.Stat(ctx, c.src) - if err != nil { - printError(c.fullCommand, c.op, err) - return err + + // Initialize an empty channel to handle single or multiple objects + var objectChan <-chan *storage.Object + + if c.src.IsWildcard() || c.src.IsPrefix() || c.src.IsBucket() { + objectChan = client.List(ctx, c.src, false) + } else { + _, err = client.Stat(ctx, c.src) + if err != nil { + printError(c.fullCommand, c.op, err) + return err + } + singleObjChan := make(chan *storage.Object, 1) + singleObjChan <- &storage.Object{URL: c.src} + close(singleObjChan) + objectChan = singleObjChan } - buf := orderedwriter.New(os.Stdout) - _, err = client.Get(ctx, c.src, buf, c.concurrency, c.partSize) - if err != nil { - printError(c.fullCommand, c.op, err) - return err + + return c.processObjects(ctx, client, objectChan) +} + +func (c Cat) processObjects(ctx context.Context, client *storage.S3, objectChan <-chan *storage.Object) error { + for obj := range objectChan { + if obj.Err != nil { + printError(c.fullCommand, c.op, obj.Err) + return obj.Err + } + if obj.Type.IsDir() { + continue + } + buf := orderedwriter.New(os.Stdout) + + _, err := client.Get(ctx, obj.URL, buf, c.concurrency, c.partSize) + if err != nil { + printError(c.fullCommand, c.op, err) + return err + } } return nil } @@ -140,17 +170,15 @@ func validateCatCommand(c *cli.Context) error { return fmt.Errorf("source must be a remote object") } - if src.IsBucket() || src.IsPrefix() { - return fmt.Errorf("remote source must be an object") - } - - if src.IsWildcard() { - return fmt.Errorf("remote source %q can not contain glob characters", src) - } - if err := checkVersioningWithGoogleEndpoint(c); err != nil { return err } + if src.IsWildcard() || src.IsPrefix() || src.IsBucket() { + if c.String("version-id") != "" { + return fmt.Errorf("wildcard/prefix operations are disabled with --version-id flag") + } + } + return nil } diff --git a/e2e/cat_test.go b/e2e/cat_test.go index df29d57e7..919c7161f 100644 --- a/e2e/cat_test.go +++ b/e2e/cat_test.go @@ -133,30 +133,6 @@ func TestCatS3ObjectFail(t *testing.T) { jsonCheck(true), }, }, - { - src: "s3://%v/prefix/file.txt/*", - name: "cat remote object with glob", - cmd: []string{ - "--json", - "cat", - }, - expected: map[int]compareFunc{ - 0: match(`{"operation":"cat","command":"cat s3:\/\/(.+)?\/prefix\/file\.txt\/\*","error":"remote source \\"s3:\/\/(.*)\/prefix\/file\.txt\/\*\\" can not contain glob characters"}`), - }, - assertOps: []assertOp{ - jsonCheck(true), - }, - }, - { - src: "s3://%v/prefix/", - name: "cat bucket", - cmd: []string{ - "cat", - }, - expected: map[int]compareFunc{ - 0: match(`ERROR "cat s3://(.+)?": remote source must be an object`), - }, - }, } for _, tc := range testcases { @@ -229,6 +205,41 @@ func TestCatLocalFileFail(t *testing.T) { } } +func TestCatInEmptyBucket(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + t.Run("EmptyBucket", func(t *testing.T) { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v", bucket)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 0}) + assertLines(t, result.Stdout(), nil) + }) + + t.Run("PrefixInEmptyBucket", func(t *testing.T) { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/", bucket)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 0}) + assertLines(t, result.Stdout(), nil) + }) + + t.Run("WildcardInEmptyBucket", func(t *testing.T) { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/*", bucket)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: contains(fmt.Sprintf(`ERROR "cat s3://%v/*": no object found`, bucket)), + }) + }) +} + // getSequentialFileContent creates a string with size bytes in size. func getSequentialFileContent(size int64) (string, map[int]compareFunc) { sb := strings.Builder{} @@ -305,4 +316,150 @@ func TestCatByVersionID(t *testing.T) { t.Errorf("(-want +got):\n%v", diff) } } + + version := "1" + + // wildcard and prefix fail cases + cmd = s5cmd("cat", "--version-id", version, "s3://"+bucket+"/") + result = icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`ERROR "cat --version-id=%v s3://%v/": wildcard/prefix operations are disabled with --version-id flag`, version, bucket), + }, strictLineCheck(false)) + + cmd = s5cmd("cat", "--version-id", version, "s3://"+bucket+"/folder/") + result = icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`ERROR "cat --version-id=%v s3://%v/folder/": wildcard/prefix operations are disabled with --version-id flag`, version, bucket), + }, strictLineCheck(false)) + + cmd = s5cmd("cat", "--version-id", version, "s3://"+bucket+"/*") + result = icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`ERROR "cat --version-id=%v s3://%v/*": wildcard/prefix operations are disabled with --version-id flag`, version, bucket), + }, strictLineCheck(false)) +} + +func TestCatPrefix(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + testCases := []struct { + files []string + prefix string + expected string + }{ + {files: []string{"file1.txt", "file2.txt"}, prefix: "", expected: "content0content1"}, + {files: []string{"dir/file3.txt", "dir/file4.txt"}, prefix: "", expected: "content0content1"}, + {files: nil, prefix: "dir/", expected: "content2content3"}, + {files: []string{"dir/nesteddir/file5.txt"}, prefix: "dir/", expected: "content2content3"}, + {files: nil, prefix: "dir/nesteddir/", expected: "content4"}, + } + + offset := 0 + for _, tc := range testCases { + if tc.files != nil { + var concatenatedContent strings.Builder + for idx, file := range tc.files { + content := fmt.Sprintf("content%d", idx+offset) + putFile(t, s3client, bucket, file, content) + concatenatedContent.WriteString(content) + } + offset += len(tc.files) + } + verifyCatCommand(t, s5cmd, bucket, tc.expected, tc.prefix) + } +} + +func TestCatWildcard(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + files := []struct { + key string + content string + }{ + {"foo1.txt", "content0"}, + {"foo2.txt", "content1"}, + {"bar1.txt", "content2"}, + {"foolder/foo3.txt", "content3"}, + {"log-file-2024-01.txt", "content4"}, + {"log-file-2024-02.txt", "content5"}, + {"log-file-2023-01.txt", "content6"}, + {"log-file-2022-01.txt", "content7"}, + } + + for _, file := range files { + putFile(t, s3client, bucket, file.key, file.content) + } + + testCases := []struct { + prefix string + expected string + }{ + {"foo*", "content0content1content3"}, + {"log-file-2024-*", "content4content5"}, + {"log-file-*", "content7content6content4content5"}, + } + + for _, tc := range testCases { + verifyCatCommand(t, s5cmd, bucket, tc.expected, tc.prefix) + } +} + +func TestPrefixWildcardFail(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + testCases := []struct { + prefix string + }{ + {"foo*"}, + {"foolder/"}, + } + + for _, tc := range testCases { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`ERROR "cat s3://%v/%v": no object found`, bucket, tc.prefix), + }, strictLineCheck(false)) + } + + for _, tc := range testCases { + cmd := s5cmd("--json", "cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`{"operation":"cat","command":"cat s3://%v/%v","error":"no object found"}`, bucket, tc.prefix), + }, strictLineCheck(false)) + } + +} + +func verifyCatCommand(t *testing.T, s5cmd func(...string) icmd.Cmd, bucket, expectedContent, prefix string) { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(expectedContent), + }, alignment(true)) } diff --git a/e2e/du_test.go b/e2e/du_test.go index 0dfb04ad7..47175d55f 100644 --- a/e2e/du_test.go +++ b/e2e/du_test.go @@ -300,3 +300,21 @@ func TestDiskUsageByVersionIDAndAllVersions(t *testing.T) { }) } } + +func TestDiskUsageEmptyBucket(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + cmd := s5cmd("du", "s3://"+bucket) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: suffix(`0 bytes in 0 objects: s3://%v`, bucket), + }) +} diff --git a/e2e/ls_test.go b/e2e/ls_test.go index 203b1cac9..9f3f09dfe 100644 --- a/e2e/ls_test.go +++ b/e2e/ls_test.go @@ -786,3 +786,19 @@ func TestListNestedLocalFolders(t *testing.T) { 2: match(filepath.ToSlash("file.txt")), }, trimMatch(dateRe), alignment(true)) } + +func TestEmptyBucket(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + cmd := s5cmd("ls", "s3://"+bucket) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), nil) +} diff --git a/e2e/select_test.go b/e2e/select_test.go index df68a6adb..dc74379af 100644 --- a/e2e/select_test.go +++ b/e2e/select_test.go @@ -218,6 +218,18 @@ func TestSelectCommand(t *testing.T) { outformat: "json", expectedValue: "id0\n", }, + { + name: "input:json-lines,output:json-lines,all-versions:true", + cmd: []string{ + "select", "json", + "--all-versions", + "--query", query, + }, + informat: "json", + structure: "lines", + outformat: "json", + expectedValue: "", + }, }, "csv": { { diff --git a/storage/s3.go b/storage/s3.go index 96c54f92d..34213d939 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -288,7 +288,7 @@ func (s *S3) listObjectVersions(ctx context.Context, url *url.URL) <-chan *Objec return } - if !objectFound { + if !objectFound && !url.IsBucket() { objCh <- &Object{Err: ErrNoObjectFound} } }() @@ -378,7 +378,7 @@ func (s *S3) listObjectsV2(ctx context.Context, url *url.URL) <-chan *Object { return } - if !objectFound { + if !objectFound && !url.IsBucket() { objCh <- &Object{Err: ErrNoObjectFound} } }() @@ -470,7 +470,7 @@ func (s *S3) listObjects(ctx context.Context, url *url.URL) <-chan *Object { return } - if !objectFound { + if !objectFound && !url.IsBucket() { objCh <- &Object{Err: ErrNoObjectFound} } }() From a9e2ccb3faa3bc0a0058c365789a960980ccbe10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tar=C4=B1k=20Bu=C4=9Fra=20=C3=96zyurt?= <74174778+tarikozyurtt@users.noreply.github.com> Date: Wed, 10 Jul 2024 19:53:51 +0300 Subject: [PATCH 2/8] fix help format for wildcard example --- command/cat.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/cat.go b/command/cat.go index 2751676eb..4ececabbc 100644 --- a/command/cat.go +++ b/command/cat.go @@ -30,7 +30,7 @@ Examples: > s5cmd {{.HelpName}} --version-id VERSION_ID s3://bucket/prefix/object 3. Concatenate multiple objects matching a prefix or wildcard and print to stdout - > s5cmd {{.HelpName}} s3://bucket/prefix/* + > s5cmd {{.HelpName}} "s3://bucket/prefix/*" ` func NewCatCommand() *cli.Command { From 5410f8ca9d88b073abd1edc6e1619db000c0acd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tar=C4=B1k=20Bu=C4=9Fra=20=C3=96zyurt?= <74174778+tarikozyurtt@users.noreply.github.com> Date: Wed, 10 Jul 2024 19:55:44 +0300 Subject: [PATCH 3/8] remove unintended test case --- e2e/select_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/e2e/select_test.go b/e2e/select_test.go index 5c0516baf..c7d9b1e40 100644 --- a/e2e/select_test.go +++ b/e2e/select_test.go @@ -218,18 +218,6 @@ func TestSelectCommand(t *testing.T) { outformat: "csv", expectedValue: "id0\n", }, - { - name: "input:json-lines,output:json-lines,all-versions:true", - cmd: []string{ - "select", "json", - "--all-versions", - "--query", query, - }, - informat: "json", - structure: "lines", - outformat: "json", - expectedValue: "", - }, }, "csv": { { From cb9b318e86a30fbd91076e64ae491d23fc47f7ff Mon Sep 17 00:00:00 2001 From: ozyurt19 Date: Thu, 11 Jul 2024 10:25:46 +0300 Subject: [PATCH 4/8] make single object and multiple object case seperated and clear --- command/cat.go | 55 +++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/command/cat.go b/command/cat.go index 4ececabbc..dbf2d352a 100644 --- a/command/cat.go +++ b/command/cat.go @@ -115,46 +115,47 @@ func (c Cat) Run(ctx context.Context) error { return err } - // Initialize an empty channel to handle single or multiple objects - var objectChan <-chan *storage.Object - if c.src.IsWildcard() || c.src.IsPrefix() || c.src.IsBucket() { - objectChan = client.List(ctx, c.src, false) - } else { - _, err = client.Stat(ctx, c.src) - if err != nil { - printError(c.fullCommand, c.op, err) - return err - } - singleObjChan := make(chan *storage.Object, 1) - singleObjChan <- &storage.Object{URL: c.src} - close(singleObjChan) - objectChan = singleObjChan + objectChan := client.List(ctx, c.src, false) + return c.processObjects(ctx, client, objectChan) } - return c.processObjects(ctx, client, objectChan) + _, err = client.Stat(ctx, c.src) + if err != nil { + printError(c.fullCommand, c.op, err) + return err + } + return c.processSingleObject(ctx, client, &storage.Object{URL: c.src}) } func (c Cat) processObjects(ctx context.Context, client *storage.S3, objectChan <-chan *storage.Object) error { for obj := range objectChan { - if obj.Err != nil { - printError(c.fullCommand, c.op, obj.Err) - return obj.Err - } - if obj.Type.IsDir() { - continue - } - buf := orderedwriter.New(os.Stdout) - - _, err := client.Get(ctx, obj.URL, buf, c.concurrency, c.partSize) - if err != nil { - printError(c.fullCommand, c.op, err) + if err := c.processSingleObject(ctx, client, obj); err != nil { return err } } return nil } +func (c Cat) processSingleObject(ctx context.Context, client *storage.S3, obj *storage.Object) error { + if obj.Err != nil { + printError(c.fullCommand, c.op, obj.Err) + return obj.Err + } + if obj.Type.IsDir() { + return nil + } + buf := orderedwriter.New(os.Stdout) + + _, err := client.Get(ctx, obj.URL, buf, c.concurrency, c.partSize) + if err != nil { + printError(c.fullCommand, c.op, err) + return err + } + + return nil +} + func validateCatCommand(c *cli.Context) error { if c.Args().Len() != 1 { return fmt.Errorf("expected only one argument") From bece2fbcf65055cedaf8ed7bfba080eec3f89024 Mon Sep 17 00:00:00 2001 From: ozyurt19 Date: Thu, 11 Jul 2024 10:47:35 +0300 Subject: [PATCH 5/8] combine and make concurrent for wildcard-prefix fail and rename empty bucket tests --- e2e/cat_test.go | 56 ++++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/e2e/cat_test.go b/e2e/cat_test.go index 919c7161f..b09005f43 100644 --- a/e2e/cat_test.go +++ b/e2e/cat_test.go @@ -213,7 +213,7 @@ func TestCatInEmptyBucket(t *testing.T) { bucket := s3BucketFromTestName(t) createBucket(t, s3client, bucket) - t.Run("EmptyBucket", func(t *testing.T) { + t.Run("without prefix", func(t *testing.T) { cmd := s5cmd("cat", fmt.Sprintf("s3://%v", bucket)) result := icmd.RunCmd(cmd) @@ -221,7 +221,7 @@ func TestCatInEmptyBucket(t *testing.T) { assertLines(t, result.Stdout(), nil) }) - t.Run("PrefixInEmptyBucket", func(t *testing.T) { + t.Run("with prefix", func(t *testing.T) { cmd := s5cmd("cat", fmt.Sprintf("s3://%v/", bucket)) result := icmd.RunCmd(cmd) @@ -229,7 +229,7 @@ func TestCatInEmptyBucket(t *testing.T) { assertLines(t, result.Stdout(), nil) }) - t.Run("WildcardInEmptyBucket", func(t *testing.T) { + t.Run("with wildcard", func(t *testing.T) { cmd := s5cmd("cat", fmt.Sprintf("s3://%v/*", bucket)) result := icmd.RunCmd(cmd) @@ -426,30 +426,42 @@ func TestPrefixWildcardFail(t *testing.T) { createBucket(t, s3client, bucket) testCases := []struct { + name string prefix string }{ - {"foo*"}, - {"foolder/"}, - } - - for _, tc := range testCases { - cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) - result := icmd.RunCmd(cmd) - - result.Assert(t, icmd.Expected{ExitCode: 1}) - assertLines(t, result.Stderr(), map[int]compareFunc{ - 0: equals(`ERROR "cat s3://%v/%v": no object found`, bucket, tc.prefix), - }, strictLineCheck(false)) + { + name: "wildcard", + prefix: "foo*", + }, + { + name: "prefix", + prefix: "foolder/", + }, } for _, tc := range testCases { - cmd := s5cmd("--json", "cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) - result := icmd.RunCmd(cmd) - - result.Assert(t, icmd.Expected{ExitCode: 1}) - assertLines(t, result.Stderr(), map[int]compareFunc{ - 0: equals(`{"operation":"cat","command":"cat s3://%v/%v","error":"no object found"}`, bucket, tc.prefix), - }, strictLineCheck(false)) + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Run("default", func(t *testing.T) { + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`ERROR "cat s3://%v/%v": no object found`, bucket, tc.prefix), + }, strictLineCheck(false)) + }) + t.Run("json", func(t *testing.T) { + t.Parallel() + cmd := s5cmd("--json", "cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: 1}) + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals(`{"operation":"cat","command":"cat s3://%v/%v","error":"no object found"}`, bucket, tc.prefix), + }, strictLineCheck(false)) + }) + }) } } From 70882e21dc98364038d618a4707d2266a914ca8b Mon Sep 17 00:00:00 2001 From: ozyurt19 Date: Thu, 11 Jul 2024 11:25:30 +0300 Subject: [PATCH 6/8] use suggested best practices --- e2e/cat_test.go | 153 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 47 deletions(-) diff --git a/e2e/cat_test.go b/e2e/cat_test.go index b09005f43..3a8accfc9 100644 --- a/e2e/cat_test.go +++ b/e2e/cat_test.go @@ -214,6 +214,8 @@ func TestCatInEmptyBucket(t *testing.T) { createBucket(t, s3client, bucket) t.Run("without prefix", func(t *testing.T) { + t.Parallel() + cmd := s5cmd("cat", fmt.Sprintf("s3://%v", bucket)) result := icmd.RunCmd(cmd) @@ -222,6 +224,8 @@ func TestCatInEmptyBucket(t *testing.T) { }) t.Run("with prefix", func(t *testing.T) { + t.Parallel() + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/", bucket)) result := icmd.RunCmd(cmd) @@ -230,6 +234,8 @@ func TestCatInEmptyBucket(t *testing.T) { }) t.Run("with wildcard", func(t *testing.T) { + t.Parallel() + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/*", bucket)) result := icmd.RunCmd(cmd) @@ -348,34 +354,64 @@ func TestCatByVersionID(t *testing.T) { func TestCatPrefix(t *testing.T) { t.Parallel() - s3client, s5cmd := setup(t) - bucket := s3BucketFromTestName(t) - createBucket(t, s3client, bucket) - testCases := []struct { files []string + contents []string prefix string expected string }{ - {files: []string{"file1.txt", "file2.txt"}, prefix: "", expected: "content0content1"}, - {files: []string{"dir/file3.txt", "dir/file4.txt"}, prefix: "", expected: "content0content1"}, - {files: nil, prefix: "dir/", expected: "content2content3"}, - {files: []string{"dir/nesteddir/file5.txt"}, prefix: "dir/", expected: "content2content3"}, - {files: nil, prefix: "dir/nesteddir/", expected: "content4"}, + { + files: []string{"file1.txt", "file2.txt"}, + contents: []string{"content0", "content1"}, + expected: "content0content1"}, + { + files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt"}, + contents: []string{"content0", "content1", "content2", "content3"}, + expected: "content0content1", + }, + { + files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt"}, + contents: []string{"content0", "content1", "content2", "content3"}, + prefix: "dir/", + expected: "content2content3", + }, + { + files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt", "dir/nesteddir/file5.txt"}, + contents: []string{"content0", "content1", "content2", "content3", "content4"}, + prefix: "dir/", + expected: "content2content3", + }, + { + files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt", "dir/nesteddir/file5.txt"}, + contents: []string{"content0", "content1", "content2", "content3", "content4"}, + prefix: "dir/nesteddir/", + expected: "content4", + }, } - offset := 0 for _, tc := range testCases { - if tc.files != nil { - var concatenatedContent strings.Builder + tc := tc + t.Run(tc.expected, func(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + for idx, file := range tc.files { - content := fmt.Sprintf("content%d", idx+offset) + content := tc.contents[idx] putFile(t, s3client, bucket, file, content) - concatenatedContent.WriteString(content) } - offset += len(tc.files) - } - verifyCatCommand(t, s5cmd, bucket, tc.expected, tc.prefix) + + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(tc.expected), + }, alignment(true)) + }) } } @@ -405,73 +441,96 @@ func TestCatWildcard(t *testing.T) { } testCases := []struct { - prefix string - expected string + name string + expression string + expected string }{ - {"foo*", "content0content1content3"}, - {"log-file-2024-*", "content4content5"}, - {"log-file-*", "content7content6content4content5"}, + { + name: "wildcard matching with both file and folder", + expression: "foo*", + expected: "content0content1content3", + }, + { + name: "log files 2024", + expression: "log-file-2024-*", + expected: "content4content5", + }, + { + name: "all log files", + expression: "log-file-*", + expected: "content7content6content4content5", + }, } for _, tc := range testCases { - verifyCatCommand(t, s5cmd, bucket, tc.expected, tc.prefix) + tc := tc + t.Run("", func(t *testing.T) { + t.Parallel() + + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.expression)) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(tc.expected), + }, alignment(true)) + }) } } func TestPrefixWildcardFail(t *testing.T) { t.Parallel() - s3client, s5cmd := setup(t) - bucket := s3BucketFromTestName(t) - createBucket(t, s3client, bucket) - testCases := []struct { - name string - prefix string + name string + expression string }{ { - name: "wildcard", - prefix: "foo*", + name: "wildcard", + expression: "foo*", }, { - name: "prefix", - prefix: "foolder/", + name: "prefix", + expression: "foolder/", }, } for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { + t.Parallel() t.Run("default", func(t *testing.T) { - cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, tc.expression)) result := icmd.RunCmd(cmd) result.Assert(t, icmd.Expected{ExitCode: 1}) assertLines(t, result.Stderr(), map[int]compareFunc{ - 0: equals(`ERROR "cat s3://%v/%v": no object found`, bucket, tc.prefix), + 0: equals(`ERROR "cat s3://%v/%v": no object found`, bucket, tc.expression), }, strictLineCheck(false)) }) t.Run("json", func(t *testing.T) { t.Parallel() - cmd := s5cmd("--json", "cat", fmt.Sprintf("s3://%v/%v", bucket, tc.prefix)) + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + cmd := s5cmd("--json", "cat", fmt.Sprintf("s3://%v/%v", bucket, tc.expression)) result := icmd.RunCmd(cmd) result.Assert(t, icmd.Expected{ExitCode: 1}) assertLines(t, result.Stderr(), map[int]compareFunc{ - 0: equals(`{"operation":"cat","command":"cat s3://%v/%v","error":"no object found"}`, bucket, tc.prefix), + 0: equals(`{"operation":"cat","command":"cat s3://%v/%v","error":"no object found"}`, bucket, tc.expression), }, strictLineCheck(false)) }) }) } } - -func verifyCatCommand(t *testing.T, s5cmd func(...string) icmd.Cmd, bucket, expectedContent, prefix string) { - cmd := s5cmd("cat", fmt.Sprintf("s3://%v/%v", bucket, prefix)) - result := icmd.RunCmd(cmd) - - result.Assert(t, icmd.Success) - assertLines(t, result.Stdout(), map[int]compareFunc{ - 0: equals(expectedContent), - }, alignment(true)) -} From fd902a9c14f9ddad9929ac584b56373dbd196ade Mon Sep 17 00:00:00 2001 From: ozyurt19 Date: Thu, 11 Jul 2024 18:58:12 +0300 Subject: [PATCH 7/8] fix remove need for dummy storage object --- command/cat.go | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/command/cat.go b/command/cat.go index dbf2d352a..ccb1ef25b 100644 --- a/command/cat.go +++ b/command/cat.go @@ -125,35 +125,33 @@ func (c Cat) Run(ctx context.Context) error { printError(c.fullCommand, c.op, err) return err } - return c.processSingleObject(ctx, client, &storage.Object{URL: c.src}) + return c.processSingleObject(ctx, client, c.src) } func (c Cat) processObjects(ctx context.Context, client *storage.S3, objectChan <-chan *storage.Object) error { for obj := range objectChan { - if err := c.processSingleObject(ctx, client, obj); err != nil { + if obj.Err != nil { + printError(c.fullCommand, c.op, obj.Err) + return obj.Err + } + + if obj.Type.IsDir() { + continue + } + + err := c.processSingleObject(ctx, client, obj.URL) + if err != nil { + printError(c.fullCommand, c.op, err) return err } } return nil } -func (c Cat) processSingleObject(ctx context.Context, client *storage.S3, obj *storage.Object) error { - if obj.Err != nil { - printError(c.fullCommand, c.op, obj.Err) - return obj.Err - } - if obj.Type.IsDir() { - return nil - } +func (c Cat) processSingleObject(ctx context.Context, client *storage.S3, url *url.URL) error { buf := orderedwriter.New(os.Stdout) - - _, err := client.Get(ctx, obj.URL, buf, c.concurrency, c.partSize) - if err != nil { - printError(c.fullCommand, c.op, err) - return err - } - - return nil + _, err := client.Get(ctx, url, buf, c.concurrency, c.partSize) + return err } func validateCatCommand(c *cli.Context) error { From 6d59a16e2197f788763dc325968699ad65a36616 Mon Sep 17 00:00:00 2001 From: ozyurt19 Date: Thu, 11 Jul 2024 19:14:59 +0300 Subject: [PATCH 8/8] fix: use map for key and values --- e2e/cat_test.go | 86 ++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 36 deletions(-) diff --git a/e2e/cat_test.go b/e2e/cat_test.go index 3a8accfc9..8eeca3199 100644 --- a/e2e/cat_test.go +++ b/e2e/cat_test.go @@ -96,7 +96,6 @@ func TestCatS3Object(t *testing.T) { assertLines(t, result.Stdout(), tc.expected) }) } - } func TestCatS3ObjectFail(t *testing.T) { @@ -274,7 +273,7 @@ func TestCatByVersionID(t *testing.T) { const filename = "testfile.txt" - var contents = []string{ + contents := []string{ "This is first content", "Second content it is, and it is a bit longer!!!", } @@ -355,43 +354,63 @@ func TestCatPrefix(t *testing.T) { t.Parallel() testCases := []struct { - files []string - contents []string - prefix string - expected string + filesAndContents map[string]string + prefix string + expected string }{ { - files: []string{"file1.txt", "file2.txt"}, - contents: []string{"content0", "content1"}, - expected: "content0content1"}, + filesAndContents: map[string]string{ + "file1.txt": "content0", + "file2.txt": "content1", + }, + expected: "content0content1", + }, { - files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt"}, - contents: []string{"content0", "content1", "content2", "content3"}, + filesAndContents: map[string]string{ + "file1.txt": "content0", + "file2.txt": "content1", + "dir/file3.txt": "content2", + "dir/file4.txt": "content3", + }, expected: "content0content1", }, { - files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt"}, - contents: []string{"content0", "content1", "content2", "content3"}, + filesAndContents: map[string]string{ + "file1.txt": "content0", + "file2.txt": "content1", + "dir/file3.txt": "content2", + "dir/file4.txt": "content3", + }, prefix: "dir/", expected: "content2content3", }, { - files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt", "dir/nesteddir/file5.txt"}, - contents: []string{"content0", "content1", "content2", "content3", "content4"}, + filesAndContents: map[string]string{ + "file1.txt": "content0", + "file2.txt": "content1", + "dir/file3.txt": "content2", + "dir/file4.txt": "content3", + "dir/nesteddir/file5.txt": "content4", + }, prefix: "dir/", expected: "content2content3", }, { - files: []string{"file1.txt", "file2.txt", "dir/file3.txt", "dir/file4.txt", "dir/nesteddir/file5.txt"}, - contents: []string{"content0", "content1", "content2", "content3", "content4"}, + filesAndContents: map[string]string{ + "file1.txt": "content0", + "file2.txt": "content1", + "dir/file3.txt": "content2", + "dir/file4.txt": "content3", + "dir/nesteddir/file5.txt": "content4", + }, prefix: "dir/nesteddir/", expected: "content4", }, } - for _, tc := range testCases { + for idx, tc := range testCases { tc := tc - t.Run(tc.expected, func(t *testing.T) { + t.Run(fmt.Sprintf("case-%v", idx), func(t *testing.T) { t.Parallel() s3client, s5cmd := setup(t) @@ -399,8 +418,7 @@ func TestCatPrefix(t *testing.T) { bucket := s3BucketFromTestName(t) createBucket(t, s3client, bucket) - for idx, file := range tc.files { - content := tc.contents[idx] + for file, content := range tc.filesAndContents { putFile(t, s3client, bucket, file, content) } @@ -422,22 +440,19 @@ func TestCatWildcard(t *testing.T) { bucket := s3BucketFromTestName(t) createBucket(t, s3client, bucket) - files := []struct { - key string - content string - }{ - {"foo1.txt", "content0"}, - {"foo2.txt", "content1"}, - {"bar1.txt", "content2"}, - {"foolder/foo3.txt", "content3"}, - {"log-file-2024-01.txt", "content4"}, - {"log-file-2024-02.txt", "content5"}, - {"log-file-2023-01.txt", "content6"}, - {"log-file-2022-01.txt", "content7"}, + fileAndContent := map[string]string{ + "foo1.txt": "content0", + "foo2.txt": "content1", + "bar1.txt": "content2", + "foolder/foo3.txt": "content3", + "log-file-2024-01.txt": "content4", + "log-file-2024-02.txt": "content5", + "log-file-2023-01.txt": "content6", + "log-file-2022-01.txt": "content7", } - for _, file := range files { - putFile(t, s3client, bucket, file.key, file.content) + for file, content := range fileAndContent { + putFile(t, s3client, bucket, file, content) } testCases := []struct { @@ -532,5 +547,4 @@ func TestPrefixWildcardFail(t *testing.T) { }) }) } - }