From c26e3d80e35352ab4c7f5a0a4da6c1304ecac421 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Tue, 10 Oct 2023 12:46:12 -0700 Subject: [PATCH] Playlist: Add create+update timestamps to the database (#76295) --- .github/CODEOWNERS | 2 +- pkg/services/playlist/model.go | 13 +- pkg/services/playlist/model_test.go | 64 ------ .../playlist/playlistimpl/playlist.go | 13 +- .../playlist/playlistimpl/sqlx_store.go | 201 ------------------ .../playlist/playlistimpl/sqlx_store_test.go | 16 -- .../playlist/playlistimpl/store_test.go | 39 ++++ .../playlist/playlistimpl/xorm_store.go | 53 ++--- .../sqlstore/migrations/playlist_mig.go | 8 + 9 files changed, 81 insertions(+), 328 deletions(-) delete mode 100644 pkg/services/playlist/model_test.go delete mode 100644 pkg/services/playlist/playlistimpl/sqlx_store.go delete mode 100644 pkg/services/playlist/playlistimpl/sqlx_store_test.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 79de8c6fff9..934e37fdc62 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -114,7 +114,7 @@ /pkg/services/navtree/ @grafana/backend-platform /pkg/services/notifications/ @grafana/backend-platform /pkg/services/org/ @grafana/backend-platform -/pkg/services/playlist/ @grafana/backend-platform +/pkg/services/playlist/ @grafana/grafana-app-platform-squad /pkg/services/plugindashboards/ @grafana/backend-platform /pkg/services/preference/ @grafana/backend-platform /pkg/services/provisioning/ @grafana/backend-platform diff --git a/pkg/services/playlist/model.go b/pkg/services/playlist/model.go index 8c1b564793d..f0a0b4b4ae7 100644 --- a/pkg/services/playlist/model.go +++ b/pkg/services/playlist/model.go @@ -8,9 +8,8 @@ import ( // Typed errors var ( - ErrPlaylistNotFound = errors.New("Playlist not found") - ErrPlaylistFailedGenerateUniqueUid = errors.New("failed to generate unique playlist UID") - ErrCommandValidationFailed = errors.New("command missing required fields") + ErrPlaylistNotFound = errors.New("Playlist not found") + ErrCommandValidationFailed = errors.New("command missing required fields") ) // Playlist model @@ -20,6 +19,12 @@ type Playlist struct { Name string `json:"name" db:"name"` Interval string `json:"interval" db:"interval"` OrgId int64 `json:"-" db:"org_id"` + + // Added for kubernetes migration + synchronization + // Hidden from json because this is used for openapi generation + // Using int64 rather than time.Time to avoid database issues with time support + CreatedAt int64 `json:"-" db:"created_at"` + UpdatedAt int64 `json:"-" db:"updated_at"` } type PlaylistDTO = playlist.Spec @@ -54,6 +59,8 @@ type CreatePlaylistCommand struct { Interval string `json:"interval"` Items []PlaylistItem `json:"items"` OrgId int64 `json:"-"` + // Used to create playlists from kubectl with a known uid/name + UID string `json:"-"` } type DeletePlaylistCommand struct { diff --git a/pkg/services/playlist/model_test.go b/pkg/services/playlist/model_test.go deleted file mode 100644 index f0eb722e10f..00000000000 --- a/pkg/services/playlist/model_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package playlist - -import ( - "encoding/json" - "fmt" - "testing" - - "github.com/grafana/grafana/pkg/kinds/playlist" - "github.com/grafana/grafana/pkg/util" - "github.com/stretchr/testify/require" -) - -func TestPlaylistConversion(t *testing.T) { - src := PlaylistDTO{ - Uid: "abc", - Name: "TeamA", - Interval: "10s", - Items: []playlist.Item{ - {Title: util.Pointer("First"), Type: playlist.ItemTypeDashboardByUid, Value: "UID0"}, - {Title: util.Pointer("Second"), Type: playlist.ItemTypeDashboardByTag, Value: "tagA"}, - {Title: util.Pointer("Third"), Type: playlist.ItemTypeDashboardById, Value: "123"}, - }, - } - - dst := PlaylistToResource(src) - - require.Equal(t, "abc", src.Uid) - require.Equal(t, "abc", dst.Metadata.Name) - require.Equal(t, src.Name, dst.Spec.Name) - - out, err := json.MarshalIndent(dst, "", " ") - require.NoError(t, err) - fmt.Printf("%s", string(out)) - require.JSONEq(t, `{ - "apiVersion": "v0-0-alpha", - "kind": "Playlist", - "metadata": { - "name": "abc", - "creationTimestamp": null - }, - "spec": { - "interval": "10s", - "items": [ - { - "title": "First", - "type": "dashboard_by_uid", - "value": "UID0" - }, - { - "title": "Second", - "type": "dashboard_by_tag", - "value": "tagA" - }, - { - "title": "Third", - "type": "dashboard_by_id", - "value": "123" - } - ], - "name": "TeamA", - "uid": "" - } - }`, string(out)) -} diff --git a/pkg/services/playlist/playlistimpl/playlist.go b/pkg/services/playlist/playlistimpl/playlist.go index 054b96a7257..3111d701e58 100644 --- a/pkg/services/playlist/playlistimpl/playlist.go +++ b/pkg/services/playlist/playlistimpl/playlist.go @@ -16,17 +16,8 @@ type Service struct { var _ playlist.Service = &Service{} func ProvideService(db db.DB, toggles featuremgmt.FeatureToggles, objserver entity.EntityStoreServer) playlist.Service { - var sqlstore store - - // 🐢🐢🐢 pick the store - if toggles.IsEnabled(featuremgmt.FlagNewDBLibrary) { - sqlstore = &sqlxStore{ - sess: db.GetSqlxSession(), - } - } else { - sqlstore = &sqlStore{ - db: db, - } + sqlstore := &sqlStore{ + db: db, } return &Service{store: sqlstore} } diff --git a/pkg/services/playlist/playlistimpl/sqlx_store.go b/pkg/services/playlist/playlistimpl/sqlx_store.go deleted file mode 100644 index a6321ae44a0..00000000000 --- a/pkg/services/playlist/playlistimpl/sqlx_store.go +++ /dev/null @@ -1,201 +0,0 @@ -package playlistimpl - -import ( - "context" - "database/sql" - "errors" - - "github.com/grafana/grafana/pkg/services/playlist" - "github.com/grafana/grafana/pkg/services/sqlstore/session" - "github.com/grafana/grafana/pkg/services/star" -) - -type sqlxStore struct { - sess *session.SessionDB -} - -func (s *sqlxStore) Insert(ctx context.Context, cmd *playlist.CreatePlaylistCommand) (*playlist.Playlist, error) { - p := playlist.Playlist{} - var err error - uid, err := newGenerateAndValidateNewPlaylistUid(ctx, s.sess, cmd.OrgId) - if err != nil { - return nil, err - } - - p = playlist.Playlist{ - Name: cmd.Name, - Interval: cmd.Interval, - OrgId: cmd.OrgId, - UID: uid, - } - - err = s.sess.WithTransaction(ctx, func(tx *session.SessionTx) error { - query := `INSERT INTO playlist (name, "interval", org_id, uid) VALUES (?, ?, ?, ?)` - var err error - p.Id, err = tx.ExecWithReturningId(ctx, query, p.Name, p.Interval, p.OrgId, p.UID) - if err != nil { - return err - } - - if len(cmd.Items) > 0 { - playlistItems := make([]playlist.PlaylistItem, 0) - for order, item := range cmd.Items { - playlistItems = append(playlistItems, playlist.PlaylistItem{ - PlaylistId: p.Id, - Type: item.Type, - Value: item.Value, - Order: order + 1, - Title: item.Title, - }) - } - query := `INSERT INTO playlist_item (playlist_id, type, value, title, "order") VALUES (:playlist_id, :type, :value, :title, :order)` - _, err = tx.NamedExec(ctx, query, playlistItems) - if err != nil { - return err - } - } - return nil - }) - - return &p, err -} - -func (s *sqlxStore) Update(ctx context.Context, cmd *playlist.UpdatePlaylistCommand) (*playlist.PlaylistDTO, error) { - dto := playlist.PlaylistDTO{} - - // Get the id of playlist to be updated with orgId and UID - existingPlaylist, err := s.Get(ctx, &playlist.GetPlaylistByUidQuery{UID: cmd.UID, OrgId: cmd.OrgId}) - if err != nil { - return nil, err - } - - // Create object to be update to - p := playlist.Playlist{ - Id: existingPlaylist.Id, - UID: cmd.UID, - OrgId: cmd.OrgId, - Name: cmd.Name, - Interval: cmd.Interval, - } - - err = s.sess.WithTransaction(ctx, func(tx *session.SessionTx) error { - query := `UPDATE playlist SET uid=:uid, org_id=:org_id, name=:name, "interval"=:interval WHERE id=:id` - _, err = tx.NamedExec(ctx, query, p) - if err != nil { - return err - } - - if _, err = tx.Exec(ctx, "DELETE FROM playlist_item WHERE playlist_id = ?", p.Id); err != nil { - return err - } - - playlistItems := make([]playlist.PlaylistItem, 0) - - for index, item := range cmd.Items { - playlistItems = append(playlistItems, playlist.PlaylistItem{ - PlaylistId: p.Id, - Type: item.Type, - Value: item.Value, - Order: index + 1, - Title: item.Title, - }) - } - query = `INSERT INTO playlist_item (playlist_id, type, value, title, "order") VALUES (:playlist_id, :type, :value, :title, :order)` - _, err = tx.NamedExec(ctx, query, playlistItems) - return err - }) - - return &dto, err -} - -func (s *sqlxStore) Get(ctx context.Context, query *playlist.GetPlaylistByUidQuery) (*playlist.Playlist, error) { - if query.UID == "" || query.OrgId == 0 { - return nil, playlist.ErrCommandValidationFailed - } - - p := playlist.Playlist{} - err := s.sess.Get(ctx, &p, "SELECT * FROM playlist WHERE uid=? AND org_id=?", query.UID, query.OrgId) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, playlist.ErrPlaylistNotFound - } - return nil, err - } - return &p, err -} - -func (s *sqlxStore) Delete(ctx context.Context, cmd *playlist.DeletePlaylistCommand) error { - if cmd.UID == "" || cmd.OrgId == 0 { - return playlist.ErrCommandValidationFailed - } - - p := playlist.Playlist{} - if err := s.sess.Get(ctx, &p, "SELECT * FROM playlist WHERE uid=? AND org_id=?", cmd.UID, cmd.OrgId); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil - } - return err - } - - err := s.sess.WithTransaction(ctx, func(tx *session.SessionTx) error { - if _, err := tx.Exec(ctx, "DELETE FROM playlist WHERE uid = ? and org_id = ?", cmd.UID, cmd.OrgId); err != nil { - return err - } - - if _, err := tx.Exec(ctx, "DELETE FROM playlist_item WHERE playlist_id = ?", p.Id); err != nil { - return err - } - return nil - }) - - return err -} - -func (s *sqlxStore) List(ctx context.Context, query *playlist.GetPlaylistsQuery) (playlist.Playlists, error) { - playlists := make(playlist.Playlists, 0) - if query.OrgId == 0 { - return playlists, playlist.ErrCommandValidationFailed - } - - var err error - if query.Name == "" { - err = s.sess.Select( - ctx, &playlists, "SELECT * FROM playlist WHERE org_id = ? LIMIT ?", query.OrgId, query.Limit) - } else { - err = s.sess.Select( - ctx, &playlists, "SELECT * FROM playlist WHERE org_id = ? AND name LIKE ? LIMIT ?", query.OrgId, "%"+query.Name+"%", query.Limit) - } - return playlists, err -} - -func (s *sqlxStore) GetItems(ctx context.Context, query *playlist.GetPlaylistItemsByUidQuery) ([]playlist.PlaylistItem, error) { - var playlistItems = make([]playlist.PlaylistItem, 0) - if query.PlaylistUID == "" || query.OrgId == 0 { - return playlistItems, star.ErrCommandValidationFailed - } - - var p = playlist.Playlist{} - err := s.sess.Get(ctx, &p, "SELECT * FROM playlist WHERE uid=? AND org_id=?", query.PlaylistUID, query.OrgId) - if err != nil { - return playlistItems, err - } - - err = s.sess.Select(ctx, &playlistItems, "SELECT * FROM playlist_item WHERE playlist_id=?", p.Id) - return playlistItems, err -} - -func newGenerateAndValidateNewPlaylistUid(ctx context.Context, sess *session.SessionDB, orgId int64) (string, error) { - for i := 0; i < 3; i++ { - uid := generateNewUid() - p := playlist.Playlist{OrgId: orgId, UID: uid} - err := sess.Get(ctx, &p, "SELECT * FROM playlist WHERE uid=? AND org_id=?", uid, orgId) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - return uid, nil - } - return "", err - } - } - - return "", playlist.ErrPlaylistFailedGenerateUniqueUid -} diff --git a/pkg/services/playlist/playlistimpl/sqlx_store_test.go b/pkg/services/playlist/playlistimpl/sqlx_store_test.go deleted file mode 100644 index 5b77bd5b850..00000000000 --- a/pkg/services/playlist/playlistimpl/sqlx_store_test.go +++ /dev/null @@ -1,16 +0,0 @@ -package playlistimpl - -import ( - "testing" - - "github.com/grafana/grafana/pkg/infra/db" -) - -func TestIntegrationSQLxPlaylistDataAccess(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - testIntegrationPlaylistDataAccess(t, func(ss db.DB) store { - return &sqlxStore{sess: ss.GetSqlxSession()} - }) -} diff --git a/pkg/services/playlist/playlistimpl/store_test.go b/pkg/services/playlist/playlistimpl/store_test.go index 1bb2d82d0ba..b2d79b59c83 100644 --- a/pkg/services/playlist/playlistimpl/store_test.go +++ b/pkg/services/playlist/playlistimpl/store_test.go @@ -3,6 +3,7 @@ package playlistimpl import ( "context" "testing" + "time" "github.com/stretchr/testify/require" @@ -15,6 +16,7 @@ type getStore func(db.DB) store func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { t.Helper() + start := time.Now().UnixMilli() ss := db.InitTestDB(t) playlistStore := fn(ss) @@ -33,6 +35,8 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { pl, err := playlistStore.Get(context.Background(), get) require.NoError(t, err) require.Equal(t, p.Id, pl.Id) + require.GreaterOrEqual(t, pl.CreatedAt, start) + require.GreaterOrEqual(t, pl.UpdatedAt, start) }) t.Run("Can get playlist items", func(t *testing.T) { @@ -43,6 +47,7 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { }) t.Run("Can update playlist", func(t *testing.T) { + time.Sleep(time.Millisecond * 2) items := []playlist.PlaylistItem{ {Title: "influxdb", Value: "influxdb", Type: "dashboard_by_tag"}, {Title: "Backend response times", Value: "2", Type: "dashboard_by_id"}, @@ -50,6 +55,14 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { query := playlist.UpdatePlaylistCommand{Name: "NYC office ", OrgId: 1, UID: uid, Interval: "10s", Items: items} _, err = playlistStore.Update(context.Background(), &query) require.NoError(t, err) + + // Now check that UpdatedAt has increased + pl, err := playlistStore.Get(context.Background(), &playlist.GetPlaylistByUidQuery{UID: uid, OrgId: 1}) + require.NoError(t, err) + require.Equal(t, p.Id, pl.Id) + require.Equal(t, p.CreatedAt, pl.CreatedAt) + require.Greater(t, pl.UpdatedAt, p.UpdatedAt) + require.Greater(t, pl.UpdatedAt, pl.CreatedAt) }) t.Run("Can remove playlist", func(t *testing.T) { @@ -64,6 +77,32 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { }) }) + t.Run("Can create playlist with known UID", func(t *testing.T) { + items := []playlist.PlaylistItem{ + {Title: "graphite", Value: "graphite", Type: "dashboard_by_tag"}, + {Title: "Backend response times", Value: "3", Type: "dashboard_by_id"}, + } + cmd := playlist.CreatePlaylistCommand{Name: "NYC office", Interval: "10m", OrgId: 1, + Items: items, + UID: "abcd", + } + p, err := playlistStore.Insert(context.Background(), &cmd) + require.NoError(t, err) + require.Equal(t, "abcd", p.UID) + + // Should get an error with an invalid UID + cmd.UID = "invalid uid" + _, err = playlistStore.Insert(context.Background(), &cmd) + require.Error(t, err) + + // cleanup + err = playlistStore.Delete(context.Background(), &playlist.DeletePlaylistCommand{ + OrgId: 1, + UID: "abcd", + }) + require.NoError(t, err) + }) + t.Run("Search playlist", func(t *testing.T) { items := []playlist.PlaylistItem{ {Title: "graphite", Value: "graphite", Type: "dashboard_by_tag"}, diff --git a/pkg/services/playlist/playlistimpl/xorm_store.go b/pkg/services/playlist/playlistimpl/xorm_store.go index 218588dbeba..871252d3a65 100644 --- a/pkg/services/playlist/playlistimpl/xorm_store.go +++ b/pkg/services/playlist/playlistimpl/xorm_store.go @@ -2,6 +2,7 @@ package playlistimpl import ( "context" + "time" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/playlist" @@ -13,22 +14,31 @@ type sqlStore struct { db db.DB } +var _ store = &sqlStore{} + func (s *sqlStore) Insert(ctx context.Context, cmd *playlist.CreatePlaylistCommand) (*playlist.Playlist, error) { p := playlist.Playlist{} - err := s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { - uid, err := generateAndValidateNewPlaylistUid(sess, cmd.OrgId) + if cmd.UID == "" { + cmd.UID = util.GenerateShortUID() + } else { + err := util.ValidateUID(cmd.UID) if err != nil { - return err + return nil, err } + } + err := s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + ts := time.Now().UnixMilli() p = playlist.Playlist{ - Name: cmd.Name, - Interval: cmd.Interval, - OrgId: cmd.OrgId, - UID: uid, + Name: cmd.Name, + Interval: cmd.Interval, + OrgId: cmd.OrgId, + UID: cmd.UID, + CreatedAt: ts, + UpdatedAt: ts, } - _, err = sess.Insert(&p) + _, err := sess.Insert(&p) if err != nil { return err } @@ -67,6 +77,8 @@ func (s *sqlStore) Update(ctx context.Context, cmd *playlist.UpdatePlaylistComma return err } p.Id = existingPlaylist.Id + p.CreatedAt = existingPlaylist.CreatedAt + p.UpdatedAt = time.Now().UnixMilli() dto = playlist.PlaylistDTO{ Uid: p.UID, @@ -74,7 +86,7 @@ func (s *sqlStore) Update(ctx context.Context, cmd *playlist.UpdatePlaylistComma Interval: p.Interval, } - _, err = sess.Where("id=?", p.Id).Cols("name", "interval").Update(&p) + _, err = sess.Where("id=?", p.Id).Cols("name", "interval", "updated_at").Update(&p) if err != nil { return err } @@ -187,26 +199,3 @@ func (s *sqlStore) GetItems(ctx context.Context, query *playlist.GetPlaylistItem }) return playlistItems, 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 *db.Session, orgId int64) (string, error) { - for i := 0; i < 3; i++ { - uid := generateNewUid() - - playlist := playlist.Playlist{OrgId: orgId, UID: uid} - exists, err := sess.Get(&playlist) - if err != nil { - return "", err - } - - if !exists { - return uid, nil - } - } - - return "", playlist.ErrPlaylistFailedGenerateUniqueUid -} - -var generateNewUid func() string = util.GenerateShortUID diff --git a/pkg/services/sqlstore/migrations/playlist_mig.go b/pkg/services/sqlstore/migrations/playlist_mig.go index 9ea365d45c8..443de889215 100644 --- a/pkg/services/sqlstore/migrations/playlist_mig.go +++ b/pkg/services/sqlstore/migrations/playlist_mig.go @@ -33,6 +33,14 @@ func addPlaylistMigrations(mg *Migrator) { {Name: "value", Type: DB_Text, Nullable: false}, {Name: "title", Type: DB_Text, Nullable: false}, })) + + // Add columns used for kubernetes dual write synchronization + mg.AddMigration("Add playlist column created_at", NewAddColumnMigration(playlistV2(), &Column{ + Name: "created_at", Type: DB_BigInt, Nullable: false, Default: "0", + })) + mg.AddMigration("Add playlist column updated_at", NewAddColumnMigration(playlistV2(), &Column{ + Name: "updated_at", Type: DB_BigInt, Nullable: false, Default: "0", + })) } func addPlaylistUIDMigration(mg *Migrator) {