Chore: replace xorm by sqlx in dashboardversion service (#53869)

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) {

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
+}

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(),
+		}
+	})
+}

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
-}

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,

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
+}

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(),
+		}
+	})
+}

pkg/services/dashboardversion/model.go

@@ -13,17 +13,17 @@ var (
 )
 
 type DashboardVersion struct {
-	ID            int64 `json:"id" xorm:"pk autoincr 'id'"`
+	ID            int64 `json:"id" xorm:"pk autoincr 'id'" db:"id"`
-	DashboardID   int64 `json:"dashboardId"  xorm:"dashboard_id"`
+	DashboardID   int64 `json:"dashboardId"  xorm:"dashboard_id" db:"dashboard_id"`
-	ParentVersion int   `json:"parentVersion"`
+	ParentVersion int   `json:"parentVersion" db:"parent_version"`
-	RestoredFrom  int   `json:"restoredFrom"`
+	RestoredFrom  int   `json:"restoredFrom" db:"restored_from"`
-	Version       int   `json:"version"`
+	Version       int   `json:"version" db:"version"`
 
-	Created   time.Time `json:"created"`
+	Created   time.Time `json:"created" db:"created"`
-	CreatedBy int64     `json:"createdBy"`
+	CreatedBy int64     `json:"createdBy" db:"created_by"`
 
-	Message string           `json:"message"`
+	Message string           `json:"message" db:"message"`
-	Data    *simplejson.Json `json:"data"`
+	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"`
+	ID            int64     `json:"id" xorm:"id" db:"id"`
-	DashboardID   int64     `json:"dashboardId" xorm:"dashboard_id"`
+	DashboardID   int64     `json:"dashboardId" xorm:"dashboard_id" db:"dashboard_id"`
-	DashboardUID  string    `json:"dashboardUid" xorm:"dashboard_uid"`
+	DashboardUID  string    `json:"dashboardUid" xorm:"dashboard_uid" db:"dashboard_uid"`
-	ParentVersion int       `json:"parentVersion"`
+	ParentVersion int       `json:"parentVersion" db:"parent_version"`
-	RestoredFrom  int       `json:"restoredFrom"`
+	RestoredFrom  int       `json:"restoredFrom" db:"restored_from"`
-	Version       int       `json:"version"`
+	Version       int       `json:"version" db:"version"`
-	Created       time.Time `json:"created"`
+	Created       time.Time `json:"created" db:"created"`
-	CreatedBy     string    `json:"createdBy"`
+	// Since we get created by with left join user table, this can be null technically,
-	Message       string    `json:"message"`
+	// 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

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