SQL Store: Fix parse time setup (#92484)

pkg/services/sqlstore/database_config.go

@@ -166,10 +166,6 @@ func (dbCfg *DatabaseConfig) buildConnectionString(cfg *setting.Cfg, features fe
 			cnnstr += fmt.Sprintf("&transaction_isolation=%s", val)
 		}
 
-		if features != nil && features.IsEnabledGlobally(featuremgmt.FlagMysqlParseTime) {
-			cnnstr += "&parseTime=true"
-		}
-
 		if features != nil && features.IsEnabledGlobally(featuremgmt.FlagMysqlAnsiQuotes) {
 			cnnstr += "&sql_mode='ANSI_QUOTES'"
 		}

pkg/services/sqlstore/sqlstore.go

@@ -61,11 +61,10 @@ func ProvideService(cfg *setting.Cfg,
 	// by that mimic the functionality of how it was functioning before
 	// xorm's changes above.
 	xorm.DefaultPostgresSchema = ""
-	s, err := newSQLStore(cfg, nil, migrations, bus, tracer)
+	s, err := newSQLStore(cfg, nil, features, migrations, bus, tracer)
 	if err != nil {
 		return nil, err
 	}
-	s.features = features
 
 	if err := s.Migrate(s.dbCfg.MigrationLock); err != nil {
 		return nil, err
@@ -100,18 +99,10 @@ func ProvideServiceForTests(t sqlutil.ITestDB, cfg *setting.Cfg, features featur
 func NewSQLStoreWithoutSideEffects(cfg *setting.Cfg,
 	features featuremgmt.FeatureToggles,
 	bus bus.Bus, tracer tracing.Tracer) (*SQLStore, error) {
-	s, err := newSQLStore(cfg, nil, nil, bus, tracer)
-	if err != nil {
-		return nil, err
-	}
-
-	s.features = features
-	s.tracer = tracer
-
-	return s, nil
+	return newSQLStore(cfg, nil, features, nil, bus, tracer)
 }
 
-func newSQLStore(cfg *setting.Cfg, engine *xorm.Engine,
+func newSQLStore(cfg *setting.Cfg, engine *xorm.Engine, features featuremgmt.FeatureToggles,
 	migrations registry.DatabaseMigrator, bus bus.Bus, tracer tracing.Tracer, opts ...InitTestDBOpt) (*SQLStore, error) {
 	ss := &SQLStore{
 		cfg:                         cfg,
@@ -120,6 +111,7 @@ func newSQLStore(cfg *setting.Cfg, engine *xorm.Engine,
 		migrations:                  migrations,
 		bus:                         bus,
 		tracer:                      tracer,
+		features:                    features,
 	}
 	for _, opt := range opts {
 		if !opt.EnsureDefaultOrgAndUser {
@@ -296,24 +288,24 @@ func (ss *SQLStore) initEngine(engine *xorm.Engine) error {
 		}
 	}
 	if engine == nil {
-		connection := ss.dbCfg.ConnectionString
-
 		// Ensure that parseTime is enabled for MySQL
-		if ss.dbCfg.Type == migrator.MySQL && !strings.Contains(connection, "parseTime=") {
-			if ss.features.IsEnabledGlobally(featuremgmt.FlagMysqlParseTime) {
-				connection += "&parseTime=true"
+		if ss.features.IsEnabledGlobally(featuremgmt.FlagMysqlParseTime) && ss.dbCfg.Type == migrator.MySQL && !strings.Contains(ss.dbCfg.ConnectionString, "parseTime=") {
+			if strings.Contains(ss.dbCfg.ConnectionString, "?") {
+				ss.dbCfg.ConnectionString += "&parseTime=true"
+			} else {
+				ss.dbCfg.ConnectionString += "?parseTime=true"
 			}
 		}
 
 		var err error
-		engine, err = xorm.NewEngine(ss.dbCfg.Type, connection)
+		engine, err = xorm.NewEngine(ss.dbCfg.Type, ss.dbCfg.ConnectionString)
 		if err != nil {
 			return err
 		}
 		// Only for MySQL or MariaDB, verify we can connect with the current connection string's system var for transaction isolation.
 		// If not, create a new engine with a compatible connection string.
 		if ss.dbCfg.Type == migrator.MySQL {
-			engine, err = ss.ensureTransactionIsolationCompatibility(engine, connection)
+			engine, err = ss.ensureTransactionIsolationCompatibility(engine, ss.dbCfg.ConnectionString)
 			if err != nil {
 				return err
 			}
@@ -603,7 +595,7 @@ func TestMain(m *testing.M) {
 
 		tracer := tracing.InitializeTracerForTest()
 		bus := bus.ProvideBus(tracer)
-		testSQLStore, err = newSQLStore(cfg, engine, migration, bus, tracer, opts...)
+		testSQLStore, err = newSQLStore(cfg, engine, features, migration, bus, tracer, opts...)
 		if err != nil {
 			return nil, err
 		}

pkg/services/sqlstore/sqlstore_test.go

@@ -8,8 +8,12 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/ini.v1"
 
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/org"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 func TestMain(m *testing.M) {
@@ -66,3 +70,68 @@ func TestIntegrationIsUniqueConstraintViolation(t *testing.T) {
 		})
 	}
 }
+
+func TestInitEngine_ParseTimeInConnectionString(t *testing.T) {
+	tests := []struct {
+		name               string
+		connectionString   string
+		dbType             string
+		featureEnabled     bool
+		expectedConnection string
+	}{
+		{
+			name:               "MySQL with parseTime already present",
+			connectionString:   "mysql://user:password@localhost:3306/alreadypresent?parseTime=false",
+			dbType:             "mysql",
+			featureEnabled:     true,
+			expectedConnection: "user:password@tcp(localhost:3306)/alreadypresent?collation=utf8mb4_unicode_ci&allowNativePasswords=true&clientFoundRows=true&parseTime=false",
+		},
+		{
+			name:               "MySQL with feature enabled",
+			connectionString:   "mysql://user:password@localhost:3306/existingparams?charset=utf8",
+			dbType:             "mysql",
+			featureEnabled:     true,
+			expectedConnection: "user:password@tcp(localhost:3306)/existingparams?collation=utf8mb4_unicode_ci&allowNativePasswords=true&clientFoundRows=true&charset=utf8&parseTime=true",
+		},
+		{
+			name:               "MySQL with feature disabled",
+			connectionString:   "mysql://user:password@localhost:3306/disabled",
+			dbType:             "mysql",
+			featureEnabled:     false,
+			expectedConnection: "user:password@tcp(localhost:3306)/disabled?collation=utf8mb4_unicode_ci&allowNativePasswords=true&clientFoundRows=true",
+		},
+		{
+			name:               "Postgres",
+			connectionString:   "postgres://username:password@localhost:5432/mydatabase",
+			dbType:             "postgres",
+			featureEnabled:     true,
+			expectedConnection: "user=username host=localhost port=5432 dbname=mydatabase sslmode='' sslcert='' sslkey='' sslrootcert='' password=password",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			raw, err := ini.Load([]byte(`
+				[database]
+				url = ` + tt.connectionString))
+			require.NoError(t, err)
+
+			ftMgr := featuremgmt.WithFeatures()
+			if tt.featureEnabled {
+				ftMgr = featuremgmt.WithFeatures(featuremgmt.FlagMysqlParseTime)
+			}
+
+			ss := &SQLStore{
+				cfg: &setting.Cfg{
+					Raw: raw,
+				},
+				features: ftMgr,
+				log:      log.New(),
+			}
+
+			// don't check the error, the db isn't running. Just check the connection string is okay
+			_ = ss.initEngine(nil)
+			assert.Equal(t, tt.expectedConnection, ss.dbCfg.ConnectionString)
+		})
+	}
+}