From 69b61d437357235700306f28d22d21acc39bb2b5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 7 Nov 2021 11:11:27 +0800 Subject: [PATCH] Fix bug on admin subcommand (#17533) * Fix bug on admin subcommand * Add signals for all initDB Co-authored-by: Lauris BH --- cmd/admin.go | 55 +++++++++++++++++++++++++++++-------- cmd/admin_auth_ldap.go | 23 ++++++++++++---- cmd/admin_auth_ldap_test.go | 9 +++--- cmd/cmd.go | 9 +++--- cmd/convert.go | 5 +++- cmd/doctor.go | 10 +++++-- cmd/dump.go | 5 +++- cmd/dump_repo.go | 5 +++- cmd/migrate.go | 5 +++- cmd/migrate_storage.go | 5 +++- models/db/engine.go | 55 ++++++++++--------------------------- modules/doctor/doctor.go | 9 +++--- routers/install/install.go | 4 +-- 13 files changed, 120 insertions(+), 79 deletions(-) diff --git a/cmd/admin.go b/cmd/admin.go index 099083ae9..64106f506 100644 --- a/cmd/admin.go +++ b/cmd/admin.go @@ -339,7 +339,10 @@ func runChangePassword(c *cli.Context) error { return err } - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } if !pwd.IsComplexEnough(c.String("password")) { @@ -393,7 +396,10 @@ func runCreateUser(c *cli.Context) error { fmt.Fprintf(os.Stderr, "--name flag is deprecated. Use --username instead.\n") } - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -456,7 +462,10 @@ func runCreateUser(c *cli.Context) error { } func runListUsers(c *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -493,7 +502,10 @@ func runDeleteUser(c *cli.Context) error { return fmt.Errorf("You must provide the id, username or email of a user to delete") } - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -525,7 +537,10 @@ func runDeleteUser(c *cli.Context) error { } func runRepoSyncReleases(_ *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -591,14 +606,20 @@ func getReleaseCount(id int64) (int64, error) { } func runRegenerateHooks(_ *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } return repo_module.SyncRepositoryHooks(graceful.GetManager().ShutdownContext()) } func runRegenerateKeys(_ *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } return models.RewriteAllPublicKeys() @@ -628,7 +649,10 @@ func parseOAuth2Config(c *cli.Context) *oauth2.Source { } func runAddOauth(c *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -645,7 +669,10 @@ func runUpdateOauth(c *cli.Context) error { return fmt.Errorf("--id flag is missing") } - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -712,7 +739,10 @@ func runUpdateOauth(c *cli.Context) error { } func runListAuth(c *cli.Context) error { - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } @@ -748,7 +778,10 @@ func runDeleteAuth(c *cli.Context) error { return fmt.Errorf("--id flag is missing") } - if err := initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := initDB(ctx); err != nil { return err } diff --git a/cmd/admin_auth_ldap.go b/cmd/admin_auth_ldap.go index 517904957..950d515e3 100644 --- a/cmd/admin_auth_ldap.go +++ b/cmd/admin_auth_ldap.go @@ -5,6 +5,7 @@ package cmd import ( + "context" "fmt" "strings" @@ -16,7 +17,7 @@ import ( type ( authService struct { - initDB func() error + initDB func(ctx context.Context) error createLoginSource func(loginSource *login.Source) error updateLoginSource func(loginSource *login.Source) error getLoginSourceByID func(id int64) (*login.Source, error) @@ -299,7 +300,10 @@ func (a *authService) addLdapBindDn(c *cli.Context) error { return err } - if err := a.initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := a.initDB(ctx); err != nil { return err } @@ -321,7 +325,10 @@ func (a *authService) addLdapBindDn(c *cli.Context) error { // updateLdapBindDn updates a new LDAP via Bind DN authentication source. func (a *authService) updateLdapBindDn(c *cli.Context) error { - if err := a.initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := a.initDB(ctx); err != nil { return err } @@ -344,7 +351,10 @@ func (a *authService) addLdapSimpleAuth(c *cli.Context) error { return err } - if err := a.initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := a.initDB(ctx); err != nil { return err } @@ -366,7 +376,10 @@ func (a *authService) addLdapSimpleAuth(c *cli.Context) error { // updateLdapBindDn updates a new LDAP (simple auth) authentication source. func (a *authService) updateLdapSimpleAuth(c *cli.Context) error { - if err := a.initDB(); err != nil { + ctx, cancel := installSignals() + defer cancel() + + if err := a.initDB(ctx); err != nil { return err } diff --git a/cmd/admin_auth_ldap_test.go b/cmd/admin_auth_ldap_test.go index db1ba13bc..15880639d 100644 --- a/cmd/admin_auth_ldap_test.go +++ b/cmd/admin_auth_ldap_test.go @@ -5,6 +5,7 @@ package cmd import ( + "context" "testing" "code.gitea.io/gitea/models/login" @@ -207,7 +208,7 @@ func TestAddLdapBindDn(t *testing.T) { // Mock functions. var createdLoginSource *login.Source service := &authService{ - initDB: func() error { + initDB: func(context.Context) error { return nil }, createLoginSource: func(loginSource *login.Source) error { @@ -438,7 +439,7 @@ func TestAddLdapSimpleAuth(t *testing.T) { // Mock functions. var createdLoginSource *login.Source service := &authService{ - initDB: func() error { + initDB: func(context.Context) error { return nil }, createLoginSource: func(loginSource *login.Source) error { @@ -863,7 +864,7 @@ func TestUpdateLdapBindDn(t *testing.T) { // Mock functions. var updatedLoginSource *login.Source service := &authService{ - initDB: func() error { + initDB: func(context.Context) error { return nil }, createLoginSource: func(loginSource *login.Source) error { @@ -1227,7 +1228,7 @@ func TestUpdateLdapSimpleAuth(t *testing.T) { // Mock functions. var updatedLoginSource *login.Source service := &authService{ - initDB: func() error { + initDB: func(context.Context) error { return nil }, createLoginSource: func(loginSource *login.Source) error { diff --git a/cmd/cmd.go b/cmd/cmd.go index ea025cd98..b89dd5d8b 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -56,16 +56,15 @@ func confirm() (bool, error) { } } -func initDB() error { - return initDBDisableConsole(false) +func initDB(ctx context.Context) error { + return initDBDisableConsole(ctx, false) } -func initDBDisableConsole(disableConsole bool) error { +func initDBDisableConsole(ctx context.Context, disableConsole bool) error { setting.NewContext() setting.InitDBConfig() - setting.NewXORMLogService(disableConsole) - if err := db.InitEngine(); err != nil { + if err := db.InitEngine(ctx); err != nil { return fmt.Errorf("models.SetEngine: %v", err) } return nil diff --git a/cmd/convert.go b/cmd/convert.go index 20e4045c3..0b2e240b3 100644 --- a/cmd/convert.go +++ b/cmd/convert.go @@ -23,7 +23,10 @@ var CmdConvert = cli.Command{ } func runConvert(ctx *cli.Context) error { - if err := initDB(); err != nil { + stdCtx, cancel := installSignals() + defer cancel() + + if err := initDB(stdCtx); err != nil { return err } diff --git a/cmd/doctor.go b/cmd/doctor.go index 19b26f09c..498b859e4 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -96,7 +96,10 @@ func runRecreateTable(ctx *cli.Context) error { setting.Cfg.Section("log").Key("XORM").SetValue(",") setting.NewXORMLogService(!ctx.Bool("debug")) - if err := db.InitEngine(); err != nil { + stdCtx, cancel := installSignals() + defer cancel() + + if err := db.InitEngine(stdCtx); err != nil { fmt.Println(err) fmt.Println("Check if you are using the right config file. You can use a --config directive to specify one.") return nil @@ -128,6 +131,9 @@ func runDoctor(ctx *cli.Context) error { log.DelNamedLogger("console") log.DelNamedLogger(log.DEFAULT) + stdCtx, cancel := installSignals() + defer cancel() + // Now setup our own logFile := ctx.String("log-file") if !ctx.IsSet("log-file") { @@ -210,5 +216,5 @@ func runDoctor(ctx *cli.Context) error { logger := log.GetLogger("doctorouter") defer logger.Close() - return doctor.RunChecks(logger, ctx.Bool("fix"), checks) + return doctor.RunChecks(stdCtx, logger, ctx.Bool("fix"), checks) } diff --git a/cmd/dump.go b/cmd/dump.go index 70ed6c2b5..efb939720 100644 --- a/cmd/dump.go +++ b/cmd/dump.go @@ -173,7 +173,10 @@ func runDump(ctx *cli.Context) error { } setting.NewServices() // cannot access session settings otherwise - err := db.InitEngine() + stdCtx, cancel := installSignals() + defer cancel() + + err := db.InitEngine(stdCtx) if err != nil { return err } diff --git a/cmd/dump_repo.go b/cmd/dump_repo.go index 3ea82f6d1..6274b4d86 100644 --- a/cmd/dump_repo.go +++ b/cmd/dump_repo.go @@ -76,7 +76,10 @@ wiki, issues, labels, releases, release_assets, milestones, pull_requests, comme } func runDumpRepository(ctx *cli.Context) error { - if err := initDB(); err != nil { + stdCtx, cancel := installSignals() + defer cancel() + + if err := initDB(stdCtx); err != nil { return err } diff --git a/cmd/migrate.go b/cmd/migrate.go index 7bb87d840..054772d9e 100644 --- a/cmd/migrate.go +++ b/cmd/migrate.go @@ -24,7 +24,10 @@ var CmdMigrate = cli.Command{ } func runMigrate(ctx *cli.Context) error { - if err := initDB(); err != nil { + stdCtx, cancel := installSignals() + defer cancel() + + if err := initDB(stdCtx); err != nil { return err } diff --git a/cmd/migrate_storage.go b/cmd/migrate_storage.go index 7c7629490..1829ad00b 100644 --- a/cmd/migrate_storage.go +++ b/cmd/migrate_storage.go @@ -107,7 +107,10 @@ func migrateRepoAvatars(dstStorage storage.ObjectStorage) error { } func runMigrateStorage(ctx *cli.Context) error { - if err := initDB(); err != nil { + stdCtx, cancel := installSignals() + defer cancel() + + if err := initDB(stdCtx); err != nil { return err } diff --git a/models/db/engine.go b/models/db/engine.go index 411e39a13..d5f2470a7 100755 --- a/models/db/engine.go +++ b/models/db/engine.go @@ -128,40 +128,8 @@ func syncTables() error { return x.StoreEngine("InnoDB").Sync2(tables...) } -// InitInstallEngineWithMigration creates a new xorm.Engine for testing during install -// -// This function will cause the basic database schema to be created -func InitInstallEngineWithMigration(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err error) { - x, err = NewEngine() - if err != nil { - return fmt.Errorf("failed to connect to database: %w", err) - } - - x.SetMapper(names.GonicMapper{}) - x.SetLogger(NewXORMLogger(!setting.IsProd)) - x.ShowSQL(!setting.IsProd) - - x.SetDefaultContext(ctx) - - if err = x.Ping(); err != nil { - return err - } - - // We have to run migrateFunc here in case the user is re-running installation on a previously created DB. - // If we do not then table schemas will be changed and there will be conflicts when the migrations run properly. - // - // Installation should only be being re-run if users want to recover an old database. - // However, we should think carefully about should we support re-install on an installed instance, - // as there may be other problems due to secret reinitialization. - if err = migrateFunc(x); err != nil { - return fmt.Errorf("migrate: %v", err) - } - - return syncTables() -} - // InitEngine sets the xorm.Engine -func InitEngine() (err error) { +func InitEngine(ctx context.Context) (err error) { x, err = NewEngine() if err != nil { return fmt.Errorf("Failed to connect to database: %v", err) @@ -175,6 +143,12 @@ func InitEngine() (err error) { x.SetMaxOpenConns(setting.Database.MaxOpenConns) x.SetMaxIdleConns(setting.Database.MaxIdleConns) x.SetConnMaxLifetime(setting.Database.ConnMaxLifetime) + + DefaultContext = &Context{ + Context: ctx, + e: x, + } + x.SetDefaultContext(ctx) return nil } @@ -184,21 +158,20 @@ func InitEngine() (err error) { // that prevents the doctor from fixing anything in the database if the migration level // is different from the expected value. func InitEngineWithMigration(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err error) { - if err = InitEngine(); err != nil { + if err = InitEngine(ctx); err != nil { return err } - DefaultContext = &Context{ - Context: ctx, - e: x, - } - - x.SetDefaultContext(ctx) - if err = x.Ping(); err != nil { return err } + // We have to run migrateFunc here in case the user is re-running installation on a previously created DB. + // If we do not then table schemas will be changed and there will be conflicts when the migrations run properly. + // + // Installation should only be being re-run if users want to recover an old database. + // However, we should think carefully about should we support re-install on an installed instance, + // as there may be other problems due to secret reinitialization. if err = migrateFunc(x); err != nil { return fmt.Errorf("migrate: %v", err) } diff --git a/modules/doctor/doctor.go b/modules/doctor/doctor.go index 9a05e828f..6451788f9 100644 --- a/modules/doctor/doctor.go +++ b/modules/doctor/doctor.go @@ -5,6 +5,7 @@ package doctor import ( + "context" "fmt" "sort" "strings" @@ -42,12 +43,12 @@ func (w *wrappedLevelLogger) Log(skip int, level log.Level, format string, v ... }, v...)...) } -func initDBDisableConsole(disableConsole bool) error { +func initDBDisableConsole(ctx context.Context, disableConsole bool) error { setting.NewContext() setting.InitDBConfig() setting.NewXORMLogService(disableConsole) - if err := db.InitEngine(); err != nil { + if err := db.InitEngine(ctx); err != nil { return fmt.Errorf("models.SetEngine: %v", err) } return nil @@ -57,7 +58,7 @@ func initDBDisableConsole(disableConsole bool) error { var Checks []*Check // RunChecks runs the doctor checks for the provided list -func RunChecks(logger log.Logger, autofix bool, checks []*Check) error { +func RunChecks(ctx context.Context, logger log.Logger, autofix bool, checks []*Check) error { wrappedLogger := log.LevelLoggerLogger{ LevelLogger: &wrappedLevelLogger{logger}, } @@ -67,7 +68,7 @@ func RunChecks(logger log.Logger, autofix bool, checks []*Check) error { if !dbIsInit && !check.SkipDatabaseInitialization { // Only open database after the most basic configuration check setting.EnableXORMLog = false - if err := initDBDisableConsole(true); err != nil { + if err := initDBDisableConsole(ctx, true); err != nil { logger.Error("Error whilst initializing the database: %v", err) logger.Error("Check if you are using the right config file. You can use a --config directive to specify one.") return nil diff --git a/routers/install/install.go b/routers/install/install.go index e38b51fb7..837467056 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -198,7 +198,7 @@ func SubmitInstall(ctx *context.Context) { setting.Database.SSLMode = form.SSLMode setting.Database.Charset = form.Charset setting.Database.Path = form.DbPath - + setting.Database.LogSQL = !setting.IsProd setting.PasswordHashAlgo = form.PasswordAlgorithm if (setting.Database.Type == "sqlite3") && @@ -209,7 +209,7 @@ func SubmitInstall(ctx *context.Context) { } // Set test engine. - if err = db.InitInstallEngineWithMigration(ctx, migrations.Migrate); err != nil { + if err = db.InitEngineWithMigration(ctx, migrations.Migrate); err != nil { if strings.Contains(err.Error(), `Unknown database type: sqlite3`) { ctx.Data["Err_DbType"] = true ctx.RenderWithErr(ctx.Tr("install.sqlite3_not_available", "https://docs.gitea.io/en-us/install-from-binary/"), tplInstall, &form)