Skip to content

Commit

Permalink
Delete existing empty dest folder before rename (#2568)
Browse files Browse the repository at this point in the history
* add delete command for empty folder

* add unit test

* linux test fix

* linux test fix

* add e2e test

* small fix

* small fix

* small fix

* small fix

* using const

* review comments

* remove env files
  • Loading branch information
Tulsishah authored Oct 14, 2024
1 parent 41497db commit 0718955
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 3 deletions.
13 changes: 10 additions & 3 deletions internal/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -2090,20 +2090,27 @@ func (fs *fileSystem) renameHierarchicalDir(ctx context.Context, oldParent inode
return err
}

oldDirName := inode.NewDirName(oldParent.Name(), oldName)
newDirName := inode.NewDirName(newParent.Name(), newName)

// If the call for getBucketDirInode fails it means directory does not exist.
newDirInode, err := fs.getBucketDirInode(ctx, newParent, newName)
if err == nil {
// If the directory exists, then check if it is empty or not.
if err = fs.checkDirNotEmpty(newDirInode, newName); err != nil {
return err
}

// This refers to an empty destination directory.
// The RenameFolder API does not allow renaming to an existing empty directory.
// To make this work, we delete the empty directory first from gcsfuse and then perform rename.
newParent.Lock()
_ = newParent.DeleteChildDir(ctx, newName, false, newDirInode)
newParent.Unlock()
pendingInodes = append(pendingInodes, newDirInode)
}

// Note:The renameDirLimit is not utilized in the folder rename operation because there is no user-defined limit on new renames.

oldDirName := inode.NewDirName(oldParent.Name(), oldName)
newDirName := inode.NewDirName(newParent.Name(), newName)
oldParent.Lock()
defer oldParent.Unlock()

Expand Down
28 changes: 28 additions & 0 deletions internal/fs/hns_bucket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
package fs_test

import (
"fmt"
"os"
"os/exec"
"path"
"strings"
"testing"
Expand Down Expand Up @@ -215,6 +217,32 @@ func (t *HNSBucketTests) TestRenameFolderWithSameParent() {
assert.ElementsMatch(t.T(), actualDirEntries, expectedFooDirEntries)
}

func (t *HNSBucketTests) TestRenameFolderWithExistingEmptyDestDirectory() {
oldDirPath := path.Join(mntDir, "foo", "test")
_, err = os.Stat(oldDirPath)
require.NoError(t.T(), err)
newDirPath := path.Join(mntDir, "foo", "test2")
_, err = os.Stat(newDirPath)
require.NoError(t.T(), err)

// Go's Rename function does not support renaming a directory into an existing empty directory.
// To achieve this, we call a Python rename function as a workaround.
cmd := exec.Command("python", "-c", fmt.Sprintf("import os; os.rename('%s', '%s')", oldDirPath, newDirPath))
_, err = cmd.CombinedOutput()

assert.NoError(t.T(), err)
_, err = os.Stat(oldDirPath)
assert.Error(t.T(), err)
assert.True(t.T(), strings.Contains(err.Error(), "no such file or directory"))
_, err = os.Stat(newDirPath)
assert.NoError(t.T(), err)
dirEntries, err := os.ReadDir(newDirPath)
assert.NoError(t.T(), err)
assert.Equal(t.T(), 1, len(dirEntries))
assert.Equal(t.T(), "file3.txt", dirEntries[0].Name())
assert.False(t.T(), dirEntries[0].IsDir())
}

func (t *HNSBucketTests) TestRenameFolderWithDifferentParents() {
oldDirPath := path.Join(mntDir, "foo")
_, err = os.Stat(oldDirPath)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const DirectoryWithTwoFilesOneNonEmptyDirectory = "directoryWithTwoFilesOneNonEm
const EmptySubDirectory = "emptySubDirectory"
const NonEmptySubDirectory = "nonEmptySubDirectory"
const RenamedDirectory = "renamedDirectory"
const SrcDirectory = "srcDirectory"
const EmptyDestDirectory = "emptyDestDirectory"
const PrefixTempFile = "temp"
const onlyDirMounted = "OnlyDirMountRenameDirLimit"

Expand Down
34 changes: 34 additions & 0 deletions tools/integration_tests/rename_dir_limit/rename_dir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@
package rename_dir_limit_test

import (
"fmt"
"os"
"os/exec"
"path"
"testing"

"github.com/googlecloudplatform/gcsfuse/v2/tools/integration_tests/util/operations"
"github.com/googlecloudplatform/gcsfuse/v2/tools/integration_tests/util/setup"
"github.com/stretchr/testify/assert"
)

// As --rename-directory-limit = 3, and the number of objects in the directory is three,
Expand Down Expand Up @@ -160,3 +163,34 @@ func TestRenameDirectoryWithTwoFilesAndOneNonEmptyDirectory(t *testing.T) {
t.Errorf("Renaming directory succeeded with objects greater than rename-dir-limit.")
}
}

func TestRenameDirectoryWithExistingEmptyDestDirectory(t *testing.T) {
testDir := setup.SetupTestDirectory(DirForRenameDirLimitTests)
// Creating directory structure
// testBucket/dirForRenameDirLimitTests/srcDirectory -- Dir
// testBucket/dirForRenameDirLimitTests/srcDirectory/temp1.txt -- File
// testBucket/dirForRenameDirLimitTests/srcDirectory/NonEmptySubDirectory -- Dir
// testBucket/dirForRenameDirLimitTests/srcDirectory/NonEmptySubDirectory/temp3.txt -- File
oldDirPath := path.Join(testDir, SrcDirectory)
subDirPath := path.Join(oldDirPath, NonEmptySubDirectory)
operations.CreateDirectoryWithNFiles(1, oldDirPath, PrefixTempFile, t)
operations.CreateDirectoryWithNFiles(1, subDirPath, PrefixTempFile, t)
newDirPath := path.Join(testDir, EmptyDestDirectory)
operations.CreateDirectory(newDirPath, t)

// Go's Rename function does not support renaming a directory into an existing empty directory.
// To achieve this, we call a Python rename function as a workaround.
cmd := exec.Command("python", "-c", fmt.Sprintf("import os; os.rename('%s', '%s')", oldDirPath, newDirPath))
_, err := cmd.CombinedOutput()

assert.NoError(t, err)
_, err = os.Stat(newDirPath)
assert.NoError(t, err)
dirEntries, err := os.ReadDir(newDirPath)
assert.NoError(t, err)
assert.Equal(t, 2, len(dirEntries))
assert.Equal(t, NonEmptySubDirectory, dirEntries[0].Name())
assert.True(t, dirEntries[0].IsDir())
assert.Equal(t, "temp1", dirEntries[1].Name())
assert.False(t, dirEntries[1].IsDir())
}

0 comments on commit 0718955

Please sign in to comment.