Nested Folders: Support listing nested folder children (#58566)

* Nested Folders: Support listing nested folder children

* Filter out subfolders with no permissions

* Apply suggestion from code review

pkg/api/dtos/folder.go

@@ -31,4 +31,5 @@ type FolderSearchHit struct {
 	Uid           string                 `json:"uid"`
 	Title         string                 `json:"title"`
 	AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"`
+	ParentUID     string                 `json:"parent_uid,omitempty"`
 }

pkg/api/folder.go

@@ -22,6 +22,7 @@ import (
 // Get all folders.
 //
 // Returns all folders that the authenticated user has permission to view.
+// If nested folders are enabled, it expects an additional query parameter with the parent folder UID.
 //
 // Responses:
 // 200: getFoldersResponse
@@ -29,7 +30,13 @@ import (
 // 403: forbiddenError
 // 500: internalServerError
 func (hs *HTTPServer) GetFolders(c *models.ReqContext) response.Response {
-	folders, err := hs.folderService.GetFolders(c.Req.Context(), c.SignedInUser, c.OrgID, c.QueryInt64("limit"), c.QueryInt64("page"))
+	folders, err := hs.folderService.GetChildren(c.Req.Context(), &folder.GetChildrenQuery{
+		OrgID:        c.OrgID,
+		Limit:        c.QueryInt64("limit"),
+		Page:         c.QueryInt64("page"),
+		UID:          c.Query("parent_uid"),
+		SignedInUser: c.SignedInUser,
+	})
 
 	if err != nil {
 		return apierrors.ToFolderErrorResponse(err)
@@ -38,11 +45,12 @@ func (hs *HTTPServer) GetFolders(c *models.ReqContext) response.Response {
 	uids := make(map[string]bool, len(folders))
 	result := make([]dtos.FolderSearchHit, 0)
 	for _, f := range folders {
-		uids[f.Uid] = true
+		uids[f.UID] = true
 		result = append(result, dtos.FolderSearchHit{
-			Id:    f.Id,
+			Id:        f.ID,
-			Uid:   f.Uid,
+			Uid:       f.UID,
-			Title: f.Title,
+			Title:     f.Title,
+			ParentUID: f.ParentUID,
 		})
 	}
 
@@ -288,6 +296,10 @@ type GetFoldersParams struct {
 	// required:false
 	// default:1
 	Page int64 `json:"page"`
+	// The parent folder UID
+	// in:query
+	// required:false
+	ParentUID string `json:"parent_uid"`
 }
 
 // swagger:parameters getFolderByUID

pkg/api/folder_test.go

@@ -150,7 +150,7 @@ func TestHTTPServer_FolderMetadata(t *testing.T) {
 	})
 
 	t.Run("Should attach access control metadata to multiple folders", func(t *testing.T) {
-		folderService.ExpectedFolders = []*models.Folder{{Uid: "1"}, {Uid: "2"}, {Uid: "3"}}
+		folderService.ExpectedFolders = []*folder.Folder{{UID: "1"}, {UID: "2"}, {UID: "3"}}
 
 		req := server.NewGetRequest("/api/folders?accesscontrol=true")
 		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{

pkg/services/folder/folderimpl/folder.go

@@ -134,28 +134,53 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder.
 	}
 }
 
-func (s *Service) GetFolders(ctx context.Context, user *user.SignedInUser, orgID int64, limit int64, page int64) ([]*models.Folder, error) {
+func (s *Service) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery) ([]*folder.Folder, error) {
+	if cmd.SignedInUser == nil {
+		return nil, folder.ErrBadRequest.Errorf("missing signed in user")
+	}
+
+	if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
+		children, err := s.store.GetChildren(ctx, *cmd)
+		if err != nil {
+			return nil, err
+		}
+
+		filtered := make([]*folder.Folder, 0, len(children))
+		for _, f := range children {
+			g, err := guardian.New(ctx, f.ID, f.OrgID, cmd.SignedInUser)
+			if err != nil {
+				return nil, err
+			}
+			canView, err := g.CanView()
+			if err != nil || canView {
+				filtered = append(filtered, f)
+			}
+		}
+
+		return filtered, nil
+	}
+
 	searchQuery := search.Query{
-		SignedInUser: user,
+		SignedInUser: cmd.SignedInUser,
 		DashboardIds: make([]int64, 0),
 		FolderIds:    make([]int64, 0),
-		Limit:        limit,
+		Limit:        cmd.Limit,
-		OrgId:        orgID,
+		OrgId:        cmd.OrgID,
 		Type:         "dash-folder",
 		Permission:   models.PERMISSION_VIEW,
-		Page:         page,
+		Page:         cmd.Page,
 	}
 
 	if err := s.searchService.SearchHandler(ctx, &searchQuery); err != nil {
 		return nil, err
 	}
 
-	folders := make([]*models.Folder, 0)
+	folders := make([]*folder.Folder, 0)
 
 	for _, hit := range searchQuery.Result {
-		folders = append(folders, &models.Folder{
+		folders = append(folders, &folder.Folder{
-			Id:    hit.ID,
+			ID:    hit.ID,
-			Uid:   hit.UID,
+			UID:   hit.UID,
 			Title: hit.Title,
 		})
 	}
@@ -533,7 +558,7 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e
 		return err
 	}
 
-	folders, err := s.store.GetChildren(ctx, folder.GetTreeQuery{UID: cmd.UID, OrgID: cmd.OrgID})
+	folders, err := s.store.GetChildren(ctx, folder.GetChildrenQuery{UID: cmd.UID, OrgID: cmd.OrgID})
 	if err != nil {
 		return err
 	}
@@ -560,12 +585,6 @@ func (s *Service) GetParents(ctx context.Context, cmd *folder.GetParentsQuery) (
 	return s.store.GetParents(ctx, *cmd)
 }
 
-func (s *Service) GetTree(ctx context.Context, cmd *folder.GetTreeQuery) ([]*folder.Folder, error) {
-	// check the flag, if old - do whatever did before
-	//  for new only the store
-	return s.store.GetChildren(ctx, *cmd)
-}
-
 func (s *Service) MakeUserAdmin(ctx context.Context, orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error {
 	return s.dashboardService.MakeUserAdmin(ctx, orgID, userID, folderID, setViewAndEditPermissions)
 }

pkg/services/folder/folderimpl/folder_test.go

@@ -361,9 +361,30 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
 				UID: "test4",
 			},
 		}
-		res, err := folderService.GetTree(context.Background(),
+
-			&folder.GetTreeQuery{
+		g := guardian.New
-				UID: "test",
+		guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{})
+		t.Cleanup(func() {
+			guardian.New = g
+		})
+
+		res, err := folderService.GetChildren(context.Background(),
+			&folder.GetChildrenQuery{
+				UID:          "test",
+				SignedInUser: usr,
+			})
+		require.NoError(t, err)
+		require.Equal(t, 0, len(res))
+
+		guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanViewValue: true})
+		t.Cleanup(func() {
+			guardian.New = g
+		})
+
+		res, err = folderService.GetChildren(context.Background(),
+			&folder.GetChildrenQuery{
+				UID:          "test",
+				SignedInUser: usr,
 			})
 		require.NoError(t, err)
 		require.Equal(t, 4, len(res))

pkg/services/folder/folderimpl/sqlstore.go

@@ -212,21 +212,28 @@ func (ss *sqlStore) GetParents(ctx context.Context, q folder.GetParentsQuery) ([
 	return util.Reverse(folders[1:]), nil
 }
 
-func (ss *sqlStore) GetChildren(ctx context.Context, q folder.GetTreeQuery) ([]*folder.Folder, error) {
+func (ss *sqlStore) GetChildren(ctx context.Context, q folder.GetChildrenQuery) ([]*folder.Folder, error) {
 	var folders []*folder.Folder
 
 	err := ss.db.WithDbSession(ctx, func(sess *db.Session) error {
 		sql := strings.Builder{}
-		sql.Write([]byte("SELECT * FROM folder WHERE parent_uid=? AND org_id=? ORDER BY id"))
+		args := make([]interface{}, 0, 2)
+		if q.UID == "" {
+			sql.Write([]byte("SELECT * FROM folder WHERE parent_uid IS NULL AND org_id=?"))
+			args = append(args, q.OrgID)
+		} else {
+			sql.Write([]byte("SELECT * FROM folder WHERE parent_uid=? AND org_id=?"))
+			args = append(args, q.UID, q.OrgID)
+		}
 
 		if q.Limit != 0 {
-			var offset int64 = 1
+			var offset int64 = 0
-			if q.Page != 0 {
+			if q.Page > 0 {
-				offset = q.Page
+				offset = q.Limit * (q.Page - 1)
 			}
 			sql.Write([]byte(ss.db.GetDialect().LimitOffset(q.Limit, offset)))
 		}
-		err := sess.SQL(sql.String(), q.UID, q.OrgID).Find(&folders)
+		err := sess.SQL(sql.String(), args...).Find(&folders)
 		if err != nil {
 			return folder.ErrDatabaseError.Errorf("failed to get folder children: %w", err)
 		}
@@ -277,7 +284,7 @@ func (ss *sqlStore) GetHeight(ctx context.Context, foldrUID string, orgID int64,
 			if parentUID != nil && *parentUID == ele {
 				return 0, folder.ErrCircularReference
 			}
-			folders, err := ss.GetChildren(ctx, folder.GetTreeQuery{UID: ele, OrgID: orgID})
+			folders, err := ss.GetChildren(ctx, folder.GetChildrenQuery{UID: ele, OrgID: orgID})
 			if err != nil {
 				return 0, err
 			}

pkg/services/folder/folderimpl/sqlstore_test.go

@@ -177,7 +177,7 @@ func TestIntegrationDelete(t *testing.T) {
 		err := folderStore.Delete(context.Background(), ancestorUIDs[len(ancestorUIDs)-1], orgID)
 		require.NoError(t, err)
 
-		children, err := folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err := folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   ancestorUIDs[len(ancestorUIDs)-2],
 			OrgID: orgID,
 		})
@@ -456,7 +456,7 @@ func TestIntegrationGetChildren(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	treeLeaves := CreateLeaves(t, folderStore, parent, 4)
+	treeLeaves := CreateLeaves(t, folderStore, parent, 8)
 
 	t.Cleanup(func() {
 		for _, uid := range treeLeaves {
@@ -473,7 +473,7 @@ func TestIntegrationGetChildren(t *testing.T) {
 	*/
 
 	t.Run("should successfully get all children", func(t *testing.T) {
-		children, err := folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err := folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   parent.UID,
 			OrgID: orgID,
 		})
@@ -489,12 +489,24 @@ func TestIntegrationGetChildren(t *testing.T) {
 		}
 	})
 
+	t.Run("should default to general folder if UID is missing", func(t *testing.T) {
+		children, err := folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
+			OrgID: orgID,
+		})
+		require.NoError(t, err)
+
+		childrenUIDs := make([]string, 0, len(children))
+		for _, c := range children {
+			childrenUIDs = append(childrenUIDs, c.UID)
+		}
+		assert.Equal(t, []string{parent.UID}, childrenUIDs)
+	})
+
 	t.Run("query with pagination should work as expected", func(t *testing.T) {
-		children, err := folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err := folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   parent.UID,
 			OrgID: orgID,
-			Limit: 1,
+			Limit: 2,
-			Page:  1,
 		})
 		require.NoError(t, err)
 
@@ -503,14 +515,31 @@ func TestIntegrationGetChildren(t *testing.T) {
 			childrenUIDs = append(childrenUIDs, c.UID)
 		}
 
-		if diff := cmp.Diff(treeLeaves[1:2], childrenUIDs); diff != "" {
+		if diff := cmp.Diff(treeLeaves[:2], childrenUIDs); diff != "" {
 			t.Errorf("Result mismatch (-want +got):\n%s", diff)
 		}
 
-		children, err = folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   parent.UID,
 			OrgID: orgID,
-			Limit: 1,
+			Limit: 2,
+			Page:  1,
+		})
+		require.NoError(t, err)
+
+		childrenUIDs = make([]string, 0, len(children))
+		for _, c := range children {
+			childrenUIDs = append(childrenUIDs, c.UID)
+		}
+
+		if diff := cmp.Diff(treeLeaves[:2], childrenUIDs); diff != "" {
+			t.Errorf("Result mismatch (-want +got):\n%s", diff)
+		}
+
+		children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
+			UID:   parent.UID,
+			OrgID: orgID,
+			Limit: 2,
 			Page:  2,
 		})
 		require.NoError(t, err)
@@ -520,12 +549,12 @@ func TestIntegrationGetChildren(t *testing.T) {
 			childrenUIDs = append(childrenUIDs, c.UID)
 		}
 
-		if diff := cmp.Diff(treeLeaves[2:3], childrenUIDs); diff != "" {
+		if diff := cmp.Diff(treeLeaves[2:4], childrenUIDs); diff != "" {
 			t.Errorf("Result mismatch (-want +got):\n%s", diff)
 		}
 
 		// no page is set
-		children, err = folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   parent.UID,
 			OrgID: orgID,
 			Limit: 1,
@@ -537,12 +566,12 @@ func TestIntegrationGetChildren(t *testing.T) {
 			childrenUIDs = append(childrenUIDs, c.UID)
 		}
 
-		if diff := cmp.Diff(treeLeaves[1:2], childrenUIDs); diff != "" {
+		if diff := cmp.Diff(treeLeaves[:1], childrenUIDs); diff != "" {
 			t.Errorf("Result mismatch (-want +got):\n%s", diff)
 		}
 
 		// page is set but limit is not set, it should return them all
-		children, err = folderStore.GetChildren(context.Background(), folder.GetTreeQuery{
+		children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
 			UID:   parent.UID,
 			OrgID: orgID,
 			Page:  1,
@@ -679,7 +708,7 @@ func assertAncestorUIDs(t *testing.T, store *sqlStore, f *folder.Folder, expecte
 func assertChildrenUIDs(t *testing.T, store *sqlStore, f *folder.Folder, expected []string) {
 	t.Helper()
 
-	ancestors, err := store.GetChildren(context.Background(), folder.GetTreeQuery{
+	ancestors, err := store.GetChildren(context.Background(), folder.GetChildrenQuery{
 		UID:   f.UID,
 		OrgID: f.OrgID,
 	})

pkg/services/folder/folderimpl/store.go

@@ -26,7 +26,7 @@ type store interface {
 
 	// GetChildren returns the set of immediate children folders (depth=1) of the
 	// given folder.
-	GetChildren(ctx context.Context, cmd folder.GetTreeQuery) ([]*folder.Folder, error)
+	GetChildren(ctx context.Context, cmd folder.GetChildrenQuery) ([]*folder.Folder, error)
 
 	// GetHeight returns the height of the folder tree. When parentUID is set, the function would
 	// verify in the meanwhile that parentUID is not present in the subtree of the folder with the given UID.

pkg/services/folder/folderimpl/store_fake.go

@@ -48,7 +48,7 @@ func (f *FakeStore) GetParents(ctx context.Context, cmd folder.GetParentsQuery)
 	return f.ExpectedParentFolders, f.ExpectedError
 }
 
-func (f *FakeStore) GetChildren(ctx context.Context, cmd folder.GetTreeQuery) ([]*folder.Folder, error) {
+func (f *FakeStore) GetChildren(ctx context.Context, cmd folder.GetChildrenQuery) ([]*folder.Folder, error) {
 	return f.ExpectedChildFolders, f.ExpectedError
 }
 

pkg/services/folder/foldertest/foldertest.go

@@ -9,7 +9,7 @@ import (
 )
 
 type FakeService struct {
-	ExpectedFolders []*models.Folder
+	ExpectedFolders []*folder.Folder
 	ExpectedFolder  *folder.Folder
 	ExpectedError   error
 }
@@ -20,10 +20,9 @@ func NewFakeService() *FakeService {
 
 var _ folder.Service = (*FakeService)(nil)
 
-func (s *FakeService) GetFolders(ctx context.Context, user *user.SignedInUser, orgID int64, limit int64, page int64) ([]*models.Folder, error) {
+func (s *FakeService) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery) ([]*folder.Folder, error) {
 	return s.ExpectedFolders, s.ExpectedError
 }
-
 func (s *FakeService) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) {
 	return s.ExpectedFolder, s.ExpectedError
 }
@@ -46,32 +45,11 @@ func (s *FakeService) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (
 }
 
 func (s *FakeService) GetParents(ctx context.Context, orgID int64, folderUID string) ([]*folder.Folder, error) {
-	return modelsToFolders(s.ExpectedFolders), s.ExpectedError
+	return s.ExpectedFolders, s.ExpectedError
 }
 
 func (s *FakeService) GetTree(ctx context.Context, orgID int64, folderUID string, depth int64) (map[string][]*folder.Folder, error) {
 	ret := make(map[string][]*folder.Folder)
-	ret[folderUID] = modelsToFolders(s.ExpectedFolders)
+	ret[folderUID] = s.ExpectedFolders
 	return ret, s.ExpectedError
 }
-
-// temporary helper until all Folder service methods are updated to use
-// folder.Folder instead of model.Folder
-func modelsToFolders(m []*models.Folder) []*folder.Folder {
-	if m == nil {
-		return nil
-	}
-	ret := make([]*folder.Folder, len(m))
-	for i, f := range m {
-		ret[i] = &folder.Folder{
-			ID:          f.Id,
-			UID:         f.Uid,
-			Title:       f.Title,
-			Description: "", // model.Folder does not have a description
-			Created:     f.Created,
-			Updated:     f.Updated,
-			//UpdatedBy:   f.UpdatedBy,
-		}
-	}
-	return ret
-}

pkg/services/folder/model.go

@@ -124,10 +124,10 @@ type GetParentsQuery struct {
 	OrgID int64  `xorm:"org_id"`
 }
 
-// GetTreeCommand captures the information required by the folder service to
+// GetChildrenQuery captures the information required by the folder service to
 // return a list of child folders of the given folder.
 
-type GetTreeQuery struct {
+type GetChildrenQuery struct {
 	UID   string `xorm:"uid"`
 	OrgID int64  `xorm:"org_id"`
 	Depth int64
@@ -135,6 +135,8 @@ type GetTreeQuery struct {
 	// Pagination options
 	Limit int64
 	Page  int64
+
+	SignedInUser *user.SignedInUser `json:"-"`
 }
 
 // ToLegacyModel is temporary until the two folder services are merged

pkg/services/folder/service.go

@@ -8,8 +8,8 @@ import (
 )
 
 type Service interface {
-	GetFolders(ctx context.Context, user *user.SignedInUser, orgID int64, limit int64, page int64) ([]*models.Folder, error)
+	// GetChildren returns an array containing all child folders.
-
+	GetChildren(ctx context.Context, cmd *GetChildrenQuery) ([]*Folder, error)
 	Create(ctx context.Context, cmd *CreateFolderCommand) (*Folder, error)
 
 	// GetFolder takes a GetFolderCommand and returns a folder matching the
@@ -45,11 +45,6 @@ type NestedFolderService interface {
 	// node.
 	GetParents(ctx context.Context, cmd *GetParentsQuery) ([]*Folder, error)
 
-	// GetTree returns an map containing all child folders starting from the
+	// GetChildren returns an array containing all child folders.
-	// given parent folder UID and descending to the requested depth. Use the
+	GetChildren(ctx context.Context, cmd *GetChildrenQuery) ([]*Folder, error)
-	// sentinel value -1 to return all child folders.
-	//
-	// The map keys are folder uids and the values are the list of child folders
-	// for that parent.
-	GetTree(ctx context.Context, cmd *GetTreeQuery) ([]*Folder, error)
 }

public/api-merged.json

@@ -5240,7 +5240,7 @@
     },
     "/folders": {
       "get": {
-        "description": "Returns all folders that the authenticated user has permission to view.",
+        "description": "Returns all folders that the authenticated user has permission to view.\nIf nested folders are enabled, it expects an additional query parameter with the parent folder UID.",
         "tags": [
           "folders"
         ],
@@ -5262,6 +5262,12 @@
             "description": "Page index for starting fetching folders",
             "name": "page",
             "in": "query"
+          },
+          {
+            "type": "string",
+            "description": "The parent folder UID",
+            "name": "parent_uid",
+            "in": "query"
           }
         ],
         "responses": {
@@ -13131,6 +13137,9 @@
           "type": "integer",
           "format": "int64"
         },
+        "parent_uid": {
+          "type": "string"
+        },
         "title": {
           "type": "string"
         },

public/api-spec.json

@@ -4568,7 +4568,7 @@
     },
     "/folders": {
       "get": {
-        "description": "Returns all folders that the authenticated user has permission to view.",
+        "description": "Returns all folders that the authenticated user has permission to view.\nIf nested folders are enabled, it expects an additional query parameter with the parent folder UID.",
         "tags": [
           "folders"
         ],
@@ -4590,6 +4590,12 @@
             "description": "Page index for starting fetching folders",
             "name": "page",
             "in": "query"
+          },
+          {
+            "type": "string",
+            "description": "The parent folder UID",
+            "name": "parent_uid",
+            "in": "query"
           }
         ],
         "responses": {
@@ -11871,6 +11877,9 @@
           "type": "integer",
           "format": "int64"
         },
+        "parent_uid": {
+          "type": "string"
+        },
         "title": {
           "type": "string"
         },

public/openapi3.json

@@ -4427,6 +4427,9 @@
             "format": "int64",
             "type": "integer"
           },
+          "parent_uid": {
+            "type": "string"
+          },
           "title": {
             "type": "string"
           },
@@ -15963,7 +15966,7 @@
     },
     "/folders": {
       "get": {
-        "description": "Returns all folders that the authenticated user has permission to view.",
+        "description": "Returns all folders that the authenticated user has permission to view.\nIf nested folders are enabled, it expects an additional query parameter with the parent folder UID.",
         "operationId": "getFolders",
         "parameters": [
           {
@@ -15985,6 +15988,14 @@
               "format": "int64",
               "type": "integer"
             }
+          },
+          {
+            "description": "The parent folder UID",
+            "in": "query",
+            "name": "parent_uid",
+            "schema": {
+              "type": "string"
+            }
           }
         ],
         "responses": {