From fd01161bcc647794e46df375216293fb2f0c9ec1 Mon Sep 17 00:00:00 2001 From: ying-jeanne <74549700+ying-jeanne@users.noreply.github.com> Date: Thu, 25 Aug 2022 16:04:16 -0500 Subject: [PATCH] Chore: replace xorm by sqlx in dashboardversion service (#53869) --- pkg/components/simplejson/simplejson.go | 22 ++++ .../dashverimpl/sqlx_store.go | 87 ++++++++++++++ .../dashverimpl/sqlx_store_test.go | 15 +++ .../dashboardversion/dashverimpl/store.go | 101 ---------------- .../dashverimpl/store_test.go | 31 ++--- .../dashverimpl/xorm_store.go | 108 ++++++++++++++++++ .../dashverimpl/xorm_store_test.go | 16 +++ pkg/services/dashboardversion/model.go | 38 +++--- pkg/services/sqlstore/session/session.go | 2 + 9 files changed, 278 insertions(+), 142 deletions(-) create mode 100644 pkg/services/dashboardversion/dashverimpl/sqlx_store.go create mode 100644 pkg/services/dashboardversion/dashverimpl/sqlx_store_test.go create mode 100644 pkg/services/dashboardversion/dashverimpl/xorm_store.go create mode 100644 pkg/services/dashboardversion/dashverimpl/xorm_store_test.go diff --git a/pkg/components/simplejson/simplejson.go b/pkg/components/simplejson/simplejson.go index 75249fb424d..bd7617d8580 100644 --- a/pkg/components/simplejson/simplejson.go +++ b/pkg/components/simplejson/simplejson.go @@ -7,6 +7,7 @@ package simplejson import ( "bytes" + "database/sql/driver" "encoding/json" "errors" "fmt" @@ -38,6 +39,27 @@ func (j *Json) ToDB() ([]byte, error) { return j.Encode() } +func (j *Json) Scan(val interface{}) error { + switch v := val.(type) { + case []byte: + if len(v) == 0 { + return nil + } + return json.Unmarshal(v, &j) + case string: + if len(v) == 0 { + return nil + } + return json.Unmarshal([]byte(v), &j) + default: + return fmt.Errorf("unsupported type: %T", v) + } +} + +func (j *Json) Value() (driver.Value, error) { + return j.ToDB() +} + // NewJson returns a pointer to a new `Json` object // after unmarshaling `body` bytes func NewJson(body []byte) (*Json, error) { diff --git a/pkg/services/dashboardversion/dashverimpl/sqlx_store.go b/pkg/services/dashboardversion/dashverimpl/sqlx_store.go new file mode 100644 index 00000000000..11bca594f16 --- /dev/null +++ b/pkg/services/dashboardversion/dashverimpl/sqlx_store.go @@ -0,0 +1,87 @@ +package dashverimpl + +import ( + "context" + "database/sql" + "errors" + "strings" + + dashver "github.com/grafana/grafana/pkg/services/dashboardversion" + "github.com/grafana/grafana/pkg/services/sqlstore/session" +) + +type sqlxStore struct { + sess *session.SessionDB +} + +func (ss *sqlxStore) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) { + var version dashver.DashboardVersion + qr := `SELECT dashboard_version.* + FROM dashboard_version + LEFT JOIN dashboard ON dashboard.id=dashboard_version.dashboard_id + WHERE dashboard_version.dashboard_id=? AND dashboard_version.version=? AND dashboard.org_id=? + ` + err := ss.sess.Get(ctx, &version, qr, query.DashboardID, query.Version, query.OrgID) + if err != nil && errors.Is(err, sql.ErrNoRows) { + return nil, dashver.ErrDashboardVersionNotFound + } + return &version, err +} + +func (ss *sqlxStore) GetBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, perBatch int, versionsToKeep int) ([]interface{}, error) { + var versionIds []interface{} + versionIdsToDeleteQuery := `SELECT id + FROM dashboard_version, ( + SELECT dashboard_id, count(version) as count, min(version) as min + FROM dashboard_version + GROUP BY dashboard_id + ) AS vtd + WHERE dashboard_version.dashboard_id=vtd.dashboard_id + AND version < vtd.min + vtd.count - ? + LIMIT ?` + err := ss.sess.Get(ctx, &versionIds, versionIdsToDeleteQuery, versionsToKeep, perBatch) + return versionIds, err +} + +// This service is used by cleanup which need to belong to the same transaction +// Here we need to make sure that the transaction is shared between services +func (ss *sqlxStore) DeleteBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, versionIdsToDelete []interface{}) (int64, error) { + var deleted int64 + err := ss.sess.WithTransaction(ctx, func(tx *session.SessionTx) error { + deleteExpiredSQL := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` + expiredResponse, err := tx.Exec(ctx, deleteExpiredSQL, versionIdsToDelete...) + if err != nil { + return err + } + deleted, err = expiredResponse.RowsAffected() + return err + }) + return deleted, err +} + +func (ss *sqlxStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { + var dashboardVersion []*dashver.DashboardVersionDTO + qr := `SELECT dashboard_version.id, + dashboard_version.dashboard_id, + dashboard_version.parent_version, + dashboard_version.restored_from, + dashboard_version.version, + dashboard_version.created, + dashboard_version.message, + "user".login as created_by_login + FROM dashboard_version + LEFT JOIN "user" ON "user".id = dashboard_version.created_by + LEFT JOIN dashboard ON dashboard.id = dashboard_version.dashboard_id + WHERE dashboard_version.dashboard_id=? AND dashboard.org_id=? + ORDER BY dashboard_version.version DESC + LIMIT ? OFFSET ?` + + err := ss.sess.Select(ctx, &dashboardVersion, qr, query.DashboardID, query.OrgID, query.Limit, query.Start) + if err != nil { + return nil, err + } + if len(dashboardVersion) < 1 { + return nil, dashver.ErrNoVersionsForDashboardID + } + return dashboardVersion, nil +} diff --git a/pkg/services/dashboardversion/dashverimpl/sqlx_store_test.go b/pkg/services/dashboardversion/dashverimpl/sqlx_store_test.go new file mode 100644 index 00000000000..b66d2495635 --- /dev/null +++ b/pkg/services/dashboardversion/dashverimpl/sqlx_store_test.go @@ -0,0 +1,15 @@ +package dashverimpl + +import ( + "testing" + + "github.com/grafana/grafana/pkg/services/sqlstore" +) + +func TestIntegrationSQLxGetDashboardVersion(t *testing.T) { + testIntegrationGetDashboardVersion(t, func(ss *sqlstore.SQLStore) store { + return &sqlxStore{ + sess: ss.GetSqlxSession(), + } + }) +} diff --git a/pkg/services/dashboardversion/dashverimpl/store.go b/pkg/services/dashboardversion/dashverimpl/store.go index 8a5f86881a9..d378a5eb663 100644 --- a/pkg/services/dashboardversion/dashverimpl/store.go +++ b/pkg/services/dashboardversion/dashverimpl/store.go @@ -2,12 +2,8 @@ package dashverimpl import ( "context" - "strings" dashver "github.com/grafana/grafana/pkg/services/dashboardversion" - "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/services/sqlstore/db" - "github.com/grafana/grafana/pkg/services/sqlstore/migrator" ) type store interface { @@ -16,100 +12,3 @@ type store interface { DeleteBatch(context.Context, *dashver.DeleteExpiredVersionsCommand, []interface{}) (int64, error) List(context.Context, *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) } - -type sqlStore struct { - db db.DB - dialect migrator.Dialect -} - -func (ss *sqlStore) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) { - var version dashver.DashboardVersion - err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - has, err := sess.Where("dashboard_version.dashboard_id=? AND dashboard_version.version=? AND dashboard.org_id=?", query.DashboardID, query.Version, query.OrgID). - Join("LEFT", "dashboard", `dashboard.id = dashboard_version.dashboard_id`). - Get(&version) - - if err != nil { - return err - } - - if !has { - return dashver.ErrDashboardVersionNotFound - } - return nil - }) - if err != nil { - return nil, err - } - return &version, nil -} - -func (ss *sqlStore) GetBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, perBatch int, versionsToKeep int) ([]interface{}, error) { - var versionIds []interface{} - err := ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - versionIdsToDeleteQuery := `SELECT id - FROM dashboard_version, ( - SELECT dashboard_id, count(version) as count, min(version) as min - FROM dashboard_version - GROUP BY dashboard_id - ) AS vtd - WHERE dashboard_version.dashboard_id=vtd.dashboard_id - AND version < vtd.min + vtd.count - ? - LIMIT ?` - - err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep, perBatch).Find(&versionIds) - return err - }) - return versionIds, err -} - -func (ss *sqlStore) DeleteBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, versionIdsToDelete []interface{}) (int64, error) { - var deleted int64 - err := ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - deleteExpiredSQL := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` - sqlOrArgs := append([]interface{}{deleteExpiredSQL}, versionIdsToDelete...) - expiredResponse, err := sess.Exec(sqlOrArgs...) - if err != nil { - return err - } - - deleted, err = expiredResponse.RowsAffected() - return err - }) - return deleted, err -} - -func (ss *sqlStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { - var dashboardVersion []*dashver.DashboardVersionDTO - err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - err := sess.Table("dashboard_version"). - Select(`dashboard_version.id, - dashboard_version.dashboard_id, - dashboard_version.parent_version, - dashboard_version.restored_from, - dashboard_version.version, - dashboard_version.created, - dashboard_version.created_by as created_by_id, - dashboard_version.message, - dashboard_version.data,`+ - ss.dialect.Quote("user")+`.login as created_by`). - Join("LEFT", ss.dialect.Quote("user"), `dashboard_version.created_by = `+ss.dialect.Quote("user")+`.id`). - Join("LEFT", "dashboard", `dashboard.id = dashboard_version.dashboard_id`). - Where("dashboard_version.dashboard_id=? AND dashboard.org_id=?", query.DashboardID, query.OrgID). - OrderBy("dashboard_version.version DESC"). - Limit(query.Limit, query.Start). - Find(&dashboardVersion) - if err != nil { - return err - } - - if len(dashboardVersion) < 1 { - return dashver.ErrNoVersionsForDashboardID - } - return nil - }) - if err != nil { - return nil, err - } - return dashboardVersion, nil -} diff --git a/pkg/services/dashboardversion/dashverimpl/store_test.go b/pkg/services/dashboardversion/dashverimpl/store_test.go index d696ef4e2a0..0e31e578c21 100644 --- a/pkg/services/dashboardversion/dashverimpl/store_test.go +++ b/pkg/services/dashboardversion/dashverimpl/store_test.go @@ -16,12 +16,14 @@ import ( "github.com/stretchr/testify/require" ) -func TestIntegrationGetDashboardVersion(t *testing.T) { +type getStore func(*sqlstore.SQLStore) store + +func testIntegrationGetDashboardVersion(t *testing.T, fn getStore) { if testing.Short() { t.Skip("skipping integration test") } ss := sqlstore.InitTestDB(t) - dashVerStore := sqlStore{db: ss} + dashVerStore := fn(ss) t.Run("Get a Dashboard ID and version ID", func(t *testing.T) { savedDash := insertTestDashboard(t, ss, "test dash 26", 1, 0, false, "diff") @@ -60,21 +62,12 @@ func TestIntegrationGetDashboardVersion(t *testing.T) { require.Error(t, err) assert.Equal(t, dashver.ErrDashboardVersionNotFound, err) }) -} - -func TestIntegrationDeleteExpiredVersions(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - versionsToWrite := 10 - ss := sqlstore.InitTestDB(t) - dashVerStore := sqlStore{db: ss} - - for i := 0; i < versionsToWrite-1; i++ { - insertTestDashboard(t, ss, "test dash 53", 1, int64(i), false, "diff-all") - } t.Run("Clean up old dashboard versions", func(t *testing.T) { + versionsToWrite := 10 + for i := 0; i < versionsToWrite-1; i++ { + insertTestDashboard(t, ss, "test dash 53", 1, int64(i), false, "diff-all") + } versionIDsToDelete := []interface{}{1, 2, 3, 4} res, err := dashVerStore.DeleteBatch( context.Background(), @@ -84,16 +77,8 @@ func TestIntegrationDeleteExpiredVersions(t *testing.T) { require.Nil(t, err) assert.EqualValues(t, 4, res) }) -} -func TestIntegrationListDashboardVersions(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - ss := sqlstore.InitTestDB(t) - dashVerStore := sqlStore{db: ss, dialect: ss.Dialect} savedDash := insertTestDashboard(t, ss, "test dash 43", 1, 0, false, "diff-all") - t.Run("Get all versions for a given Dashboard ID", func(t *testing.T) { query := dashver.ListDashboardVersionsQuery{ DashboardID: savedDash.Id, diff --git a/pkg/services/dashboardversion/dashverimpl/xorm_store.go b/pkg/services/dashboardversion/dashverimpl/xorm_store.go new file mode 100644 index 00000000000..20ffd2f572b --- /dev/null +++ b/pkg/services/dashboardversion/dashverimpl/xorm_store.go @@ -0,0 +1,108 @@ +package dashverimpl + +import ( + "context" + "strings" + + dashver "github.com/grafana/grafana/pkg/services/dashboardversion" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/services/sqlstore/db" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" +) + +type sqlStore struct { + db db.DB + dialect migrator.Dialect +} + +func (ss *sqlStore) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) { + var version dashver.DashboardVersion + err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { + has, err := sess.Where("dashboard_version.dashboard_id=? AND dashboard_version.version=? AND dashboard.org_id=?", query.DashboardID, query.Version, query.OrgID). + Join("LEFT", "dashboard", `dashboard.id = dashboard_version.dashboard_id`). + Get(&version) + + if err != nil { + return err + } + + if !has { + return dashver.ErrDashboardVersionNotFound + } + return nil + }) + if err != nil { + return nil, err + } + return &version, nil +} + +func (ss *sqlStore) GetBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, perBatch int, versionsToKeep int) ([]interface{}, error) { + var versionIds []interface{} + err := ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + versionIdsToDeleteQuery := `SELECT id + FROM dashboard_version, ( + SELECT dashboard_id, count(version) as count, min(version) as min + FROM dashboard_version + GROUP BY dashboard_id + ) AS vtd + WHERE dashboard_version.dashboard_id=vtd.dashboard_id + AND version < vtd.min + vtd.count - ? + LIMIT ?` + + err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep, perBatch).Find(&versionIds) + return err + }) + return versionIds, err +} + +func (ss *sqlStore) DeleteBatch(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand, versionIdsToDelete []interface{}) (int64, error) { + var deleted int64 + err := ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + deleteExpiredSQL := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` + sqlOrArgs := append([]interface{}{deleteExpiredSQL}, versionIdsToDelete...) + expiredResponse, err := sess.Exec(sqlOrArgs...) + if err != nil { + return err + } + + deleted, err = expiredResponse.RowsAffected() + return err + }) + return deleted, err +} + +func (ss *sqlStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { + var dashboardVersion []*dashver.DashboardVersionDTO + err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { + err := sess.Table("dashboard_version"). + Select(`dashboard_version.id, + dashboard_version.dashboard_id, + dashboard_version.parent_version, + dashboard_version.restored_from, + dashboard_version.version, + dashboard_version.created, + dashboard_version.created_by as created_by_id, + dashboard_version.message, + dashboard_version.data,`+ + ss.dialect.Quote("user")+`.login as created_by`). + Join("LEFT", ss.dialect.Quote("user"), `dashboard_version.created_by = `+ss.dialect.Quote("user")+`.id`). + Join("LEFT", "dashboard", `dashboard.id = dashboard_version.dashboard_id`). + Where("dashboard_version.dashboard_id=? AND dashboard.org_id=?", query.DashboardID, query.OrgID). + OrderBy("dashboard_version.version DESC"). + Limit(query.Limit, query.Start). + Find(&dashboardVersion) + if err != nil { + return err + } + + if len(dashboardVersion) < 1 { + return dashver.ErrNoVersionsForDashboardID + } + return nil + }) + if err != nil { + return nil, err + } + return dashboardVersion, nil +} diff --git a/pkg/services/dashboardversion/dashverimpl/xorm_store_test.go b/pkg/services/dashboardversion/dashverimpl/xorm_store_test.go new file mode 100644 index 00000000000..3619b65dd51 --- /dev/null +++ b/pkg/services/dashboardversion/dashverimpl/xorm_store_test.go @@ -0,0 +1,16 @@ +package dashverimpl + +import ( + "testing" + + "github.com/grafana/grafana/pkg/services/sqlstore" +) + +func TestIntegrationXORMGetDashboardVersion(t *testing.T) { + testIntegrationGetDashboardVersion(t, func(ss *sqlstore.SQLStore) store { + return &sqlStore{ + db: ss, + dialect: ss.GetDialect(), + } + }) +} diff --git a/pkg/services/dashboardversion/model.go b/pkg/services/dashboardversion/model.go index 4141430f529..86c3756da47 100644 --- a/pkg/services/dashboardversion/model.go +++ b/pkg/services/dashboardversion/model.go @@ -13,17 +13,17 @@ var ( ) type DashboardVersion struct { - ID int64 `json:"id" xorm:"pk autoincr 'id'"` - DashboardID int64 `json:"dashboardId" xorm:"dashboard_id"` - ParentVersion int `json:"parentVersion"` - RestoredFrom int `json:"restoredFrom"` - Version int `json:"version"` + ID int64 `json:"id" xorm:"pk autoincr 'id'" db:"id"` + DashboardID int64 `json:"dashboardId" xorm:"dashboard_id" db:"dashboard_id"` + ParentVersion int `json:"parentVersion" db:"parent_version"` + RestoredFrom int `json:"restoredFrom" db:"restored_from"` + Version int `json:"version" db:"version"` - Created time.Time `json:"created"` - CreatedBy int64 `json:"createdBy"` + Created time.Time `json:"created" db:"created"` + CreatedBy int64 `json:"createdBy" db:"created_by"` - Message string `json:"message"` - Data *simplejson.Json `json:"data"` + Message string `json:"message" db:"message"` + Data *simplejson.Json `json:"data" db:"data"` } type GetDashboardVersionQuery struct { @@ -45,15 +45,17 @@ type ListDashboardVersionsQuery struct { } type DashboardVersionDTO struct { - ID int64 `json:"id" xorm:"id"` - DashboardID int64 `json:"dashboardId" xorm:"dashboard_id"` - DashboardUID string `json:"dashboardUid" xorm:"dashboard_uid"` - ParentVersion int `json:"parentVersion"` - RestoredFrom int `json:"restoredFrom"` - Version int `json:"version"` - Created time.Time `json:"created"` - CreatedBy string `json:"createdBy"` - Message string `json:"message"` + ID int64 `json:"id" xorm:"id" db:"id"` + DashboardID int64 `json:"dashboardId" xorm:"dashboard_id" db:"dashboard_id"` + DashboardUID string `json:"dashboardUid" xorm:"dashboard_uid" db:"dashboard_uid"` + ParentVersion int `json:"parentVersion" db:"parent_version"` + RestoredFrom int `json:"restoredFrom" db:"restored_from"` + Version int `json:"version" db:"version"` + Created time.Time `json:"created" db:"created"` + // Since we get created by with left join user table, this can be null technically, + // but in reality it will always be set, when database is not corrupted. + CreatedBy *string `json:"createdBy" db:"created_by_login"` + Message string `json:"message" db:"message"` } // DashboardVersionMeta extends the dashboard version model with the names diff --git a/pkg/services/sqlstore/session/session.go b/pkg/services/sqlstore/session/session.go index 3f548693d2d..9cf72b7eadc 100644 --- a/pkg/services/sqlstore/session/session.go +++ b/pkg/services/sqlstore/session/session.go @@ -48,6 +48,8 @@ func (gs *SessionDB) Beginx() (*SessionTx, error) { } func (gs *SessionDB) WithTransaction(ctx context.Context, callback func(*SessionTx) error) error { + // Instead of begin a transaction, we need to check the transaction in context, if it exists, + // we can reuse it. tx, err := gs.Beginx() if err != nil { return err