From bea87bef29212b5cd7487a5b266e13af1c1f2a3e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 28 Oct 2021 16:58:38 +0100 Subject: [PATCH 1/3] Run Migrate in Install rather than just SyncTables The underlying problem in #17328 appears to be that users are re-running the install page during upgrades. The function that tests and creates the db did not intend for this and thus instead the migration scripts being run - a simple sync tables occurs. This then causes a weird partially migrated DB which causes, in this release cycle, the duplicate column in task table error. It is likely the cause of some weird partial migration errors in other cycles too. This PR simply ensures that the migration scripts are also run at this point too. Fix #17328 Signed-off-by: Andrew Thornton --- models/db/engine.go | 13 ++++++++++++- routers/install/install.go | 3 ++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/models/db/engine.go b/models/db/engine.go index 78b4ac22dde85..6e84c1ffdd9ac 100755 --- a/models/db/engine.go +++ b/models/db/engine.go @@ -129,7 +129,7 @@ func syncTables() error { } // NewTestEngine sets a new test xorm.Engine -func NewTestEngine() (err error) { +func NewTestEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err error) { x, err = GetNewEngine() if err != nil { return fmt.Errorf("Connect to database: %v", err) @@ -138,6 +138,17 @@ func NewTestEngine() (err error) { x.SetMapper(names.GonicMapper{}) x.SetLogger(NewXORMLogger(!setting.IsProd)) x.ShowSQL(!setting.IsProd) + + x.SetDefaultContext(ctx) + + if err = x.Ping(); err != nil { + return err + } + + if err = migrateFunc(x); err != nil { + return fmt.Errorf("migrate: %v", err) + } + return syncTables() } diff --git a/routers/install/install.go b/routers/install/install.go index 8143ad8089a8d..e27444b831f28 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/migrations" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/generate" @@ -208,7 +209,7 @@ func SubmitInstall(ctx *context.Context) { } // Set test engine. - if err = db.NewTestEngine(); err != nil { + if err = db.NewTestEngine(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) From d09d9b5c5bf433c8b77761584718103655a459e5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 29 Oct 2021 00:48:14 +0800 Subject: [PATCH 2/3] Update engine.go --- models/db/engine.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/models/db/engine.go b/models/db/engine.go index 6e84c1ffdd9ac..a34cbab6430bc 100755 --- a/models/db/engine.go +++ b/models/db/engine.go @@ -192,6 +192,10 @@ func NewEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err e } if err = migrateFunc(x); err != nil { + // The only case to re-run the installation on an installed instance is: users may want to recover an old database. + // In such case, we should run the migrations first, + // otherwise the table schemas may conflict in the normal startup migrations, because `syncTables` below already changed the schemas to the latest. + // However, we should think carefully about should we support re-install on an installed instance, it may bring other problems, eg: secrets will be lost. return fmt.Errorf("migrate: %v", err) } From 883006a8b6a22c1acaee91dd2a178e8b6018e1f5 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 28 Oct 2021 18:05:30 +0100 Subject: [PATCH 3/3] Rename NewTestEngine to NewInstallTestEngine Signed-off-by: Andrew Thornton --- models/db/engine.go | 18 +++++++++++------- routers/install/install.go | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/models/db/engine.go b/models/db/engine.go index a34cbab6430bc..bd55aa7a039c6 100755 --- a/models/db/engine.go +++ b/models/db/engine.go @@ -128,11 +128,13 @@ func syncTables() error { return x.StoreEngine("InnoDB").Sync2(tables...) } -// NewTestEngine sets a new test xorm.Engine -func NewTestEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err error) { +// NewInstallTestEngine creates a new xorm.Engine for testing during install +// +// This function will cause the basic database schema to be created +func NewInstallTestEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err error) { x, err = GetNewEngine() if err != nil { - return fmt.Errorf("Connect to database: %v", err) + return fmt.Errorf("failed to connect to database: %w", err) } x.SetMapper(names.GonicMapper{}) @@ -145,6 +147,12 @@ func NewTestEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (e 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) } @@ -192,10 +200,6 @@ func NewEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err e } if err = migrateFunc(x); err != nil { - // The only case to re-run the installation on an installed instance is: users may want to recover an old database. - // In such case, we should run the migrations first, - // otherwise the table schemas may conflict in the normal startup migrations, because `syncTables` below already changed the schemas to the latest. - // However, we should think carefully about should we support re-install on an installed instance, it may bring other problems, eg: secrets will be lost. return fmt.Errorf("migrate: %v", err) } diff --git a/routers/install/install.go b/routers/install/install.go index e27444b831f28..1c042f9b4e2dc 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -209,7 +209,7 @@ func SubmitInstall(ctx *context.Context) { } // Set test engine. - if err = db.NewTestEngine(ctx, migrations.Migrate); err != nil { + if err = db.NewInstallTestEngine(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)