diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 59ea1b93f4..53ab95d216 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -98,7 +98,7 @@ func resolveMigrations() { migrationResolutionComplete = true } -func inDBMigrationIDs(x *xorm.Engine) (container.Set[string], error) { +func getInDBMigrationIDs(x *xorm.Engine) (container.Set[string], error) { var inDBMigrations []ForgejoMigration err := x.Find(&inDBMigrations) if err != nil { @@ -117,7 +117,7 @@ func inDBMigrationIDs(x *xorm.Engine) (container.Set[string], error) { func EnsureUpToDate(x *xorm.Engine) error { resolveMigrations() - inDBMigrationIDs, err := inDBMigrationIDs(x) + inDBMigrationIDs, err := getInDBMigrationIDs(x) if err != nil { return err } @@ -137,8 +137,18 @@ func EnsureUpToDate(x *xorm.Engine) error { return nil } +func recordMigrationComplete(x *xorm.Engine, migration *Migration) error { + affected, err := x.Insert(&ForgejoMigration{ID: migration.id}) + if err != nil { + return err + } else if affected != 1 { + return fmt.Errorf("migration[%s]: failed to insert into DB, %d records affected", migration.id, affected) + } + return nil +} + // Migrate Forgejo database to current version. -func Migrate(x *xorm.Engine) error { +func Migrate(x *xorm.Engine, freshDB bool) error { resolveMigrations() // Set a new clean the default mapper to GonicMapper as that is the default for . @@ -147,9 +157,25 @@ func Migrate(x *xorm.Engine) error { return fmt.Errorf("sync: %w", err) } - inDBMigrationIDs, err := inDBMigrationIDs(x) + inDBMigrationIDs, err := getInDBMigrationIDs(x) if err != nil { return err + } else if len(inDBMigrationIDs) == 0 && freshDB { + // During startup on a new, empty database, and during integration tests, we rely only on `SyncAllTables` to + // create the DB schema. No migrations can be applied because `SyncAllTables` occurs later in the + // initialization cycle. We mark all migrations as complete up to this point and only run future migrations. + for _, migration := range orderedMigrations { + err := recordMigrationComplete(x, migration) + if err != nil { + return err + } + } + inDBMigrationIDs, err = getInDBMigrationIDs(x) + if err != nil { + return err + } + } else if freshDB { + return fmt.Errorf("unexpected state: migrator called with freshDB=true, but existing migrations in DB %#v", inDBMigrationIDs) } // invalidMigrations are those that are in the database, but aren't registered. @@ -186,11 +212,9 @@ func Migrate(x *xorm.Engine) error { return fmt.Errorf("migration[%s]: %s failed: %w", migration.id, migration.Description, err) } - affected, err := x.Insert(&ForgejoMigration{ID: migration.id}) + err := recordMigrationComplete(x, migration) if err != nil { return err - } else if affected != 1 { - return fmt.Errorf("migration[%s]: failed to insert into DB, %d records affected", migration.id, affected) } } diff --git a/models/forgejo_migrations/migrate_test.go b/models/forgejo_migrations/migrate_test.go index dc17a4a2f9..360f6a16d5 100644 --- a/models/forgejo_migrations/migrate_test.go +++ b/models/forgejo_migrations/migrate_test.go @@ -104,12 +104,12 @@ func TestResolveMigrations(t *testing.T) { }) } -func TestInDBMigrationIDs(t *testing.T) { +func TestGetInDBMigrationIDs(t *testing.T) { x, deferable := migration_tests.PrepareTestEnv(t, 0, new(ForgejoMigration)) defer deferable() require.NotNil(t, x) - migrationIDs, err := inDBMigrationIDs(x) + migrationIDs, err := getInDBMigrationIDs(x) require.NoError(t, err) require.NotNil(t, migrationIDs) assert.Empty(t, migrationIDs) @@ -119,7 +119,7 @@ func TestInDBMigrationIDs(t *testing.T) { _, err = x.Insert(&ForgejoMigration{ID: "v99b_neat_migration"}) require.NoError(t, err) - migrationIDs, err = inDBMigrationIDs(x) + migrationIDs, err = getInDBMigrationIDs(x) require.NoError(t, err) require.NotNil(t, migrationIDs) assert.Len(t, migrationIDs, 2) @@ -232,13 +232,13 @@ func TestMigrate(t *testing.T) { }, }) - err = Migrate(x) + err = Migrate(x, false) require.NoError(t, err) assert.False(t, v77aRun, "v77aRun") // was already marked as run in the DB so shouldn't have run again assert.True(t, v99bRun, "v99bRun") assert.True(t, v99cRun, "v99cRun") - migrationIDs, err := inDBMigrationIDs(x) + migrationIDs, err := getInDBMigrationIDs(x) require.NoError(t, err) assert.Contains(t, migrationIDs, "v77a_neat_migration") assert.Contains(t, migrationIDs, "v99b_neat_migration") @@ -250,3 +250,65 @@ func TestMigrate(t *testing.T) { err = x.Cols("id", "name", "new_field").Table("forgejo_magic_functionality").Find(&rec) assert.NoError(t, err) } + +func TestMigrateFreshDB(t *testing.T) { + resetMigrations() + x, deferable := migration_tests.PrepareTestEnv(t, 0, new(ForgejoMigration)) + defer deferable() + require.NotNil(t, x) + + v77aRun := false + defer test.MockVariableValue(&getMigrationFilename, func() string { + return "some-path/v77a_neat_migration.go" + })() + registerMigration(&Migration{ + Description: "nothing", + Upgrade: func(x *xorm.Engine) error { + v77aRun = true + return nil + }, + }) + + v99bRun := false + defer test.MockVariableValue(&getMigrationFilename, func() string { + return "some-path/v99b_neat_migration.go" + })() + registerMigration(&Migration{ + Description: "nothing", + Upgrade: func(x *xorm.Engine) error { + v99bRun = true + type ForgejoMagicFunctionality struct { + ID int64 `xorm:"pk autoincr"` + Name string + } + return x.Sync(new(ForgejoMagicFunctionality)) + }, + }) + + v99cRun := false + defer test.MockVariableValue(&getMigrationFilename, func() string { + return "some-path/v99c_neat_migration.go" + })() + registerMigration(&Migration{ + Description: "nothing", + Upgrade: func(x *xorm.Engine) error { + v99cRun = true + type ForgejoMagicFunctionality struct { + NewField string + } + return x.Sync(new(ForgejoMagicFunctionality)) + }, + }) + + err := Migrate(x, true) + require.NoError(t, err) + + assert.False(t, v77aRun, "v77aRun") // none should be run due to freshDB flag + assert.False(t, v99bRun, "v99bRun") + assert.False(t, v99cRun, "v99cRun") + migrationIDs, err := getInDBMigrationIDs(x) + require.NoError(t, err) + assert.Contains(t, migrationIDs, "v77a_neat_migration") + assert.Contains(t, migrationIDs, "v99b_neat_migration") + assert.Contains(t, migrationIDs, "v99c_neat_migration") +} diff --git a/models/forgejo_migrations_legacy/migrate.go b/models/forgejo_migrations_legacy/migrate.go index 8f3b787050..72bbf66bdc 100644 --- a/models/forgejo_migrations_legacy/migrate.go +++ b/models/forgejo_migrations_legacy/migrate.go @@ -184,6 +184,7 @@ func Migrate(x *xorm.Engine) error { currentVersion := &ForgejoVersion{ID: 1} has, err := x.Get(currentVersion) + freshDB := false if err != nil { return fmt.Errorf("get: %w", err) } else if !has { @@ -191,6 +192,7 @@ func Migrate(x *xorm.Engine) error { // it is a fresh installation and we can skip all migrations. currentVersion.ID = 0 currentVersion.Version = ExpectedVersion() + freshDB = true if _, err = x.InsertOne(currentVersion); err != nil { return fmt.Errorf("insert: %w", err) @@ -240,5 +242,5 @@ func Migrate(x *xorm.Engine) error { return fmt.Errorf("SetVersionStringWithEngine: %w", err) } - return forgejo_migrations.Migrate(x) + return forgejo_migrations.Migrate(x, freshDB) }