Recursively delete empty parent folders

localfs/emptydir_test.go

@@ -0,0 +1,111 @@
+package localfs
+
+import (
+	"os"
+	"path"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDirectoryCleanup(t *testing.T) {
+	testDir, err := os.MkdirTemp("/tmp", "dirtest")
+	require.NoError(t, err)
+	defer os.RemoveAll(testDir)
+
+	makeDummyFile := func(filePath string) string {
+		fullPath := path.Join(testDir, filePath)
+		dir := path.Dir(fullPath)
+		require.NotEmpty(t, dir)
+		err := os.MkdirAll(dir, 0755)
+		require.NoError(t, err)
+		err = os.WriteFile(fullPath, []byte("don't delete this folder"), 0755)
+		require.NoError(t, err)
+		return fullPath
+	}
+
+	fileExists := func(filePath string) bool {
+		_, err := os.Stat(filePath)
+		if err == nil {
+			return true
+		}
+		if os.IsNotExist(err) {
+			return false
+		}
+		require.FailNow(t, "failed to get status of file %s", filePath)
+		return false
+	}
+
+	require.False(t, fileExists("/heythisdoesntexist/overhere"))
+
+	// /testDir
+	//   a/
+	//     dummyfile3
+	//     b/
+	//        c/
+	//           dummyfile1
+	//           dummyfile2
+	//     d/
+	//           dummyfile4
+
+	d1 := makeDummyFile("a/b/c/dummyfile1")
+	d2 := makeDummyFile("a/b/c/dummyfile2")
+	d3 := makeDummyFile("a/dummyfile3")
+	d4 := makeDummyFile("a/d/dummyfile4")
+
+	l := &LocalStore{}
+
+	t.Run("delete-nonempty-dir", func(t *testing.T) {
+		err = l.deleteParentDirs(path.Join(testDir, "a/d"))
+		require.NoError(t, err)
+		require.True(t, fileExists(d1))
+		require.True(t, fileExists(d2))
+		require.True(t, fileExists(d3))
+		require.True(t, fileExists(d4))
+	})
+
+	t.Run("delete-nonempty-nested-child-dir", func(t *testing.T) {
+		err = l.deleteParentDirs(path.Join(testDir, "a/b/c"))
+		require.NoError(t, err)
+		require.True(t, fileExists(d1))
+		require.True(t, fileExists(d2))
+		require.True(t, fileExists(d3))
+		require.True(t, fileExists(d4))
+	})
+
+	t.Run("delete-nonempty-nested-parent-dir", func(t *testing.T) {
+		err = l.deleteParentDirs(path.Join(testDir, "a/b"))
+		require.NoError(t, err)
+		require.True(t, fileExists(d1))
+		require.True(t, fileExists(d2))
+		require.True(t, fileExists(d3))
+		require.True(t, fileExists(d4))
+	})
+
+	require.NoError(t, os.Remove(d4))
+
+	t.Run("delete-empty-dir", func(t *testing.T) {
+		err = l.deleteParentDirs(d4)
+		require.NoError(t, err)
+		require.True(t, fileExists(d1))
+		require.True(t, fileExists(d2))
+		require.True(t, fileExists(d3))
+		require.False(t, fileExists(d4))
+		require.False(t, fileExists(path.Join(testDir, "a/d")))
+	})
+
+	require.NoError(t, os.Remove(d1))
+	require.NoError(t, os.Remove(d2))
+
+	t.Run("delete-empty-nested-dir", func(t *testing.T) {
+		err = l.deleteParentDirs(d2)
+		require.NoError(t, err)
+		require.False(t, fileExists(d1))
+		require.False(t, fileExists(d2))
+		require.False(t, fileExists(path.Join(testDir, "a/b/c")))
+		require.False(t, fileExists(path.Join(testDir, "a/b")))
+		require.True(t, fileExists(d3))
+		require.False(t, fileExists(d4))
+		require.False(t, fileExists(path.Join(testDir, "a/d")))
+	})
+}

localfs/store.go

@@ -280,31 +280,46 @@ func (l *LocalStore) Get(ctx context.Context, o string) (cloudstorage.Object, er
 func (l *LocalStore) Delete(ctx context.Context, obj string) error {
 	fo := path.Join(l.storepath, obj)
 	if err := os.Remove(fo); err != nil {
-		return fmt.Errorf("removing dir=%s: %w", fo, err)
+		return fmt.Errorf("removing file=%s: %w", fo, err)
 	}
 	mf := fo + ".metadata"
 	if cloudstorage.Exists(mf) {
 		if err := os.Remove(mf); err != nil {
-			return fmt.Errorf("removing dir=%s: %w", mf, err)
+			return fmt.Errorf("removing file=%s: %w", mf, err)
 		}
 	}
 
 	// When the last item in a folder is deleted, the folder
 	// should also be deleted. This matches the behavior in GCS.
-	dir, err := os.Open(l.storepath)
-	if err != nil {
-		return fmt.Errorf("failed to open store dir=%s err=%w", l.storepath, err)
-	}
-	if _, err = dir.Readdirnames(1); errors.Is(err, io.EOF) {
-		dir.Close()
-		// it's empty, so remove it.
-		if err := os.Remove(l.storepath); err != nil {
-			return fmt.Errorf("failed to remove store dir=%s err=%w", l.storepath, err)
+	return l.deleteParentDirs(fo)
+}
+
+// deleteParentDirs deletes all the parent dirs of some filepath
+// if those dirs are empty.
+func (l *LocalStore) deleteParentDirs(filePath string) error {
+
+	for dirName := path.Dir(filePath); len(dirName) > 0; dirName = path.Dir(dirName) {
+		dir, err := os.Open(dirName)
+		if os.IsNotExist(err) {
+			// it's already deleted; nothing to do.
+			return nil
+		}
+		if err != nil {
+			return fmt.Errorf("failed to open store dir=%s err=%w", dirName, err)
+		}
+		files, err := dir.Readdirnames(1)
+		if len(files) == 0 && errors.Is(err, io.EOF) {
+			dir.Close()
+			// it's empty, so remove it.
+			if err := os.Remove(dirName); err != nil {
+				return fmt.Errorf("failed to remove store dir=%s err=%w", dirName, err)
+			}
+		} else {
+			dir.Close()
+			// it's not empty.
+			return nil
 		}
-	} else {
-		dir.Close()
 	}
-
 	return nil
 }
 

testutils/testutils.go

@@ -213,6 +213,11 @@ func BasicRW(t TestingT, store cloudstorage.Store) {
 	assert.Equal(t, cloudstorage.ErrObjectNotFound, err)
 	assert.Equal(t, nil, obj)
 
+	// Store should be empty again
+	all, err = store.List(context.Background(), cloudstorage.NewQueryAll())
+	assert.NoError(t, err)
+	assert.NotNil(t, all)
+	assert.Empty(t, all.Objects)
 }
 
 func createFile(t TestingT, store cloudstorage.Store, name, data string) cloudstorage.Object {