From 763547f43f8eb9686e3b5617da33afdf04610326 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Mon, 29 Dec 2025 03:49:03 +0100 Subject: [PATCH] [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/.md` to be be used for the release notes instead of the title. Co-authored-by: Mathieu Fenniak Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10621 Reviewed-by: Mathieu Fenniak Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- .../forgejo_migrations/foreign_key_utils.go | 36 ++++----- .../v14a_add-foreign-keys-collaboration.go | 9 ++- ...14a_add-foreign-keys-collaboration_test.go | 53 +++++++++++++ ...14a_add-foreign-keys-forgejo_auth_token.go | 6 +- ...dd-foreign-keys-forgejo_auth_token_test.go | 50 +++++++++++++ .../v14a_add-foreign-keys-pull_request-1.go | 9 ++- ...4a_add-foreign-keys-pull_request-1_test.go | 69 +++++++++++++++++ models/forgejo_migrations_legacy/v41.go | 56 +++++++++----- models/forgejo_migrations_legacy/v41_test.go | 75 +++++++++++++++++++ models/forgejo_migrations_legacy/v44.go | 9 ++- models/forgejo_migrations_legacy/v44_test.go | 50 +++++++++++++ .../Test_AddForeignKeysAccess/access.yml | 28 +++++++ .../Test_AddForeignKeysAccess/repository.yml | 2 + .../tracked_time.yml | 33 ++++++++ .../Test_AddForeignKeysAccess/user.yml | 2 + .../issue.yml | 2 + .../stopwatch.yml | 28 +++++++ .../tracked_time.yml | 33 ++++++++ .../user.yml | 2 + .../collaboration.yml | 16 ++++ .../repository.yml | 2 + .../Test_addForeignKeysCollaboration/user.yml | 2 + .../forgejo_auth_token.yml | 16 ++++ .../user.yml | 2 + .../Test_addForeignKeysPullRequest1/issue.yml | 2 + .../pull_request.yml | 28 +++++++ .../repository.yml | 2 + 27 files changed, 578 insertions(+), 44 deletions(-) create mode 100644 models/forgejo_migrations/v14a_add-foreign-keys-collaboration_test.go create mode 100644 models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token_test.go create mode 100644 models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1_test.go create mode 100644 models/forgejo_migrations_legacy/v41_test.go create mode 100644 models/forgejo_migrations_legacy/v44_test.go create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/access.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/repository.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/tracked_time.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/user.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/issue.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/stopwatch.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/tracked_time.yml create mode 100644 models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/user.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/collaboration.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/repository.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/user.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/forgejo_auth_token.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/user.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/issue.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/pull_request.yml create mode 100644 models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/repository.yml diff --git a/models/forgejo_migrations/foreign_key_utils.go b/models/forgejo_migrations/foreign_key_utils.go index fc2680e43e..2a792fd66b 100644 --- a/models/forgejo_migrations/foreign_key_utils.go +++ b/models/forgejo_migrations/foreign_key_utils.go @@ -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 } diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-collaboration.go b/models/forgejo_migrations/v14a_add-foreign-keys-collaboration.go index a52e8d6220..774dfe158c 100644 --- a/models/forgejo_migrations/v14a_add-foreign-keys-collaboration.go +++ b/models/forgejo_migrations/v14a_add-foreign-keys-collaboration.go @@ -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)"), + ), + ) } diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-collaboration_test.go b/models/forgejo_migrations/v14a_add-foreign-keys-collaboration_test.go new file mode 100644 index 0000000000..657bdf377a --- /dev/null +++ b/models/forgejo_migrations/v14a_add-foreign-keys-collaboration_test.go @@ -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) +} diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token.go b/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token.go index a5d8126d91..4aea62da50 100644 --- a/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token.go +++ b/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token.go @@ -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)"), + ) } diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token_test.go b/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token_test.go new file mode 100644 index 0000000000..7f01a9ccf3 --- /dev/null +++ b/models/forgejo_migrations/v14a_add-foreign-keys-forgejo_auth_token_test.go @@ -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) +} diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1.go b/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1.go index 6d262f385f..6e67207c7d 100644 --- a/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1.go +++ b/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1.go @@ -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)"), + ), + ) } diff --git a/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1_test.go b/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1_test.go new file mode 100644 index 0000000000..ebe8600350 --- /dev/null +++ b/models/forgejo_migrations/v14a_add-foreign-keys-pull_request-1_test.go @@ -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) +} diff --git a/models/forgejo_migrations_legacy/v41.go b/models/forgejo_migrations_legacy/v41.go index 4b3306debd..6dd25486c6 100644 --- a/models/forgejo_migrations_legacy/v41.go +++ b/models/forgejo_migrations_legacy/v41.go @@ -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)"), + ), + ) } diff --git a/models/forgejo_migrations_legacy/v41_test.go b/models/forgejo_migrations_legacy/v41_test.go new file mode 100644 index 0000000000..47fc7f582e --- /dev/null +++ b/models/forgejo_migrations_legacy/v41_test.go @@ -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") +} diff --git a/models/forgejo_migrations_legacy/v44.go b/models/forgejo_migrations_legacy/v44.go index 459c7ac29d..9f2fcef4f4 100644 --- a/models/forgejo_migrations_legacy/v44.go +++ b/models/forgejo_migrations_legacy/v44.go @@ -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)"), + ), + ) } diff --git a/models/forgejo_migrations_legacy/v44_test.go b/models/forgejo_migrations_legacy/v44_test.go new file mode 100644 index 0000000000..e78aa2fe46 --- /dev/null +++ b/models/forgejo_migrations_legacy/v44_test.go @@ -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) +} diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/access.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/access.yml new file mode 100644 index 0000000000..8ce770fbd0 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/access.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/repository.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/repository.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/repository.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/tracked_time.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/tracked_time.yml new file mode 100644 index 0000000000..0be83d55a5 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/tracked_time.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/user.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/user.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysAccess/user.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/issue.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/issue.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/issue.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/stopwatch.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/stopwatch.yml new file mode 100644 index 0000000000..5d84c3a78d --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/stopwatch.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/tracked_time.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/tracked_time.yml new file mode 100644 index 0000000000..0be83d55a5 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/tracked_time.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/user.yml b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/user.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_AddForeignKeysStopwatchTrackedTime/user.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/collaboration.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/collaboration.yml new file mode 100644 index 0000000000..a96c2340c8 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/collaboration.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/repository.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/repository.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/repository.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/user.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/user.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysCollaboration/user.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/forgejo_auth_token.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/forgejo_auth_token.yml new file mode 100644 index 0000000000..ee151f769e --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/forgejo_auth_token.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/user.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/user.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysForgejoAuthToken/user.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/issue.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/issue.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/issue.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/pull_request.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/pull_request.yml new file mode 100644 index 0000000000..390bb2ff4d --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/pull_request.yml @@ -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 diff --git a/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/repository.yml b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/repository.yml new file mode 100644 index 0000000000..a88c2ef89f --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addForeignKeysPullRequest1/repository.yml @@ -0,0 +1,2 @@ +- + id: 1