diff --git a/pkg/services/dashboards/database/database.go b/pkg/services/dashboards/database/database.go index 4126fe1e8b0..9169581424e 100644 --- a/pkg/services/dashboards/database/database.go +++ b/pkg/services/dashboards/database/database.go @@ -952,10 +952,6 @@ func (d *dashboardStore) FindDashboards(ctx context.Context, query *dashboards.F filters = append(filters, searchstore.DashboardIDFilter{IDs: query.DashboardIds}) } - if query.IsStarred { - filters = append(filters, searchstore.StarredFilter{UserId: query.SignedInUser.UserID}) - } - if len(query.Title) > 0 { filters = append(filters, searchstore.TitleFilter{Dialect: d.store.GetDialect(), Title: query.Title}) } diff --git a/pkg/services/dashboards/database/database_test.go b/pkg/services/dashboards/database/database_test.go index 3f36a7c8f39..11265a95267 100644 --- a/pkg/services/dashboards/database/database_test.go +++ b/pkg/services/dashboards/database/database_test.go @@ -20,8 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/search/model" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" - "github.com/grafana/grafana/pkg/services/star" - "github.com/grafana/grafana/pkg/services/star/starimpl" "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" @@ -35,11 +33,9 @@ func TestIntegrationDashboardDataAccess(t *testing.T) { var cfg *setting.Cfg var savedFolder, savedDash, savedDash2 *dashboards.Dashboard var dashboardStore dashboards.Store - var starService star.Service setup := func() { sqlStore, cfg = db.InitTestDBwithCfg(t) - starService = starimpl.ProvideService(sqlStore, cfg) quotaService := quotatest.New(false, nil) var err error dashboardStore, err = ProvideDashboardStore(sqlStore, cfg, testFeatureToggles, tagimpl.ProvideService(sqlStore, cfg), quotaService) @@ -455,39 +451,6 @@ func TestIntegrationDashboardDataAccess(t *testing.T) { require.Equal(t, len(hit2.Tags), 1) }) - t.Run("Should be able to find starred dashboards", func(t *testing.T) { - setup() - starredDash := insertTestDashboard(t, dashboardStore, "starred dash", 1, 0, false) - err := starService.Add(context.Background(), &star.StarDashboardCommand{ - DashboardID: starredDash.ID, - UserID: 10, - }) - require.NoError(t, err) - - err = starService.Add(context.Background(), &star.StarDashboardCommand{ - DashboardID: savedDash.ID, - UserID: 1, - }) - require.NoError(t, err) - - query := dashboards.FindPersistedDashboardsQuery{ - SignedInUser: &user.SignedInUser{ - UserID: 10, - OrgID: 1, - OrgRole: org.RoleEditor, - Permissions: map[int64]map[string][]string{ - 1: {dashboards.ActionDashboardsRead: []string{dashboards.ScopeDashboardsAll}}, - }, - }, - IsStarred: true, - } - res, err := dashboardStore.FindDashboards(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(res), 1) - require.Equal(t, res[0].Title, "starred dash") - }) - t.Run("Can count dashboards by parent folder", func(t *testing.T) { setup() // setup() saves one dashboard in the general folder and two in the "savedFolder". diff --git a/pkg/services/dashboards/models.go b/pkg/services/dashboards/models.go index 4f0192ad1e5..bc57eb36ae4 100644 --- a/pkg/services/dashboards/models.go +++ b/pkg/services/dashboards/models.go @@ -405,7 +405,6 @@ type FindPersistedDashboardsQuery struct { Title string OrgId int64 SignedInUser *user.SignedInUser - IsStarred bool DashboardIds []int64 DashboardUIDs []string Type string diff --git a/pkg/services/guardian/accesscontrol_guardian.go b/pkg/services/guardian/accesscontrol_guardian.go index 1a57bba508f..462b9fcbcbb 100644 --- a/pkg/services/guardian/accesscontrol_guardian.go +++ b/pkg/services/guardian/accesscontrol_guardian.go @@ -224,11 +224,19 @@ func (a *AccessControlDashboardGuardian) CanCreate(folderID int64, isFolder bool func (a *AccessControlDashboardGuardian) evaluate(evaluator accesscontrol.Evaluator) (bool, error) { ok, err := a.ac.Evaluate(a.ctx, a.user, evaluator) if err != nil { - a.log.Debug("Failed to evaluate access control to folder or dashboard", "error", err, "userId", a.user.UserID, "id", a.dashboard.ID) + id := 0 + if a.dashboard != nil { + id = int(a.dashboard.ID) + } + a.log.Debug("Failed to evaluate access control to folder or dashboard", "error", err, "userId", a.user.UserID, "id", id) } if !ok && err == nil { - a.log.Debug("Access denied to folder or dashboard", "userId", a.user.UserID, "id", a.dashboard.ID, "permissions", evaluator.GoString()) + id := 0 + if a.dashboard != nil { + id = int(a.dashboard.ID) + } + a.log.Debug("Access denied to folder or dashboard", "userId", a.user.UserID, "id", id, "permissions", evaluator.GoString()) } return ok, err diff --git a/pkg/services/navtree/navtreeimpl/navtree.go b/pkg/services/navtree/navtreeimpl/navtree.go index 5216bed869f..7888632b0f8 100644 --- a/pkg/services/navtree/navtreeimpl/navtree.go +++ b/pkg/services/navtree/navtreeimpl/navtree.go @@ -345,25 +345,20 @@ func (s *ServiceImpl) buildStarredItemsNavLinks(c *contextmodel.ReqContext) ([]* return nil, err } - starredDashboards := []*dashboards.Dashboard{} - starredDashboardsCounter := 0 - for dashboardId := range starredDashboardResult.UserStars { - // Set a loose limit to the first 50 starred dashboards found - if starredDashboardsCounter > 50 { - break + if len(starredDashboardResult.UserStars) > 0 { + var ids []int64 + for id := range starredDashboardResult.UserStars { + ids = append(ids, id) } - starredDashboardsCounter++ - query := &dashboards.GetDashboardQuery{ - ID: dashboardId, - OrgID: c.OrgID, + starredDashboards, err := s.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardIDs: ids, OrgID: c.OrgID}) + if err != nil { + return nil, err } - queryResult, err := s.dashboardService.GetDashboard(c.Req.Context(), query) - if err == nil { - starredDashboards = append(starredDashboards, queryResult) + // Set a loose limit to the first 50 starred dashboards found + if len(starredDashboards) > 50 { + starredDashboards = starredDashboards[:50] } - } - if len(starredDashboards) > 0 { sort.Slice(starredDashboards, func(i, j int) bool { return starredDashboards[i].Title < starredDashboards[j].Title }) diff --git a/pkg/services/search/service.go b/pkg/services/search/service.go index 64d130023e2..344d4481e74 100644 --- a/pkg/services/search/service.go +++ b/pkg/services/search/service.go @@ -58,10 +58,30 @@ type SearchService struct { } func (s *SearchService) SearchHandler(ctx context.Context, query *Query) error { + starredQuery := star.GetUserStarsQuery{ + UserID: query.SignedInUser.UserID, + } + staredDashIDs, err := s.starService.GetByUser(ctx, &starredQuery) + if err != nil { + return err + } + + // No starred dashboards will be found + if query.IsStarred && len(staredDashIDs.UserStars) == 0 { + query.Result = model.HitList{} + return nil + } + + // filter by starred dashboard IDs when starred dashboards are requested and no UID or ID filters are specified to improve query performance + if query.IsStarred && len(query.DashboardIds) == 0 && len(query.DashboardUIDs) == 0 { + for id := range staredDashIDs.UserStars { + query.DashboardIds = append(query.DashboardIds, id) + } + } + dashboardQuery := dashboards.FindPersistedDashboardsQuery{ Title: query.Title, SignedInUser: query.SignedInUser, - IsStarred: query.IsStarred, DashboardUIDs: query.DashboardUIDs, DashboardIds: query.DashboardIds, Type: query.Type, @@ -85,11 +105,24 @@ func (s *SearchService) SearchHandler(ctx context.Context, query *Query) error { hits = sortedHits(hits) } - if err := s.setStarredDashboards(ctx, query.SignedInUser.UserID, hits); err != nil { - return err + // set starred dashboards + for _, dashboard := range hits { + if _, ok := staredDashIDs.UserStars[dashboard.ID]; ok { + dashboard.IsStarred = true + } } - query.Result = hits + // filter for starred dashboards if requested + if !query.IsStarred { + query.Result = hits + } else { + query.Result = model.HitList{} + for _, dashboard := range hits { + if dashboard.IsStarred { + query.Result = append(query.Result, dashboard) + } + } + } return nil } @@ -106,22 +139,3 @@ func sortedHits(unsorted model.HitList) model.HitList { return hits } - -func (s *SearchService) setStarredDashboards(ctx context.Context, userID int64, hits []*model.Hit) error { - query := star.GetUserStarsQuery{ - UserID: userID, - } - - res, err := s.starService.GetByUser(ctx, &query) - if err != nil { - return err - } - iuserstars := res.UserStars - for _, dashboard := range hits { - if _, ok := iuserstars[dashboard.ID]; ok { - dashboard.IsStarred = true - } - } - - return nil -} diff --git a/pkg/services/search/service_test.go b/pkg/services/search/service_test.go index c0c9245b01d..701b00335e8 100644 --- a/pkg/services/search/service_test.go +++ b/pkg/services/search/service_test.go @@ -62,3 +62,39 @@ func TestSearch_SortedResults(t *testing.T) { assert.Equal(t, "BB", query.Result[3].Tags[1]) assert.Equal(t, "EE", query.Result[3].Tags[2]) } + +func TestSearch_StarredResults(t *testing.T) { + ss := startest.NewStarServiceFake() + db := dbtest.NewFakeDB() + us := usertest.NewUserServiceFake() + ds := dashboards.NewFakeDashboardService(t) + ds.On("SearchDashboards", mock.Anything, mock.AnythingOfType("*dashboards.FindPersistedDashboardsQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*dashboards.FindPersistedDashboardsQuery) + q.Result = model.HitList{ + &model.Hit{ID: 1, Title: "A", Type: "dash-db"}, + &model.Hit{ID: 2, Title: "B", Type: "dash-db"}, + &model.Hit{ID: 3, Title: "C", Type: "dash-db"}, + } + }).Return(nil) + us.ExpectedSignedInUser = &user.SignedInUser{} + ss.ExpectedUserStars = &star.GetUserStarsResult{UserStars: map[int64]bool{1: true, 3: true, 4: true}} + svc := &SearchService{ + sqlstore: db, + starService: ss, + dashboardService: ds, + } + + query := &Query{ + Limit: 2000, + IsStarred: true, + SignedInUser: &user.SignedInUser{}, + } + + err := svc.SearchHandler(context.Background(), query) + require.Nil(t, err) + + // Assert only starred dashboards are returned + assert.Equal(t, 2, query.Result.Len()) + assert.Equal(t, "A", query.Result[0].Title) + assert.Equal(t, "C", query.Result[1].Title) +} diff --git a/pkg/services/sqlstore/searchstore/filters.go b/pkg/services/sqlstore/searchstore/filters.go index 5d82e1720f9..f5cc63e421a 100644 --- a/pkg/services/sqlstore/searchstore/filters.go +++ b/pkg/services/sqlstore/searchstore/filters.go @@ -68,16 +68,6 @@ func (f OrgFilter) Where() (string, []interface{}) { return "dashboard.org_id=?", []interface{}{f.OrgId} } -type StarredFilter struct { - UserId int64 -} - -func (f StarredFilter) Where() (string, []interface{}) { - return `(SELECT count(*) - FROM star - WHERE star.dashboard_id = dashboard.id AND star.user_id = ?) > 0`, []interface{}{f.UserId} -} - type TitleFilter struct { Dialect migrator.Dialect Title string diff --git a/pkg/services/star/api/api.go b/pkg/services/star/api/api.go index ec231596e4c..486a69bc11c 100644 --- a/pkg/services/star/api/api.go +++ b/pkg/services/star/api/api.go @@ -52,22 +52,26 @@ func (api *API) GetStars(c *contextmodel.ReqContext) response.Response { iuserstars, err := api.starService.GetByUser(c.Req.Context(), &query) if err != nil { - return response.Error(500, "Failed to get user stars", err) + return response.Error(http.StatusInternalServerError, "Failed to get user stars", err) } uids := []string{} - for dashboardId := range iuserstars.UserStars { - query := &dashboards.GetDashboardQuery{ - ID: dashboardId, - OrgID: c.OrgID, + if len(iuserstars.UserStars) > 0 { + var ids []int64 + for id := range iuserstars.UserStars { + ids = append(ids, id) + } + starredDashboards, err := api.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardIDs: ids, OrgID: c.OrgID}) + if err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to fetch dashboards", err) } - queryResult, err := api.dashboardService.GetDashboard(c.Req.Context(), query) - // Grafana admin users may have starred dashboards in multiple orgs. This will avoid returning errors when the dashboard is in another org - if err == nil { - uids = append(uids, queryResult.UID) + uids = make([]string, len(starredDashboards)) + for i, dash := range starredDashboards { + uids[i] = dash.UID } } + return response.JSON(200, uids) }