From b12c731d597c8e0464dcd3fa7be94c597826903f Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Fri, 15 Nov 2024 08:51:31 -0700 Subject: [PATCH] Stars: Add dashboard_uid and org_id to table (#96408) --- docs/sources/developers/http_api/user.md | 8 +- pkg/api/folder_bench_test.go | 2 +- pkg/services/navtree/navtreeimpl/navtree.go | 8 +- pkg/services/org/orgimpl/store.go | 2 +- pkg/services/search/service.go | 6 +- pkg/services/search/service_test.go | 20 ++-- .../sqlstore/migrations/migrations.go | 18 +--- pkg/services/sqlstore/migrations/star_mig.go | 91 +++++++++++++++++++ pkg/services/star/api/api.go | 30 +++--- pkg/services/star/api/api_test.go | 75 ++++++++++++++- pkg/services/star/model.go | 44 ++++++--- pkg/services/star/starimpl/star.go | 14 ++- pkg/services/star/starimpl/store_test.go | 65 +++++++++++-- pkg/services/star/starimpl/xorm_store.go | 36 +++++++- 14 files changed, 340 insertions(+), 79 deletions(-) create mode 100644 pkg/services/sqlstore/migrations/star_mig.go diff --git a/docs/sources/developers/http_api/user.md b/docs/sources/developers/http_api/user.md index bde419c26b5..03e418e0511 100644 --- a/docs/sources/developers/http_api/user.md +++ b/docs/sources/developers/http_api/user.md @@ -570,14 +570,14 @@ Content-Type: application/json ## Star a dashboard -`POST /api/user/stars/dashboard/:dashboardId` +`POST /api/user/stars/dashboard/uid/:uid` Stars the given Dashboard for the actual user. **Example Request**: ```http -POST /api/user/stars/dashboard/1 HTTP/1.1 +POST /api/user/stars/dashboard/uid/BqokFhx7z HTTP/1.1 Accept: application/json Content-Type: application/json Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk @@ -594,14 +594,14 @@ Content-Type: application/json ## Unstar a dashboard -`DELETE /api/user/stars/dashboard/:dashboardId` +`DELETE /api/user/stars/dashboard/uid/:uid` Deletes the starring of the given Dashboard for the actual user. **Example Request**: ```http -DELETE /api/user/stars/dashboard/1 HTTP/1.1 +DELETE /api/user/stars/dashboard/uid/BqokFhx7z HTTP/1.1 Accept: application/json Content-Type: application/json Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk diff --git a/pkg/api/folder_bench_test.go b/pkg/api/folder_bench_test.go index d4544318886..9581cb5795a 100644 --- a/pkg/api/folder_bench_test.go +++ b/pkg/api/folder_bench_test.go @@ -483,7 +483,7 @@ func setupServer(b testing.TB, sc benchScenario, features featuremgmt.FeatureTog require.NoError(b, err) starSvc := startest.NewStarServiceFake() - starSvc.ExpectedUserStars = &star.GetUserStarsResult{UserStars: make(map[int64]bool)} + starSvc.ExpectedUserStars = &star.GetUserStarsResult{UserStars: make(map[string]bool)} hs := &HTTPServer{ CacheService: localcache.New(5*time.Minute, 10*time.Minute), diff --git a/pkg/services/navtree/navtreeimpl/navtree.go b/pkg/services/navtree/navtreeimpl/navtree.go index 448893bffd0..7db5ee4a8cc 100644 --- a/pkg/services/navtree/navtreeimpl/navtree.go +++ b/pkg/services/navtree/navtreeimpl/navtree.go @@ -322,11 +322,11 @@ func (s *ServiceImpl) buildStarredItemsNavLinks(c *contextmodel.ReqContext) ([]* } if len(starredDashboardResult.UserStars) > 0 { - var ids []int64 - for id := range starredDashboardResult.UserStars { - ids = append(ids, id) + var uids []string + for uid := range starredDashboardResult.UserStars { + uids = append(uids, uid) } - starredDashboards, err := s.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardIDs: ids, OrgID: c.SignedInUser.GetOrgID()}) + starredDashboards, err := s.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardUIDs: uids, OrgID: c.SignedInUser.GetOrgID()}) if err != nil { return nil, err } diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index 0668934d1d0..b1d5a711747 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -220,7 +220,7 @@ func (ss *sqlStore) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error } deletes := []string{ - "DELETE FROM star WHERE EXISTS (SELECT 1 FROM dashboard WHERE org_id = ? AND star.dashboard_id = dashboard.id)", + "DELETE FROM star WHERE EXISTS (SELECT 1 FROM dashboard WHERE org_id = ? AND star.dashboard_uid = dashboard.uid)", "DELETE FROM dashboard_tag WHERE EXISTS (SELECT 1 FROM dashboard WHERE org_id = ? AND dashboard_tag.dashboard_id = dashboard.id)", "DELETE FROM dashboard WHERE org_id = ?", "DELETE FROM api_key WHERE org_id = ?", diff --git a/pkg/services/search/service.go b/pkg/services/search/service.go index 74c04f525cf..2eea2f28864 100644 --- a/pkg/services/search/service.go +++ b/pkg/services/search/service.go @@ -82,8 +82,8 @@ func (s *SearchService) SearchHandler(ctx context.Context, query *Query) (model. // 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) + for uid := range staredDashIDs.UserStars { + query.DashboardUIDs = append(query.DashboardUIDs, uid) } } @@ -118,7 +118,7 @@ func (s *SearchService) SearchHandler(ctx context.Context, query *Query) (model. // set starred dashboards for _, dashboard := range hits { - if _, ok := staredDashIDs.UserStars[dashboard.ID]; ok { + if _, ok := staredDashIDs.UserStars[dashboard.UID]; ok { dashboard.IsStarred = true } } diff --git a/pkg/services/search/service_test.go b/pkg/services/search/service_test.go index 58177e5e1a9..297884efbec 100644 --- a/pkg/services/search/service_test.go +++ b/pkg/services/search/service_test.go @@ -23,14 +23,14 @@ func TestSearch_SortedResults(t *testing.T) { us := usertest.NewUserServiceFake() ds := dashboards.NewFakeDashboardService(t) ds.On("SearchDashboards", mock.Anything, mock.AnythingOfType("*dashboards.FindPersistedDashboardsQuery")).Return(model.HitList{ - &model.Hit{ID: 16, Title: "CCAA", Type: "dash-db", Tags: []string{"BB", "AA"}}, - &model.Hit{ID: 10, Title: "AABB", Type: "dash-db", Tags: []string{"CC", "AA"}}, - &model.Hit{ID: 15, Title: "BBAA", Type: "dash-db", Tags: []string{"EE", "AA", "BB"}}, - &model.Hit{ID: 25, Title: "bbAAa", Type: "dash-db", Tags: []string{"EE", "AA", "BB"}}, - &model.Hit{ID: 17, Title: "FOLDER", Type: "dash-folder"}, + &model.Hit{UID: "test", Title: "CCAA", Type: "dash-db", Tags: []string{"BB", "AA"}}, + &model.Hit{UID: "test2", Title: "AABB", Type: "dash-db", Tags: []string{"CC", "AA"}}, + &model.Hit{UID: "test3", Title: "BBAA", Type: "dash-db", Tags: []string{"EE", "AA", "BB"}}, + &model.Hit{UID: "test4", Title: "bbAAa", Type: "dash-db", Tags: []string{"EE", "AA", "BB"}}, + &model.Hit{UID: "test5", Title: "FOLDER", Type: "dash-folder"}, }, nil) us.ExpectedSignedInUser = &user.SignedInUser{IsGrafanaAdmin: true} - ss.ExpectedUserStars = &star.GetUserStarsResult{UserStars: map[int64]bool{10: true, 12: true}} + ss.ExpectedUserStars = &star.GetUserStarsResult{UserStars: map[string]bool{"test2": true, "test7": true}} svc := &SearchService{ sqlstore: db, starService: ss, @@ -66,12 +66,12 @@ func TestSearch_StarredResults(t *testing.T) { us := usertest.NewUserServiceFake() ds := dashboards.NewFakeDashboardService(t) ds.On("SearchDashboards", mock.Anything, mock.AnythingOfType("*dashboards.FindPersistedDashboardsQuery")).Return(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"}, + &model.Hit{UID: "test", Title: "A", Type: "dash-db"}, + &model.Hit{UID: "test2", Title: "B", Type: "dash-db"}, + &model.Hit{UID: "test3", Title: "C", Type: "dash-db"}, }, nil) us.ExpectedSignedInUser = &user.SignedInUser{} - ss.ExpectedUserStars = &star.GetUserStarsResult{UserStars: map[int64]bool{1: true, 3: true, 4: true}} + ss.ExpectedUserStars = &star.GetUserStarsResult{UserStars: map[string]bool{"test": true, "test3": true, "test4": true}} svc := &SearchService{ sqlstore: db, starService: ss, diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 5ff48e9849a..9ba0bcaea9a 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -35,6 +35,7 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { addStarMigrations(mg) addOrgMigrations(mg) addDashboardMigration(mg) // Do NOT add more migrations to this function. + addDashboardUIDStarMigrations(mg) addDataSourceMigration(mg) addApiKeyMigrations(mg) addDashboardSnapshotMigrations(mg) @@ -141,20 +142,3 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { accesscontrol.AddReceiverCreateScopeMigration(mg) } - -func addStarMigrations(mg *Migrator) { - starV1 := Table{ - Name: "star", - Columns: []*Column{ - {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, - {Name: "user_id", Type: DB_BigInt, Nullable: false}, - {Name: "dashboard_id", Type: DB_BigInt, Nullable: false}, - }, - Indices: []*Index{ - {Cols: []string{"user_id", "dashboard_id"}, Type: UniqueIndex}, - }, - } - - mg.AddMigration("create star table", NewAddTableMigration(starV1)) - mg.AddMigration("add unique index star.user_id_dashboard_id", NewAddIndexMigration(starV1, starV1.Indices[0])) -} diff --git a/pkg/services/sqlstore/migrations/star_mig.go b/pkg/services/sqlstore/migrations/star_mig.go new file mode 100644 index 00000000000..66b0e301f76 --- /dev/null +++ b/pkg/services/sqlstore/migrations/star_mig.go @@ -0,0 +1,91 @@ +package migrations + +import ( + "fmt" + + . "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "xorm.io/xorm" +) + +// does not rely on dashboard table existing, can be run before dashboard migrations +func addStarMigrations(mg *Migrator) { + starV1 := Table{ + Name: "star", + Columns: []*Column{ + {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, + {Name: "user_id", Type: DB_BigInt, Nullable: false}, + {Name: "dashboard_id", Type: DB_BigInt, Nullable: false}, + }, + Indices: []*Index{ + {Cols: []string{"user_id", "dashboard_id"}, Type: UniqueIndex}, + }, + } + + mg.AddMigration("create star table", NewAddTableMigration(starV1)) + mg.AddMigration("add unique index star.user_id_dashboard_id", NewAddIndexMigration(starV1, starV1.Indices[0])) + mg.AddMigration("Add column dashboard_uid in star", NewAddColumnMigration(starV1, &Column{ + Name: "dashboard_uid", Type: DB_NVarchar, Length: 40, Nullable: true, + })) + mg.AddMigration("Add column org_id in star", NewAddColumnMigration(starV1, &Column{ + Name: "org_id", Type: DB_BigInt, Nullable: true, Default: "1", + })) + mg.AddMigration("Add column updated in star", NewAddColumnMigration(starV1, &Column{ + Name: "updated", Type: DB_DateTime, Nullable: true, + })) + mg.AddMigration("add index in star table on dashboard_uid, org_id and user_id columns", + NewAddIndexMigration(starV1, &Index{ + Cols: []string{"user_id", "dashboard_uid", "org_id"}, + Type: UniqueIndex, + })) +} + +// relies on the dashboard table existing & must be run after the dashboard migrations are run +func addDashboardUIDStarMigrations(mg *Migrator) { + mg.AddMigration("Add missing dashboard_uid and org_id to star", &FillDashbordUIDMigration{}) +} + +type FillDashbordUIDMigration struct { + MigrationBase +} + +func (m *FillDashbordUIDMigration) SQL(dialect Dialect) string { + return "code migration" +} + +func (m *FillDashbordUIDMigration) Exec(sess *xorm.Session, mg *Migrator) error { + return RunStarMigrations(sess, mg.Dialect.DriverName()) +} + +func RunStarMigrations(sess *xorm.Session, driverName string) error { + // sqlite + sql := `UPDATE star + SET + dashboard_uid = (SELECT uid FROM dashboard WHERE dashboard.id = star.dashboard_id), + org_id = (SELECT org_id FROM dashboard WHERE dashboard.id = star.dashboard_id), + updated = DATETIME('now') + WHERE + (dashboard_uid IS NULL OR org_id IS NULL) + AND EXISTS (SELECT 1 FROM dashboard WHERE dashboard.id = star.dashboard_id);` + if driverName == Postgres { + sql = `UPDATE star + SET dashboard_uid = dashboard.uid, + org_id = dashboard.org_id, + updated = NOW() + FROM dashboard + WHERE star.dashboard_id = dashboard.id + AND (star.dashboard_uid IS NULL OR star.org_id IS NULL);` + } else if driverName == MySQL { + sql = `UPDATE star + LEFT JOIN dashboard ON star.dashboard_id = dashboard.id + SET star.dashboard_uid = dashboard.uid, + star.org_id = dashboard.org_id, + star.updated = NOW() + WHERE star.dashboard_uid IS NULL OR star.org_id IS NULL;` + } + + if _, err := sess.Exec(sql); err != nil { + return fmt.Errorf("failed to set dashboard_uid, org_id, and updated for star: %w", err) + } + + return nil +} diff --git a/pkg/services/star/api/api.go b/pkg/services/star/api/api.go index 57ebdaa844f..9bc9dff1451 100644 --- a/pkg/services/star/api/api.go +++ b/pkg/services/star/api/api.go @@ -4,9 +4,11 @@ import ( "context" "net/http" "strconv" + "time" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/infra/log" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/star" @@ -16,15 +18,18 @@ import ( type API struct { starService star.Service dashboardService dashboards.DashboardService + logger log.Logger } func ProvideApi( starService star.Service, dashboardService dashboards.DashboardService, ) *API { + starLogger := log.New("stars.api") api := &API{ starService: starService, dashboardService: dashboardService, + logger: starLogger, } return api } @@ -63,11 +68,11 @@ func (api *API) GetStars(c *contextmodel.ReqContext) response.Response { uids := []string{} if len(iuserstars.UserStars) > 0 { - var ids []int64 - for id := range iuserstars.UserStars { - ids = append(ids, id) + var uids []string + for uid := range iuserstars.UserStars { + uids = append(uids, uid) } - starredDashboards, err := api.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardIDs: ids, OrgID: c.SignedInUser.GetOrgID()}) + starredDashboards, err := api.dashboardService.GetDashboards(c.Req.Context(), &dashboards.GetDashboardsQuery{DashboardUIDs: uids, OrgID: c.SignedInUser.GetOrgID()}) if err != nil { return response.ErrOrFallback(http.StatusInternalServerError, "Failed to fetch dashboards", err) } @@ -106,7 +111,10 @@ func (api *API) StarDashboard(c *contextmodel.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "Invalid dashboard ID", nil) } - cmd := star.StarDashboardCommand{UserID: userID, DashboardID: id} + api.logger.Warn("POST /user/stars/dashboard/{dashboard_id} is deprecated, please use POST /user/stars/dashboard/uid/{dashboard_uid} instead") + + cmd := star.StarDashboardCommand{UserID: userID, DashboardID: id, Updated: time.Now()} + // nolint:staticcheck if cmd.DashboardID <= 0 { return response.Error(http.StatusBadRequest, "Missing dashboard id", nil) } @@ -146,7 +154,7 @@ func (api *API) StarDashboardByUID(c *contextmodel.ReqContext) response.Response return rsp } - cmd := star.StarDashboardCommand{UserID: userID, DashboardID: dash.ID} + cmd := star.StarDashboardCommand{UserID: userID, DashboardID: dash.ID, DashboardUID: uid, OrgID: c.SignedInUser.GetOrgID(), Updated: time.Now()} if err := api.starService.Add(c.Req.Context(), &cmd); err != nil { return response.Error(http.StatusInternalServerError, "Failed to star dashboard", err) @@ -182,7 +190,10 @@ func (api *API) UnstarDashboard(c *contextmodel.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "Only users and service accounts can star dashboards", nil) } + api.logger.Warn("DELETE /user/stars/dashboard/{dashboard_id} is deprecated, please use DELETE /user/stars/dashboard/uid/{dashboard_uid} instead") + cmd := star.UnstarDashboardCommand{UserID: userID, DashboardID: id} + // nolint:staticcheck if cmd.DashboardID <= 0 { return response.Error(http.StatusBadRequest, "Missing dashboard id", nil) } @@ -217,12 +228,7 @@ func (api *API) UnstarDashboardByUID(c *contextmodel.ReqContext) response.Respon return response.Error(http.StatusBadRequest, "Only users and service accounts can star dashboards", nil) } - dash, rsp := api.getDashboardHelper(c.Req.Context(), c.SignedInUser.GetOrgID(), 0, uid) - if rsp != nil { - return rsp - } - - cmd := star.UnstarDashboardCommand{UserID: userID, DashboardID: dash.ID} + cmd := star.UnstarDashboardCommand{UserID: userID, DashboardUID: uid, OrgID: c.SignedInUser.GetOrgID()} if err := api.starService.Delete(c.Req.Context(), &cmd); err != nil { return response.Error(http.StatusInternalServerError, "Failed to unstar dashboard", err) diff --git a/pkg/services/star/api/api_test.go b/pkg/services/star/api/api_test.go index 38b78ad675c..40638d1f10e 100644 --- a/pkg/services/star/api/api_test.go +++ b/pkg/services/star/api/api_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/dashboards" @@ -13,7 +14,7 @@ import ( "github.com/grafana/grafana/pkg/web" ) -func TestStarDashboard(t *testing.T) { +func TestStarDashboardID(t *testing.T) { api := ProvideApi(startest.NewStarServiceFake(), dashboards.NewFakeDashboardService(t)) testCases := []struct { @@ -82,3 +83,75 @@ func TestStarDashboard(t *testing.T) { }) } } + +func TestStarDashboardUID(t *testing.T) { + svc := dashboards.NewFakeDashboardService(t) + svc.On("GetDashboard", mock.Anything, mock.Anything).Return(&dashboards.Dashboard{UID: "test", OrgID: 1}, nil) + api := ProvideApi(startest.NewStarServiceFake(), svc) + + testCases := []struct { + name string + signedInUser *user.SignedInUser + expectedStatus int + params map[string]string + }{ + { + name: "Star dashboard with user", + params: map[string]string{ + ":uid": "test", + }, + signedInUser: &user.SignedInUser{ + UserID: 1, + OrgID: 1, + IsAnonymous: false, + }, + expectedStatus: 200, + }, + { + name: "Star dashboard with anonymous user", + params: map[string]string{ + ":uid": "test", + }, + signedInUser: &user.SignedInUser{ + UserID: 0, + OrgID: 1, + IsAnonymous: true, + }, + expectedStatus: 400, + }, + { + name: "Star dashboard with API Key", + params: map[string]string{ + ":uid": "test", + }, + signedInUser: &user.SignedInUser{ + UserID: 0, + OrgID: 1, + ApiKeyID: 3, + IsAnonymous: false, + }, + expectedStatus: 400, + }, + { + name: "Star dashboard with Service Account", + params: map[string]string{ + ":uid": "test", + }, + signedInUser: &user.SignedInUser{ + UserID: 1, + OrgID: 3, + IsServiceAccount: true, + }, + expectedStatus: 200, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := web.SetURLParams(&http.Request{}, tc.params) + c := &contextmodel.ReqContext{SignedInUser: tc.signedInUser, Context: &web.Context{Req: req}} + resp := api.StarDashboardByUID(c) + assert.Equal(t, tc.expectedStatus, resp.Status()) + }) + } +} diff --git a/pkg/services/star/model.go b/pkg/services/star/model.go index 81cb64b12e7..2f4694c5c8c 100644 --- a/pkg/services/star/model.go +++ b/pkg/services/star/model.go @@ -1,37 +1,52 @@ package star -import "errors" +import ( + "errors" + "time" +) var ErrCommandValidationFailed = errors.New("command missing required fields") type Star struct { - ID int64 `xorm:"pk autoincr 'id'" db:"id"` - UserID int64 `xorm:"user_id" db:"user_id"` - DashboardID int64 `xorm:"dashboard_id" db:"dashboard_id"` + ID int64 `xorm:"pk autoincr 'id'" db:"id"` + UserID int64 `xorm:"user_id" db:"user_id"` + // Deprecated: use DashboardUID + DashboardID int64 `xorm:"dashboard_id" db:"dashboard_id"` + DashboardUID string `xorm:"dashboard_uid" db:"dashboard_uid"` + OrgID int64 `xorm:"org_id" db:"org_id"` + Updated time.Time `xorm:"updated" db:"updated"` } // ---------------------- // COMMANDS type StarDashboardCommand struct { - UserID int64 `xorm:"user_id"` - DashboardID int64 `xorm:"dashboard_id"` + UserID int64 `xorm:"user_id"` + // Deprecated: use DashboardUID + DashboardID int64 `xorm:"dashboard_id"` + DashboardUID string `xorm:"dashboard_uid"` + OrgID int64 `xorm:"org_id"` + Updated time.Time `xorm:"updated"` } func (cmd *StarDashboardCommand) Validate() error { - if cmd.DashboardID == 0 || cmd.UserID == 0 { + // nolint:staticcheck + if (cmd.DashboardID == 0 && cmd.DashboardUID == "" && cmd.OrgID == 0) || cmd.UserID == 0 { return ErrCommandValidationFailed } return nil } type UnstarDashboardCommand struct { - UserID int64 `xorm:"user_id"` - DashboardID int64 `xorm:"dashboard_id"` + UserID int64 `xorm:"user_id"` + DashboardID int64 `xorm:"dashboard_id"` + DashboardUID string `xorm:"dashboard_uid"` + OrgID int64 `xorm:"org_id"` } func (cmd *UnstarDashboardCommand) Validate() error { - if cmd.DashboardID == 0 || cmd.UserID == 0 { + // nolint:staticcheck + if (cmd.DashboardID == 0 && cmd.DashboardUID == "" && cmd.OrgID == 0) || cmd.UserID == 0 { return ErrCommandValidationFailed } return nil @@ -45,10 +60,13 @@ type GetUserStarsQuery struct { } type IsStarredByUserQuery struct { - UserID int64 `xorm:"user_id"` - DashboardID int64 `xorm:"dashboard_id"` + UserID int64 `xorm:"user_id"` + DashboardID int64 `xorm:"dashboard_id"` + DashboardUID string `xorm:"dashboard_uid"` + OrgID int64 `xorm:"org_id"` + Updated time.Time `xorm:"updated"` } type GetUserStarsResult struct { - UserStars map[int64]bool + UserStars map[string]bool } diff --git a/pkg/services/star/starimpl/star.go b/pkg/services/star/starimpl/star.go index fe1d593af61..5b39442c45b 100644 --- a/pkg/services/star/starimpl/star.go +++ b/pkg/services/star/starimpl/star.go @@ -4,18 +4,30 @@ import ( "context" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/sqlstore/migrations" "github.com/grafana/grafana/pkg/services/star" ) type Service struct { - store store + store store + logger log.Logger } func ProvideService(db db.DB) star.Service { + starLogger := log.New("stars") + // fill out dashboard_uid, org_id and updated columns for stars + // need to run this at startup in case any downgrade happened after + // the initial migration + err := migrations.RunStarMigrations(db.GetEngine().NewSession(), db.GetDialect().DriverName()) + if err != nil { + starLogger.Error("Failed to run star migrations", "err", err) + } return &Service{ store: &sqlStore{ db: db, }, + logger: starLogger, } } diff --git a/pkg/services/star/starimpl/store_test.go b/pkg/services/star/starimpl/store_test.go index 1b725ca5224..1661f152db7 100644 --- a/pkg/services/star/starimpl/store_test.go +++ b/pkg/services/star/starimpl/store_test.go @@ -3,6 +3,7 @@ package starimpl import ( "context" "testing" + "time" "github.com/stretchr/testify/require" @@ -24,7 +25,7 @@ func testIntegrationUserStarsDataAccess(t *testing.T, fn getStore) { ss := db.InitTestDB(t) starStore := fn(ss) - t.Run("Given saved star", func(t *testing.T) { + t.Run("Given saved star by dashboard id", func(t *testing.T) { cmd := star.StarDashboardCommand{ DashboardID: 10, UserID: 12, @@ -64,22 +65,72 @@ func testIntegrationUserStarsDataAccess(t *testing.T, fn getStore) { }) }) + t.Run("Given saved star by dashboard UID", func(t *testing.T) { + cmd := star.StarDashboardCommand{ + DashboardUID: "test", + OrgID: 1, + UserID: 12, + } + err := starStore.Insert(context.Background(), &cmd) + require.NoError(t, err) + + t.Run("Get should return true when starred", func(t *testing.T) { + query := star.IsStarredByUserQuery{UserID: 12, DashboardUID: "test", OrgID: 1} + isStarred, err := starStore.Get(context.Background(), &query) + require.NoError(t, err) + require.True(t, isStarred) + }) + + t.Run("Get should return false when not starred", func(t *testing.T) { + query := star.IsStarredByUserQuery{UserID: 12, DashboardUID: "testing", OrgID: 1} + isStarred, err := starStore.Get(context.Background(), &query) + require.NoError(t, err) + require.False(t, isStarred) + }) + + t.Run("List should return a list of size 1", func(t *testing.T) { + query := star.GetUserStarsQuery{UserID: 12} + result, err := starStore.List(context.Background(), &query) + require.NoError(t, err) + require.Equal(t, 1, len(result.UserStars)) + }) + + t.Run("Delete should remove the star", func(t *testing.T) { + deleteQuery := star.UnstarDashboardCommand{DashboardUID: "test", OrgID: 1, UserID: 12} + err := starStore.Delete(context.Background(), &deleteQuery) + require.NoError(t, err) + getQuery := star.IsStarredByUserQuery{UserID: 12, DashboardUID: "test", OrgID: 1} + isStarred, err := starStore.Get(context.Background(), &getQuery) + require.NoError(t, err) + require.False(t, isStarred) + }) + }) + t.Run("DeleteByUser should remove the star for user", func(t *testing.T) { star1 := star.StarDashboardCommand{ - DashboardID: 10, - UserID: 12, + DashboardUID: "test", + OrgID: 1, + Updated: time.Now(), + DashboardID: 10, + UserID: 12, } err := starStore.Insert(context.Background(), &star1) require.NoError(t, err) star2 := star.StarDashboardCommand{ - DashboardID: 11, - UserID: 12, + DashboardUID: "test2", + OrgID: 1, + Updated: time.Now(), + DashboardID: 11, + UserID: 12, } err = starStore.Insert(context.Background(), &star2) require.NoError(t, err) star3 := star.StarDashboardCommand{ - DashboardID: 11, - UserID: 11, + DashboardUID: "test2", + OrgID: 1, + Updated: time.Now(), + DashboardID: 11, + UserID: 11, } err = starStore.Insert(context.Background(), &star3) require.NoError(t, err) diff --git a/pkg/services/star/starimpl/xorm_store.go b/pkg/services/star/starimpl/xorm_store.go index 914bafa193d..a8d4a367189 100644 --- a/pkg/services/star/starimpl/xorm_store.go +++ b/pkg/services/star/starimpl/xorm_store.go @@ -14,9 +14,22 @@ type sqlStore struct { func (s *sqlStore) Get(ctx context.Context, query *star.IsStarredByUserQuery) (bool, error) { var isStarred bool err := s.db.WithDbSession(ctx, func(sess *db.Session) error { + if query.DashboardUID != "" && query.OrgID != 0 { + rawSQL := "SELECT 1 from star where user_id=? and dashboard_uid=? and org_id=?" + results, err := sess.Query(rawSQL, query.UserID, query.DashboardUID, query.OrgID) + + if err != nil { + return err + } + + isStarred = len(results) != 0 + return nil + } + + // TODO: Remove this block after all dashboards have a UID + // && the deprecated endpoints have been removed rawSQL := "SELECT 1 from star where user_id=? and dashboard_id=?" results, err := sess.Query(rawSQL, query.UserID, query.DashboardID) - if err != nil { return err } @@ -30,8 +43,12 @@ func (s *sqlStore) Get(ctx context.Context, query *star.IsStarredByUserQuery) (b func (s *sqlStore) Insert(ctx context.Context, cmd *star.StarDashboardCommand) error { return s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { entity := star.Star{ - UserID: cmd.UserID, - DashboardID: cmd.DashboardID, + UserID: cmd.UserID, + // nolint:staticcheck + DashboardID: cmd.DashboardID, + DashboardUID: cmd.DashboardUID, + OrgID: cmd.OrgID, + Updated: cmd.Updated, } _, err := sess.Insert(&entity) @@ -45,7 +62,16 @@ func (s *sqlStore) Insert(ctx context.Context, cmd *star.StarDashboardCommand) e func (s *sqlStore) Delete(ctx context.Context, cmd *star.UnstarDashboardCommand) error { return s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + if cmd.DashboardUID != "" && cmd.OrgID != 0 { + var rawSQL = "DELETE FROM star WHERE user_id=? and dashboard_uid=? and org_id=?" + _, err := sess.Exec(rawSQL, cmd.UserID, cmd.DashboardUID, cmd.OrgID) + return err + } + + // TODO: Remove this block after all dashboards have a UID + // && the deprecated endpoints have been removed var rawSQL = "DELETE FROM star WHERE user_id=? and dashboard_id=?" + // nolint:staticcheck _, err := sess.Exec(rawSQL, cmd.UserID, cmd.DashboardID) return err }) @@ -60,12 +86,12 @@ func (s *sqlStore) DeleteByUser(ctx context.Context, userID int64) error { } func (s *sqlStore) List(ctx context.Context, query *star.GetUserStarsQuery) (*star.GetUserStarsResult, error) { - userStars := make(map[int64]bool) + userStars := make(map[string]bool) err := s.db.WithDbSession(ctx, func(dbSession *db.Session) error { var stars = make([]star.Star, 0) err := dbSession.Where("user_id=?", query.UserID).Find(&stars) for _, star := range stars { - userStars[star.DashboardID] = true + userStars[star.DashboardUID] = true } return err })