fix: integration tests & empty DB creation will fail as soon as forgejo_migrations accesses an existing table (#9708)

Integration tests and new Forgejo installations work in a manner that I didn't expect, and didn't come up during testing of #9561.
- They run the migrations (gitea migrations -> forgejo migrations legacy -> forgejo migrations).
- The gitea & forgejo migrations detect if the `version` and `forgejo_version` table are **empty**, because they are on an empty DB and the tables were just created
- When they hit this case, they pretend all the migrations are applied by setting the version to the highest version number
- And then they rely on the DB engine executing `SyncAllTables` to actually create the schema.

The new `forgejo_migrations` module was not doing this.  So, it would attempt to execute migrations and find that dependent database tables didn't exist, causing unexpected errors in running the integration tests (or starting forgejo on an empty database): https://codeberg.org/forgejo/forgejo/pulls/9397#issuecomment-7745969

This fixes the issue by following the same pattern in `forgejo_migrations`; relying on `SyncAllTables` for initial schema creation.

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9708
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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)
 		}
 	}
 

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")
+}

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)
 }