From bb0c4ad2d8af8af1ff9abea54dbdf97eadc9018f Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 30 Sep 2021 11:11:46 +0100 Subject: [PATCH] union: fix rename not working with union of local disk and bucket based remote Before this change the union's feature flags were a strict AND of the underlying remotes. This means that a union of a local disk (which can Move but not Copy) and a bucket based remote (which can Copy but not Move) could neither Move nor Copy. This fix advertises Move in the union if all the remotes can Move or Copy. It also implements Move as Copy+Delete (like rclone does normally) if the underlying union does not support Move. This enables renames to work with unions of local disk and bucket based remotes expected. Fixes #5632 --- backend/union/union.go | 31 ++++++-- backend/union/union_internal_test.go | 101 +++++++++++++++++++++++++++ backend/union/union_test.go | 34 +++------ 3 files changed, 136 insertions(+), 30 deletions(-) diff --git a/backend/union/union.go b/backend/union/union.go index 5009ee28c..966f8ccc1 100644 --- a/backend/union/union.go +++ b/backend/union/union.go @@ -254,7 +254,7 @@ func (f *Fs) Move(ctx context.Context, src fs.Object, remote string) (fs.Object, return nil, err } for _, e := range entries { - if e.UpstreamFs().Features().Move == nil { + if !operations.CanServerSideMove(e.UpstreamFs()) { return nil, fs.ErrorCantMove } } @@ -277,12 +277,27 @@ func (f *Fs) Move(ctx context.Context, src fs.Object, remote string) (fs.Object, errs[i] = errors.Wrap(fs.ErrorCantMove, su.Name()+":"+remote) return } - mo, err := du.Features().Move(ctx, o.UnWrap(), remote) - if err != nil || mo == nil { + srcObj := o.UnWrap() + duFeatures := du.Features() + do := duFeatures.Move + if duFeatures.Move == nil { + do = duFeatures.Copy + } + // Do the Move or Copy + dstObj, err := do(ctx, srcObj, remote) + if err != nil || dstObj == nil { errs[i] = errors.Wrap(err, su.Name()) return } - objs[i] = du.WrapObject(mo) + objs[i] = du.WrapObject(dstObj) + // Delete the source object if Copy + if duFeatures.Move == nil { + err = srcObj.Remove(ctx) + if err != nil { + errs[i] = errors.Wrap(err, su.Name()) + return + } + } }) var en []upstream.Entry for _, o := range objs { @@ -848,8 +863,16 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e SetTier: true, GetTier: true, }).Fill(ctx, f) + canMove := true for _, f := range upstreams { features = features.Mask(ctx, f) // Mask all upstream fs + if !operations.CanServerSideMove(f) { + canMove = false + } + } + // We can move if all remotes support Move or Copy + if canMove { + features.Move = f.Move } // Enable ListR when upstreams either support ListR or is local diff --git a/backend/union/union_internal_test.go b/backend/union/union_internal_test.go index db5367561..8171f593f 100644 --- a/backend/union/union_internal_test.go +++ b/backend/union/union_internal_test.go @@ -3,10 +3,15 @@ package union import ( "bytes" "context" + "fmt" + "io/ioutil" + "os" "testing" "time" + "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/object" + "github.com/rclone/rclone/fs/operations" "github.com/rclone/rclone/fstest" "github.com/rclone/rclone/fstest/fstests" "github.com/rclone/rclone/lib/random" @@ -14,6 +19,22 @@ import ( "github.com/stretchr/testify/require" ) +// MakeTestDirs makes directories in /tmp for testing +func MakeTestDirs(t *testing.T, n int) (dirs []string, clean func()) { + for i := 1; i <= n; i++ { + dir, err := ioutil.TempDir("", fmt.Sprintf("rclone-union-test-%d", n)) + require.NoError(t, err) + dirs = append(dirs, dir) + } + clean = func() { + for _, dir := range dirs { + err := os.RemoveAll(dir) + assert.NoError(t, err) + } + } + return dirs, clean +} + func (f *Fs) TestInternalReadOnly(t *testing.T) { if f.name != "TestUnionRO" { t.Skip("Only on RO union") @@ -65,3 +86,83 @@ func (f *Fs) InternalTest(t *testing.T) { } var _ fstests.InternalTester = (*Fs)(nil) + +// This specifically tests a union of local which can Move but not +// Copy and :memory: which can Copy but not Move to makes sure that +// the resulting union can Move +func TestMoveCopy(t *testing.T) { + if *fstest.RemoteName != "" { + t.Skip("Skipping as -remote set") + } + ctx := context.Background() + dirs, clean := MakeTestDirs(t, 1) + defer clean() + fsString := fmt.Sprintf(":union,upstreams='%s :memory:bucket':", dirs[0]) + f, err := fs.NewFs(ctx, fsString) + require.NoError(t, err) + + unionFs := f.(*Fs) + fLocal := unionFs.upstreams[0].Fs + fMemory := unionFs.upstreams[1].Fs + + t.Run("Features", func(t *testing.T) { + assert.NotNil(t, f.Features().Move) + assert.Nil(t, f.Features().Copy) + + // Check underlying are as we are expect + assert.NotNil(t, fLocal.Features().Move) + assert.Nil(t, fLocal.Features().Copy) + assert.Nil(t, fMemory.Features().Move) + assert.NotNil(t, fMemory.Features().Copy) + }) + + // Put a file onto the local fs + contentsLocal := random.String(50) + fileLocal := fstest.NewItem("local.txt", contentsLocal, time.Now()) + _, _ = fstests.PutTestContents(ctx, t, fLocal, &fileLocal, contentsLocal, true) + objLocal, err := f.NewObject(ctx, fileLocal.Path) + require.NoError(t, err) + + // Put a file onto the memory fs + contentsMemory := random.String(60) + fileMemory := fstest.NewItem("memory.txt", contentsMemory, time.Now()) + _, _ = fstests.PutTestContents(ctx, t, fMemory, &fileMemory, contentsMemory, true) + objMemory, err := f.NewObject(ctx, fileMemory.Path) + require.NoError(t, err) + + fstest.CheckListing(t, f, []fstest.Item{fileLocal, fileMemory}) + + t.Run("MoveLocal", func(t *testing.T) { + fileLocal.Path = "local-renamed.txt" + _, err := operations.Move(ctx, f, nil, fileLocal.Path, objLocal) + require.NoError(t, err) + fstest.CheckListing(t, f, []fstest.Item{fileLocal, fileMemory}) + + // Check can retrieve object from union + obj, err := f.NewObject(ctx, fileLocal.Path) + require.NoError(t, err) + assert.Equal(t, fileLocal.Size, obj.Size()) + + // Check can retrieve object from underlying + obj, err = fLocal.NewObject(ctx, fileLocal.Path) + require.NoError(t, err) + assert.Equal(t, fileLocal.Size, obj.Size()) + + t.Run("MoveMemory", func(t *testing.T) { + fileMemory.Path = "memory-renamed.txt" + _, err := operations.Move(ctx, f, nil, fileMemory.Path, objMemory) + require.NoError(t, err) + fstest.CheckListing(t, f, []fstest.Item{fileLocal, fileMemory}) + + // Check can retrieve object from union + obj, err := f.NewObject(ctx, fileMemory.Path) + require.NoError(t, err) + assert.Equal(t, fileMemory.Size, obj.Size()) + + // Check can retrieve object from underlying + obj, err = fMemory.NewObject(ctx, fileMemory.Path) + require.NoError(t, err) + assert.Equal(t, fileMemory.Size, obj.Size()) + }) + }) +} diff --git a/backend/union/union_test.go b/backend/union/union_test.go index 07b3bb230..c58936e9d 100644 --- a/backend/union/union_test.go +++ b/backend/union/union_test.go @@ -2,16 +2,13 @@ package union_test import ( - "fmt" - "io/ioutil" - "os" "testing" _ "github.com/rclone/rclone/backend/local" + _ "github.com/rclone/rclone/backend/memory" + "github.com/rclone/rclone/backend/union" "github.com/rclone/rclone/fstest" "github.com/rclone/rclone/fstest/fstests" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) // TestIntegration runs integration tests against the remote @@ -26,26 +23,11 @@ func TestIntegration(t *testing.T) { }) } -func makeTestDirs(t *testing.T, n int) (dirs []string, clean func()) { - for i := 1; i <= n; i++ { - dir, err := ioutil.TempDir("", fmt.Sprintf("rclone-union-test-%d", n)) - require.NoError(t, err) - dirs = append(dirs, dir) - } - clean = func() { - for _, dir := range dirs { - err := os.RemoveAll(dir) - assert.NoError(t, err) - } - } - return dirs, clean -} - func TestStandard(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnion" @@ -67,7 +49,7 @@ func TestRO(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + ":ro " + dirs[2] + ":ro" name := "TestUnionRO" @@ -89,7 +71,7 @@ func TestNC(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + ":nc " + dirs[2] + ":nc" name := "TestUnionNC" @@ -111,7 +93,7 @@ func TestPolicy1(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy1" @@ -133,7 +115,7 @@ func TestPolicy2(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy2" @@ -155,7 +137,7 @@ func TestPolicy3(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - dirs, clean := makeTestDirs(t, 3) + dirs, clean := union.MakeTestDirs(t, 3) defer clean() upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy3"