Skip to content

Commit

Permalink
move/copy: support objects that end with a trailing slash
Browse files Browse the repository at this point in the history 
unlike a filesystem, where a slash is a special character,
there are no restrictions or requirements about object keys
in object storage. an object can end with a trailing slash,
and listing can choose a delimiter other than a slash.

so, as a result, it is insufficient to test if a user
is trying to do a recursive copy or move by checking if
the input ends with a slash. they may genuinely have
an object with a trailing slash in the object name, or
may want one, and we need to support move and copy with
those objects.

see storj/customer-issues#2066 (comment)
for more

Change-Id: I2cdc581e19d6bc1a9b47b3b4f7bae1ceb3cd1446
  • Loading branch information
@jtolio
jtolio committed Aug 1, 2024
1 parent 390cb61 commit b159dd0
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 22 deletions.
5 changes: 0 additions & 5 deletions move.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package uplink
import (
"context"
"crypto/rand"
"strings"

"github.com/zeebo/errs"

Expand Down Expand Up @@ -92,14 +91,10 @@ func validateMoveCopyInput(oldbucket, oldkey, newbucket, newkey string) error {
return errwrapf("%w (%q)", ErrBucketNameInvalid, oldbucket)
case oldkey == "":
return errwrapf("%w (%q)", ErrObjectKeyInvalid, oldkey)
case strings.HasSuffix(oldkey, "/"):
return packageError.New("oldkey cannot be a prefix")
case newbucket == "": // TODO should we make this error different
return errwrapf("%w (%q)", ErrBucketNameInvalid, newbucket)
case newkey == "": // TODO should we make this error different
return errwrapf("%w (%q)", ErrObjectKeyInvalid, newkey)
case strings.HasSuffix(newkey, "/"):
return packageError.New("newkey cannot be a prefix")
}

return nil
Expand Down
5 changes: 0 additions & 5 deletions private/metaclient/movecopy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package metaclient
import (
"context"
"crypto/rand"
"strings"

"github.com/zeebo/errs"

Expand Down Expand Up @@ -173,14 +172,10 @@ func validateMoveCopyInput(oldbucket, oldkey, newbucket, newkey string) error {
return ErrNoBucket.New(oldbucket)
case oldkey == "":
return ErrNoPath.New(oldkey)
case strings.HasSuffix(oldkey, "/"):
return errs.New("oldkey cannot be a prefix")
case newbucket == "": // TODO should we make this error different
return ErrNoBucket.New(newbucket)
case newkey == "": // TODO should we make this error different
return ErrNoPath.New(newkey)
case strings.HasSuffix(newkey, "/"):
return errs.New("newkey cannot be a prefix")
}

return nil
Expand Down
43 changes: 37 additions & 6 deletions testsuite/copy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ func TestCopyObject(t *testing.T) {
createBucket(t, ctx, project, "testbucket")
createBucket(t, ctx, project, "new-testbucket")

testCases := []struct {
type testCase struct {
Bucket string
Key string
NewBucket string
NewKey string
ObjectSize memory.Size
}{
}

testCases := []testCase{
// the same bucket
{"testbucket", "empty", "testbucket", "new-empty", 0},
{"testbucket", "inline", "testbucket", "new-inline", memory.KiB},
Expand All @@ -47,7 +49,7 @@ func TestCopyObject(t *testing.T) {
{"testbucket", "remote+inline", "testbucket", "new-remote+inline", 11 * memory.KiB},
//// 3 remote segments
{"testbucket", "multiple-remote-segments", "testbucket", "new-multiple-remote-segments", 29 * memory.KiB},
{"testbucket", "remote-with-prefix", "testbucket", "a/prefix/remote-with-prefix", 9 * memory.KiB},
{"testbucket", "remote-with-delimiters", "testbucket", "a/prefix/remote-with-delimiters", 9 * memory.KiB},
//
//// different buckets
{"testbucket", "empty", "new-testbucket", "new-empty", 0},
Expand All @@ -58,14 +60,37 @@ func TestCopyObject(t *testing.T) {
{"testbucket", "remote+inline", "new-testbucket", "new-remote+inline", 11 * memory.KiB},
// 3 remote segments
{"testbucket", "multiple-remote-segments", "new-testbucket", "new-multiple-remote-segments", 29 * memory.KiB},
{"testbucket", "remote-with-prefix", "new-testbucket", "a/prefix/remote-with-prefix", 9 * memory.KiB},
{"testbucket", "remote-with-delimiters", "new-testbucket", "a/prefix/remote-with-delimiters", 9 * memory.KiB},
// same destination
{"testbucket", "same-destination", "testbucket", "same-destination", 10 * memory.KiB},
}

for _, destBucket := range []string{"testbucket", "new-testbucket"} {
for _, objectSize := range []memory.Size{1 * memory.KiB, 9 * memory.KiB, 21 * memory.KiB} {
for _, sourceKey := range []string{"an/object/key", "an/object/key/"} {
for _, destTrailingSlash := range []bool{true, false} {
for _, destKey := range []string{
"an/object/key2",
"an/object/key/2",
"an/object2",
"an/object",
"another/object",
} {
if destTrailingSlash {
destKey += "/"
}
testCases = append(testCases, testCase{
"testbucket", sourceKey, destBucket, destKey, objectSize,
})
}
}
}
}
}

for _, tc := range testCases {
tc := tc
t.Run(tc.Key, func(t *testing.T) {
t.Run(tc.Bucket+"/"+tc.Key+" to "+tc.NewBucket+"/"+tc.NewKey+" "+tc.ObjectSize.Base2String(), func(t *testing.T) {
expectedData := testrand.Bytes(tc.ObjectSize)
err := planet.Uplinks[0].Upload(newCtx, planet.Satellites[0], tc.Bucket, tc.Key, expectedData)
require.NoError(t, err)
Expand Down Expand Up @@ -101,6 +126,12 @@ func TestCopyObject(t *testing.T) {
// verify that original object still exists
_, err = project.StatObject(ctx, tc.Bucket, tc.Key)
require.NoError(t, err)

// cleanup
_, err = project.DeleteObject(ctx, tc.Bucket, tc.Key)
require.NoError(t, err)
_, err = project.DeleteObject(ctx, tc.NewBucket, tc.NewKey)
require.NoError(t, err)
})
}
})
Expand Down Expand Up @@ -209,7 +240,7 @@ func TestCopyObject_Errors(t *testing.T) {
require.Contains(t, err.Error(), "(\"non-existing-bucket\")")

_, err = project.CopyObject(ctx, "testbucket", "prefix/", "testbucket", "new-key", nil)
require.Error(t, err)
require.True(t, errors.Is(err, uplink.ErrObjectNotFound))

_, err = project.CopyObject(ctx, "testbucket", "key", "testbucket", "prefix/", nil)
require.Error(t, err)
Expand Down
41 changes: 35 additions & 6 deletions testsuite/move_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ func TestMoveObject(t *testing.T) {
createBucket(t, ctx, project, "testbucket")
createBucket(t, ctx, project, "new-testbucket")

testCases := []struct {
type testCase struct {
Bucket string
Key string
NewBucket string
NewKey string
ObjectSize memory.Size
}{
}

testCases := []testCase{
// the same bucket
{"testbucket", "empty", "testbucket", "new-empty", 0},
{"testbucket", "inline", "testbucket", "new-inline", memory.KiB},
Expand All @@ -47,7 +49,7 @@ func TestMoveObject(t *testing.T) {
{"testbucket", "remote+inline", "testbucket", "new-remote+inline", 11 * memory.KiB},
// 3 remote segment
{"testbucket", "multiple-remote-segments", "testbucket", "new-multiple-remote-segments", 29 * memory.KiB},
{"testbucket", "remote-with-prefix", "testbucket", "a/prefix/remote-with-prefix", 9 * memory.KiB},
{"testbucket", "remote-with-delimiters", "testbucket", "a/prefix/remote-with-delimiters", 9 * memory.KiB},

// different bucket
{"testbucket", "empty", "new-testbucket", "new-empty", 0},
Expand All @@ -58,14 +60,37 @@ func TestMoveObject(t *testing.T) {
{"testbucket", "remote+inline", "new-testbucket", "new-remote+inline", 11 * memory.KiB},
// 3 remote segment
{"testbucket", "multiple-remote-segments", "new-testbucket", "new-multiple-remote-segments", 29 * memory.KiB},
{"testbucket", "remote-with-prefix", "new-testbucket", "a/prefix/remote-with-prefix", 9 * memory.KiB},
{"testbucket", "remote-with-delimiters", "new-testbucket", "a/prefix/remote-with-delimiters", 9 * memory.KiB},

// TODO write tests to move to existing key
}

for _, destBucket := range []string{"testbucket", "new-testbucket"} {
for _, objectSize := range []memory.Size{1 * memory.KiB, 9 * memory.KiB, 21 * memory.KiB} {
for _, sourceKey := range []string{"an/object/key", "an/object/key/"} {
for _, destTrailingSlash := range []bool{true, false} {
for _, destKey := range []string{
"an/object/key2",
"an/object/key/2",
"an/object2",
"an/object",
"another/object",
} {
if destTrailingSlash {
destKey += "/"
}
testCases = append(testCases, testCase{
"testbucket", sourceKey, destBucket, destKey, objectSize,
})
}
}
}
}
}

for _, tc := range testCases {
tc := tc
t.Run(tc.Key, func(t *testing.T) {
t.Run(tc.Bucket+"/"+tc.Key+" to "+tc.NewBucket+"/"+tc.NewKey+" "+tc.ObjectSize.Base2String(), func(t *testing.T) {
expectedData := testrand.Bytes(tc.ObjectSize)
err := planet.Uplinks[0].Upload(newCtx, planet.Satellites[0], tc.Bucket, tc.Key, expectedData)
require.NoError(t, err)
Expand All @@ -88,6 +113,10 @@ func TestMoveObject(t *testing.T) {

_, err = project.StatObject(ctx, tc.Bucket, tc.Key)
require.True(t, errors.Is(err, uplink.ErrObjectNotFound))

// cleanup
_, err = project.DeleteObject(ctx, tc.NewBucket, tc.NewKey)
require.NoError(t, err)
})
}
})
Expand Down Expand Up @@ -128,7 +157,7 @@ func TestMoveObject_Errors(t *testing.T) {
require.Contains(t, err.Error(), "(\"non-existing-bucket\")")

err = project.MoveObject(ctx, "testbucket", "prefix/", "testbucket", "new-key", nil)
require.Error(t, err)
require.True(t, errors.Is(err, uplink.ErrObjectNotFound))

err = project.MoveObject(ctx, "testbucket", "key", "testbucket", "prefix/", nil)
require.Error(t, err)
Expand Down

0 comments on commit b159dd0

Please sign in to comment.