From c79c768421f48268e57df67987fc593d9bd716ac Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Tue, 18 Mar 2025 04:56:06 -0600 Subject: [PATCH] Folders: Add pagination to list (#102334) --- pkg/registry/apis/folders/legacy_storage.go | 20 ++++-- .../apis/folders/legacy_storage_test.go | 62 +++++++++++++++++++ pkg/services/folder/folderimpl/sqlstore.go | 6 +- .../folder/folderimpl/sqlstore_test.go | 19 +++++- pkg/services/folder/foldertest/foldertest.go | 2 + pkg/services/folder/model.go | 4 ++ 6 files changed, 105 insertions(+), 8 deletions(-) diff --git a/pkg/registry/apis/folders/legacy_storage.go b/pkg/registry/apis/folders/legacy_storage.go index 9ec2d7c98b8..ac34b4c9683 100644 --- a/pkg/registry/apis/folders/legacy_storage.go +++ b/pkg/registry/apis/folders/legacy_storage.go @@ -90,14 +90,22 @@ func (s *legacyStorage) List(ctx context.Context, options *internalversion.ListO return nil, err } - // List must return all folders - hits, err := s.service.GetFoldersLegacy(ctx, folder.GetFoldersQuery{ + query := folder.GetFoldersQuery{ SignedInUser: user, OrgID: orgId, - // TODO: enable pagination - // Limit: paging.page, - // Page: paging.limit, - }) + } + if options.Continue != "" { + query.Page = paging.page + query.Limit = paging.limit + } else if options.Limit > 0 { + query.Limit = options.Limit + query.Page = 1 + // also need to update the paging token so the continue token is correct + paging.limit = options.Limit + paging.page = 1 + } + + hits, err := s.service.GetFoldersLegacy(ctx, query) if err != nil { return nil, err } diff --git a/pkg/registry/apis/folders/legacy_storage_test.go b/pkg/registry/apis/folders/legacy_storage_test.go index eb3c494f49d..824b04030c3 100644 --- a/pkg/registry/apis/folders/legacy_storage_test.go +++ b/pkg/registry/apis/folders/legacy_storage_test.go @@ -2,12 +2,15 @@ package folders import ( "context" + "fmt" "testing" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + "encoding/base64" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" "github.com/grafana/grafana/pkg/services/folder" @@ -57,3 +60,62 @@ func TestLegacyStorageList(t *testing.T) { } require.ElementsMatch(t, uidsFromServiceFolder, uidsReturnedByList) } + +func TestLegacyStorage_List_Pagination(t *testing.T) { + usr := &user.SignedInUser{UserID: 1} + ctx := identity.WithRequester(context.Background(), usr) + folderService := &foldertest.FakeService{} + storage := legacyStorage{ + service: folderService, + namespacer: func(_ int64) string { return "1" }, + } + + t.Run("should set correct continue token", func(t *testing.T) { + options := &metainternalversion.ListOptions{ + Limit: 2, + } + folders := make([]*folder.Folder, 2) + for i := range folders { + folders[i] = &folder.Folder{ + UID: fmt.Sprintf("folder-%d", i), + Title: fmt.Sprintf("Folder %d", i), + } + } + folderService.ExpectedFolders = folders + + result, err := storage.List(ctx, options) + require.NoError(t, err) + + list, ok := result.(*v0alpha1.FolderList) + require.True(t, ok) + token, err := base64.StdEncoding.DecodeString(list.Continue) + require.NoError(t, err) + require.Equal(t, "2|2", string(token)) + require.Equal(t, folderService.LastQuery.Limit, int64(2)) + require.Equal(t, folderService.LastQuery.Page, int64(1)) + }) + + t.Run("should set page to 1 when limit is set without continue token", func(t *testing.T) { + options := &metainternalversion.ListOptions{ + Limit: 2, + } + folders := make([]*folder.Folder, 2) + for i := range folders { + folders[i] = &folder.Folder{ + UID: fmt.Sprintf("folder-%d", i), + Title: fmt.Sprintf("Folder %d", i), + } + } + folderService.ExpectedFolders = folders + + result, err := storage.List(ctx, options) + require.NoError(t, err) + list, ok := result.(*v0alpha1.FolderList) + require.True(t, ok) + token, err := base64.StdEncoding.DecodeString(list.Continue) + require.NoError(t, err) + require.Equal(t, "2|2", string(token)) + require.Equal(t, int64(2), folderService.LastQuery.Limit) + require.Equal(t, int64(1), folderService.LastQuery.Page) + }) +} diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index 13a9943b7b2..14601344a82 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -498,7 +498,11 @@ func (ss *FolderStoreImpl) GetFolders(ctx context.Context, q folder.GetFoldersFr } if len(q.AncestorUIDs) == 0 { - if q.OrderByTitle { + if q.Limit > 0 { + s.WriteString(` ORDER BY f0.title ASC`) + s.WriteString(` LIMIT ? OFFSET ?`) + args = append(args, q.Limit, (q.Page-1)*q.Limit) + } else if q.OrderByTitle { s.WriteString(` ORDER BY f0.title ASC`) } diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index aff18e4e3d7..7d5e8a8bad1 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -878,7 +878,7 @@ func TestIntegrationGetFolders(t *testing.T) { for i := 0; i < foldersNum; i++ { uid := util.GenerateShortUID() f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{ - Title: folderTitle, + Title: folderTitle + fmt.Sprintf("-%d", i), Description: folderDsc, OrgID: orgID, UID: uid, @@ -982,6 +982,23 @@ func TestIntegrationGetFolders(t *testing.T) { assert.NotEmpty(t, actualFolder.Updated) assert.NotEmpty(t, actualFolder.URL) }) + + t.Run("get folders with limit and page should work as expected", func(t *testing.T) { + q := folder.NewGetFoldersQuery(folder.GetFoldersQuery{ + OrgID: orgID, + UIDs: uids, + Limit: 3, + Page: 2, + }) + + actualFolders, err := folderStore.GetFolders(context.Background(), q) + require.NoError(t, err) + assert.Equal(t, 3, len(actualFolders)) + + for i, actualFolder := range actualFolders { + assert.Equal(t, fmt.Sprintf("folder1-%d", i+3), actualFolder.Title) + } + }) } func CreateOrg(t *testing.T, db db.DB, cfg *setting.Cfg) int64 { diff --git a/pkg/services/folder/foldertest/foldertest.go b/pkg/services/folder/foldertest/foldertest.go index 7a5edf096e6..43df5478980 100644 --- a/pkg/services/folder/foldertest/foldertest.go +++ b/pkg/services/folder/foldertest/foldertest.go @@ -13,6 +13,7 @@ type FakeService struct { ExpectedHitList model.HitList ExpectedError error ExpectedDescendantCounts map[string]int64 + LastQuery folder.GetFoldersQuery } func NewFakeService() *FakeService { @@ -90,5 +91,6 @@ func (s *FakeService) SearchFolders(ctx context.Context, q folder.SearchFoldersQ } func (s *FakeService) GetFoldersLegacy(ctx context.Context, q folder.GetFoldersQuery) ([]*folder.Folder, error) { + s.LastQuery = q return s.ExpectedFolders, s.ExpectedError } diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index ffd947c6737..73544a08713 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -175,6 +175,10 @@ type GetFoldersQuery struct { WithFullpathUIDs bool BatchSize uint64 + // Pagination options + Limit int64 + Page int64 + // OrderByTitle is used to sort the folders by title // Set to true when ordering is meaningful (used for listing folders) // otherwise better to keep it false since ordering can have a performance impact