diff --git a/docs/sources/developers/http_api/playlist.md b/docs/sources/developers/http_api/playlist.md index feb9ab077b9..36ab50f672c 100644 --- a/docs/sources/developers/http_api/playlist.md +++ b/docs/sources/developers/http_api/playlist.md @@ -42,7 +42,7 @@ HTTP/1.1 200 Content-Type: application/json [ { - "id": 1, + "uid": "1", "name": "my playlist", "interval": "5m" } @@ -51,7 +51,7 @@ Content-Type: application/json ## Get one playlist -`GET /api/playlists/:id` +`GET /api/playlists/:uid` **Example Request**: @@ -67,14 +67,14 @@ Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk HTTP/1.1 200 Content-Type: application/json { - "id" : 1, + "uid" : "1", "name": "my playlist", "interval": "5m", "orgId": "my org", "items": [ { "id": 1, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_id", "value": "3", "order": 1, @@ -82,7 +82,7 @@ Content-Type: application/json }, { "id": 2, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_tag", "value": "myTag", "order": 2, @@ -94,7 +94,7 @@ Content-Type: application/json ## Get Playlist items -`GET /api/playlists/:id/items` +`GET /api/playlists/:uid/items` **Example Request**: @@ -112,7 +112,7 @@ Content-Type: application/json [ { "id": 1, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_id", "value": "3", "order": 1, @@ -120,7 +120,7 @@ Content-Type: application/json }, { "id": 2, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_tag", "value": "myTag", "order": 2, @@ -131,7 +131,7 @@ Content-Type: application/json ## Get Playlist dashboards -`GET /api/playlists/:id/dashboards` +`GET /api/playlists/:uid/dashboards` **Example Request**: @@ -198,7 +198,7 @@ Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk HTTP/1.1 200 Content-Type: application/json { - "id": 1, + "uid": "1", "name": "my playlist", "interval": "5m" } @@ -206,7 +206,7 @@ Content-Type: application/json ## Update a playlist -`PUT /api/playlists/:id` +`PUT /api/playlists/:uid` **Example Request**: @@ -220,14 +220,14 @@ Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk "interval": "5m", "items": [ { - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_id", "value": "3", "order": 1, "title":"my third dashboard" }, { - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_tag", "value": "myTag", "order": 2, @@ -243,14 +243,14 @@ Authorization: Bearer eyJrIjoiT0tTcG1pUlY2RnVKZTFVaDFsNFZXdE9ZWmNrMkZYbk HTTP/1.1 200 Content-Type: application/json { - "id" : 1, + "uid" : "1", "name": "my playlist", "interval": "5m", "orgId": "my org", "items": [ { "id": 1, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_id", "value": "3", "order": 1, @@ -258,7 +258,7 @@ Content-Type: application/json }, { "id": 2, - "playlistId": 1, + "playlistUid": "1", "type": "dashboard_by_tag", "value": "myTag", "order": 2, @@ -270,7 +270,7 @@ Content-Type: application/json ## Delete a playlist -`DELETE /api/playlists/:id` +`DELETE /api/playlists/:uid` **Example Request**: diff --git a/pkg/api/api.go b/pkg/api/api.go index cd64251b4ab..23624135aeb 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -438,11 +438,11 @@ func (hs *HTTPServer) registerRoutes() { // Playlist apiRoute.Group("/playlists", func(playlistRoute routing.RouteRegister) { playlistRoute.Get("/", routing.Wrap(hs.SearchPlaylists)) - playlistRoute.Get("/:id", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylist)) - playlistRoute.Get("/:id/items", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylistItems)) - playlistRoute.Get("/:id/dashboards", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylistDashboards)) - playlistRoute.Delete("/:id", reqEditorRole, hs.ValidateOrgPlaylist, routing.Wrap(hs.DeletePlaylist)) - playlistRoute.Put("/:id", reqEditorRole, hs.ValidateOrgPlaylist, routing.Wrap(hs.UpdatePlaylist)) + playlistRoute.Get("/:uid", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylist)) + playlistRoute.Get("/:uid/items", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylistItems)) + playlistRoute.Get("/:uid/dashboards", hs.ValidateOrgPlaylist, routing.Wrap(hs.GetPlaylistDashboards)) + playlistRoute.Delete("/:uid", reqEditorRole, hs.ValidateOrgPlaylist, routing.Wrap(hs.DeletePlaylist)) + playlistRoute.Put("/:uid", reqEditorRole, hs.ValidateOrgPlaylist, routing.Wrap(hs.UpdatePlaylist)) playlistRoute.Post("/", reqEditorRole, routing.Wrap(hs.CreatePlaylist)) }) diff --git a/pkg/api/playlist.go b/pkg/api/playlist.go index d30f8c58b23..47889948e13 100644 --- a/pkg/api/playlist.go +++ b/pkg/api/playlist.go @@ -3,7 +3,6 @@ package api import ( "context" "net/http" - "strconv" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" @@ -11,13 +10,9 @@ import ( ) func (hs *HTTPServer) ValidateOrgPlaylist(c *models.ReqContext) { - id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - c.JsonApiErr(http.StatusBadRequest, "id is invalid", nil) - return - } - query := models.GetPlaylistByIdQuery{Id: id} - err = hs.SQLStore.GetPlaylist(c.Req.Context(), &query) + uid := web.Params(c.Req)[":uid"] + query := models.GetPlaylistByUidQuery{UID: uid, OrgId: c.OrgId} + err := hs.SQLStore.GetPlaylist(c.Req.Context(), &query) if err != nil { c.JsonApiErr(404, "Playlist not found", err) @@ -58,20 +53,18 @@ func (hs *HTTPServer) SearchPlaylists(c *models.ReqContext) response.Response { } func (hs *HTTPServer) GetPlaylist(c *models.ReqContext) response.Response { - id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - return response.Error(http.StatusBadRequest, "id is invalid", err) - } - cmd := models.GetPlaylistByIdQuery{Id: id} + uid := web.Params(c.Req)[":uid"] + cmd := models.GetPlaylistByUidQuery{UID: uid, OrgId: c.OrgId} if err := hs.SQLStore.GetPlaylist(c.Req.Context(), &cmd); err != nil { return response.Error(500, "Playlist not found", err) } - playlistDTOs, _ := hs.LoadPlaylistItemDTOs(c.Req.Context(), id) + playlistDTOs, _ := hs.LoadPlaylistItemDTOs(c.Req.Context(), uid, c.OrgId) dto := &models.PlaylistDTO{ Id: cmd.Result.Id, + UID: cmd.Result.UID, Name: cmd.Result.Name, Interval: cmd.Result.Interval, OrgId: cmd.Result.OrgId, @@ -81,8 +74,8 @@ func (hs *HTTPServer) GetPlaylist(c *models.ReqContext) response.Response { return response.JSON(http.StatusOK, dto) } -func (hs *HTTPServer) LoadPlaylistItemDTOs(ctx context.Context, id int64) ([]models.PlaylistItemDTO, error) { - playlistitems, err := hs.LoadPlaylistItems(ctx, id) +func (hs *HTTPServer) LoadPlaylistItemDTOs(ctx context.Context, uid string, orgId int64) ([]models.PlaylistItemDTO, error) { + playlistitems, err := hs.LoadPlaylistItems(ctx, uid, orgId) if err != nil { return nil, err @@ -104,8 +97,8 @@ func (hs *HTTPServer) LoadPlaylistItemDTOs(ctx context.Context, id int64) ([]mod return playlistDTOs, nil } -func (hs *HTTPServer) LoadPlaylistItems(ctx context.Context, id int64) ([]models.PlaylistItem, error) { - itemQuery := models.GetPlaylistItemsByIdQuery{PlaylistId: id} +func (hs *HTTPServer) LoadPlaylistItems(ctx context.Context, uid string, orgId int64) ([]models.PlaylistItem, error) { + itemQuery := models.GetPlaylistItemsByUidQuery{PlaylistUID: uid, OrgId: orgId} if err := hs.SQLStore.GetPlaylistItem(ctx, &itemQuery); err != nil { return nil, err } @@ -114,12 +107,9 @@ func (hs *HTTPServer) LoadPlaylistItems(ctx context.Context, id int64) ([]models } func (hs *HTTPServer) GetPlaylistItems(c *models.ReqContext) response.Response { - id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - return response.Error(http.StatusBadRequest, "id is invalid", err) - } + uid := web.Params(c.Req)[":uid"] - playlistDTOs, err := hs.LoadPlaylistItemDTOs(c.Req.Context(), id) + playlistDTOs, err := hs.LoadPlaylistItemDTOs(c.Req.Context(), uid, c.OrgId) if err != nil { return response.Error(500, "Could not load playlist items", err) @@ -129,12 +119,9 @@ func (hs *HTTPServer) GetPlaylistItems(c *models.ReqContext) response.Response { } func (hs *HTTPServer) GetPlaylistDashboards(c *models.ReqContext) response.Response { - playlistID, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - return response.Error(http.StatusBadRequest, "id is invalid", err) - } + playlistUID := web.Params(c.Req)[":uid"] - playlists, err := hs.LoadPlaylistDashboards(c.Req.Context(), c.OrgId, c.SignedInUser, playlistID) + playlists, err := hs.LoadPlaylistDashboards(c.Req.Context(), c.OrgId, c.SignedInUser, playlistUID) if err != nil { return response.Error(500, "Could not load dashboards", err) } @@ -143,12 +130,9 @@ func (hs *HTTPServer) GetPlaylistDashboards(c *models.ReqContext) response.Respo } func (hs *HTTPServer) DeletePlaylist(c *models.ReqContext) response.Response { - id, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - return response.Error(http.StatusBadRequest, "id is invalid", err) - } + uid := web.Params(c.Req)[":uid"] - cmd := models.DeletePlaylistCommand{Id: id, OrgId: c.OrgId} + cmd := models.DeletePlaylistCommand{UID: uid, OrgId: c.OrgId} if err := hs.SQLStore.DeletePlaylist(c.Req.Context(), &cmd); err != nil { return response.Error(500, "Failed to delete playlist", err) } @@ -176,17 +160,13 @@ func (hs *HTTPServer) UpdatePlaylist(c *models.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "bad request data", err) } cmd.OrgId = c.OrgId - var err error - cmd.Id, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { - return response.Error(http.StatusBadRequest, "id is invalid", err) - } + cmd.UID = web.Params(c.Req)[":uid"] if err := hs.SQLStore.UpdatePlaylist(c.Req.Context(), &cmd); err != nil { return response.Error(500, "Failed to save playlist", err) } - playlistDTOs, err := hs.LoadPlaylistItemDTOs(c.Req.Context(), cmd.Id) + playlistDTOs, err := hs.LoadPlaylistItemDTOs(c.Req.Context(), cmd.UID, c.OrgId) if err != nil { return response.Error(500, "Failed to save playlist", err) } diff --git a/pkg/api/playlist_play.go b/pkg/api/playlist_play.go index 712cffe8ce5..c75a550d579 100644 --- a/pkg/api/playlist_play.go +++ b/pkg/api/playlist_play.go @@ -65,8 +65,8 @@ func (hs *HTTPServer) populateDashboardsByTag(ctx context.Context, orgID int64, return result } -func (hs *HTTPServer) LoadPlaylistDashboards(ctx context.Context, orgID int64, signedInUser *models.SignedInUser, playlistID int64) (dtos.PlaylistDashboardsSlice, error) { - playlistItems, _ := hs.LoadPlaylistItems(ctx, playlistID) +func (hs *HTTPServer) LoadPlaylistDashboards(ctx context.Context, orgID int64, signedInUser *models.SignedInUser, playlistUID string) (dtos.PlaylistDashboardsSlice, error) { + playlistItems, _ := hs.LoadPlaylistItems(ctx, playlistUID, orgID) dashboardByIDs := make([]int64, 0) dashboardByTag := make([]string, 0) diff --git a/pkg/models/playlist.go b/pkg/models/playlist.go index e421f948e58..7be6cba05d0 100644 --- a/pkg/models/playlist.go +++ b/pkg/models/playlist.go @@ -6,12 +6,14 @@ import ( // Typed errors var ( - ErrPlaylistNotFound = errors.New("Playlist not found") + ErrPlaylistNotFound = errors.New("Playlist not found") + ErrPlaylistFailedGenerateUniqueUid = errors.New("failed to generate unique playlist UID") ) // Playlist model type Playlist struct { Id int64 `json:"id"` + UID string `json:"uid" xorm:"uid"` Name string `json:"name"` Interval string `json:"interval"` OrgId int64 `json:"-"` @@ -19,6 +21,7 @@ type Playlist struct { type PlaylistDTO struct { Id int64 `json:"id"` + UID string `json:"uid"` Name string `json:"name"` Interval string `json:"interval"` OrgId int64 `json:"-"` @@ -51,7 +54,7 @@ type Playlists []*Playlist type UpdatePlaylistCommand struct { OrgId int64 `json:"-"` - Id int64 `json:"id"` + UID string `json:"uid"` Name string `json:"name" binding:"Required"` Interval string `json:"interval"` Items []PlaylistItemDTO `json:"items"` @@ -69,7 +72,7 @@ type CreatePlaylistCommand struct { } type DeletePlaylistCommand struct { - Id int64 + UID string OrgId int64 } @@ -85,12 +88,14 @@ type GetPlaylistsQuery struct { Result Playlists } -type GetPlaylistByIdQuery struct { - Id int64 +type GetPlaylistByUidQuery struct { + UID string + OrgId int64 Result *Playlist } -type GetPlaylistItemsByIdQuery struct { - PlaylistId int64 - Result *[]PlaylistItem +type GetPlaylistItemsByUidQuery struct { + PlaylistUID string + OrgId int64 + Result *[]PlaylistItem } diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index b6474644b48..02894d18d87 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -91,6 +91,7 @@ func (*OSSMigrations) AddMigration(mg *Migrator) { accesscontrol.AddManagedPermissionsMigration(mg, accesscontrol.ManagedPermissionsMigrationID) accesscontrol.AddManagedFolderAlertActionsMigration(mg) accesscontrol.AddActionNameMigrator(mg) + addPlaylistUIDMigration(mg) } func addMigrationLogMigrations(mg *Migrator) { diff --git a/pkg/services/sqlstore/migrations/playlist_mig.go b/pkg/services/sqlstore/migrations/playlist_mig.go index 44ae4b6cb0e..9ea365d45c8 100644 --- a/pkg/services/sqlstore/migrations/playlist_mig.go +++ b/pkg/services/sqlstore/migrations/playlist_mig.go @@ -6,18 +6,8 @@ func addPlaylistMigrations(mg *Migrator) { mg.AddMigration("Drop old table playlist table", NewDropTableMigration("playlist")) mg.AddMigration("Drop old table playlist_item table", NewDropTableMigration("playlist_item")) - playlistV2 := Table{ - Name: "playlist", - Columns: []*Column{ - {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, - {Name: "name", Type: DB_NVarchar, Length: 255, Nullable: false}, - {Name: "interval", Type: DB_NVarchar, Length: 255, Nullable: false}, - {Name: "org_id", Type: DB_BigInt, Nullable: false}, - }, - } - // create table - mg.AddMigration("create playlist table v2", NewAddTableMigration(playlistV2)) + mg.AddMigration("create playlist table v2", NewAddTableMigration(playlistV2())) playlistItemV2 := Table{ Name: "playlist_item", @@ -44,3 +34,32 @@ func addPlaylistMigrations(mg *Migrator) { {Name: "title", Type: DB_Text, Nullable: false}, })) } + +func addPlaylistUIDMigration(mg *Migrator) { + // Replacing auto-incremented playlistIDs with string UIDs + mg.AddMigration("Add UID column to playlist", NewAddColumnMigration(playlistV2(), &Column{ + Name: "uid", Type: DB_NVarchar, Length: 80, Nullable: false, Default: "0", + })) + + // copy the (string representation of) existing IDs into the new uid column. + mg.AddMigration("Update uid column values in playlist", NewRawSQLMigration(""). + SQLite("UPDATE playlist SET uid=printf('%d',id);"). + Postgres("UPDATE playlist SET uid=id::text;"). + Mysql("UPDATE playlist SET uid=id;")) + + mg.AddMigration("Add index for uid in playlist", NewAddIndexMigration(playlistV2(), &Index{ + Cols: []string{"org_id", "uid"}, Type: UniqueIndex, + })) +} + +func playlistV2() Table { + return Table{ + Name: "playlist", + Columns: []*Column{ + {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, + {Name: "name", Type: DB_NVarchar, Length: 255, Nullable: false}, + {Name: "interval", Type: DB_NVarchar, Length: 255, Nullable: false}, + {Name: "org_id", Type: DB_BigInt, Nullable: false}, + }, + } +} diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index c49b48b2535..77b73c6948c 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/pkg/services/sqlstore/mockstore/mockstore.go @@ -355,7 +355,7 @@ func (m *SQLStoreMock) UpdatePlaylist(ctx context.Context, cmd *models.UpdatePla return m.ExpectedError } -func (m *SQLStoreMock) GetPlaylist(ctx context.Context, query *models.GetPlaylistByIdQuery) error { +func (m *SQLStoreMock) GetPlaylist(ctx context.Context, query *models.GetPlaylistByUidQuery) error { return m.ExpectedError } @@ -367,7 +367,7 @@ func (m *SQLStoreMock) SearchPlaylists(ctx context.Context, query *models.GetPla return m.ExpectedError } -func (m *SQLStoreMock) GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByIdQuery) error { +func (m *SQLStoreMock) GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByUidQuery) error { return m.ExpectedError } diff --git a/pkg/services/sqlstore/playlist.go b/pkg/services/sqlstore/playlist.go index 49f3aa311c8..9fbc3b02bbb 100644 --- a/pkg/services/sqlstore/playlist.go +++ b/pkg/services/sqlstore/playlist.go @@ -8,13 +8,19 @@ import ( 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) + _, err = sess.Insert(&playlist) if err != nil { return err } @@ -40,33 +46,34 @@ func (ss *SQLStore) CreatePlaylist(ctx context.Context, cmd *models.CreatePlayli func (ss *SQLStore) UpdatePlaylist(ctx context.Context, cmd *models.UpdatePlaylistCommand) error { return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { playlist := models.Playlist{ - Id: cmd.Id, + UID: cmd.UID, OrgId: cmd.OrgId, Name: cmd.Name, Interval: cmd.Interval, } - existingPlaylist := sess.Where("id = ? AND org_id = ?", cmd.Id, cmd.OrgId).Find(models.Playlist{}) - - if existingPlaylist == nil { - return models.ErrPlaylistNotFound + 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.ID(cmd.Id).Cols("name", "interval").Update(&playlist) - + _, 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, cmd.Id) + _, err = sess.Exec(rawSQL, playlist.Id) if err != nil { return err @@ -85,20 +92,18 @@ func (ss *SQLStore) UpdatePlaylist(ctx context.Context, cmd *models.UpdatePlayli } _, err = sess.Insert(&playlistItems) - return err }) } -func (ss *SQLStore) GetPlaylist(ctx context.Context, query *models.GetPlaylistByIdQuery) error { - if query.Id == 0 { +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{} - _, err := sess.ID(query.Id).Get(&playlist) - + playlist := models.Playlist{UID: query.UID, OrgId: query.OrgId} + _, err := sess.Get(&playlist) query.Result = &playlist return err @@ -106,26 +111,35 @@ func (ss *SQLStore) GetPlaylist(ctx context.Context, query *models.GetPlaylistBy } func (ss *SQLStore) DeletePlaylist(ctx context.Context, cmd *models.DeletePlaylistCommand) error { - if cmd.Id == 0 || cmd.OrgId == 0 { + if cmd.UID == "" || cmd.OrgId == 0 { return models.ErrCommandValidationFailed } return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - var rawPlaylistSQL = "DELETE FROM playlist WHERE id = ? and org_id = ?" - _, err := sess.Exec(rawPlaylistSQL, cmd.Id, cmd.OrgId) + 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 = ?" - _, err2 := sess.Exec(rawItemSQL, cmd.Id) + _, err = sess.Exec(rawItemSQL, playlist.Id) - return err2 + 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) @@ -143,17 +157,44 @@ func (ss *SQLStore) SearchPlaylists(ctx context.Context, query *models.GetPlayli }) } -func (ss *SQLStore) GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByIdQuery) error { +func (ss *SQLStore) GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByUidQuery) error { return ss.WithDbSession(ctx, func(sess *DBSession) error { - if query.PlaylistId == 0 { + if query.PlaylistUID == "" || query.OrgId == 0 { return models.ErrCommandValidationFailed } - var playlistItems = make([]models.PlaylistItem, 0) - err := sess.Where("playlist_id=?", query.PlaylistId).Find(&playlistItems) + // 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 index 168245b7869..762b3f93d11 100644 --- a/pkg/services/sqlstore/playlist_test.go +++ b/pkg/services/sqlstore/playlist_test.go @@ -4,8 +4,9 @@ import ( "context" "testing" - "github.com/grafana/grafana/pkg/models" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/models" ) func TestIntegrationPlaylistDataAccess(t *testing.T) { @@ -22,31 +23,39 @@ func TestIntegrationPlaylistDataAccess(t *testing.T) { 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 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, Id: 1, Interval: "10s", Items: items} + 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{Id: 1, OrgId: 1} + deleteQuery := models.DeletePlaylistCommand{UID: uid, OrgId: 1} err = ss.DeletePlaylist(context.Background(), &deleteQuery) require.NoError(t, err) - getQuery := models.GetPlaylistByIdQuery{Id: 1} + getQuery := models.GetPlaylistByUidQuery{UID: uid, OrgId: 1} err = ss.GetPlaylist(context.Background(), &getQuery) require.NoError(t, err) - require.Equal(t, int64(0), getQuery.Result.Id, "playlist should've been removed") + require.Equal(t, uid, getQuery.Result.UID, "playlist should've been removed") }) }) t.Run("Delete playlist that doesn't exist", func(t *testing.T) { - deleteQuery := models.DeletePlaylistCommand{Id: 1, OrgId: 1} + deleteQuery := models.DeletePlaylistCommand{UID: "654312", OrgId: 1} err := ss.DeletePlaylist(context.Background(), &deleteQuery) require.NoError(t, err) }) @@ -57,8 +66,8 @@ func TestIntegrationPlaylistDataAccess(t *testing.T) { cmd models.DeletePlaylistCommand }{ {desc: "none", cmd: models.DeletePlaylistCommand{}}, - {desc: "no OrgId", cmd: models.DeletePlaylistCommand{Id: 1}}, - {desc: "no Id", cmd: models.DeletePlaylistCommand{OrgId: 1}}, + {desc: "no OrgId", cmd: models.DeletePlaylistCommand{UID: "1"}}, + {desc: "no Uid", cmd: models.DeletePlaylistCommand{OrgId: 1}}, } for _, tc := range testCases { diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index ef063540abf..1e3124f148a 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -70,10 +70,10 @@ type Store interface { InTransaction(ctx context.Context, fn func(ctx context.Context) error) error CreatePlaylist(ctx context.Context, cmd *models.CreatePlaylistCommand) error UpdatePlaylist(ctx context.Context, cmd *models.UpdatePlaylistCommand) error - GetPlaylist(ctx context.Context, query *models.GetPlaylistByIdQuery) error + GetPlaylist(ctx context.Context, query *models.GetPlaylistByUidQuery) error DeletePlaylist(ctx context.Context, cmd *models.DeletePlaylistCommand) error SearchPlaylists(ctx context.Context, query *models.GetPlaylistsQuery) error - GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByIdQuery) error + GetPlaylistItem(ctx context.Context, query *models.GetPlaylistItemsByUidQuery) error GetAlertById(ctx context.Context, query *models.GetAlertByIdQuery) error GetAllAlertQueryHandler(ctx context.Context, query *models.GetAllAlertsQuery) error HandleAlertsQuery(ctx context.Context, query *models.GetAlertsQuery) error diff --git a/public/app/features/playlist/PlaylistEditPage.test.tsx b/public/app/features/playlist/PlaylistEditPage.test.tsx index 4815eb4e183..8cf0dbc155c 100644 --- a/public/app/features/playlist/PlaylistEditPage.test.tsx +++ b/public/app/features/playlist/PlaylistEditPage.test.tsx @@ -19,12 +19,12 @@ jest.mock('../../core/components/TagFilter/TagFilter', () => ({ }, })); -async function getTestContext({ name, interval, items }: Partial = {}) { +async function getTestContext({ name, interval, items, uid }: Partial = {}) { jest.clearAllMocks(); - const playlist = { name, items, interval } as unknown as Playlist; + const playlist = { name, items, interval, uid } as unknown as Playlist; const queryParams = {}; const route: any = {}; - const match: any = { params: { id: 1 } }; + const match: any = { params: { uid: 'foo' } }; const location: any = {}; const history: any = {}; const navModel: any = { @@ -37,6 +37,7 @@ async function getTestContext({ name, interval, items }: Partial = {}) name: 'Test Playlist', interval: '5s', items: [{ title: 'First item', type: 'dashboard_by_id', order: 1, value: '1' }], + uid: 'foo', }); const { rerender } = render( { await userEvent.type(screen.getByRole('textbox', { name: /playlist interval/i }), '10s'); fireEvent.submit(screen.getByRole('button', { name: /save/i })); await waitFor(() => expect(putMock).toHaveBeenCalledTimes(1)); - expect(putMock).toHaveBeenCalledWith('/api/playlists/1', { + expect(putMock).toHaveBeenCalledWith('/api/playlists/foo', { name: 'A Name', interval: '10s', items: [{ title: 'First item', type: 'dashboard_by_id', order: 1, value: '1' }], diff --git a/public/app/features/playlist/PlaylistEditPage.tsx b/public/app/features/playlist/PlaylistEditPage.tsx index 15855980dd3..c98ba7ee00d 100644 --- a/public/app/features/playlist/PlaylistEditPage.tsx +++ b/public/app/features/playlist/PlaylistEditPage.tsx @@ -21,16 +21,16 @@ interface ConnectedProps { } export interface RouteParams { - id: number; + uid: string; } interface Props extends ConnectedProps, GrafanaRouteComponentProps {} export const PlaylistEditPage: FC = ({ navModel, match }) => { const styles = useStyles2(getPlaylistStyles); - const { playlist, loading } = usePlaylist(match.params.id); + const { playlist, loading } = usePlaylist(match.params.uid); const onSubmit = async (playlist: Playlist) => { - await updatePlaylist(match.params.id, playlist); + await updatePlaylist(match.params.uid, playlist); locationService.push('/playlists'); }; diff --git a/public/app/features/playlist/PlaylistForm.test.tsx b/public/app/features/playlist/PlaylistForm.test.tsx index eb699483a47..0fcec7b5d3b 100644 --- a/public/app/features/playlist/PlaylistForm.test.tsx +++ b/public/app/features/playlist/PlaylistForm.test.tsx @@ -12,9 +12,9 @@ jest.mock('../../core/components/TagFilter/TagFilter', () => ({ }, })); -function getTestContext({ name, interval, items }: Partial = {}) { +function getTestContext({ name, interval, items, uid }: Partial = {}) { const onSubmitMock = jest.fn(); - const playlist = { name, items, interval } as unknown as Playlist; + const playlist = { name, items, interval, uid } as unknown as Playlist; const { rerender } = render(); return { onSubmitMock, playlist, rerender }; @@ -28,6 +28,7 @@ const playlist: Playlist = { { title: 'Middle item', type: 'dashboard_by_id', order: 2, value: '2' }, { title: 'Last item', type: 'dashboard_by_tag', order: 2, value: 'Last item' }, ], + uid: 'foo', }; function rows() { @@ -135,7 +136,15 @@ describe('PlaylistForm', () => { await userEvent.click(screen.getByRole('button', { name: /save/i })); expect(onSubmitMock).toHaveBeenCalledTimes(1); - expect(onSubmitMock).toHaveBeenCalledWith(playlist); + expect(onSubmitMock).toHaveBeenCalledWith({ + name: 'A test playlist', + interval: '10m', + items: [ + { title: 'First item', type: 'dashboard_by_id', order: 1, value: '1' }, + { title: 'Middle item', type: 'dashboard_by_id', order: 2, value: '2' }, + { title: 'Last item', type: 'dashboard_by_tag', order: 2, value: 'Last item' }, + ], + }); }); describe('and name is missing', () => { diff --git a/public/app/features/playlist/PlaylistPage.test.tsx b/public/app/features/playlist/PlaylistPage.test.tsx index 861bd8759e0..db381034faf 100644 --- a/public/app/features/playlist/PlaylistPage.test.tsx +++ b/public/app/features/playlist/PlaylistPage.test.tsx @@ -69,6 +69,7 @@ describe('PlaylistPage', () => { { title: 'Middle item', type: 'dashboard_by_id', order: 2, value: '2' }, { title: 'Last item', type: 'dashboard_by_tag', order: 2, value: 'Last item' }, ], + uid: 'playlist-0', }, ]); const { getByText } = getTestContext(); diff --git a/public/app/features/playlist/PlaylistPage.tsx b/public/app/features/playlist/PlaylistPage.tsx index 715de58040d..8513249222e 100644 --- a/public/app/features/playlist/PlaylistPage.tsx +++ b/public/app/features/playlist/PlaylistPage.tsx @@ -52,7 +52,7 @@ export const PlaylistPage: FC = ({ navModel }) => { if (!playlistToDelete) { return; } - deletePlaylist(playlistToDelete.id).finally(() => { + deletePlaylist(playlistToDelete.uid).finally(() => { setForcePlaylistsFetch(forcePlaylistsFetch + 1); setPlaylistToDelete(undefined); }); diff --git a/public/app/features/playlist/PlaylistPageList.tsx b/public/app/features/playlist/PlaylistPageList.tsx index d8b77397b05..12cd9319d04 100644 --- a/public/app/features/playlist/PlaylistPageList.tsx +++ b/public/app/features/playlist/PlaylistPageList.tsx @@ -21,7 +21,7 @@ export const PlaylistPageList = ({ playlists, setStartPlaylist, setPlaylistToDel return (
    {playlists!.map((playlist: PlaylistDTO) => ( -
  • +
  • {playlist.name} @@ -33,7 +33,7 @@ export const PlaylistPageList = ({ playlists, setStartPlaylist, setPlaylistToDel iconSize="lg" onClick={() => { showModal(ShareModal, { - playlistId: playlist.id, + playlistUid: playlist.uid, onDismiss: hideModal, }); }} @@ -47,12 +47,12 @@ export const PlaylistPageList = ({ playlists, setStartPlaylist, setPlaylistToDel {contextSrv.isEditor && ( <> - + Edit playlist