chore: sqlstore cleanup (#60415)

* chore: remove unused test helper from sqlstore

TimeNow() is no longer used in any tests in this package.

* chore: move sqlstore.SQLBuilder to the infra/db package

This required some minor refactoring; we need to be a little more explicit about passing around the dialect and engine. On the other hand, that's a few fewer uses of the `dialect` global constant!

* chore: move UserDeletions into the only package using it

* cleanup around moving sqlbuilder

* remove dialect and sqlog global vars

* rename userDeletions to serviceAccountDeletions

pkg/infra/db/db.go

@@ -23,13 +23,11 @@ type DB interface {
 }
 
 type Session = sqlstore.DBSession
-type SQLBuilder = sqlstore.SQLBuilder
 type InitTestDBOpt = sqlstore.InitTestDBOpt
 
 var InitTestDB = sqlstore.InitTestDB
 var InitTestDBwithCfg = sqlstore.InitTestDBWithCfg
 var ProvideService = sqlstore.ProvideService
-var NewSqlBuilder = sqlstore.NewSqlBuilder
 
 func IsTestDbSQLite() bool {
 	if db, present := os.LookupEnv("GRAFANA_TEST_DB"); !present || db == "sqlite" {

pkg/infra/db/sqlbuilder.go

@@ -1,23 +1,25 @@
-package sqlstore
+package db
 
 import (
 	"bytes"
 
 	"github.com/grafana/grafana/pkg/models"
 	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
+	"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
 	"github.com/grafana/grafana/pkg/services/sqlstore/permissions"
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
-func NewSqlBuilder(cfg *setting.Cfg) SQLBuilder {
+func NewSqlBuilder(cfg *setting.Cfg, dialect migrator.Dialect) SQLBuilder {
-	return SQLBuilder{cfg: cfg}
+	return SQLBuilder{cfg: cfg, dialect: dialect}
 }
 
 type SQLBuilder struct {
 	cfg     *setting.Cfg
 	sql     bytes.Buffer
 	params  []interface{}
+	dialect migrator.Dialect
 }
 
 func (sb *SQLBuilder) Write(sql string, params ...interface{}) {
@@ -50,7 +52,7 @@ func (sb *SQLBuilder) WriteDashboardPermissionFilter(user *user.SignedInUser, pe
 	} else {
 		sql, params = permissions.DashboardPermissionFilter{
 			OrgRole:         user.OrgRole,
-			Dialect:         dialect,
+			Dialect:         sb.dialect,
 			UserId:          user.UserID,
 			OrgId:           user.OrgID,
 			PermissionLevel: permission,

pkg/infra/db/sqlbuilder_test.go

@@ -1,4 +1,4 @@
-package sqlstore
+package db
 
 import (
 	"context"
@@ -16,6 +16,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/dashboards"
 	dashver "github.com/grafana/grafana/pkg/services/dashboardversion"
 	"github.com/grafana/grafana/pkg/services/org"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/util"
 )
@@ -197,7 +198,7 @@ func test(t *testing.T, dashboardProps DashboardProps, dashboardPermission *Dash
 	})
 }
 
-func createDummyUser(t *testing.T, sqlStore *SQLStore) *user.User {
+func createDummyUser(t *testing.T, sqlStore DB) *user.User {
 	t.Helper()
 
 	uid := strconv.Itoa(rand.Intn(9999999))
@@ -214,7 +215,7 @@ func createDummyUser(t *testing.T, sqlStore *SQLStore) *user.User {
 	}
 
 	var id int64
-	err := sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error {
+	err := sqlStore.WithDbSession(context.Background(), func(sess *Session) error {
 		sess.UseBool("is_admin")
 		var err error
 		id, err = sess.Insert(usr)
@@ -225,7 +226,7 @@ func createDummyUser(t *testing.T, sqlStore *SQLStore) *user.User {
 	return usr
 }
 
-func createDummyDashboard(t *testing.T, sqlStore *SQLStore, dashboardProps DashboardProps) *models.Dashboard {
+func createDummyDashboard(t *testing.T, sqlStore *sqlstore.SQLStore, dashboardProps DashboardProps) *models.Dashboard {
 	t.Helper()
 
 	json, err := simplejson.NewJson([]byte(`{"schemaVersion":17,"title":"gdev dashboards","uid":"","version":1}`))
@@ -255,7 +256,7 @@ func createDummyDashboard(t *testing.T, sqlStore *SQLStore, dashboardProps Dashb
 	return dash
 }
 
-func createDummyACL(t *testing.T, sqlStore *SQLStore, dashboardPermission *DashboardPermission, search Search, dashboardID int64) int64 {
+func createDummyACL(t *testing.T, sqlStore *sqlstore.SQLStore, dashboardPermission *DashboardPermission, search Search, dashboardID int64) int64 {
 	t.Helper()
 
 	acl := &models.DashboardACL{
@@ -291,7 +292,7 @@ func createDummyACL(t *testing.T, sqlStore *SQLStore, dashboardPermission *Dashb
 	return 0
 }
 
-func getDashboards(t *testing.T, sqlStore *SQLStore, search Search, aclUserID int64) []*dashboardResponse {
+func getDashboards(t *testing.T, sqlStore *sqlstore.SQLStore, search Search, aclUserID int64) []*dashboardResponse {
 	t.Helper()
 
 	old := sqlStore.Cfg.RBACEnabled
@@ -300,7 +301,7 @@ func getDashboards(t *testing.T, sqlStore *SQLStore, search Search, aclUserID in
 		sqlStore.Cfg.RBACEnabled = old
 	}()
 
-	builder := NewSqlBuilder(sqlStore.Cfg)
+	builder := NewSqlBuilder(sqlStore.Cfg, sqlStore.GetDialect())
 	signedInUser := &user.SignedInUser{
 		UserID: 9999999999,
 	}
@@ -324,13 +325,13 @@ func getDashboards(t *testing.T, sqlStore *SQLStore, search Search, aclUserID in
 	builder.Write("SELECT * FROM dashboard WHERE true")
 	builder.WriteDashboardPermissionFilter(signedInUser, search.RequiredPermission)
 	t.Logf("Searching for dashboards, SQL: %q\n", builder.GetSQLString())
-	err := sqlStore.engine.SQL(builder.GetSQLString(), builder.params...).Find(&res)
+	err := sqlStore.GetEngine().SQL(builder.GetSQLString(), builder.params...).Find(&res)
 	require.NoError(t, err)
 	return res
 }
 
 // TODO: Use FakeDashboardStore when org has its own service
-func insertTestDashboard(t *testing.T, sqlStore *SQLStore, title string, orgId int64,
+func insertTestDashboard(t *testing.T, sqlStore *sqlstore.SQLStore, title string, orgId int64,
 	folderId int64, isFolder bool, tags ...interface{}) *models.Dashboard {
 	t.Helper()
 	cmd := models.SaveDashboardCommand{
@@ -345,7 +346,7 @@ func insertTestDashboard(t *testing.T, sqlStore *SQLStore, title string, orgId i
 	}
 
 	var dash *models.Dashboard
-	err := sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error {
+	err := sqlStore.WithDbSession(context.Background(), func(sess *Session) error {
 		dash = cmd.GetDashboardModel()
 		dash.SetVersion(1)
 		dash.Created = time.Now()
@@ -360,7 +361,7 @@ func insertTestDashboard(t *testing.T, sqlStore *SQLStore, title string, orgId i
 	dash.Data.Set("id", dash.Id)
 	dash.Data.Set("uid", dash.Uid)
 
-	err = sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error {
+	err = sqlStore.WithDbSession(context.Background(), func(sess *Session) error {
 		dashVersion := &dashver.DashboardVersion{
 			DashboardID:   dash.Id,
 			ParentVersion: dash.Version,
@@ -387,10 +388,10 @@ func insertTestDashboard(t *testing.T, sqlStore *SQLStore, title string, orgId i
 }
 
 // TODO: Use FakeDashboardStore when org has its own service
-func updateDashboardACL(t *testing.T, sqlStore *SQLStore, dashboardID int64, items ...*models.DashboardACL) error {
+func updateDashboardACL(t *testing.T, sqlStore *sqlstore.SQLStore, dashboardID int64, items ...*models.DashboardACL) error {
 	t.Helper()
 
-	err := sqlStore.WithDbSession(context.Background(), func(sess *DBSession) error {
+	err := sqlStore.WithDbSession(context.Background(), func(sess *Session) error {
 		_, err := sess.Exec("DELETE FROM dashboard_acl WHERE dashboard_id=?", dashboardID)
 		if err != nil {
 			return fmt.Errorf("deleting from dashboard_acl failed: %w", err)

pkg/services/alerting/store.go

@@ -105,7 +105,7 @@ func deleteAlertByIdInternal(alertId int64, reason string, sess *db.Session, log
 
 func (ss *sqlStore) HandleAlertsQuery(ctx context.Context, query *models.GetAlertsQuery) error {
 	return ss.db.WithDbSession(ctx, func(sess *db.Session) error {
-		builder := db.NewSqlBuilder(ss.cfg)
+		builder := db.NewSqlBuilder(ss.cfg, ss.db.GetDialect())
 
 		builder.Write(`SELECT
 		alert.id,

pkg/services/dashboards/database/acl.go

@@ -103,7 +103,7 @@ func (d *DashboardStore) HasEditPermissionInFolders(ctx context.Context, query *
 			return nil
 		}
 
-		builder := db.NewSqlBuilder(d.cfg)
+		builder := db.NewSqlBuilder(d.cfg, d.store.GetDialect())
 		builder.Write("SELECT COUNT(dashboard.id) AS count FROM dashboard WHERE dashboard.org_id = ? AND dashboard.is_folder = ?",
 			query.SignedInUser.OrgID, d.store.GetDialect().BooleanStr(true))
 		builder.WriteDashboardPermissionFilter(query.SignedInUser, models.PERMISSION_EDIT)
@@ -131,7 +131,7 @@ func (d *DashboardStore) HasAdminPermissionInDashboardsOrFolders(ctx context.Con
 			return nil
 		}
 
-		builder := db.NewSqlBuilder(d.cfg)
+		builder := db.NewSqlBuilder(d.cfg, d.store.GetDialect())
 		builder.Write("SELECT COUNT(dashboard.id) AS count FROM dashboard WHERE dashboard.org_id = ?", query.SignedInUser.OrgID)
 		builder.WriteDashboardPermissionFilter(query.SignedInUser, models.PERMISSION_ADMIN)
 

pkg/services/libraryelements/database.go

@@ -229,7 +229,7 @@ func (l *LibraryElementService) deleteLibraryElement(c context.Context, signedIn
 func getLibraryElements(c context.Context, store db.DB, cfg *setting.Cfg, signedInUser *user.SignedInUser, params []Pair) ([]LibraryElementDTO, error) {
 	libraryElements := make([]LibraryElementWithMeta, 0)
 	err := store.WithDbSession(c, func(session *db.Session) error {
-		builder := db.NewSqlBuilder(cfg)
+		builder := db.NewSqlBuilder(cfg, store.GetDialect())
 		builder.Write(selectLibraryElementDTOWithMeta)
 		builder.Write(", 'General' as folder_name ")
 		builder.Write(", '' as folder_uid ")
@@ -333,7 +333,7 @@ func (l *LibraryElementService) getAllLibraryElements(c context.Context, signedI
 		return LibraryElementSearchResult{}, folderFilter.parseError
 	}
 	err := l.SQLStore.WithDbSession(c, func(session *db.Session) error {
-		builder := db.NewSqlBuilder(l.Cfg)
+		builder := db.NewSqlBuilder(l.Cfg, l.SQLStore.GetDialect())
 		if folderFilter.includeGeneralFolder {
 			builder.Write(selectLibraryElementDTOWithMeta)
 			builder.Write(", 'General' as folder_name ")
@@ -568,7 +568,7 @@ func (l *LibraryElementService) getConnections(c context.Context, signedInUser *
 			return err
 		}
 		var libraryElementConnections []libraryElementConnectionWithMeta
-		builder := db.NewSqlBuilder(l.Cfg)
+		builder := db.NewSqlBuilder(l.Cfg, l.SQLStore.GetDialect())
 		builder.Write("SELECT lec.*, u1.login AS created_by_name, u1.email AS created_by_email, dashboard.uid AS connection_uid")
 		builder.Write(" FROM " + models.LibraryElementConnectionTableName + " AS lec")
 		builder.Write(" LEFT JOIN " + l.SQLStore.GetDialect().Quote("user") + " AS u1 ON lec.created_by = u1.id")

pkg/services/serviceaccounts/database/store.go

@@ -16,7 +16,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/apikey"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
-	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/setting"
 )
@@ -162,11 +162,11 @@ func (s *ServiceAccountsStoreImpl) UpdateServiceAccount(
 	return updatedUser, err
 }
 
-func ServiceAccountDeletions() []string {
+func ServiceAccountDeletions(dialect migrator.Dialect) []string {
 	deletes := []string{
 		"DELETE FROM api_key WHERE service_account_id = ?",
 	}
-	deletes = append(deletes, sqlstore.UserDeletions()...)
+	deletes = append(deletes, serviceAccountDeletions(dialect)...)
 	return deletes
 }
 
@@ -187,7 +187,7 @@ func (s *ServiceAccountsStoreImpl) deleteServiceAccount(sess *db.Session, orgId,
 	if !has {
 		return serviceaccounts.ErrServiceAccountNotFound
 	}
-	for _, sql := range ServiceAccountDeletions() {
+	for _, sql := range ServiceAccountDeletions(s.sqlStore.GetDialect()) {
 		_, err := sess.Exec(sql, user.ID)
 		if err != nil {
 			return err
@@ -530,3 +530,18 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, saId int64,
 	}
 	return nil
 }
+
+func serviceAccountDeletions(dialect migrator.Dialect) []string {
+	deletes := []string{
+		"DELETE FROM star WHERE user_id = ?",
+		"DELETE FROM " + dialect.Quote("user") + " WHERE id = ?",
+		"DELETE FROM org_user WHERE user_id = ?",
+		"DELETE FROM dashboard_acl WHERE user_id = ?",
+		"DELETE FROM preferences WHERE user_id = ?",
+		"DELETE FROM team_member WHERE user_id = ?",
+		"DELETE FROM user_auth WHERE user_id = ?",
+		"DELETE FROM user_auth_token WHERE user_id = ?",
+		"DELETE FROM quota WHERE user_id = ?",
+	}
+	return deletes
+}

pkg/services/sqlstore/session.go

@@ -9,11 +9,12 @@ import (
 
 	"xorm.io/xorm"
 
+	"github.com/mattn/go-sqlite3"
+
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
 	"github.com/grafana/grafana/pkg/util/errutil"
 	"github.com/grafana/grafana/pkg/util/retryer"
-	"github.com/mattn/go-sqlite3"
 )
 
 var sessionLogger = log.New("sqlstore.session")
@@ -116,7 +117,7 @@ func (ss *SQLStore) withDbSession(ctx context.Context, engine *xorm.Engine, call
 	return retryer.Retry(ss.retryOnLocks(ctx, callback, sess, retry), ss.dbCfg.QueryRetries, time.Millisecond*time.Duration(10), time.Second)
 }
 
-func (sess *DBSession) InsertId(bean interface{}) (int64, error) {
+func (sess *DBSession) InsertId(bean interface{}, dialect migrator.Dialect) (int64, error) {
 	table := sess.DB().Mapper.Obj2Table(getTypeName(bean))
 
 	if err := dialect.PreInsertId(table, sess.Session); err != nil {

pkg/services/sqlstore/sqlstore.go

@@ -36,12 +36,6 @@ import (
 	"github.com/grafana/grafana/pkg/util"
 )
 
-var (
-	dialect migrator.Dialect
-
-	sqlog log.Logger = log.New("sqlstore")
-)
-
 // ContextSessionKey is used as key to save values in `context.Context`
 type ContextSessionKey struct{}
 
@@ -117,8 +111,6 @@ func newSQLStore(cfg *setting.Cfg, cacheService *localcache.CacheService, engine
 
 	ss.Dialect = migrator.NewDialect(ss.engine)
 
-	dialect = ss.Dialect
-
 	// if err := ss.Reset(); err != nil {
 	// 	return nil, err
 	// }
@@ -188,6 +180,10 @@ func (ss *SQLStore) GetDBType() core.DbType {
 	return ss.engine.Dialect().DBType()
 }
 
+func (ss *SQLStore) GetEngine() *xorm.Engine {
+	return ss.engine
+}
+
 func (ss *SQLStore) Bus() bus.Bus {
 	return ss.bus
 }
@@ -209,7 +205,7 @@ func (ss *SQLStore) ensureMainOrgAndAdminUser(test bool) error {
 			var stats models.SystemUserCountStats
 			// TODO: Should be able to rename "Count" to "count", for more standard SQL style
 			// Just have to make sure it gets deserialized properly into models.SystemUserCountStats
-			rawSQL := `SELECT COUNT(id) AS Count FROM ` + dialect.Quote("user")
+			rawSQL := `SELECT COUNT(id) AS Count FROM ` + ss.Dialect.Quote("user")
 			if _, err := sess.SQL(rawSQL).Get(&stats); err != nil {
 				return fmt.Errorf("could not determine if admin user exists: %w", err)
 			}
@@ -354,7 +350,7 @@ func (ss *SQLStore) buildConnectionString() (string, error) {
 // initEngine initializes ss.engine.
 func (ss *SQLStore) initEngine(engine *xorm.Engine) error {
 	if ss.engine != nil {
-		sqlog.Debug("Already connected to database")
+		ss.log.Debug("Already connected to database")
 		return nil
 	}
 
@@ -367,7 +363,7 @@ func (ss *SQLStore) initEngine(engine *xorm.Engine) error {
 		ss.dbCfg.Type = WrapDatabaseDriverWithHooks(ss.dbCfg.Type, ss.tracer)
 	}
 
-	sqlog.Info("Connecting to DB", "dbtype", ss.dbCfg.Type)
+	ss.log.Info("Connecting to DB", "dbtype", ss.dbCfg.Type)
 	if ss.dbCfg.Type == migrator.SQLite && strings.HasPrefix(connectionString, "file:") &&
 		!strings.HasPrefix(connectionString, "file::memory:") {
 		exists, err := fs.Exists(ss.dbCfg.Path)
@@ -614,7 +610,7 @@ func initTestDB(migration registry.DatabaseMigrator, opts ...InitTestDBOpt) (*SQ
 			return nil, err
 		}
 
-		if err := dialect.TruncateDBTables(); err != nil {
+		if err := testSQLStore.Dialect.TruncateDBTables(); err != nil {
 			return nil, err
 		}
 
@@ -630,8 +626,6 @@ func initTestDB(migration registry.DatabaseMigrator, opts ...InitTestDBOpt) (*SQ
 			}
 		}
 
-		// temp global var until we get rid of global vars
-		dialect = testSQLStore.Dialect
 		return testSQLStore, nil
 	}
 
@@ -644,7 +638,7 @@ func initTestDB(migration registry.DatabaseMigrator, opts ...InitTestDBOpt) (*SQ
 		return false
 	}
 
-	if err := dialect.TruncateDBTables(); err != nil {
+	if err := testSQLStore.Dialect.TruncateDBTables(); err != nil {
 		return nil, err
 	}
 	if err := testSQLStore.Reset(); err != nil {

pkg/services/sqlstore/time.go

@@ -1,16 +0,0 @@
-package sqlstore
-
-import "time"
-
-// TimeNow makes it possible to test usage of time
-var TimeNow = time.Now
-
-func MockTimeNow(constTime time.Time) {
-	TimeNow = func() time.Time {
-		return constTime
-	}
-}
-
-func ResetTimeNow() {
-	TimeNow = time.Now
-}

pkg/services/sqlstore/user.go

@@ -72,9 +72,9 @@ func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args user.C
 		Login:      args.Login,
 		IsAdmin:    args.IsAdmin,
 		OrgID:      orgID,
-		Created:    TimeNow(),
+		Created:    time.Now(),
-		Updated:    TimeNow(),
+		Updated:    time.Now(),
-		LastSeenAt: TimeNow().AddDate(-10, 0, 0),
+		LastSeenAt: time.Now().AddDate(-10, 0, 0),
 	}
 
 	salt, err := util.GetRandomString(10)
@@ -114,8 +114,8 @@ func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args user.C
 		OrgId:   orgID,
 		UserId:  usr.ID,
 		Role:    org.RoleAdmin,
-		Created: TimeNow(),
+		Created: time.Now(),
-		Updated: TimeNow(),
+		Updated: time.Now(),
 	}
 
 	if ss.Cfg.AutoAssignOrg && !usr.IsAdmin {
@@ -133,21 +133,6 @@ func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args user.C
 	return usr, nil
 }
 
-func UserDeletions() []string {
-	deletes := []string{
-		"DELETE FROM star WHERE user_id = ?",
-		"DELETE FROM " + dialect.Quote("user") + " WHERE id = ?",
-		"DELETE FROM org_user WHERE user_id = ?",
-		"DELETE FROM dashboard_acl WHERE user_id = ?",
-		"DELETE FROM preferences WHERE user_id = ?",
-		"DELETE FROM team_member WHERE user_id = ?",
-		"DELETE FROM user_auth WHERE user_id = ?",
-		"DELETE FROM user_auth_token WHERE user_id = ?",
-		"DELETE FROM quota WHERE user_id = ?",
-	}
-	return deletes
-}
-
 func verifyExistingOrg(sess *DBSession, orgId int64) error {
 	var org models.Org
 	has, err := sess.Where("id=?", orgId).Get(&org)
@@ -188,7 +173,7 @@ func (ss *SQLStore) getOrCreateOrg(sess *DBSession, orgName string) (int64, erro
 	org.Updated = time.Now()
 
 	if org.Id != 0 {
-		if _, err := sess.InsertId(&org); err != nil {
+		if _, err := sess.InsertId(&org, ss.Dialect); err != nil {
 			return 0, err
 		}
 	} else {

pkg/services/stats/statsimpl/stats.go

@@ -8,7 +8,6 @@ import (
 	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/org"
-	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
 	"github.com/grafana/grafana/pkg/services/stats"
 )
@@ -56,7 +55,7 @@ func notServiceAccount(dialect migrator.Dialect) string {
 
 func (ss *sqlStatsService) GetSystemStats(ctx context.Context, query *models.GetSystemStatsQuery) error {
 	return ss.db.WithDbSession(ctx, func(dbSession *db.Session) error {
-		sb := &sqlstore.SQLBuilder{}
+		sb := &db.SQLBuilder{}
 		sb.Write("SELECT ")
 		dialect := ss.db.GetDialect()
 		sb.Write(`(SELECT COUNT(*) FROM ` + dialect.Quote("user") + ` WHERE ` + notServiceAccount(dialect) + `) AS users,`)