From c7a7ebd3e0894b9728e8dd2ac8826044f2a7754d Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Mon, 23 Jan 2023 14:09:09 +0200 Subject: [PATCH] Chore: Drop search service dependency from folder service (#61789) * Chore: Drop search service dependency from folder service --- pkg/api/common_test.go | 1 + pkg/api/folder.go | 53 +++++++++++-- pkg/api/folder_test.go | 3 + pkg/services/folder/folderimpl/folder.go | 75 ++++++------------- pkg/services/folder/folderimpl/folder_test.go | 3 +- .../libraryelements/libraryelements_test.go | 4 +- .../librarypanels/librarypanels_test.go | 4 +- pkg/services/ngalert/tests/util.go | 2 +- 8 files changed, 76 insertions(+), 69 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index c4dfeda1659..bb96b785526 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -504,6 +504,7 @@ func SetupAPITestServer(t *testing.T, opts ...APITestServerOption) *webtest.Serv } hs.registerRoutes() + s := webtest.NewServer(t, hs.RouteRegister) return s } diff --git a/pkg/api/folder.go b/pkg/api/folder.go index ae5093e04c8..1a5f35f11bd 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/libraryelements" + "github.com/grafana/grafana/pkg/services/search" "github.com/grafana/grafana/pkg/web" ) @@ -22,7 +23,8 @@ 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. +// If nested folders are enabled, it expects an additional query parameter with the parent folder UID +// and returns the immediate subfolders. // // Responses: // 200: getFoldersResponse @@ -30,13 +32,19 @@ import ( // 403: forbiddenError // 500: internalServerError func (hs *HTTPServer) GetFolders(c *models.ReqContext) response.Response { - 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, - }) + var folders []*folder.Folder + var err error + if hs.Features.IsEnabled(featuremgmt.FlagNestedFolders) { + 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, + }) + } else { + folders, err = hs.searchFolders(c) + } if err != nil { return apierrors.ToFolderErrorResponse(err) @@ -288,6 +296,35 @@ func (hs *HTTPServer) newToFolderDto(c *models.ReqContext, g guardian.DashboardG } } +func (hs *HTTPServer) searchFolders(c *models.ReqContext) ([]*folder.Folder, error) { + searchQuery := search.Query{ + SignedInUser: c.SignedInUser, + DashboardIds: make([]int64, 0), + FolderIds: make([]int64, 0), + Limit: c.QueryInt64("limit"), + OrgId: c.OrgID, + Type: "dash-folder", + Permission: models.PERMISSION_VIEW, + Page: c.QueryInt64("page"), + } + + if err := hs.SearchService.SearchHandler(c.Req.Context(), &searchQuery); err != nil { + return nil, err + } + + folders := make([]*folder.Folder, 0) + + for _, hit := range searchQuery.Result { + folders = append(folders, &folder.Folder{ + ID: hit.ID, + UID: hit.UID, + Title: hit.Title, + }) + } + + return folders, nil +} + // swagger:parameters getFolders type GetFoldersParams struct { // Limit the maximum number of folders to return diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 4dc0fffc9bb..620e8c53659 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -149,6 +149,9 @@ func TestHTTPServer_FolderMetadata(t *testing.T) { hs.folderService = folderService hs.AccessControl = acmock.New() hs.QuotaService = quotatest.New(false, nil) + hs.SearchService = &mockSearchService{ + ExpectedResult: models.HitList{}, + } }) t.Run("Should attach access control metadata to multiple folders", func(t *testing.T) { diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 7917beff553..f84e8f7565e 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -22,7 +22,6 @@ import ( "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/services/search" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -34,7 +33,6 @@ type Service struct { cfg *setting.Cfg dashboardStore dashboards.Store dashboardFolderStore dashboards.FolderStore - searchService *search.SearchService features featuremgmt.FeatureToggles permissions accesscontrol.FolderPermissionsService accessControl accesscontrol.AccessControl @@ -52,7 +50,6 @@ func ProvideService( db db.DB, // DB for the (new) nested folder store features featuremgmt.FeatureToggles, folderPermissionsService accesscontrol.FolderPermissionsService, - searchService *search.SearchService, ) folder.Service { ac.RegisterScopeAttributeResolver(dashboards.NewFolderNameScopeResolver(dashboardStore, folderStore)) ac.RegisterScopeAttributeResolver(dashboards.NewFolderIDScopeResolver(dashboardStore, folderStore)) @@ -63,7 +60,6 @@ func ProvideService( dashboardStore: dashboardStore, dashboardFolderStore: folderStore, store: store, - searchService: searchService, features: features, permissions: folderPermissionsService, accessControl: ac, @@ -160,62 +156,33 @@ func (s *Service) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery) 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 { - // fetch folder from dashboard store - dashFolder, err := s.dashboardFolderStore.GetFolderByUID(ctx, f.OrgID, f.UID) - if err != nil { - s.log.Error("failed to fetch folder by UID: %s from dashboard store", f.UID, err) - continue - } - - 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 { - // always expose the dashboard store sequential ID - f.ID = dashFolder.ID - filtered = append(filtered, f) - } - } - - return filtered, nil - } - - searchQuery := search.Query{ - SignedInUser: cmd.SignedInUser, - DashboardIds: make([]int64, 0), - FolderIds: make([]int64, 0), - Limit: cmd.Limit, - OrgId: cmd.OrgID, - Type: "dash-folder", - Permission: models.PERMISSION_VIEW, - Page: cmd.Page, - } - - if err := s.searchService.SearchHandler(ctx, &searchQuery); err != nil { + children, err := s.store.GetChildren(ctx, *cmd) + if err != nil { return nil, err } - folders := make([]*folder.Folder, 0) + filtered := make([]*folder.Folder, 0, len(children)) + for _, f := range children { + // fetch folder from dashboard store + dashFolder, err := s.dashboardFolderStore.GetFolderByUID(ctx, f.OrgID, f.UID) + if err != nil { + s.log.Error("failed to fetch folder by UID: %s from dashboard store", f.UID, err) + continue + } - for _, hit := range searchQuery.Result { - folders = append(folders, &folder.Folder{ - ID: hit.ID, - UID: hit.UID, - Title: hit.Title, - }) + 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 { + // always expose the dashboard store sequential ID + f.ID = dashFolder.ID + filtered = append(filtered, f) + } } - return folders, nil + return filtered, nil } func (s *Service) getFolderByID(ctx context.Context, user *user.SignedInUser, id int64, orgID int64) (*folder.Folder, error) { diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index c2455f5c448..4c27d2c5f87 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -38,7 +38,7 @@ func TestIntegrationProvideFolderService(t *testing.T) { t.Run("should register scope resolvers", func(t *testing.T) { cfg := setting.NewCfg() ac := acmock.New() - ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, nil, nil, nil, &featuremgmt.FeatureManager{}, nil, nil) + ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, nil, nil, nil, &featuremgmt.FeatureManager{}, nil) require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 2) }) @@ -66,7 +66,6 @@ func TestIntegrationFolderService(t *testing.T) { dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, - searchService: nil, features: features, permissions: folderPermissions, bus: bus.ProvideBus(tracing.InitializeTracerForTest()), diff --git a/pkg/services/libraryelements/libraryelements_test.go b/pkg/services/libraryelements/libraryelements_test.go index fc5cf88969a..c3d47e8d66f 100644 --- a/pkg/services/libraryelements/libraryelements_test.go +++ b/pkg/services/libraryelements/libraryelements_test.go @@ -314,7 +314,7 @@ func createFolderWithACL(t *testing.T, sqlStore db.DB, title string, user user.S dashboardStore, err := database.ProvideDashboardStore(sqlStore, cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sqlStore, cfg), quotaService) require.NoError(t, err) - s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions, nil) + s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions) t.Logf("Creating folder with title and UID %q", title) ctx := appcontext.WithUser(context.Background(), &user) folder, err := s.Create(ctx, &folder.CreateFolderCommand{ @@ -442,7 +442,7 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo service := LibraryElementService{ Cfg: sqlStore.Cfg, SQLStore: sqlStore, - folderService: folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), sqlStore.Cfg, dashboardStore, dashboardStore, nil, features, folderPermissions, nil), + folderService: folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), sqlStore.Cfg, dashboardStore, dashboardStore, nil, features, folderPermissions), } // deliberate difference between signed in user and user in db to make it crystal clear diff --git a/pkg/services/librarypanels/librarypanels_test.go b/pkg/services/librarypanels/librarypanels_test.go index cc82aba2989..a01af5e1fcf 100644 --- a/pkg/services/librarypanels/librarypanels_test.go +++ b/pkg/services/librarypanels/librarypanels_test.go @@ -726,7 +726,7 @@ func createFolderWithACL(t *testing.T, sqlStore db.DB, title string, user *user. quotaService := quotatest.New(false, nil) dashboardStore, err := database.ProvideDashboardStore(sqlStore, cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sqlStore, cfg), quotaService) require.NoError(t, err) - s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions, nil) + s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions) t.Logf("Creating folder with title and UID %q", title) ctx := appcontext.WithUser(context.Background(), user) @@ -835,7 +835,7 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo ac := acmock.New() folderPermissions := acmock.NewMockedPermissionsService() - folderService := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions, nil) + folderService := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, dashboardStore, dashboardStore, nil, features, folderPermissions) elementService := libraryelements.ProvideService(cfg, sqlStore, routing.NewRouteRegister(), folderService) service := LibraryPanelService{ diff --git a/pkg/services/ngalert/tests/util.go b/pkg/services/ngalert/tests/util.go index a14b0083f60..28c7a55e4e0 100644 --- a/pkg/services/ngalert/tests/util.go +++ b/pkg/services/ngalert/tests/util.go @@ -85,7 +85,7 @@ func SetupTestEnv(tb testing.TB, baseInterval time.Duration) (*ngalert.AlertNG, tracer := tracing.InitializeTracerForTest() bus := bus.ProvideBus(tracer) - folderService := folderimpl.ProvideService(ac, bus, cfg, dashboardStore, dashboardStore, nil, features, folderPermissions, nil) + folderService := folderimpl.ProvideService(ac, bus, cfg, dashboardStore, dashboardStore, nil, features, folderPermissions) ng, err := ngalert.ProvideService( cfg, featuremgmt.WithFeatures(), nil, nil, routing.NewRouteRegister(), sqlStore, nil, nil, nil, quotatest.New(false, nil),