public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

backend: add PlaylistUIDs to Playlist; remove playlist IDs from API (#49609) (#50828)

Browse Source 
* backend/api: refactor PlaylistId to PlaylistUid
* Add org_id to Get and Update playlist functions
Fix migration - no longer pad the uid; fix mysql syntax

The relevant tests are passing using postgres, mysql and the default sqllite backends, but there are a number of other failing tests when using postgres and myself so I'm not entirely confident with those results.

* fix bad query in GetPlaylistItem and add a test that would have caught the mistake in the first place. Reverted the playlist_uid column addition in playlist_item; it became unnecessary after this PR.

Added default value to the new UID column based on PR feedback.

* break this PRs migration into its own function

* Playlists: Update UI to use the updated API

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
(cherry picked from commit a33a023629)

Co-authored-by: Kristin Laemmert <mildwonkey@users.noreply.github.com>
This commit is contained in:
Grot (@grafanabot)
2022-06-14 15:48:44 -04:00
committed by GitHub
co-authored by Kristin Laemmert
parent d78c04c8e6
commit 8ef7fd3859
26 changed files with 231 additions and 163 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/sqlstore/migrations/migrations.go
+1
View File
@@ -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) {
pkg/services/sqlstore/migrations/playlist_mig.go
+30 -11
View File
@@ -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},
},
}
}
pkg/services/sqlstore/mockstore/mockstore.go
+2 -2
View File
@@ -375,7 +375,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
}
@@ -387,7 +387,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
}
pkg/services/sqlstore/playlist.go
+65 -24
View File
@@ -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
}
pkg/services/sqlstore/playlist_test.go
+17 -8
View File
@@ -7,8 +7,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 {
pkg/services/sqlstore/store.go
+2 -2
View File
@@ -80,10 +80,10 @@ type Store interface {
GetDashboardAclInfoList(ctx context.Context, query *models.GetDashboardAclInfoListQuery) 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