Repository: peak/s5cmd
Pull request #459 (2022-07-05 15:49:27)

storage/url: Allow usage of adjacent slashes
This PR allows url with adjacent slashes to be handled correctly. 

- Disabled Automatic URI cleaning ([search in this link for more information](https://docs.aws.amazon.com/sdk-for-go/api/service/s3/)) as it do not allow adjacent slashes.
- Changed path.Join method to strings.Join when joined remote urls as path.Join also cleans paths which removes additional slashes.
-  Kept local urls to be joined with path.Join.
- Added test cases for copy command for following cases:
    - Remote to local
    - Local to remote
    - Remote to remote
- Removed redundant leading slashes in the keys of s3 objects in ls_test.go and sync_test.go files.

Users should be careful while handling slashes in objectnames. Before this PR,`s5cmd cp /file.txt s3://bucket/` would output
```
cp /file.txt s3://bucket/file.txt
```
But now it will result in double slashes as follows:
```
cp /file.txt s3://bucket//file.txt
```

Fixes: https://github.com/peak/s5cmd/issues/352
Fixes: https://github.com/peak/s5cmd/issues/449

seruman: > @boraberke could you update the changelog file to reflect this change please? I'm not sure if this is a bugfix or an improvement :)

Might even breaking for users those rely on URI cleaning 🙃 .
boraberke: > @boraberke could you update the changelog file to reflect this change please? I'm not sure if this is a bugfix or an improvement :)

Added three more entries to `CHANGELOG.MD` further explaining the details of this PR :)

Patch:
diff --git a/.goreleaser.yml b/.goreleaser.yml
index 282524f..d931a37 100644
--- a/.goreleaser.yml
+++ b/.goreleaser.yml
@@ -54,10 +54,24 @@ brews:
     folder: Formula
     name: s5cmd
     description: Parallel S3 and local filesystem execution tool
-    homepage:  https://github.com/peak/s5cmd
+    homepage: https://github.com/peak/s5cmd
     license: MIT
     test: |
       system "#{bin}/s5cmd version"
 
+nfpms:
+  -
+    vendor: Peak
+    homepage: https://github.com/peak/s5cmd
+    license: MIT
+    maintainer: Peak <packaging@peak.com>
+    description: |-
+      Very fast S3 and local filesystem execution tool. It comes with support
+      for a multitude of operations including tab completion and wildcard
+      support for files, which can be very handy for your object storage
+      workflow while working with large number of files.
+    formats:
+      - deb
+
 release:
   draft: true
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ee761f..4ae646c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ## not released yet
 
+#### Breaking changes
+- Adjacent slashes in key are no longer removed when uploading to remote. Before `s5cmd cp file.txt s3://bucket/a//b///c/` would copy to `s3://bucket/a/b/c/file.txt` but now to `s3://bucket/a//b///c/file.txt`.([#459](https://github.com/peak/s5cmd/pull/459))
+
 #### Features
 - Added `--content-type` and `--content-encoding` flags to `cp` command. ([#264](https://github.com/peak/s5cmd/issues/264))
 - Added `--profile` flag to allow users to specify a [named profile](https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-profiles.html). ([#353](https://github.com/peak/s5cmd/issues/353))
@@ -14,7 +17,11 @@
 - Fixed a bug where (`--stat`) prints unnecessarily when used with help and version commands ([#452](https://github.com/peak/s5cmd/issues/452))
 - Changed cp error message to be more precise. "given object not found" error message now will also include absolute path of the file. ([#463](https://github.com/peak/s5cmd/pull/463))
 - Fixed a bug where proxy is not being used when `--no-verify-ssl` flag is used. ([#445](https://github.com/peak/s5cmd/issues/445))
+- Fixed `unknown url format` error when object key also includes `s3://` e.g. `s5cmd ls s3://foo/bar/s3://baz` ([#449](https://github.com/peak/s5cmd/issues/449))
 
+#### Improvements
+- Disable AWS SDK logger if log level is not "trace"
+- Allow adjacent slashes to be used as keys when uploading to remote. ([#459](https://github.com/peak/s5cmd/pull/459))
 
 ## v2.0.0 - 4 Jul 2022
 
@@ -35,6 +42,7 @@
 
 #### Improvements
 - Upgraded minimum required Go version to 1.16.
+- Debian packages are provided on [releases page](https://github.com/peak/s5cmd/releases) ([#380](https://github.com/peak/s5cmd/issues/380))
 
 #### Bugfixes
 - Fixed a bug about precedence of region detection, which auto region detection would always override region defined in environment or profile. ([#325](https://github.com/peak/s5cmd/issues/325))
diff --git a/storage/s3.go b/storage/s3.go
index 7f9fbdd..2c78070 100644
--- a/storage/s3.go
+++ b/storage/s3.go
@@ -803,12 +803,13 @@ func (sc *SessionCache) newSession(ctx context.Context, opts Options) (*session.
 	if opts.NoVerifySSL {
 		httpClient = insecureHTTPClient
 	}
-
 	awsCfg = awsCfg.
 		WithEndpoint(endpointURL.String()).
 		WithS3ForcePathStyle(!isVirtualHostStyle).
 		WithS3UseAccelerate(useAccelerate).
-		WithHTTPClient(httpClient)
+		WithHTTPClient(httpClient).
+		// Disable URI cleaning to allow adjacent slashes to be used in S3 object keys.
+		WithDisableRestProtocolURICleaning(true)
 
 	if opts.LogLevel == log.LevelTrace {
 		awsCfg = awsCfg.WithLogLevel(aws.LogDebug).
diff --git a/storage/url/url.go b/storage/url/url.go
index df56d5e..daa48cf 100644
--- a/storage/url/url.go
+++ b/storage/url/url.go
@@ -58,9 +58,9 @@ func WithRaw(mode bool) Option {
 
 // New creates a new URL from given path string.
 func New(s string, opts ...Option) (*URL, error) {
-	split := strings.Split(s, "://")
-
-	if len(split) == 1 {
+	// TODO Change strCut to strings.Cut when minimum required Go version is 1.18
+	scheme, rest, isFound := strCut(s, "://")
+	if !isFound {
 		url := &URL{
 			Type:   localObject,
 			Scheme: "",
@@ -81,12 +81,6 @@ func New(s string, opts ...Option) (*URL, error) {
 		return url, nil
 	}
 
-	if len(split) != 2 {
-		return nil, fmt.Errorf("storage: unknown url format %q", s)
-	}
-
-	scheme, rest := split[0], split[1]
-
 	if scheme != "s3" {
 		return nil, fmt.Errorf("s3 url should start with %q", s3Scheme)
 	}
@@ -96,7 +90,7 @@ func New(s string, opts ...Option) (*URL, error) {
 	key := ""
 	bucket := parts[0]
 	if len(parts) == 2 {
-		key = strings.TrimLeft(parts[1], s3Separator)
+		key = parts[1]
 	}
 
 	if bucket == "" {
@@ -124,6 +118,17 @@ func New(s string, opts ...Option) (*URL, error) {
 	return url, nil
 }
 
+// strCut slices s around the first instance of sep,
+// returning the text before and after sep.
+// The found result reports whether sep appears in s.
+// If sep does not appear in s, cut returns s, "", false.
+func strCut(s string, sep string) (before string, after string, isFound bool) {
+	if i := strings.Index(s, sep); i >= 0 {
+		return s[:i], s[i+len(sep):], true
+	}
+	return s, "", false
+}
+
 // IsRemote reports whether the object is stored on a remote storage system.
 func (u *URL) IsRemote() bool {
 	return u.Type == remoteObject
@@ -185,8 +190,15 @@ func (u *URL) Join(s string) *URL {
 	}
 
 	clone := u.Clone()
-	clone.Path = path.Join(clone.Path, s)
-
+	if !clone.IsRemote() {
+		// URL is local, thus clean the path by using path.Join which
+		// removes adjacent slashes.
+		clone.Path = path.Join(clone.Path, s)
+		return clone
+	}
+	// URL is remote, keep them as it is and join using string.Join which
+	// allows to use adjacent slashes
+	clone.Path = strings.Join([]string{clone.Path, s}, "")
 	return clone
 }
 


Test patch:
diff --git a/e2e/cp_test.go b/e2e/cp_test.go
index 5d1540f..484c427 100644
--- a/e2e/cp_test.go
+++ b/e2e/cp_test.go
@@ -78,6 +78,35 @@ func TestCopySingleS3ObjectToLocal(t *testing.T) {
 			expected:       fs.WithDir("dir", fs.WithFile("file1.txt", fileContent, fs.WithMode(0644))),
 			expectedOutput: "cp s3://bucket/file1.txt dir/file1.txt",
 		},
+		// Cases with adjacent slashes. Expected behavior is to remove all duplicate slashes in local files.
+		{
+			name:           "cp s3://bucket//a/b///c////object .",
+			src:            "/a/b///c////file1.txt",
+			dst:            ".",
+			expected:       fs.WithFile("file1.txt", fileContent, fs.WithMode(0644)),
+			expectedOutput: "cp s3://bucket//a/b///c////file1.txt file1.txt",
+		},
+		{
+			name:           "cp s3://bucket//a/b///c////object file",
+			src:            "/a/b///c////file1.txt",
+			dst:            "file1.txt",
+			expected:       fs.WithFile("file1.txt", fileContent, fs.WithMode(0644)),
+			expectedOutput: "cp s3://bucket//a/b///c////file1.txt file1.txt",
+		},
+		{
+			name:           "cp s3://bucket//a/b///c////object dir/",
+			src:            "/a/b///c////file1.txt",
+			dst:            "dir/",
+			expected:       fs.WithDir("dir", fs.WithFile("file1.txt", fileContent, fs.WithMode(0644))),
+			expectedOutput: "cp s3://bucket//a/b///c////file1.txt dir/file1.txt",
+		},
+		{
+			name:           "cp s3://bucket//a/b///c////object dir/file",
+			src:            "/a/b///c////file1.txt",
+			dst:            "dir/file1.txt",
+			expected:       fs.WithDir("dir", fs.WithFile("file1.txt", fileContent, fs.WithMode(0644))),
+			expectedOutput: "cp s3://bucket//a/b///c////file1.txt dir/file1.txt",
+		},
 	}
 
 	for _, tc := range testcases {
@@ -708,6 +737,83 @@ func TestCopySingleFileToS3(t *testing.T) {
 	assert.Assert(t, ensureS3Object(s3client, bucket, filename, content, ensureContentType(expectedContentType)))
 }
 
+func TestCopySingleFileToS3WithAdjacentSlashes(t *testing.T) {
+	t.Parallel()
+
+	const (
+		bucket   = "testbucket"
+		filename = "index.txt"
+		content  = "this is a test file"
+	)
+
+	testcases := []struct {
+		name          string
+		dstpathprefix string
+	}{
+		{
+			name:          "cp dir/file s3://bucket//a/b/",
+			dstpathprefix: "/a/b/",
+		},
+		{
+			name:          "cp dir/file s3://bucket/a//b/",
+			dstpathprefix: "a//b/",
+		},
+		{
+			name:          "cp dir/file s3://bucket/a/b//",
+			dstpathprefix: "a/b//",
+		},
+		{
+			name:          "cp dir/file s3://bucket//a///b/",
+			dstpathprefix: "/a///b/",
+		},
+		{
+			name:          "cp dir/file s3://bucket/a//b///",
+			dstpathprefix: "a//b///",
+		},
+		{
+			name:          "cp dir/file s3://bucket/a//b//c//d///",
+			dstpathprefix: "a//b//c//d///",
+		},
+		{
+			name:          "cp dir/file s3://bucket/bar/s3://",
+			dstpathprefix: "bar/s3://",
+		},
+	}
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			s3client, s5cmd, cleanup := setup(t)
+			defer cleanup()
+
+			createBucket(t, s3client, bucket)
+
+			workdir := fs.NewDir(t, bucket, fs.WithFile(filename, content))
+			defer workdir.Remove()
+
+			srcpath := workdir.Join(filename)
+
+			dstpath := fmt.Sprintf("s3://%v/%v", bucket, tc.dstpathprefix)
+			srcpath = filepath.ToSlash(srcpath)
+			cmd := s5cmd("cp", srcpath, dstpath)
+			result := icmd.RunCmd(cmd)
+			result.Assert(t, icmd.Success)
+
+			assertLines(t, result.Stdout(), map[int]compareFunc{
+				0: suffix(`cp %v %v%v`, srcpath, dstpath, filename),
+			})
+			// 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, tc.dstpathprefix+filename, content))
+		})
+
+	}
+
+}
+
 // --json cp dir/file s3://bucket
 func TestCopySingleFileToS3JSON(t *testing.T) {
 	t.Parallel()
@@ -1721,6 +1827,7 @@ func TestCopyAllObjectsIntoAnotherBucketIncludingSpecialCharacter(t *testing.T)
 		"sub&@$/test+1.txt":           "this is a test file 1",
 		"sub:,?/test; =2.txt":         "this is a test file 2",
 		"test&@$:,?;= 3.txt":          "this is a test file 3",
+		"sub///test&@$:,?;= 4.txt":    "this is a test file with adjacent slashes",
 		"sub/this-is-normal-file.txt": "this is a normal file",
 	}
 
@@ -1737,9 +1844,10 @@ func TestCopyAllObjectsIntoAnotherBucketIncludingSpecialCharacter(t *testing.T)
 	result.Assert(t, icmd.Success)
 	assertLines(t, result.Stdout(), map[int]compareFunc{
 		0: equals(`cp s3://%v/sub&@$/test+1.txt s3://%v/sub&@$/test+1.txt`, srcbucket, dstbucket),
-		1: equals(`cp s3://%v/sub/this-is-normal-file.txt s3://%v/sub/this-is-normal-file.txt`, srcbucket, dstbucket),
-		2: equals(`cp s3://%v/sub:,?/test; =2.txt s3://%v/sub:,?/test; =2.txt`, srcbucket, dstbucket),
-		3: equals(`cp s3://%v/test&@$:,?;= 3.txt s3://%v/test&@$:,?;= 3.txt`, srcbucket, dstbucket),
+		1: equals(`cp s3://%v/sub///test&@$:,?;= 4.txt s3://%v/sub///test&@$:,?;= 4.txt`, srcbucket, dstbucket),
+		2: equals(`cp s3://%v/sub/this-is-normal-file.txt s3://%v/sub/this-is-normal-file.txt`, srcbucket, dstbucket),
+		3: equals(`cp s3://%v/sub:,?/test; =2.txt s3://%v/sub:,?/test; =2.txt`, srcbucket, dstbucket),
+		4: equals(`cp s3://%v/test&@$:,?;= 3.txt s3://%v/test&@$:,?;= 3.txt`, srcbucket, dstbucket),
 	}, sortInput(true))
 }
 
diff --git a/e2e/ls_test.go b/e2e/ls_test.go
index 0b14cfb..2de7af3 100644
--- a/e2e/ls_test.go
+++ b/e2e/ls_test.go
@@ -172,16 +172,16 @@ func TestListMultipleWildcardS3Object(t *testing.T) {
 	defer cleanup()
 
 	createBucket(t, s3client, bucket)
-	putFile(t, s3client, bucket, "/a/testfile1.txt", "content")
-	putFile(t, s3client, bucket, "/a/testfile2.txt", "content")
-	putFile(t, s3client, bucket, "/b/testfile3.txt", "content")
-	putFile(t, s3client, bucket, "/b/testfile4.txt", "content")
-	putFile(t, s3client, bucket, "/c/testfile5.gz", "content")
-	putFile(t, s3client, bucket, "/c/testfile6.txt.gz", "content")
-	putFile(t, s3client, bucket, "/d/foo/bar/file7.txt", "content")
-	putFile(t, s3client, bucket, "/d/foo/bar/testfile8.txt", "content")
-	putFile(t, s3client, bucket, "/e/txt/testfile9.txt.gz", "content")
-	putFile(t, s3client, bucket, "/f/txt/testfile10.txt", "content")
+	putFile(t, s3client, bucket, "a/testfile1.txt", "content")
+	putFile(t, s3client, bucket, "a/testfile2.txt", "content")
+	putFile(t, s3client, bucket, "b/testfile3.txt", "content")
+	putFile(t, s3client, bucket, "b/testfile4.txt", "content")
+	putFile(t, s3client, bucket, "c/testfile5.gz", "content")
+	putFile(t, s3client, bucket, "c/testfile6.txt.gz", "content")
+	putFile(t, s3client, bucket, "d/foo/bar/file7.txt", "content")
+	putFile(t, s3client, bucket, "d/foo/bar/testfile8.txt", "content")
+	putFile(t, s3client, bucket, "e/txt/testfile9.txt.gz", "content")
+	putFile(t, s3client, bucket, "f/txt/testfile10.txt", "content")
 
 	const pattern = "/*/testfile*.txt"
 	cmd := s5cmd("ls", "s3://"+bucket+pattern)
@@ -209,16 +209,16 @@ func TestListMultipleWildcardS3ObjectWithPrefix(t *testing.T) {
 	defer cleanup()
 
 	createBucket(t, s3client, bucket)
-	putFile(t, s3client, bucket, "/a/testfile1.txt", "content")
-	putFile(t, s3client, bucket, "/a/testfile2.txt", "content")
-	putFile(t, s3client, bucket, "/a/testfile3.txt", "content")
-	putFile(t, s3client, bucket, "/b/testfile4.txt", "content")
-	putFile(t, s3client, bucket, "/c/testfile5.gz", "content")
-	putFile(t, s3client, bucket, "/c/testfile6.txt.gz", "content")
-	putFile(t, s3client, bucket, "/d/foo/bar/file7.txt", "content")
-	putFile(t, s3client, bucket, "/d/foo/bar/testfile8.txt", "content")
-	putFile(t, s3client, bucket, "/e/txt/testfile9.txt.gz", "content")
-	putFile(t, s3client, bucket, "/f/txt/testfile10.txt", "content")
+	putFile(t, s3client, bucket, "a/testfile1.txt", "content")
+	putFile(t, s3client, bucket, "a/testfile2.txt", "content")
+	putFile(t, s3client, bucket, "a/testfile3.txt", "content")
+	putFile(t, s3client, bucket, "b/testfile4.txt", "content")
+	putFile(t, s3client, bucket, "c/testfile5.gz", "content")
+	putFile(t, s3client, bucket, "c/testfile6.txt.gz", "content")
+	putFile(t, s3client, bucket, "d/foo/bar/file7.txt", "content")
+	putFile(t, s3client, bucket, "d/foo/bar/testfile8.txt", "content")
+	putFile(t, s3client, bucket, "e/txt/testfile9.txt.gz", "content")
+	putFile(t, s3client, bucket, "f/txt/testfile10.txt", "content")
 
 	const pattern = "/a/testfile*.txt"
 	cmd := s5cmd("ls", "s3://"+bucket+pattern)
diff --git a/e2e/sync_test.go b/e2e/sync_test.go
index cc15548..8626a53 100644
--- a/e2e/sync_test.go
+++ b/e2e/sync_test.go
@@ -357,7 +357,7 @@ func TestSyncS3BucketToEmptyS3Bucket(t *testing.T) {
 
 	// assert s3 objects in dest bucket
 	for key, content := range S3Content {
-		key = fmt.Sprintf("/%s/%s", prefix, key) // add the prefix
+		key = fmt.Sprintf("%s/%s", prefix, key) // add the prefix
 		assert.Assert(t, ensureS3Object(s3client, destbucket, key, content))
 	}
 }
diff --git a/e2e/util_test.go b/e2e/util_test.go
index cfede70..a84dedc 100644
--- a/e2e/util_test.go
+++ b/e2e/util_test.go
@@ -151,12 +151,13 @@ func s3client(t *testing.T, options storage.Options) *s3.S3 {
 	if *flagTestLogLevel == "debug" {
 		awsLogLevel = aws.LogDebug
 	}
-
+	// WithDisableRestProtocolURICleaning is added to allow adjacent slashes to be used in s3 object keys.
 	s3Config := aws.NewConfig().
 		WithEndpoint(options.Endpoint).
 		WithRegion(endpoints.UsEast1RegionID).
 		WithCredentials(credentials.NewStaticCredentials(defaultAccessKeyID, defaultSecretAccessKey, "")).
 		WithDisableSSL(options.NoVerifySSL).
+		WithDisableRestProtocolURICleaning(true).
 		WithS3ForcePathStyle(true).
 		WithCredentialsChainVerboseErrors(true).
 		WithLogLevel(awsLogLevel)
diff --git a/storage/url/url_test.go b/storage/url/url_test.go
index 0f30732..a1e04aa 100644
--- a/storage/url/url_test.go
+++ b/storage/url/url_test.go
@@ -118,6 +118,99 @@ func TestNew(t *testing.T) {
 	}
 }
 
+func TestJoin(t *testing.T) {
+	tests := []struct {
+		name       string
+		before     *URL
+		objectName string
+		after      *URL
+	}{
+		// URL is remote, expected to keep adjacent slashes
+		{
+			name: "remote:url_with_adjacent_slashes",
+			before: &URL{
+				Path: "s3://bucket/a//b/",
+				Type: remoteObject,
+			},
+			objectName: "test.txt",
+			after: &URL{
+				Path: "s3://bucket/a//b/test.txt",
+				Type: remoteObject,
+			},
+		},
+		{
+			name: "remote:objectName_has_adjacent_slashes",
+			before: &URL{
+				Path: "s3://bucket/a/b/",
+				Type: remoteObject,
+			},
+			objectName: "folder//test.txt",
+			after: &URL{
+				Path: "s3://bucket/a/b/folder//test.txt",
+				Type: remoteObject,
+			},
+		},
+		{
+			name: "remote:objectName_url_has_adjacent_slashes",
+			before: &URL{
+				Path: "s3://bucket/a//b/",
+				Type: remoteObject,
+			},
+			objectName: "/folder//test.txt",
+			after: &URL{
+				Path: "s3://bucket/a//b//folder//test.txt",
+				Type: remoteObject,
+			},
+		},
+		// URL is local, expected to clean adjacent slashes
+		{
+			name: "local:url_with_adjacent_slashes",
+			before: &URL{
+				Path: "dir/a//b/",
+				Type: localObject,
+			},
+			objectName: "test.txt",
+			after: &URL{
+				Path: "dir/a/b/test.txt",
+				Type: localObject,
+			},
+		},
+		{
+			name: "local:objectName_has_adjacent_slashes",
+			before: &URL{
+				Path: "dir/a/b/",
+				Type: localObject,
+			},
+			objectName: "folder//test.txt",
+			after: &URL{
+				Path: "dir/a/b/folder/test.txt",
+				Type: localObject,
+			},
+		},
+		{
+			name: "local:objectName_url_has_adjacent_slashes",
+			before: &URL{
+				Path: "dir/a//b/",
+				Type: localObject,
+			},
+			objectName: "/folder//test.txt",
+			after: &URL{
+				Path: "dir/a/b/folder/test.txt",
+				Type: localObject,
+			},
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			got := tc.before.Join(tc.objectName)
+			if !reflect.DeepEqual(got, tc.after) {
+				t.Errorf("Join() got = %v, want %v", got, tc.after)
+			}
+		})
+	}
+}
+
 func TestURLSetPrefixAndFilter(t *testing.T) {
 	tests := []struct {
 		name   string