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
This commit is contained in:
Rafael Bortolon Paulovic
@@ -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" \
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user