From af09638a0d9f870ee1228657cc7111e5d6d2f614 Mon Sep 17 00:00:00 2001 From: ying-jeanne <74549700+ying-jeanne@users.noreply.github.com> Date: Mon, 8 Aug 2022 11:06:06 -0500 Subject: [PATCH] remove the sqlstore playlist methods (#53415) --- pkg/services/export/export_sys_playlists.go | 10 +- pkg/services/export/git_export_job.go | 7 +- pkg/services/export/service.go | 8 +- .../playlist/playlistimpl/store_test.go | 7 + pkg/services/sqlstore/playlist.go | 203 ------------------ pkg/services/sqlstore/playlist_test.go | 96 --------- pkg/services/sqlstore/store.go | 12 -- 7 files changed, 24 insertions(+), 319 deletions(-) delete mode 100644 pkg/services/sqlstore/playlist.go delete mode 100644 pkg/services/sqlstore/playlist_test.go diff --git a/pkg/services/export/export_sys_playlists.go b/pkg/services/export/export_sys_playlists.go index b2a441e8c7a..fa81ee37a62 100644 --- a/pkg/services/export/export_sys_playlists.go +++ b/pkg/services/export/export_sys_playlists.go @@ -5,20 +5,20 @@ import ( "path/filepath" "time" - "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/playlist" ) func exportSystemPlaylists(helper *commitHelper, job *gitExportJob) error { - cmd := &models.GetPlaylistsQuery{ + cmd := &playlist.GetPlaylistsQuery{ OrgId: helper.orgID, Limit: 500000, } - err := job.sql.SearchPlaylists(helper.ctx, cmd) + res, err := job.playlistService.Search(helper.ctx, cmd) if err != nil { return err } - if len(cmd.Result) < 1 { + if len(res) < 1 { return nil // nothing } @@ -27,7 +27,7 @@ func exportSystemPlaylists(helper *commitHelper, job *gitExportJob) error { comment: "Export playlists", } - for _, playlist := range cmd.Result { + for _, playlist := range res { // TODO: fix the playlist API so it returns the json we need :) gitcmd.body = append(gitcmd.body, commitBody{ diff --git a/pkg/services/export/git_export_job.go b/pkg/services/export/git_export_job.go index b676a38e12f..fa8c36d7339 100644 --- a/pkg/services/export/git_export_job.go +++ b/pkg/services/export/git_export_job.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/dashboardsnapshots" + "github.com/grafana/grafana/pkg/services/playlist" "github.com/grafana/grafana/pkg/services/sqlstore" ) @@ -23,6 +24,7 @@ type gitExportJob struct { logger log.Logger sql *sqlstore.SQLStore dashboardsnapshotsService dashboardsnapshots.Service + playlistService playlist.Service rootDir string statusMu sync.Mutex @@ -32,12 +34,15 @@ type gitExportJob struct { helper *commitHelper } -func startGitExportJob(cfg ExportConfig, sql *sqlstore.SQLStore, dashboardsnapshotsService dashboardsnapshots.Service, rootDir string, orgID int64, broadcaster statusBroadcaster) (Job, error) { +func startGitExportJob(cfg ExportConfig, sql *sqlstore.SQLStore, + dashboardsnapshotsService dashboardsnapshots.Service, rootDir string, orgID int64, + broadcaster statusBroadcaster, playlistService playlist.Service) (Job, error) { job := &gitExportJob{ logger: log.New("git_export_job"), cfg: cfg, sql: sql, dashboardsnapshotsService: dashboardsnapshotsService, + playlistService: playlistService, rootDir: rootDir, broadcaster: broadcaster, status: ExportStatus{ diff --git a/pkg/services/export/service.go b/pkg/services/export/service.go index 41c7c0ca678..ec180449178 100644 --- a/pkg/services/export/service.go +++ b/pkg/services/export/service.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/services/dashboardsnapshots" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/live" + "github.com/grafana/grafana/pkg/services/playlist" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" ) @@ -150,12 +151,14 @@ type StandardExport struct { // Services sql *sqlstore.SQLStore dashboardsnapshotsService dashboardsnapshots.Service + playlistService playlist.Service // updated with mutex exportJob Job } -func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, gl *live.GrafanaLive, cfg *setting.Cfg, dashboardsnapshotsService dashboardsnapshots.Service) ExportService { +func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, gl *live.GrafanaLive, cfg *setting.Cfg, + dashboardsnapshotsService dashboardsnapshots.Service, playlistService playlist.Service) ExportService { if !features.IsEnabled(featuremgmt.FlagExport) { return &StubExport{} } @@ -165,6 +168,7 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, glive: gl, logger: log.New("export_service"), dashboardsnapshotsService: dashboardsnapshotsService, + playlistService: playlistService, exportJob: &stoppedJob{}, dataDir: cfg.DataPath, } @@ -221,7 +225,7 @@ func (ex *StandardExport) HandleRequestExport(c *models.ReqContext) response.Res if err := os.MkdirAll(dir, os.ModePerm); err != nil { return response.Error(http.StatusBadRequest, "Error creating export folder", nil) } - job, err = startGitExportJob(cfg, ex.sql, ex.dashboardsnapshotsService, dir, c.OrgId, broadcast) + job, err = startGitExportJob(cfg, ex.sql, ex.dashboardsnapshotsService, dir, c.OrgId, broadcast, ex.playlistService) default: return response.Error(http.StatusBadRequest, "Unsupported job format", nil) } diff --git a/pkg/services/playlist/playlistimpl/store_test.go b/pkg/services/playlist/playlistimpl/store_test.go index 0eb1fe842aa..5d63b3a9f54 100644 --- a/pkg/services/playlist/playlistimpl/store_test.go +++ b/pkg/services/playlist/playlistimpl/store_test.go @@ -33,6 +33,13 @@ func TestIntegrationPlaylistDataAccess(t *testing.T) { require.Equal(t, len(storedPlaylistItems), len(items)) }) + t.Run("Get playlist that doesn't exist", func(t *testing.T) { + get := &playlist.GetPlaylistByUidQuery{UID: "unknown", OrgId: 1} + _, err := playlistStore.Get(context.Background(), get) + require.Error(t, err) + require.ErrorIs(t, err, playlist.ErrPlaylistNotFound) + }) + t.Run("Can update playlist", func(t *testing.T) { items := []playlist.PlaylistItemDTO{ {Title: "influxdb", Value: "influxdb", Type: "dashboard_by_tag"}, diff --git a/pkg/services/sqlstore/playlist.go b/pkg/services/sqlstore/playlist.go deleted file mode 100644 index 269bf06183a..00000000000 --- a/pkg/services/sqlstore/playlist.go +++ /dev/null @@ -1,203 +0,0 @@ -package sqlstore - -import ( - "context" - - "github.com/grafana/grafana/pkg/models" -) - -func (ss *SQLStore) CreatePlaylist(ctx context.Context, cmd *models.CreatePlaylistCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - uid, err := generateAndValidateNewPlaylistUid(sess, cmd.OrgId) - if err != nil { - return err - } - - playlist := models.Playlist{ - Name: cmd.Name, - Interval: cmd.Interval, - OrgId: cmd.OrgId, - UID: uid, - } - - _, err = sess.Insert(&playlist) - if err != nil { - return err - } - - playlistItems := make([]models.PlaylistItem, 0) - for _, item := range cmd.Items { - playlistItems = append(playlistItems, models.PlaylistItem{ - PlaylistId: playlist.Id, - Type: item.Type, - Value: item.Value, - Order: item.Order, - Title: item.Title, - }) - } - - _, err = sess.Insert(&playlistItems) - - cmd.Result = &playlist - return err - }) -} - -func (ss *SQLStore) UpdatePlaylist(ctx context.Context, cmd *models.UpdatePlaylistCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - playlist := models.Playlist{ - UID: cmd.UID, - OrgId: cmd.OrgId, - Name: cmd.Name, - Interval: cmd.Interval, - } - - existingPlaylist := models.Playlist{UID: cmd.UID, OrgId: cmd.OrgId} - _, err := sess.Get(&existingPlaylist) - if err != nil { - return err - } - playlist.Id = existingPlaylist.Id - - cmd.Result = &models.PlaylistDTO{ - Id: playlist.Id, - UID: playlist.UID, - OrgId: playlist.OrgId, - Name: playlist.Name, - Interval: playlist.Interval, - } - - _, err = sess.Where("id=?", playlist.Id).Cols("name", "interval").Update(&playlist) - if err != nil { - return err - } - - rawSQL := "DELETE FROM playlist_item WHERE playlist_id = ?" - _, err = sess.Exec(rawSQL, playlist.Id) - - if err != nil { - return err - } - - playlistItems := make([]models.PlaylistItem, 0) - - for index, item := range cmd.Items { - playlistItems = append(playlistItems, models.PlaylistItem{ - PlaylistId: playlist.Id, - Type: item.Type, - Value: item.Value, - Order: index + 1, - Title: item.Title, - }) - } - - _, err = sess.Insert(&playlistItems) - return err - }) -} - -func (ss *SQLStore) GetPlaylist(ctx context.Context, query *models.GetPlaylistByUidQuery) error { - if query.UID == "" || query.OrgId == 0 { - return models.ErrCommandValidationFailed - } - - return ss.WithDbSession(ctx, func(sess *DBSession) error { - playlist := models.Playlist{UID: query.UID, OrgId: query.OrgId} - exists, err := sess.Get(&playlist) - if !exists { - return models.ErrPlaylistNotFound - } - query.Result = &playlist - - return err - }) -} - -func (ss *SQLStore) DeletePlaylist(ctx context.Context, cmd *models.DeletePlaylistCommand) error { - if cmd.UID == "" || cmd.OrgId == 0 { - return models.ErrCommandValidationFailed - } - - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - playlist := models.Playlist{UID: cmd.UID, OrgId: cmd.OrgId} - _, err := sess.Get(&playlist) - if err != nil { - return err - } - - var rawPlaylistSQL = "DELETE FROM playlist WHERE uid = ? and org_id = ?" - _, err = sess.Exec(rawPlaylistSQL, cmd.UID, cmd.OrgId) - if err != nil { - return err - } - - var rawItemSQL = "DELETE FROM playlist_item WHERE playlist_id = ?" - _, err = sess.Exec(rawItemSQL, playlist.Id) - - return err - }) -} - -func (ss *SQLStore) SearchPlaylists(ctx context.Context, query *models.GetPlaylistsQuery) error { - if query.OrgId == 0 { - return models.ErrCommandValidationFailed - } - - return ss.WithDbSession(ctx, func(dbSess *DBSession) error { - var playlists = make(models.Playlists, 0) - - sess := dbSess.Limit(query.Limit) - - if query.Name != "" { - sess.Where("name LIKE ?", "%"+query.Name+"%") - } - - sess.Where("org_id = ?", query.OrgId) - err := sess.Find(&playlists) - query.Result = playlists - - return err - }) -} - -func (ss *SQLStore) GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByUidQuery) error { - return ss.WithDbSession(ctx, func(sess *DBSession) error { - if query.PlaylistUID == "" || query.OrgId == 0 { - return models.ErrCommandValidationFailed - } - - // get the playlist Id - get := &models.GetPlaylistByUidQuery{UID: query.PlaylistUID, OrgId: query.OrgId} - err := ss.GetPlaylist(ctx, get) - if err != nil { - return err - } - - var playlistItems = make([]models.PlaylistItem, 0) - err = sess.Where("playlist_id=?", get.Result.Id).Find(&playlistItems) - query.Result = &playlistItems - - return err - }) -} - -// generateAndValidateNewPlaylistUid generates a playlistUID and verifies that -// the uid isn't already in use. This is deliberately overly cautious, since users -// can also specify playlist uids during provisioning. -func generateAndValidateNewPlaylistUid(sess *DBSession, orgId int64) (string, error) { - for i := 0; i < 3; i++ { - uid := generateNewUid() - - playlist := models.Playlist{OrgId: orgId, UID: uid} - exists, err := sess.Get(&playlist) - if err != nil { - return "", err - } - - if !exists { - return uid, nil - } - } - - return "", models.ErrPlaylistFailedGenerateUniqueUid -} diff --git a/pkg/services/sqlstore/playlist_test.go b/pkg/services/sqlstore/playlist_test.go deleted file mode 100644 index a3605bdb429..00000000000 --- a/pkg/services/sqlstore/playlist_test.go +++ /dev/null @@ -1,96 +0,0 @@ -package sqlstore - -import ( - "context" - "testing" - - "github.com/stretchr/testify/require" - - "github.com/grafana/grafana/pkg/models" -) - -func TestIntegrationPlaylistDataAccess(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - ss := InitTestDB(t) - - t.Run("Can create playlist", func(t *testing.T) { - items := []models.PlaylistItemDTO{ - {Title: "graphite", Value: "graphite", Type: "dashboard_by_tag"}, - {Title: "Backend response times", Value: "3", Type: "dashboard_by_id"}, - } - cmd := models.CreatePlaylistCommand{Name: "NYC office", Interval: "10m", OrgId: 1, Items: items} - err := ss.CreatePlaylist(context.Background(), &cmd) - require.NoError(t, err) - uid := cmd.Result.UID - - t.Run("Can get playlist", func(t *testing.T) { - get := &models.GetPlaylistByUidQuery{UID: uid, OrgId: 1} - err = ss.GetPlaylist(context.Background(), get) - require.NoError(t, err) - require.NotNil(t, get.Result) - require.Equal(t, get.Result.Name, "NYC office") - require.Equal(t, get.Result.Interval, "10m") - }) - - t.Run("Can get playlist items", func(t *testing.T) { - get := &models.GetPlaylistItemsByUidQuery{PlaylistUID: uid, OrgId: 1} - err = ss.GetPlaylistItem(context.Background(), get) - require.NoError(t, err) - require.Equal(t, len(*get.Result), len(items)) - }) - - t.Run("Can update playlist", func(t *testing.T) { - items := []models.PlaylistItemDTO{ - {Title: "influxdb", Value: "influxdb", Type: "dashboard_by_tag"}, - {Title: "Backend response times", Value: "2", Type: "dashboard_by_id"}, - } - query := models.UpdatePlaylistCommand{Name: "NYC office ", OrgId: 1, UID: uid, Interval: "10s", Items: items} - err = ss.UpdatePlaylist(context.Background(), &query) - require.NoError(t, err) - }) - - t.Run("Can remove playlist", func(t *testing.T) { - deleteQuery := models.DeletePlaylistCommand{UID: uid, OrgId: 1} - err = ss.DeletePlaylist(context.Background(), &deleteQuery) - require.NoError(t, err) - - getQuery := models.GetPlaylistByUidQuery{UID: uid, OrgId: 1} - err = ss.GetPlaylist(context.Background(), &getQuery) - require.Error(t, err) - require.ErrorIs(t, err, models.ErrPlaylistNotFound) - }) - }) - - t.Run("Get playlist that doesn't exist", func(t *testing.T) { - get := &models.GetPlaylistByUidQuery{UID: "unknown", OrgId: 1} - err := ss.GetPlaylist(context.Background(), get) - require.Error(t, err) - require.ErrorIs(t, err, models.ErrPlaylistNotFound) - }) - - t.Run("Delete playlist that doesn't exist", func(t *testing.T) { - deleteQuery := models.DeletePlaylistCommand{UID: "654312", OrgId: 1} - err := ss.DeletePlaylist(context.Background(), &deleteQuery) - require.NoError(t, err) - }) - - t.Run("Delete playlist with invalid command yields error", func(t *testing.T) { - testCases := []struct { - desc string - cmd models.DeletePlaylistCommand - }{ - {desc: "none", cmd: models.DeletePlaylistCommand{}}, - {desc: "no OrgId", cmd: models.DeletePlaylistCommand{UID: "1"}}, - {desc: "no Uid", cmd: models.DeletePlaylistCommand{OrgId: 1}}, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - err := ss.DeletePlaylist(context.Background(), &tc.cmd) - require.EqualError(t, err, models.ErrCommandValidationFailed.Error()) - }) - } - }) -} diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index e242a2e5bfc..d53c58dff02 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -65,18 +65,6 @@ type Store interface { GetGlobalQuotaByTarget(ctx context.Context, query *models.GetGlobalQuotaByTargetQuery) error WithTransactionalDbSession(ctx context.Context, callback DBTransactionFunc) error InTransaction(ctx context.Context, fn func(ctx context.Context) error) error - // deprecated - CreatePlaylist(ctx context.Context, cmd *models.CreatePlaylistCommand) error - // deprecated - UpdatePlaylist(ctx context.Context, cmd *models.UpdatePlaylistCommand) error - // deprecated - GetPlaylist(ctx context.Context, query *models.GetPlaylistByUidQuery) error - // deprecated - DeletePlaylist(ctx context.Context, cmd *models.DeletePlaylistCommand) error - // deprecated - SearchPlaylists(ctx context.Context, query *models.GetPlaylistsQuery) error - // deprecated - GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByUidQuery) error AddOrgUser(ctx context.Context, cmd *models.AddOrgUserCommand) error UpdateOrgUser(ctx context.Context, cmd *models.UpdateOrgUserCommand) error GetOrgUsers(ctx context.Context, query *models.GetOrgUsersQuery) error