From e69f3c55f7a5fe316ab0b989befd1e91270e6ea5 Mon Sep 17 00:00:00 2001 From: Rafael Bortolon Paulovic Date: Thu, 6 Nov 2025 15:04:34 +0100 Subject: [PATCH] fix: delete folders using postorder (#113493) * fix: delete folders using postorder * chore: use helper function and do not add method to Folder store - addresses other review comments fixing log messages and cleans up the unit tests * chore: run library element tests on modes 2,3,5 only * chore: adjust to folder.SortByPostorder(folders []*Folder) * chore: run library panels tests in mode 2,3,5 only * chore: run tests in all modes and increase timeout - adjusting the modes and tweaking configs will be done separately --- .github/workflows/pr-test-integration.yml | 6 +- pkg/services/folder/folderimpl/folder.go | 5 +- .../folderimpl/folder_unifiedstorage.go | 2 + .../folderimpl/folder_unifiedstorage_test.go | 1 + pkg/services/folder/model.go | 47 ++++++ pkg/services/folder/model_test.go | 153 ++++++++++++++++++ pkg/services/folder/store.go | 2 +- pkg/tests/apis/folder/folders_test.go | 106 +++++++++++- 8 files changed, 314 insertions(+), 8 deletions(-) create mode 100644 pkg/services/folder/model_test.go diff --git a/.github/workflows/pr-test-integration.yml b/.github/workflows/pr-test-integration.yml index 356f7a02b2b..51da3940973 100644 --- a/.github/workflows/pr-test-integration.yml +++ b/.github/workflows/pr-test-integration.yml @@ -68,7 +68,7 @@ jobs: run: | set -euo pipefail readarray -t PACKAGES <<< "$(./scripts/ci/backend-tests/pkgs-with-tests-named.sh -b TestIntegration | ./scripts/ci/backend-tests/shard.sh -N"$SHARD" -d-)" - go test -tags=sqlite -timeout=8m -run '^TestIntegration' "${PACKAGES[@]}" + go test -tags=sqlite -timeout=12m -run '^TestIntegration' "${PACKAGES[@]}" sqlite_nocgo: needs: detect-changes @@ -109,7 +109,7 @@ jobs: # Build regex pattern like: pkg1$|pkg2$|pkg3$ SKIP_PATTERN=$(echo "$SKIP_PACKAGES" | sed '/^$/d' | sed 's|.*|&$|' | paste -sd '|' -) readarray -t PACKAGES <<< "$(./scripts/ci/backend-tests/pkgs-with-tests-named.sh -b TestIntegration | ./scripts/ci/backend-tests/shard.sh -N "$SHARD" -d - | grep -Ev "($SKIP_PATTERN)")" - go test -tags=sqlite -timeout=8m -run '^TestIntegration' "${PACKAGES[@]}" + go test -tags=sqlite -timeout=12m -run '^TestIntegration' "${PACKAGES[@]}" - name: Run profiled tests id: run-profiled-tests if: matrix.shard == 'profiled' @@ -135,7 +135,7 @@ jobs: pkg_name=$(basename "$full_pkg" | tr '/' '_' | tr '.' '_') echo "📦 Running $full_pkg" set +e - go test -tags=sqlite -timeout=8m -run '^TestIntegration' \ + go test -tags=sqlite -timeout=12m -run '^TestIntegration' \ -outputdir=profiles \ -cpuprofile="cpu_${pkg_name}.prof" \ -memprofile="mem_${pkg_name}.prof" \ diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index bad681c60a8..a78d1d76b7c 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -1241,15 +1241,16 @@ func (s *Service) nestedFolderDelete(ctx context.Context, cmd *folder.DeleteFold s.log.ErrorContext(ctx, "failed to get descendant folders", "error", err) return descendantUIDs, err } + descendants = folder.SortByPostorder(descendants) for _, f := range descendants { descendantUIDs = append(descendantUIDs, f.UID) } - s.log.InfoContext(ctx, "deleting folder descendants", "org_id", cmd.OrgID, "uid", cmd.UID) + s.log.InfoContext(ctx, "deleting legacy folder descendants", "org_id", cmd.OrgID, "uid", cmd.UID, "descendantsUIDs", strings.Join(descendantUIDs, ",")) err = s.store.Delete(ctx, descendantUIDs, cmd.OrgID) if err != nil { - s.log.InfoContext(ctx, "failed deleting descendants", "org_id", cmd.OrgID, "parent_uid", cmd.UID, "err", err) + s.log.ErrorContext(ctx, "failed to delete legacy folder descendants", "org_id", cmd.OrgID, "parent_uid", cmd.UID, "descendantsUIDs", strings.Join(descendantUIDs, ","), "err", err) return descendantUIDs, err } return descendantUIDs, nil diff --git a/pkg/services/folder/folderimpl/folder_unifiedstorage.go b/pkg/services/folder/folderimpl/folder_unifiedstorage.go index 1ec9c25aac0..1551d858efe 100644 --- a/pkg/services/folder/folderimpl/folder_unifiedstorage.go +++ b/pkg/services/folder/folderimpl/folder_unifiedstorage.go @@ -618,12 +618,14 @@ func (s *Service) deleteFromApiServer(ctx context.Context, cmd *folder.DeleteFol if err != nil { return err } + descFolders = folder.SortByPostorder(descFolders) folders := []string{} for _, f := range descFolders { folders = append(folders, f.UID) } // must delete children first, then the parent folder + s.log.InfoContext(ctx, "deleting folder with descendants", "org_id", cmd.OrgID, "uid", cmd.UID, "folderUIDs", strings.Join(folders, ",")) folders = append(folders, cmd.UID) if cmd.ForceDeleteRules { diff --git a/pkg/services/folder/folderimpl/folder_unifiedstorage_test.go b/pkg/services/folder/folderimpl/folder_unifiedstorage_test.go index 3952cc67687..0ff7a25b0e9 100644 --- a/pkg/services/folder/folderimpl/folder_unifiedstorage_test.go +++ b/pkg/services/folder/folderimpl/folder_unifiedstorage_test.go @@ -880,6 +880,7 @@ func TestIntegrationDeleteFoldersFromApiServer(t *testing.T) { registry: make(map[string]folder.RegistryService), features: featuremgmt.WithFeatures(), tracer: tracer, + log: slog.New(logtest.NewNopHandler(t)), } user := &user.SignedInUser{OrgID: 1} ctx := identity.WithRequester(context.Background(), user) diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index 5dbfa4e945c..3e59f5c1b6f 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -278,3 +278,50 @@ type GetDescendantCountsQuery struct { } type DescendantCounts map[string]int64 + +// SortByPostorder returns the folders in postorder traversal order. +// That is, children folders appear before their parents in the returned slice. +func SortByPostorder(folders []*Folder) []*Folder { + if len(folders) == 0 { + return folders + } + + // Build parent-to-children map + tree := make(map[string][]*Folder) + folderMap := make(map[string]*Folder) + for _, f := range folders { + folderMap[f.UID] = f + tree[f.ParentUID] = append(tree[f.ParentUID], f) + } + + // Find all roots (folders whose parents are not in the result set) + var roots []*Folder + for _, f := range folders { + if folderMap[f.ParentUID] == nil { + roots = append(roots, f) + } + } + + // Traverse in postorder + result := make([]*Folder, 0, len(folders)) + visited := make(map[string]bool) + var traverse func(f *Folder) + traverse = func(f *Folder) { + if visited[f.UID] { + return + } + visited[f.UID] = true + // First visit all children + for _, child := range tree[f.UID] { + traverse(child) + } + // Then add current folder + result = append(result, f) + } + + for _, root := range roots { + traverse(root) + } + + return result +} diff --git a/pkg/services/folder/model_test.go b/pkg/services/folder/model_test.go new file mode 100644 index 00000000000..0fb747d2a6b --- /dev/null +++ b/pkg/services/folder/model_test.go @@ -0,0 +1,153 @@ +package folder + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFoldersSortByPostorder(t *testing.T) { + t.Run("empty list returns empty list", func(t *testing.T) { + var folders []*Folder + result := SortByPostorder(folders) + require.Empty(t, result) + }) + + t.Run("single folder returns single folder", func(t *testing.T) { + folders := []*Folder{ + {UID: "a", ParentUID: "root"}, + } + result := SortByPostorder(folders) + require.Len(t, result, 1) + require.Equal(t, "a", result[0].UID) + }) + + t.Run("linear hierarchy orders children before parents", func(t *testing.T) { + // Structure: root -> a -> b -> c + folders := []*Folder{ + {UID: "a", ParentUID: "root"}, + {UID: "c", ParentUID: "b"}, + {UID: "b", ParentUID: "a"}, + } + result := SortByPostorder(folders) + require.Len(t, result, 3) + // Postorder: c, b, a (children before parents) + require.Equal(t, "c", result[0].UID) + require.Equal(t, "b", result[1].UID) + require.Equal(t, "a", result[2].UID) + }) + + t.Run("branching hierarchy orders all children before their parent", func(t *testing.T) { + // Structure: + // root + // | + // a + // / | \ + // b c d + folders := []*Folder{ + {UID: "a", ParentUID: "root"}, + {UID: "b", ParentUID: "a"}, + {UID: "c", ParentUID: "a"}, + {UID: "d", ParentUID: "a"}, + } + result := SortByPostorder(folders) + require.Len(t, result, 4) + // 'a' must come after all its children (b, c, d) + aIndex := -1 + for i, f := range result { + if f.UID == "a" { + aIndex = i + break + } + } + require.Equal(t, 3, aIndex, "parent 'a' should be last") + // All children should come before parent + for i := 0; i < 3; i++ { + require.Contains(t, []string{"b", "c", "d"}, result[i].UID) + } + }) + + t.Run("deep hierarchy orders by depth", func(t *testing.T) { + // Structure: + // root + // | + // a + // | + // b + // / \ + // c d + // | + // e + folders := []*Folder{ + {UID: "a", ParentUID: "root"}, + {UID: "b", ParentUID: "a"}, + {UID: "c", ParentUID: "b"}, + {UID: "d", ParentUID: "b"}, + {UID: "e", ParentUID: "c"}, + } + result := SortByPostorder(folders) + require.Len(t, result, 5) + // e should come before c + eIndex := -1 + cIndex := -1 + for i, f := range result { + if f.UID == "e" { + eIndex = i + } + if f.UID == "c" { + cIndex = i + } + } + require.Less(t, eIndex, cIndex, "e should come before c") + // c and d should come before b + bIndex := -1 + dIndex := -1 + for i, f := range result { + if f.UID == "b" { + bIndex = i + } + if f.UID == "d" { + dIndex = i + } + } + require.Less(t, cIndex, bIndex, "c should come before b") + require.Less(t, dIndex, bIndex, "d should come before b") + // b should come before a + aIndex := -1 + for i, f := range result { + if f.UID == "a" { + aIndex = i + } + } + require.Less(t, bIndex, aIndex, "b should come before a") + }) + + t.Run("multiple subtrees maintains postorder per subtree", func(t *testing.T) { + // Structure: + // root1 root2 + // / \ | + // a b c + // | | + // d e + folders := []*Folder{ + {UID: "a", ParentUID: "root1"}, + {UID: "b", ParentUID: "root1"}, + {UID: "d", ParentUID: "b"}, + {UID: "c", ParentUID: "root2"}, + {UID: "e", ParentUID: "c"}, + } + result := SortByPostorder(folders) + require.Len(t, result, 5) + + // Find indices + indices := make(map[string]int) + for i, f := range result { + indices[f.UID] = i + } + + // Check postorder for first subtree: d before b, both before root1 + require.Less(t, indices["d"], indices["b"], "d should come before b") + // Check postorder for second subtree: e before c, both before root2 + require.Less(t, indices["e"], indices["c"], "e should come before c") + }) +} diff --git a/pkg/services/folder/store.go b/pkg/services/folder/store.go index 4ef49d912f4..949d8c4f0ac 100644 --- a/pkg/services/folder/store.go +++ b/pkg/services/folder/store.go @@ -46,7 +46,7 @@ type Store interface { // GetFolders returns folders with given uids GetFolders(ctx context.Context, q GetFoldersFromStoreQuery) ([]*Folder, error) - // GetDescendants returns all descendants of a folder + // GetDescendants returns all descendants of a folder (with no guaranteed order) GetDescendants(ctx context.Context, orgID int64, anchestor_uid string) ([]*Folder, error) // CountInOrg returns the number of folders in the given org diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 39e519f5632..43f5d3221ea 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -1262,8 +1262,7 @@ func TestIntegrationFolderDeletionBlockedByLibraryElements(t *testing.T) { t.Skip("test only on sqlite for now") } - // test on all dualwriter modes - for mode := 0; mode <= 2; mode++ { + for mode := 0; mode <= 5; mode++ { t.Run(fmt.Sprintf("with dual write (unified storage, mode %v, delete blocked by library elements)", grafanarest.DualWriterMode(mode)), func(t *testing.T) { modeDw := grafanarest.DualWriterMode(mode) @@ -1782,3 +1781,106 @@ func TestIntegrationMoveNestedFolderToRootK8S(t *testing.T) { require.Equal(t, "f2", get.Result.UID) require.Equal(t, "", get.Result.ParentUID) } + +// Test deleting nested folders ensures postorder deletion +func TestIntegrationDeleteNestedFoldersPostorder(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + if !db.IsTestDbSQLite() { + t.Skip("test only on sqlite for now") + } + + for mode := 0; mode <= 5; mode++ { + t.Run(fmt.Sprintf("Mode %d: Delete nested folder hierarchy in postorder", mode), func(t *testing.T) { + modeDw := grafanarest.DualWriterMode(mode) + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: true, + DisableAnonymous: true, + APIServerStorageType: "unified", + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + folders.RESOURCEGROUP: { + DualWriterMode: modeDw, + }, + }, + EnableFeatureToggles: []string{ + featuremgmt.FlagUnifiedStorageSearch, + }, + }) + client := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + GVR: gvr, + }) + // Helper function to create a folder and return its UID and ParentUID + createFolder := func(title, uid, parentUid string) (string, string) { + payload := fmt.Sprintf(`{"title":"%s","uid":"%s"%s}`, title, uid, func() string { + if parentUid != "" { + return fmt.Sprintf(`,"parentUid":"%s"`, parentUid) + } + return "" + }()) + create := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(payload), + }, &folder.Folder{}) + require.NotNil(t, create.Result) + require.Equal(t, http.StatusOK, create.Response.StatusCode) + return create.Result.UID, create.Result.ParentUID + } + + // Create a nested folder structure: + // parent + // / \ + // child1 child2 + // | + // grandchild + + // Create parent folder + parentUID, _ := createFolder(fmt.Sprintf("Parent-%d", mode), fmt.Sprintf("parent-%d", mode), "") + + // Create child1 folder + child1UID, child1ParentUID := createFolder(fmt.Sprintf("Child1-%d", mode), fmt.Sprintf("child1-%d", mode), parentUID) + require.Equal(t, parentUID, child1ParentUID) + + // Create child2 folder + child2UID, child2ParentUID := createFolder(fmt.Sprintf("Child2-%d", mode), fmt.Sprintf("child2-%d", mode), parentUID) + require.Equal(t, parentUID, child2ParentUID) + + // Create grandchild folder under child1 + grandchildUID, grandchildParentUID := createFolder(fmt.Sprintf("Grandchild-%d", mode), fmt.Sprintf("grandchild-%d", mode), child1UID) + require.Equal(t, child1UID, grandchildParentUID) + + // Verify the structure before deletion + verifyFolderExists := func(uid string, shouldExist bool) { + _, err := client.Resource.Get(context.Background(), uid, metav1.GetOptions{}) + if shouldExist { + require.NoError(t, err, "folder %s should exist", uid) + } else { + require.Error(t, err, "folder %s should not exist", uid) + } + } + + // All folders should exist + verifyFolderExists(parentUID, true) + verifyFolderExists(child1UID, true) + verifyFolderExists(child2UID, true) + verifyFolderExists(grandchildUID, true) + + // Delete the parent folder - this should trigger postorder deletion + parentDelete := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodDelete, + Path: "/api/folders/" + parentUID, + }, &folder.Folder{}) + require.NotNil(t, parentDelete.Result) + require.Equal(t, http.StatusOK, parentDelete.Response.StatusCode) + + // All folders should now be deleted (postorder deletion: grandchild, child1, child2, parent) + verifyFolderExists(grandchildUID, false) + verifyFolderExists(child1UID, false) + verifyFolderExists(child2UID, false) + verifyFolderExists(parentUID, false) + }) + } +}