Search: Improvements for starred dashboard search (#64758)

* improvements for starred dashboard search

* fix workflows for the case when no dashboards are starred

* PR feedback (don't query DB if starred dashboards and requested but no starred IDs are found) and linting

* return empty list not null in case of no starred dashboards

* return empty list not null in case of no starred dashboards pt 2

* return empty list not null in case of no starred dashboards pt 3

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})
 	}

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".

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

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

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 {
+	if len(starredDashboardResult.UserStars) > 0 {
+		var ids []int64
+		for id := range starredDashboardResult.UserStars {
+			ids = append(ids, id)
+		}
+		starredDashboards, err := s.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardIDs: ids, OrgID: c.OrgID})
+		if err != nil {
+			return nil, err
+		}
 		// Set a loose limit to the first 50 starred dashboards found
-		if starredDashboardsCounter > 50 {
-			break
+		if len(starredDashboards) > 50 {
+			starredDashboards = starredDashboards[:50]
 		}
-		starredDashboardsCounter++
-		query := &dashboards.GetDashboardQuery{
-			ID:    dashboardId,
-			OrgID: c.OrgID,
-		}
-		queryResult, err := s.dashboardService.GetDashboard(c.Req.Context(), query)
-		if err == nil {
-			starredDashboards = append(starredDashboards, queryResult)
-		}
-	}
 
-	if len(starredDashboards) > 0 {
 		sort.Slice(starredDashboards, func(i, j int) bool {
 			return starredDashboards[i].Title < starredDashboards[j].Title
 		})

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
-}

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)
+}

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

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)
 }