fix: empty DBs create tables in an ungoverned order resulting in future foreign key errors (#9709)

Discovered while testing #9708:

Fresh install databases, which is also a process used by the integration tests, create tables by using `SyncAllTables`.  The order that tables are created is ungoverned -- it occurs based upon the order that Go calls each module's `init` to register their schema models.  With the current foreign keys in the database, this does not yet cause an error.  But it will shortly.

I've manually tested this fix in this way:
- The correct order to create tables is that indicated by `foreignKeySortInsert`.  This creates tables that are *referenced* by foreign keys before creating tables that do the referencing (eg. `user` before `tracked_time`).
- If I modify this PR and sort the keys the opposite way `foreignKeySortDelete`, then integration tests fail:

```
??? [TestLogger] 2025/10/15 22:37:58 models/db/engine.go:270:InitEngineWithMigration() [E] [Error SQL Query] CREATE TABLE IF NOT EXISTS "gtestschema"."tracked_time" ("id" BIGSERIAL PRIMARY KEY  NOT NULL, "issue_id" BIGINT NULL, "user_id" BIGINT NULL, "created_unix" BIGINT NULL, "time" BIGINT NOT NULL, "deleted" BOOL DEFAULT false NOT NULL, CONSTRAINT "tracked_time_issue_id_fkey" FOREIGN KEY ("issue_id") REFERENCES "gtestschema"."issue" ("id"), CONSTRAINT "tracked_time_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "gtestschema"."user" ("id"));  [] - pq: relation "gtestschema.issue" does not exist
??? [TestLogger] 2025/10/15 22:37:58 routers/common/db.go:36:InitDBEngine() [E] ORM engine initialization attempt #1/10 failed.
```

Therefore this PR which doesn't appear to fix anything today fixes a latent bug that will occur shortly (possibly in #9397, possibly when another foreign key is added, possibly if Go changes the order in which `init` functions are invoked).

## 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...
  - [ ] 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/9709
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak 2025-10-16 16:02:49 +02:00 committed by Mathieu Fenniak
parent affadc359e
commit fe4a2ae7c1
2 changed files with 6 additions and 3 deletions

View file

@ -22,7 +22,6 @@ forgejo.org/models/db
InTransaction InTransaction
DumpTables DumpTables
GetTableNames GetTableNames
sortBeans
extendBeansForCascade extendBeansForCascade
forgejo.org/models/dbfs forgejo.org/models/dbfs

View file

@ -161,9 +161,13 @@ func (w engineGroupWrapper) AddHook(hook contexts.Hook) bool {
// SyncAllTables sync the schemas of all tables // SyncAllTables sync the schemas of all tables
func SyncAllTables() error { func SyncAllTables() error {
_, err := x.StoreEngine("InnoDB").SyncWithOptions(xorm.SyncOptions{ sortedTables, err := sortBeans(tables, foreignKeySortInsert)
if err != nil {
return err
}
_, err = x.StoreEngine("InnoDB").SyncWithOptions(xorm.SyncOptions{
WarnIfDatabaseColumnMissed: true, WarnIfDatabaseColumnMissed: true,
}, tables...) }, sortedTables...)
return err return err
} }