diff --git a/pkg/services/sqlstore/database_config.go b/pkg/services/sqlstore/database_config.go index c4af9663d13..e469455670f 100644 --- a/pkg/services/sqlstore/database_config.go +++ b/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'" } diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index 2cec8e2f975..9533aa6c512 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/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 } diff --git a/pkg/services/sqlstore/sqlstore_test.go b/pkg/services/sqlstore/sqlstore_test.go index 7c62a7a8c04..439c9f893f2 100644 --- a/pkg/services/sqlstore/sqlstore_test.go +++ b/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) + }) + } +}