diff --git a/pkg/api/dtos/folder.go b/pkg/api/dtos/folder.go index 4a5536ac0dd..54b96b603f8 100644 --- a/pkg/api/dtos/folder.go +++ b/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"` } diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 74dd028b0b9..c72c9e22b63 100644 --- a/pkg/api/folder.go +++ b/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, - Uid: f.Uid, - Title: f.Title, + Id: f.ID, + Uid: f.UID, + 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 diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index b1ae041e5ff..4de90331a16 100644 --- a/pkg/api/folder_test.go +++ b/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{ diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 338c5008ca9..e4970114a8c 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/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, - OrgId: orgID, + Limit: cmd.Limit, + 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{ - Id: hit.ID, - Uid: hit.UID, + folders = append(folders, &folder.Folder{ + ID: hit.ID, + 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) } diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index ea55e054ae9..d337d732377 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/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{ - UID: "test", + + g := guardian.New + 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)) diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index 2b3e334a025..81358e3234c 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/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 - if q.Page != 0 { - offset = q.Page + var offset int64 = 0 + if q.Page > 0 { + 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 } diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index f66c4636eb0..05843c3df62 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/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, - Page: 1, + Limit: 2, }) 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, }) diff --git a/pkg/services/folder/folderimpl/store.go b/pkg/services/folder/folderimpl/store.go index 902fddd7cfe..6155c58bb51 100644 --- a/pkg/services/folder/folderimpl/store.go +++ b/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. diff --git a/pkg/services/folder/folderimpl/store_fake.go b/pkg/services/folder/folderimpl/store_fake.go index 25c39096e20..1810b1f18d4 100644 --- a/pkg/services/folder/folderimpl/store_fake.go +++ b/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 } diff --git a/pkg/services/folder/foldertest/foldertest.go b/pkg/services/folder/foldertest/foldertest.go index ff62be316eb..7380ccb531b 100644 --- a/pkg/services/folder/foldertest/foldertest.go +++ b/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 -} diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index 8dfc80e3b53..dae038f4012 100644 --- a/pkg/services/folder/model.go +++ b/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 diff --git a/pkg/services/folder/service.go b/pkg/services/folder/service.go index 05c20196ff5..760232335cf 100644 --- a/pkg/services/folder/service.go +++ b/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 - // given parent folder UID and descending to the requested depth. Use the - // 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) + // GetChildren returns an array containing all child folders. + GetChildren(ctx context.Context, cmd *GetChildrenQuery) ([]*Folder, error) } diff --git a/public/api-merged.json b/public/api-merged.json index 482a1d14b0a..aadd50031fa 100644 --- a/public/api-merged.json +++ b/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" }, diff --git a/public/api-spec.json b/public/api-spec.json index 1a16ebff937..5114eaa198b 100644 --- a/public/api-spec.json +++ b/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" }, diff --git a/public/openapi3.json b/public/openapi3.json index 31fc5f1bca7..f66dd7e8f50 100644 --- a/public/openapi3.json +++ b/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": {