From 71ca3067bcc6c7c7772d38fc7590505c8c7148ed Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 23 Dec 2022 19:35:43 +0800 Subject: [PATCH] Check primary keys for all tables and drop ForeignReference (#21721) Some dbs require that all tables have primary keys, see - #16802 - #21086 We can add a test to keep it from being broken again. Edit: ~Added missing primary key for `ForeignReference`~ Dropped the `ForeignReference` table to satisfy the check, so it closes #21086. More context can be found in comments. Signed-off-by: Andrew Thornton Co-authored-by: zeripath --- models/db/engine_test.go | 33 ++++++++++++ models/fixtures/foreign_reference.yml | 1 - models/foreignreference/error.go | 52 ------------------ models/foreignreference/foreignreference.go | 31 ----------- models/issues/issue.go | 60 ++------------------- models/issues/issue_test.go | 34 ------------ models/migrate.go | 7 --- models/migrate_test.go | 12 ----- models/migrations/migrations.go | 2 + models/migrations/v1_19/v237.go | 15 ++++++ services/migrations/gitea_uploader.go | 11 ---- 11 files changed, 55 insertions(+), 203 deletions(-) delete mode 100644 models/fixtures/foreign_reference.yml delete mode 100644 models/foreignreference/error.go delete mode 100644 models/foreignreference/foreignreference.go create mode 100644 models/migrations/v1_19/v237.go diff --git a/models/db/engine_test.go b/models/db/engine_test.go index fa1ac08a1..551436677 100644 --- a/models/db/engine_test.go +++ b/models/db/engine_test.go @@ -12,6 +12,8 @@ import ( "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/setting" + _ "code.gitea.io/gitea/cmd" // for TestPrimaryKeys + "github.com/stretchr/testify/assert" ) @@ -51,3 +53,34 @@ func TestDeleteOrphanedObjects(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, countBefore, countAfter) } + +func TestPrimaryKeys(t *testing.T) { + // Some dbs require that all tables have primary keys, see + // https://github.com/go-gitea/gitea/issues/21086 + // https://github.com/go-gitea/gitea/issues/16802 + // To avoid creating tables without primary key again, this test will check them. + // Import "code.gitea.io/gitea/cmd" to make sure each db.RegisterModel in init functions has been called. + + beans, err := db.NamesToBean() + if err != nil { + t.Fatal(err) + } + + whitelist := map[string]string{ + "the_table_name_to_skip_checking": "Write a note here to explain why", + } + + for _, bean := range beans { + table, err := db.TableInfo(bean) + if err != nil { + t.Fatal(err) + } + if why, ok := whitelist[table.Name]; ok { + t.Logf("ignore %q because %q", table.Name, why) + continue + } + if len(table.PrimaryKeys) == 0 { + t.Errorf("table %q has no primary key", table.Name) + } + } +} diff --git a/models/fixtures/foreign_reference.yml b/models/fixtures/foreign_reference.yml deleted file mode 100644 index ca780a73a..000000000 --- a/models/fixtures/foreign_reference.yml +++ /dev/null @@ -1 +0,0 @@ -[] # empty diff --git a/models/foreignreference/error.go b/models/foreignreference/error.go deleted file mode 100644 index 07ed1052a..000000000 --- a/models/foreignreference/error.go +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright 2022 Gitea. All rights reserved. -// SPDX-License-Identifier: MIT - -package foreignreference - -import ( - "fmt" - - "code.gitea.io/gitea/modules/util" -) - -// ErrLocalIndexNotExist represents a "LocalIndexNotExist" kind of error. -type ErrLocalIndexNotExist struct { - RepoID int64 - ForeignIndex int64 - Type string -} - -// ErrLocalIndexNotExist checks if an error is a ErrLocalIndexNotExist. -func IsErrLocalIndexNotExist(err error) bool { - _, ok := err.(ErrLocalIndexNotExist) - return ok -} - -func (err ErrLocalIndexNotExist) Error() string { - return fmt.Sprintf("repository %d has no LocalIndex for ForeignIndex %d of type %s", err.RepoID, err.ForeignIndex, err.Type) -} - -func (err ErrLocalIndexNotExist) Unwrap() error { - return util.ErrNotExist -} - -// ErrForeignIndexNotExist represents a "ForeignIndexNotExist" kind of error. -type ErrForeignIndexNotExist struct { - RepoID int64 - LocalIndex int64 - Type string -} - -// ErrForeignIndexNotExist checks if an error is a ErrForeignIndexNotExist. -func IsErrForeignIndexNotExist(err error) bool { - _, ok := err.(ErrForeignIndexNotExist) - return ok -} - -func (err ErrForeignIndexNotExist) Error() string { - return fmt.Sprintf("repository %d has no ForeignIndex for LocalIndex %d of type %s", err.RepoID, err.LocalIndex, err.Type) -} - -func (err ErrForeignIndexNotExist) Unwrap() error { - return util.ErrNotExist -} diff --git a/models/foreignreference/foreignreference.go b/models/foreignreference/foreignreference.go deleted file mode 100644 index 2d2ad04c5..000000000 --- a/models/foreignreference/foreignreference.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2022 Gitea. All rights reserved. -// SPDX-License-Identifier: MIT - -package foreignreference - -import ( - "code.gitea.io/gitea/models/db" -) - -// Type* are valid values for the Type field of ForeignReference -const ( - TypeIssue = "issue" - TypePullRequest = "pull_request" - TypeComment = "comment" - TypeReview = "review" - TypeReviewComment = "review_comment" - TypeRelease = "release" -) - -// ForeignReference represents external references -type ForeignReference struct { - // RepoID is the first column in all indices. now we only need 2 indices: (repo, local) and (repo, foreign, type) - RepoID int64 `xorm:"UNIQUE(repo_foreign_type) INDEX(repo_local)" ` - LocalIndex int64 `xorm:"INDEX(repo_local)"` // the resource key inside Gitea, it can be IssueIndex, or some model ID. - ForeignIndex string `xorm:"INDEX UNIQUE(repo_foreign_type)"` - Type string `xorm:"VARCHAR(16) INDEX UNIQUE(repo_foreign_type)"` -} - -func init() { - db.RegisterModel(new(ForeignReference)) -} diff --git a/models/issues/issue.go b/models/issues/issue.go index fc93fcf45..f45e635c0 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -9,11 +9,9 @@ import ( "fmt" "regexp" "sort" - "strconv" "strings" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/foreignreference" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" @@ -136,12 +134,11 @@ type Issue struct { UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` ClosedUnix timeutil.TimeStamp `xorm:"INDEX"` - Attachments []*repo_model.Attachment `xorm:"-"` - Comments []*Comment `xorm:"-"` - Reactions ReactionList `xorm:"-"` - TotalTrackedTime int64 `xorm:"-"` - Assignees []*user_model.User `xorm:"-"` - ForeignReference *foreignreference.ForeignReference `xorm:"-"` + Attachments []*repo_model.Attachment `xorm:"-"` + Comments []*Comment `xorm:"-"` + Reactions ReactionList `xorm:"-"` + TotalTrackedTime int64 `xorm:"-"` + Assignees []*user_model.User `xorm:"-"` // IsLocked limits commenting abilities to users on an issue // with write access @@ -321,29 +318,6 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) { return nil } -func (issue *Issue) loadForeignReference(ctx context.Context) (err error) { - if issue.ForeignReference != nil { - return nil - } - reference := &foreignreference.ForeignReference{ - RepoID: issue.RepoID, - LocalIndex: issue.Index, - Type: foreignreference.TypeIssue, - } - has, err := db.GetEngine(ctx).Get(reference) - if err != nil { - return err - } else if !has { - return foreignreference.ErrForeignIndexNotExist{ - RepoID: issue.RepoID, - LocalIndex: issue.Index, - Type: foreignreference.TypeIssue, - } - } - issue.ForeignReference = reference - return nil -} - // LoadMilestone load milestone of this issue. func (issue *Issue) LoadMilestone(ctx context.Context) (err error) { if (issue.Milestone == nil || issue.Milestone.ID != issue.MilestoneID) && issue.MilestoneID > 0 { @@ -406,10 +380,6 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { } } - if err = issue.loadForeignReference(ctx); err != nil && !foreignreference.IsErrForeignIndexNotExist(err) { - return err - } - return issue.loadReactions(ctx) } @@ -1097,26 +1067,6 @@ func GetIssueByIndex(repoID, index int64) (*Issue, error) { return issue, nil } -// GetIssueByForeignIndex returns raw issue by foreign ID -func GetIssueByForeignIndex(ctx context.Context, repoID, foreignIndex int64) (*Issue, error) { - reference := &foreignreference.ForeignReference{ - RepoID: repoID, - ForeignIndex: strconv.FormatInt(foreignIndex, 10), - Type: foreignreference.TypeIssue, - } - has, err := db.GetEngine(ctx).Get(reference) - if err != nil { - return nil, err - } else if !has { - return nil, foreignreference.ErrLocalIndexNotExist{ - RepoID: repoID, - ForeignIndex: foreignIndex, - Type: foreignreference.TypeIssue, - } - } - return GetIssueByIndex(repoID, reference.LocalIndex) -} - // GetIssueWithAttrsByIndex returns issue by index in a repository. func GetIssueWithAttrsByIndex(repoID, index int64) (*Issue, error) { issue, err := GetIssueByIndex(repoID, index) diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index 6764a9e62..de1da19ab 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -7,13 +7,11 @@ import ( "context" "fmt" "sort" - "strconv" "sync" "testing" "time" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/foreignreference" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" repo_model "code.gitea.io/gitea/models/repo" @@ -501,38 +499,6 @@ func TestCorrectIssueStats(t *testing.T) { assert.EqualValues(t, issueStats.OpenCount, issueAmount) } -func TestIssueForeignReference(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 4}) - assert.NotEqualValues(t, issue.Index, issue.ID) // make sure they are different to avoid false positive - - // it is fine for an issue to not have a foreign reference - err := issue.LoadAttributes(db.DefaultContext) - assert.NoError(t, err) - assert.Nil(t, issue.ForeignReference) - - var foreignIndex int64 = 12345 - _, err = issues_model.GetIssueByForeignIndex(context.Background(), issue.RepoID, foreignIndex) - assert.True(t, foreignreference.IsErrLocalIndexNotExist(err)) - - err = db.Insert(db.DefaultContext, &foreignreference.ForeignReference{ - LocalIndex: issue.Index, - ForeignIndex: strconv.FormatInt(foreignIndex, 10), - RepoID: issue.RepoID, - Type: foreignreference.TypeIssue, - }) - assert.NoError(t, err) - - err = issue.LoadAttributes(db.DefaultContext) - assert.NoError(t, err) - - assert.EqualValues(t, issue.ForeignReference.ForeignIndex, strconv.FormatInt(foreignIndex, 10)) - - found, err := issues_model.GetIssueByForeignIndex(context.Background(), issue.RepoID, foreignIndex) - assert.NoError(t, err) - assert.EqualValues(t, found.Index, issue.Index) -} - func TestMilestoneList_LoadTotalTrackedTimes(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) miles := issues_model.MilestoneList{ diff --git a/models/migrate.go b/models/migrate.go index b1b556812..82cacd4a7 100644 --- a/models/migrate.go +++ b/models/migrate.go @@ -83,13 +83,6 @@ func insertIssue(ctx context.Context, issue *issues_model.Issue) error { } } - if issue.ForeignReference != nil { - issue.ForeignReference.LocalIndex = issue.Index - if _, err := sess.Insert(issue.ForeignReference); err != nil { - return err - } - } - return nil } diff --git a/models/migrate_test.go b/models/migrate_test.go index 48cd905e4..42102f9a7 100644 --- a/models/migrate_test.go +++ b/models/migrate_test.go @@ -4,11 +4,9 @@ package models import ( - "strconv" "testing" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/foreignreference" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -48,7 +46,6 @@ func assertCreateIssues(t *testing.T, isPull bool) { UserID: owner.ID, } - foreignIndex := int64(12345) title := "issuetitle1" is := &issues_model.Issue{ RepoID: repo.ID, @@ -62,20 +59,11 @@ func assertCreateIssues(t *testing.T, isPull bool) { IsClosed: true, Labels: []*issues_model.Label{label}, Reactions: []*issues_model.Reaction{reaction}, - ForeignReference: &foreignreference.ForeignReference{ - ForeignIndex: strconv.FormatInt(foreignIndex, 10), - RepoID: repo.ID, - Type: foreignreference.TypeIssue, - }, } err := InsertIssues(is) assert.NoError(t, err) i := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: title}) - assert.Nil(t, i.ForeignReference) - err = i.LoadAttributes(db.DefaultContext) - assert.NoError(t, err) - assert.EqualValues(t, strconv.FormatInt(foreignIndex, 10), i.ForeignReference.ForeignIndex) unittest.AssertExistsAndLoadBean(t, &issues_model.Reaction{Type: "heart", UserID: owner.ID, IssueID: i.ID}) } diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 591bfa3e8..9d9c8f516 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -444,6 +444,8 @@ var migrations = []Migration{ NewMigration("Add index for access_token", v1_19.AddIndexForAccessToken), // v236 -> v237 NewMigration("Create secrets table", v1_19.CreateSecretsTable), + // v237 -> v238 + NewMigration("Drop ForeignReference table", v1_19.DropForeignReferenceTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_19/v237.go b/models/migrations/v1_19/v237.go new file mode 100644 index 000000000..b23c765aa --- /dev/null +++ b/models/migrations/v1_19/v237.go @@ -0,0 +1,15 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_19 //nolint + +import ( + "xorm.io/xorm" +) + +func DropForeignReferenceTable(x *xorm.Engine) error { + // Drop the table introduced in `v211`, it's considered badly designed and doesn't look like to be used. + // See: https://github.com/go-gitea/gitea/issues/21086#issuecomment-1318217453 + type ForeignReference struct{} + return x.DropTables(new(ForeignReference)) +} diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index f3848e616..23aa4ac2c 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -17,7 +17,6 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/foreignreference" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -403,16 +402,6 @@ func (g *GiteaLocalUploader) CreateIssues(issues ...*base.Issue) error { Labels: labels, CreatedUnix: timeutil.TimeStamp(issue.Created.Unix()), UpdatedUnix: timeutil.TimeStamp(issue.Updated.Unix()), - ForeignReference: &foreignreference.ForeignReference{ - LocalIndex: issue.GetLocalIndex(), - ForeignIndex: strconv.FormatInt(issue.GetForeignIndex(), 10), - RepoID: g.repo.ID, - Type: foreignreference.TypeIssue, - }, - } - - if is.ForeignReference.ForeignIndex == "0" { - is.ForeignReference.ForeignIndex = strconv.FormatInt(is.Index, 10) } if err := g.remapUser(issue, &is); err != nil {