Folders: Add max depth check with descendant to /apis (#115305)

pkg/api/apierrors/folder.go

@@ -29,7 +29,8 @@ func ToFolderErrorResponse(err error) response.Response {
 		errors.Is(err, dashboards.ErrDashboardTypeMismatch) ||
 		errors.Is(err, dashboards.ErrDashboardInvalidUid) ||
 		errors.Is(err, dashboards.ErrDashboardUidTooLong) ||
-		errors.Is(err, folder.ErrFolderCannotBeParentOfItself) {
+		errors.Is(err, folder.ErrFolderCannotBeParentOfItself) ||
+		errors.Is(err, folder.ErrMaximumDepthReached) {
 		return response.Error(http.StatusBadRequest, err.Error(), nil)
 	}
 

pkg/api/apierrors/folder_test.go

@@ -30,7 +30,7 @@ func TestToFolderErrorResponse(t *testing.T) {
 		{
 			name:  "maximum depth reached",
 			input: folder.ErrMaximumDepthReached.Errorf("Maximum nested folder depth reached"),
-			want:  response.Err(folder.ErrMaximumDepthReached.Errorf("Maximum nested folder depth reached")),
+			want:  response.Error(http.StatusBadRequest, "[folder.maximum-depth-reached] Maximum nested folder depth reached", nil),
 		},
 		{
 			name:  "bad request errors",

pkg/api/folder.go

@@ -214,7 +214,7 @@ func (hs *HTTPServer) MoveFolder(c *contextmodel.ReqContext) response.Response {
 	cmd.SignedInUser = c.SignedInUser
 	theFolder, err := hs.folderService.Move(c.Req.Context(), &cmd)
 	if err != nil {
-		return response.ErrOrFallback(http.StatusInternalServerError, "move folder failed", err)
+		return apierrors.ToFolderErrorResponse(err)
 	}
 
 	folderDTO, err := hs.newToFolderDto(c, theFolder)

pkg/registry/apis/folders/register.go

@@ -356,7 +356,7 @@ func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes,
 		if !ok {
 			return fmt.Errorf("obj is not folders.Folder")
 		}
-		return validateOnUpdate(ctx, f, old, b.storage, b.parents, folder.MaxNestedFolderDepth)
+		return validateOnUpdate(ctx, f, old, b.storage, b.parents, b.searcher, folder.MaxNestedFolderDepth)
 	default:
 		return nil
 	}

pkg/registry/apis/folders/register_test.go

@@ -376,6 +376,10 @@ func TestFolderAPIBuilder_Validate_Update(t *testing.T) {
 				m.On("Get", mock.Anything, "new-parent", mock.Anything).Return(
 					&folders.Folder{},
 					nil).Once()
+				// also retrieves old parent for depth difference calculation
+				m.On("Get", mock.Anything, "valid-parent", mock.Anything).Return(
+					&folders.Folder{},
+					nil).Once()
 			},
 		},
 		{

pkg/registry/apis/folders/validate.go

@@ -6,6 +6,7 @@ import (
 	"slices"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apiserver/pkg/registry/rest"
 
 	folders "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1beta1"
@@ -13,6 +14,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/dashboards"
 	"github.com/grafana/grafana/pkg/services/folder"
+	"github.com/grafana/grafana/pkg/storage/unified/resource"
 	"github.com/grafana/grafana/pkg/storage/unified/resourcepb"
 	"github.com/grafana/grafana/pkg/util"
 )
@@ -73,6 +75,7 @@ func validateOnUpdate(ctx context.Context,
 	old *folders.Folder,
 	getter rest.Getter,
 	parents parentsGetter,
+	searcher resourcepb.ResourceIndexClient,
 	maxDepth int,
 ) error {
 	folderObj, err := utils.MetaAccessor(obj)
@@ -95,7 +98,10 @@ func validateOnUpdate(ctx context.Context,
 	// Validate the move operation
 	newParent := folderObj.GetFolder()
 
-	// If we move to root, we don't need to validate the depth.
+	// If we move to root, we don't need to validate the depth, because the folder already existed
+	// before and wasn't too deep. This move will make it more shallow.
+	//
+	// We also don't need to validate circular references because the root folder cannot have a parent.
 	if newParent == folder.RootFolderUID {
 		return nil
 	}
@@ -113,9 +119,6 @@ func validateOnUpdate(ctx context.Context,
 	if !ok {
 		return fmt.Errorf("expected folder, found %T", parentObj)
 	}
-
-	//FIXME: until we have a way to represent the tree, we can only
-	// look at folder parents to check how deep the new folder tree will be
 	info, err := parents(ctx, parent)
 	if err != nil {
 		return err
@@ -129,13 +132,162 @@ func validateOnUpdate(ctx context.Context,
 		}
 	}
 
-	// if by moving a folder we exceed the max depth, return an error
+	// if by moving a folder we exceed the max depth just from its parents + itself, return an error
 	if len(info.Items) > maxDepth+1 {
 		return folder.ErrMaximumDepthReached.Errorf("maximum folder depth reached")
 	}
+	// To try to save some computation, get the parents of the old parent (this is typically cheaper
+	// than looking at the children of the folder). If the old parent has more parents or the same
+	// number of parents as the new parent, we can return early, because we know the folder had to be
+	// safe from the creation validation. If we cannot access the older parent, we will continue to check the children.
+	if canSkipChildrenCheck(ctx, oldFolder, getter, parents, len(info.Items)) {
+		return nil
+	}
+
+	// Now comes the more expensive part: we need to check if moving this folder will cause
+	// any descendant folders to exceed the max depth.
+	//
+	// Calculate the maximum allowed subtree depth after the move.
+	allowedDepth := (maxDepth + 1) - len(info.Items)
+	if allowedDepth <= 0 {
+		return nil
+	}
+
+	return checkSubtreeDepth(ctx, searcher, obj.Namespace, obj.Name, allowedDepth, maxDepth)
+}
+
+// canSkipChildrenCheck determines if we can skip the expensive children depth check.
+// If the old parent depth is >= the new parent depth, the folder was already valid
+// and this move won't make descendants exceed max depth.
+func canSkipChildrenCheck(ctx context.Context, oldFolder utils.GrafanaMetaAccessor, getter rest.Getter, parents parentsGetter, newParentDepth int) bool {
+	if oldFolder.GetFolder() == folder.RootFolderUID {
+		return false
+	}
+
+	oldParentObj, err := getter.Get(ctx, oldFolder.GetFolder(), &metav1.GetOptions{})
+	if err != nil {
+		return false
+	}
+
+	oldParent, ok := oldParentObj.(*folders.Folder)
+	if !ok {
+		return false
+	}
+
+	oldInfo, err := parents(ctx, oldParent)
+	if err != nil {
+		return false
+	}
+
+	oldParentDepth := len(oldInfo.Items)
+	levelDifference := newParentDepth - oldParentDepth
+	return levelDifference <= 0
+}
+
+// checkSubtreeDepth uses a hybrid DFS+batching approach:
+// 1. fetches one page of children for the current folder(s)
+// 2. batches all those children into one request to get their children
+// 3. continues depth-first (batching still) until max depth or violation
+// 4. only fetches more siblings after fully exploring current batch
+func checkSubtreeDepth(ctx context.Context, searcher resourcepb.ResourceIndexClient, namespace string, folderUID string, remainingDepth int, maxDepth int) error {
+	if remainingDepth <= 0 {
+		return nil
+	}
+
+	// Start with the folder being moved
+	return checkSubtreeDepthBatched(ctx, searcher, namespace, []string{folderUID}, remainingDepth, maxDepth)
+}
+
+// checkSubtreeDepthBatched checks depth for a batch of folders at the same level
+func checkSubtreeDepthBatched(ctx context.Context, searcher resourcepb.ResourceIndexClient, namespace string, parentUIDs []string, remainingDepth int, maxDepth int) error {
+	if remainingDepth <= 0 || len(parentUIDs) == 0 {
+		return nil
+	}
+
+	const pageSize int64 = 1000
+	var offset int64
+	totalPages := 0
+	hasMore := true
+
+	// Using an upper limit to ensure no infinite loops can happen
+	for hasMore && totalPages < 1000 {
+		totalPages++
+
+		var err error
+		var children []string
+		children, hasMore, err = getChildrenBatch(ctx, searcher, namespace, parentUIDs, pageSize, offset)
+		if err != nil {
+			return fmt.Errorf("failed to get children: %w", err)
+		}
+
+		if len(children) == 0 {
+			return nil
+		}
+
+		// if we are at the last allowed depth and children exist, we will hit the max
+		if remainingDepth == 1 {
+			return folder.ErrMaximumDepthReached.Errorf("maximum folder depth %d would be exceeded after move", maxDepth)
+		}
+
+		if err := checkSubtreeDepthBatched(ctx, searcher, namespace, children, remainingDepth-1, maxDepth); err != nil {
+			return err
+		}
+
+		if !hasMore {
+			return nil
+		}
+
+		offset += pageSize
+	}
+
 	return nil
 }
 
+// getChildrenBatch fetches children for multiple parents
+func getChildrenBatch(ctx context.Context, searcher resourcepb.ResourceIndexClient, namespace string, parentUIDs []string, limit int64, offset int64) ([]string, bool, error) {
+	if len(parentUIDs) == 0 {
+		return nil, false, nil
+	}
+
+	resp, err := searcher.Search(ctx, &resourcepb.ResourceSearchRequest{
+		Options: &resourcepb.ListOptions{
+			Key: &resourcepb.ResourceKey{
+				Namespace: namespace,
+				Group:     folders.FolderResourceInfo.GroupVersionResource().Group,
+				Resource:  folders.FolderResourceInfo.GroupVersionResource().Resource,
+			},
+			Fields: []*resourcepb.Requirement{{
+				Key:      resource.SEARCH_FIELD_FOLDER,
+				Operator: string(selection.In),
+				Values:   parentUIDs,
+			}},
+		},
+		Limit:  limit,
+		Offset: offset,
+	})
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to search folders: %w", err)
+	}
+
+	if resp.Error != nil {
+		return nil, false, fmt.Errorf("search error: %s", resp.Error.Message)
+	}
+
+	if resp.Results == nil || len(resp.Results.Rows) == 0 {
+		return nil, false, nil
+	}
+
+	children := make([]string, 0, len(resp.Results.Rows))
+	for _, row := range resp.Results.Rows {
+		if row.Key != nil {
+			children = append(children, row.Key.Name)
+		}
+	}
+
+	hasMore := resp.Results.NextPageToken != ""
+	return children, hasMore, nil
+}
+
 func validateOnDelete(ctx context.Context,
 	f *folders.Folder,
 	searcher resourcepb.ResourceIndexClient,

pkg/registry/apis/folders/validate_test.go

@@ -282,6 +282,7 @@ func TestValidateUpdate(t *testing.T) {
 		old          *folders.Folder
 		parents      *folders.FolderInfoList
 		parentsError error
+		allFolders   []folders.Folder
 		expectedErr  string
 		maxDepth     int // defaults to 5 unless set
 	}{
@@ -454,6 +455,74 @@ func TestValidateUpdate(t *testing.T) {
 			},
 			expectedErr: "cannot move folder under its own descendant",
 		},
+		{
+			name: "error when moving folder from root to level2 with children exceeds max depth",
+			folder: &folders.Folder{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "folderWithChildren",
+					Annotations: map[string]string{
+						utils.AnnoKeyFolder: "level2",
+					},
+				},
+				Spec: folders.FolderSpec{
+					Title: "folder with children",
+				},
+			},
+			old: &folders.Folder{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "folderWithChildren",
+				},
+				Spec: folders.FolderSpec{
+					Title: "folder with children",
+				},
+			},
+			parents: &folders.FolderInfoList{
+				Items: []folders.FolderInfo{
+					{Name: "level2", Parent: "level1"},
+					{Name: "level1", Parent: folder.GeneralFolderUID},
+					{Name: folder.GeneralFolderUID},
+				},
+			},
+			allFolders: []folders.Folder{
+				{ObjectMeta: metav1.ObjectMeta{Name: "child1", Annotations: map[string]string{utils.AnnoKeyFolder: "folderWithChildren"}}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "grandchild1", Annotations: map[string]string{utils.AnnoKeyFolder: "child1"}}},
+			},
+			maxDepth:    4,
+			expectedErr: "[folder.maximum-depth-reached]",
+		},
+		{
+			name: "can move folder from root level to level1 with children when within max depth",
+			folder: &folders.Folder{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "folderWithChildren",
+					Annotations: map[string]string{
+						utils.AnnoKeyFolder: "level1",
+					},
+				},
+				Spec: folders.FolderSpec{
+					Title: "folder with children",
+				},
+			},
+			old: &folders.Folder{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "folderWithChildren",
+				},
+				Spec: folders.FolderSpec{
+					Title: "folder with children",
+				},
+			},
+			parents: &folders.FolderInfoList{
+				Items: []folders.FolderInfo{
+					{Name: "level1", Parent: folder.GeneralFolderUID},
+					{Name: folder.GeneralFolderUID},
+				},
+			},
+			allFolders: []folders.Folder{
+				{ObjectMeta: metav1.ObjectMeta{Name: "child1", Annotations: map[string]string{utils.AnnoKeyFolder: "folderWithChildren"}}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "grandchild1", Annotations: map[string]string{utils.AnnoKeyFolder: "child1"}}},
+			},
+			maxDepth: 4,
+		},
 	}
 
 	for _, tt := range tests {
@@ -474,11 +543,17 @@ func TestValidateUpdate(t *testing.T) {
 					}, nil).Maybe()
 				}
 			}
+			for i := range tt.allFolders {
+				f := tt.allFolders[i]
+				m.On("Get", context.Background(), f.Name, &metav1.GetOptions{}).Return(&f, nil).Maybe()
+			}
 
 			err := validateOnUpdate(context.Background(), tt.folder, tt.old, m,
 				func(ctx context.Context, folder *folders.Folder) (*folders.FolderInfoList, error) {
 					return tt.parents, tt.parentsError
-				}, maxDepth)
+				},
+				&mockSearchClient{folders: tt.allFolders},
+				maxDepth)
 
 			if tt.expectedErr == "" {
 				require.NoError(t, err)
@@ -693,8 +768,7 @@ type mockSearchClient struct {
 	stats    *resourcepb.ResourceStatsResponse
 	statsErr error
 
-	search    *resourcepb.ResourceSearchResponse
-	searchErr error
+	folders []folders.Folder
 }
 
 // GetStats implements resourcepb.ResourceIndexClient.
@@ -703,8 +777,37 @@ func (m *mockSearchClient) GetStats(ctx context.Context, in *resourcepb.Resource
 }
 
 // Search implements resourcepb.ResourceIndexClient.
-func (m *mockSearchClient) Search(ctx context.Context, in *resourcepb.ResourceSearchRequest, opts ...grpc.CallOption) (*resourcepb.ResourceSearchResponse, error) {
-	return m.search, m.searchErr
+func (m *mockSearchClient) Search(ctx context.Context, req *resourcepb.ResourceSearchRequest, opts ...grpc.CallOption) (*resourcepb.ResourceSearchResponse, error) {
+	// get the list of parents from the search request
+	parentSet := make(map[string]bool)
+	if req.Options != nil && req.Options.Fields != nil {
+		for _, field := range req.Options.Fields {
+			if field.Key == "folder" && field.Operator == "in" {
+				for _, v := range field.Values {
+					parentSet[v] = true
+				}
+			}
+		}
+	}
+
+	// find children that match the parent filter
+	var rows []*resourcepb.ResourceTableRow
+	for i := range m.folders {
+		meta, err := utils.MetaAccessor(&m.folders[i])
+		if err != nil {
+			continue
+		}
+		parentUID := meta.GetFolder()
+		if parentSet[parentUID] {
+			rows = append(rows, &resourcepb.ResourceTableRow{
+				Key: &resourcepb.ResourceKey{Name: m.folders[i].Name},
+			})
+		}
+	}
+
+	return &resourcepb.ResourceSearchResponse{
+		Results: &resourcepb.ResourceTable{Rows: rows},
+	}, nil
 }
 
 // RebuildIndexes implements resourcepb.ResourceIndexClient.

pkg/services/folder/folderimpl/folder_unifiedstorage.go

@@ -726,19 +726,6 @@ func (s *Service) moveOnApiServer(ctx context.Context, cmd *folder.MoveFolderCom
 		return nil, folder.ErrBadRequest.Errorf("k6 project may not be moved")
 	}
 
-	f, err := s.unifiedStore.Get(ctx, folder.GetFolderQuery{
-		UID:          &cmd.UID,
-		OrgID:        cmd.OrgID,
-		SignedInUser: cmd.SignedInUser,
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	if f != nil && f.ParentUID == accesscontrol.K6FolderUID {
-		return nil, folder.ErrBadRequest.Errorf("k6 project may not be moved")
-	}
-
 	// Check that the user is allowed to move the folder to the destination folder
 	hasAccess, evalErr := s.canMoveViaApiServer(ctx, cmd)
 	if evalErr != nil {
@@ -748,30 +735,7 @@ func (s *Service) moveOnApiServer(ctx context.Context, cmd *folder.MoveFolderCom
 		return nil, dashboards.ErrFolderAccessDenied
 	}
 
-	// here we get the folder, we need to get the height of current folder
-	// and the depth of the new parent folder, the sum can't bypass 8
-	folderHeight, err := s.unifiedStore.GetHeight(ctx, cmd.UID, cmd.OrgID, &cmd.NewParentUID)
-	if err != nil {
-		return nil, err
-	}
-	parents, err := s.unifiedStore.GetParents(ctx, folder.GetParentsQuery{UID: cmd.NewParentUID, OrgID: cmd.OrgID})
-	if err != nil {
-		return nil, err
-	}
-
-	// height of the folder that is being moved + this current folder itself + depth of the NewParent folder should be less than or equal MaxNestedFolderDepth
-	if folderHeight+len(parents)+1 > folder.MaxNestedFolderDepth {
-		return nil, folder.ErrMaximumDepthReached.Errorf("failed to move folder")
-	}
-
-	for _, parent := range parents {
-		// if the current folder is already a parent of newparent, we should return error
-		if parent.UID == cmd.UID {
-			return nil, folder.ErrCircularReference.Errorf("failed to move folder")
-		}
-	}
-
-	f, err = s.unifiedStore.Update(ctx, folder.UpdateFolderCommand{
+	f, err := s.unifiedStore.Update(ctx, folder.UpdateFolderCommand{
 		UID:          cmd.UID,
 		OrgID:        cmd.OrgID,
 		NewParentUID: &cmd.NewParentUID,