[v14.0/forgejo] migration: update existing foreign key migrations to automatically fix inconsistencies (#10621)

**Backport:** https://codeberg.org/forgejo/forgejo/pulls/10568

Changes foreign key database inconsistency handling so that inconsistent records are automatically deleted with an administrator warning during migration.  As noted in discussion: https://codeberg.org/forgejo/discussions/issues/385#issuecomment-9175566

Because these migrations are now deleting data, rather than allowing the administrator to do it, all migrations have been covered with an integration test that verifies expected data is deleted.  This is particularly interesting with nullable fields.

## 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

- [ ] I do not want this change to show in the release notes.
- [x] 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.

Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10621
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
This commit is contained in:
forgejo-backport-action 2025-12-29 03:49:03 +01:00 committed by Mathieu Fenniak
parent a89978a207
commit 763547f43f
27 changed files with 578 additions and 44 deletions

View file

@ -4,28 +4,30 @@
package forgejo_migrations
import (
"errors"
"fmt"
"forgejo.org/modules/log"
"xorm.io/builder"
"xorm.io/xorm"
)
func syncDoctorForeignKey(x *xorm.Engine, beans []any) error {
for _, bean := range beans {
// Sync() drops indexes by default, which will cause unnecessary rebuilding of indexes when syncDoctorForeignKey
// is used with partial bean definitions; so we disable that option
_, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, bean)
if err != nil {
if errors.Is(err, xorm.ErrForeignKeyViolation) {
tableName := x.TableName(bean)
log.Error(
"Foreign key creation on table %s failed. Run `forgejo doctor check --all` to identify the orphaned records preventing this foreign key from being created. Error was: %v",
tableName, err)
return err
}
return err
}
// syncForeignKeyWithDelete will delete any records that match `cond`, and if present, log and warn to the
// administrator; then it will perform an `xorm.Sync()` in order to create foreign keys on the table definition.
func syncForeignKeyWithDelete(x *xorm.Engine, bean any, cond builder.Cond) error {
rowsDeleted, err := x.Where(cond).Delete(bean)
if err != nil {
return fmt.Errorf("failure to delete inconsistent records before foreign key sync: %w", err)
}
return nil
if rowsDeleted > 0 {
tableName := x.TableName(bean)
log.Warn(
"Foreign key creation on table %s required deleting %d records with inconsistent foreign key values.",
tableName, rowsDeleted)
}
// Sync() drops indexes by default, which will cause unnecessary rebuilding of indexes when syncForeignKeyWithDelete
// is used with partial bean definitions; so we disable that option
_, err = x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, bean)
return err
}

View file

@ -4,6 +4,7 @@
package forgejo_migrations
import (
"xorm.io/builder"
"xorm.io/xorm"
)
@ -19,7 +20,11 @@ func addForeignKeysCollaboration(x *xorm.Engine) error {
RepoID int64 `xorm:"UNIQUE(s) INDEX NOT NULL REFERENCES(repository, id)"`
UserID int64 `xorm:"UNIQUE(s) INDEX NOT NULL REFERENCES(user, id)"`
}
return syncDoctorForeignKey(x, []any{
return syncForeignKeyWithDelete(x,
new(Collaboration),
})
builder.Or(
builder.Expr("NOT EXISTS (SELECT id FROM repository WHERE repository.id = collaboration.repo_id)"),
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = collaboration.user_id)"),
),
)
}

View file

@ -0,0 +1,53 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations
import (
"testing"
"forgejo.org/models/db"
migration_tests "forgejo.org/models/gitea_migrations/test"
"forgejo.org/modules/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_addForeignKeysCollaboration(t *testing.T) {
type AccessMode int
type Collaboration struct {
ID int64 `xorm:"pk autoincr"`
RepoID int64 `xorm:"UNIQUE(s) INDEX NOT NULL"`
UserID int64 `xorm:"UNIQUE(s) INDEX NOT NULL"`
Mode AccessMode `xorm:"DEFAULT 2 NOT NULL"`
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
}
type Repository struct {
ID int64 `xorm:"pk autoincr"`
}
type User struct {
ID int64 `xorm:"pk autoincr"`
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(Repository), new(Collaboration))
defer deferable()
if x == nil || t.Failed() {
return
}
require.NoError(t, addForeignKeysCollaboration(x))
var remainingRecords []*Collaboration
require.NoError(t,
db.GetEngine(t.Context()).
Table("collaboration").
Select("`id`, `repo_id`, `user_id`").
OrderBy("`id`").
Find(&remainingRecords))
assert.Equal(t,
[]*Collaboration{
{ID: 1, UserID: 1, RepoID: 1},
},
remainingRecords)
}

View file

@ -4,6 +4,7 @@
package forgejo_migrations
import (
"xorm.io/builder"
"xorm.io/xorm"
)
@ -18,7 +19,8 @@ func addForeignKeysForgejoAuthToken(x *xorm.Engine) error {
type ForgejoAuthToken struct {
UID int64 `xorm:"INDEX REFERENCES(user, id)"`
}
return syncDoctorForeignKey(x, []any{
return syncForeignKeyWithDelete(x,
new(ForgejoAuthToken),
})
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = forgejo_auth_token.uid)"),
)
}

View file

@ -0,0 +1,50 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations
import (
"testing"
"forgejo.org/models/db"
migration_tests "forgejo.org/models/gitea_migrations/test"
"forgejo.org/modules/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_addForeignKeysForgejoAuthToken(t *testing.T) {
type AuthorizationPurpose string
type ForgejoAuthToken struct {
ID int64 `xorm:"pk autoincr"`
UID int64 `xorm:"INDEX"`
LookupKey string `xorm:"INDEX UNIQUE"`
HashedValidator string
Purpose AuthorizationPurpose `xorm:"NOT NULL DEFAULT 'long_term_authorization'"`
Expiry timeutil.TimeStamp
}
type User struct {
ID int64 `xorm:"pk autoincr"`
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(ForgejoAuthToken))
defer deferable()
if x == nil || t.Failed() {
return
}
require.NoError(t, addForeignKeysForgejoAuthToken(x))
var remainingRecords []*ForgejoAuthToken
require.NoError(t,
db.GetEngine(t.Context()).
Table("forgejo_auth_token").
Select("`id`, `uid`").
OrderBy("`id`").
Find(&remainingRecords))
assert.Equal(t,
[]*ForgejoAuthToken{
{ID: 1, UID: 1},
},
remainingRecords)
}

View file

@ -4,6 +4,7 @@
package forgejo_migrations
import (
"xorm.io/builder"
"xorm.io/xorm"
)
@ -19,7 +20,11 @@ func addForeignKeysPullRequest1(x *xorm.Engine) error {
IssueID int64 `xorm:"INDEX REFERENCES(issue, id)"`
BaseRepoID int64 `xorm:"INDEX REFERENCES(repository, id)"`
}
return syncDoctorForeignKey(x, []any{
return syncForeignKeyWithDelete(x,
new(PullRequest),
})
builder.Or(
builder.Expr("NOT EXISTS (SELECT id FROM issue WHERE issue.id = pull_request.issue_id)"),
builder.Expr("NOT EXISTS (SELECT id FROM repository WHERE repository.id = pull_request.base_repo_id)"),
),
)
}

View file

@ -0,0 +1,69 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations
import (
"testing"
"forgejo.org/models/db"
migration_tests "forgejo.org/models/gitea_migrations/test"
"forgejo.org/modules/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_addForeignKeysPullRequest1(t *testing.T) {
type PullRequestType int
type PullRequestStatus int
type PullRequestFlow int
type PullRequest struct {
ID int64 `xorm:"pk autoincr"`
Type PullRequestType
Status PullRequestStatus
ConflictedFiles []string `xorm:"TEXT JSON"`
CommitsAhead int
CommitsBehind int
ChangedProtectedFiles []string `xorm:"TEXT JSON"`
IssueID int64 `xorm:"INDEX"`
Index int64
HeadRepoID int64 `xorm:"INDEX"`
BaseRepoID int64 `xorm:"INDEX"`
HeadBranch string
BaseBranch string
MergeBase string `xorm:"VARCHAR(64)"`
AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"`
HasMerged bool `xorm:"INDEX"`
MergedCommitID string `xorm:"VARCHAR(64)"`
MergerID int64 `xorm:"INDEX"`
MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"`
Flow PullRequestFlow `xorm:"NOT NULL DEFAULT 0"`
}
type Repository struct {
ID int64 `xorm:"pk autoincr"`
}
type Issue struct {
ID int64 `xorm:"pk autoincr"`
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(Issue), new(Repository), new(PullRequest))
defer deferable()
if x == nil || t.Failed() {
return
}
require.NoError(t, addForeignKeysPullRequest1(x))
var remainingRecords []*PullRequest
require.NoError(t,
db.GetEngine(t.Context()).
Table("pull_request").
Select("`id`, `issue_id`, `base_repo_id`").
OrderBy("`id`").
Find(&remainingRecords))
assert.Equal(t,
[]*PullRequest{
{ID: 1, BaseRepoID: 1, IssueID: 1},
},
remainingRecords)
}

View file

@ -4,7 +4,6 @@
package forgejo_migrations_legacy
import (
"errors"
"fmt"
"forgejo.org/modules/log"
@ -13,23 +12,24 @@ import (
"xorm.io/xorm"
)
func syncDoctorForeignKey(x *xorm.Engine, beans []any) error {
for _, bean := range beans {
// Sync() drops indexes by default, which will cause unnecessary rebuilding of indexes when syncDoctorForeignKey
// is used with partial bean definitions; so we disable that option
_, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, bean)
if err != nil {
if errors.Is(err, xorm.ErrForeignKeyViolation) {
tableName := x.TableName(bean)
log.Error(
"Foreign key creation on table %s failed. Run `forgejo doctor check --all` to identify the orphaned records preventing this foreign key from being created. Error was: %v",
tableName, err)
return err
}
return err
}
// syncForeignKeyWithDelete will delete any records that match `cond`, and if present, log and warn to the
// administrator; then it will perform an `xorm.Sync()` in order to create foreign keys on the table definition.
func syncForeignKeyWithDelete(x *xorm.Engine, bean any, cond builder.Cond) error {
rowsDeleted, err := x.Where(cond).Delete(bean)
if err != nil {
return fmt.Errorf("failure to delete inconsistent records before foreign key sync: %w", err)
}
return nil
if rowsDeleted > 0 {
tableName := x.TableName(bean)
log.Warn(
"Foreign key creation on table %s required deleting %d records with inconsistent foreign key values.",
tableName, rowsDeleted)
}
// Sync() drops indexes by default, which will cause unnecessary rebuilding of indexes when syncForeignKeyWithDelete
// is used with partial bean definitions; so we disable that option
_, err = x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, bean)
return err
}
func AddForeignKeysStopwatchTrackedTime(x *xorm.Engine) error {
@ -50,6 +50,7 @@ func AddForeignKeysStopwatchTrackedTime(x *xorm.Engine) error {
err := x.Table("tracked_time").
Join("LEFT", "`user`", "`tracked_time`.user_id = `user`.id").
Where(builder.IsNull{"`user`.id"}).
Where(builder.NotNull{"tracked_time.user_id"}).
Find(&trackedTime)
if err != nil {
return err
@ -63,8 +64,25 @@ func AddForeignKeysStopwatchTrackedTime(x *xorm.Engine) error {
}
}
return syncDoctorForeignKey(x, []any{
err = syncForeignKeyWithDelete(x,
new(Stopwatch),
builder.Or(
builder.Expr("NOT EXISTS (SELECT id FROM issue WHERE issue.id = stopwatch.issue_id)"),
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = stopwatch.user_id)"),
),
)
if err != nil {
return err
}
return syncForeignKeyWithDelete(x,
new(TrackedTime),
})
builder.Or(
builder.And(
builder.Expr("user_id IS NOT NULL"),
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = tracked_time.user_id)"),
),
builder.Expr("NOT EXISTS (SELECT id FROM issue WHERE issue.id = tracked_time.issue_id)"),
),
)
}

View file

@ -0,0 +1,75 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations_legacy
import (
"testing"
"forgejo.org/models/db"
migration_tests "forgejo.org/models/gitea_migrations/test"
"forgejo.org/modules/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_AddForeignKeysStopwatchTrackedTime(t *testing.T) {
type Stopwatch struct {
ID int64 `xorm:"pk autoincr"`
IssueID int64 `xorm:"INDEX"`
UserID int64 `xorm:"INDEX"`
CreatedUnix timeutil.TimeStamp `xorm:"created"`
}
type TrackedTime struct {
ID int64 `xorm:"pk autoincr"`
IssueID int64 `xorm:"INDEX"`
UserID int64 `xorm:"INDEX"`
CreatedUnix int64 `xorm:"created"`
Time int64 `xorm:"NOT NULL"`
Deleted bool `xorm:"NOT NULL DEFAULT false"`
}
type User struct {
ID int64 `xorm:"pk autoincr"`
}
type Issue struct {
ID int64 `xorm:"pk autoincr"`
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(Issue), new(Stopwatch), new(TrackedTime))
defer deferable()
if x == nil || t.Failed() {
return
}
require.NoError(t, AddForeignKeysStopwatchTrackedTime(x))
var remainingStopwatch []*Stopwatch
require.NoError(t,
db.GetEngine(t.Context()).
Table("stopwatch").
Select("`id`, `issue_id`, `user_id`").
OrderBy("`id`").
Find(&remainingStopwatch))
assert.Equal(t,
[]*Stopwatch{
{1, 1, 1, 0},
},
remainingStopwatch,
"stopwatch")
var remainingTrackedTime []*TrackedTime
require.NoError(t,
db.GetEngine(t.Context()).
Table("tracked_time").
Select("`id`, `issue_id`, `user_id`").
OrderBy("`id`").
Find(&remainingTrackedTime))
assert.Equal(t,
[]*TrackedTime{
{ID: 1, IssueID: 1, UserID: 1},
{ID: 4, IssueID: 1, UserID: 0},
{ID: 5, IssueID: 1, UserID: 0},
},
remainingTrackedTime,
"tracked_time")
}

View file

@ -4,6 +4,7 @@
package forgejo_migrations_legacy
import (
"xorm.io/builder"
"xorm.io/xorm"
)
@ -12,7 +13,11 @@ func AddForeignKeysAccess(x *xorm.Engine) error {
UserID int64 `xorm:"UNIQUE(s) REFERENCES(user, id)"`
RepoID int64 `xorm:"UNIQUE(s) REFERENCES(repository, id)"`
}
return syncDoctorForeignKey(x, []any{
return syncForeignKeyWithDelete(x,
new(Access),
})
builder.Or(
builder.Expr("NOT EXISTS (SELECT id FROM repository WHERE repository.id = access.repo_id)"),
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = access.user_id)"),
),
)
}

View file

@ -0,0 +1,50 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations_legacy
import (
"testing"
"forgejo.org/models/db"
migration_tests "forgejo.org/models/gitea_migrations/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func Test_AddForeignKeysAccess(t *testing.T) {
type AccessMode int
type Access struct {
ID int64 `xorm:"pk autoincr"`
UserID int64 `xorm:"UNIQUE(s)"`
RepoID int64 `xorm:"UNIQUE(s)"`
Mode AccessMode
}
type User struct {
ID int64 `xorm:"pk autoincr"`
}
type Repository struct {
ID int64 `xorm:"pk autoincr"`
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(Repository), new(Access))
defer deferable()
if x == nil || t.Failed() {
return
}
require.NoError(t, AddForeignKeysAccess(x))
var remainingRecords []*Access
require.NoError(t,
db.GetEngine(t.Context()).
Table("access").
Select("`id`, `user_id`, `repo_id`").
OrderBy("`id`").
Find(&remainingRecords))
assert.Equal(t,
[]*Access{
{ID: 1, UserID: 1, RepoID: 1},
},
remainingRecords)
}

View file

@ -0,0 +1,28 @@
-
id: 1
repo_id: 1
user_id: 1
# Expected to be deleted due to invalid repository foreign key
-
id: 2
repo_id: 100
user_id: 1
# Expected to be deleted due to null repository foreign key
-
id: 3
repo_id: null
user_id: 1
# Expected to be deleted due to invalid user foreign key
-
id: 4
repo_id: 1
user_id: 100
# Expected to be deleted due to null user foreign key
-
id: 5
repo_id: 1
user_id: null

View file

@ -0,0 +1,2 @@
-
id: 1

View file

@ -0,0 +1,33 @@
-
id: 1
issue_id: 1
user_id: 1
time: 100
# Expected to be deleted due to invalid issue foreign key
-
id: 2
issue_id: 100
user_id: 1
time: 100
# Expected to be deleted due to null issue foreign key
-
id: 3
issue_id: null
user_id: 1
time: 100
# Expected to be retained with null, due to invalid user foreign key
-
id: 4
issue_id: 1
user_id: 100
time: 100
# Expected to be retained with null user foreign key
-
id: 5
issue_id: 1
user_id: null
time: 100

View file

@ -0,0 +1,2 @@
-
id: 1

View file

@ -0,0 +1,28 @@
-
id: 1
issue_id: 1
user_id: 1
# Expected to be deleted due to invalid issue foreign key
-
id: 2
issue_id: 100
user_id: 1
# Expected to be deleted due to null issue foreign key
-
id: 3
issue_id: null
user_id: 1
# Expected to be deleted due to invalid user foreign key
-
id: 4
issue_id: 1
user_id: 100
# Expected to be deleted due to null user foreign key
-
id: 5
issue_id: 1
user_id: null

View file

@ -0,0 +1,33 @@
-
id: 1
issue_id: 1
user_id: 1
time: 100
# Expected to be deleted due to invalid issue foreign key
-
id: 2
issue_id: 100
user_id: 1
time: 100
# Expected to be deleted due to null issue foreign key
-
id: 3
issue_id: null
user_id: 1
time: 100
# Expected to be retained with null, due to invalid user foreign key
-
id: 4
issue_id: 1
user_id: 100
time: 100
# Expected to be retained with null user foreign key
-
id: 5
issue_id: 1
user_id: null
time: 100

View file

@ -0,0 +1,16 @@
-
id: 1
user_id: 1
repo_id: 1
# Expected to be deleted due to invalid user_id foreign key
-
id: 2
user_id: 100
repo_id: 1
# Expected to be deleted due to invalid repo_id foreign key
-
id: 3
user_id: 1
repo_id: 100

View file

@ -0,0 +1,2 @@
-
id: 1

View file

@ -0,0 +1,16 @@
-
id: 1
uid: 1
lookup_key: key-1
# Expected to be deleted due to invalid user foreign key
-
id: 2
uid: 100
lookup_key: key-2
# Expected to be deleted due to a null user foreign key
-
id: 3
uid: null
lookup_key: key-3

View file

@ -0,0 +1,2 @@
-
id: 1

View file

@ -0,0 +1,28 @@
-
id: 1
issue_id: 1
base_repo_id: 1
# Expected to be deleted due to invalid issue foreign key
-
id: 2
issue_id: 100
base_repo_id: 1
# Expected to be deleted due to null issue foreign key
-
id: 3
issue_id: null
base_repo_id: 1
# Expected to be deleted due to invalid repository foreign key
-
id: 4
issue_id: 1
base_repo_id: 100
# Expected to be deleted due to null repository foreign key
-
id: 5
issue_id: 1
base_repo_id: null