diff options
| -rw-r--r-- | models/fixtures/issue.yml | 4 | ||||
| -rw-r--r-- | models/fixtures/issue_index.yml | 24 | ||||
| -rw-r--r-- | models/index.go | 113 | ||||
| -rw-r--r-- | models/index_test.go | 27 | ||||
| -rw-r--r-- | models/issue.go | 42 | ||||
| -rw-r--r-- | models/issue_test.go | 56 | ||||
| -rw-r--r-- | models/issue_xref_test.go | 19 | ||||
| -rw-r--r-- | models/migrations/migrations.go | 2 | ||||
| -rw-r--r-- | models/migrations/v182.go | 42 | ||||
| -rw-r--r-- | models/migrations/v182_test.go | 59 | ||||
| -rw-r--r-- | models/models.go | 9 | ||||
| -rw-r--r-- | models/pull.go | 32 | ||||
| -rw-r--r-- | models/repo.go | 5 | ||||
| -rw-r--r-- | models/unit_tests.go | 2 |
14 files changed, 354 insertions, 82 deletions
diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index 31df00d9e6..946899d6ff 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -152,7 +152,7 @@ - id: 13 repo_id: 50 - index: 0 + index: 1 poster_id: 2 name: issue in active repo content: we'll be testing github issue 13171 with this. @@ -164,7 +164,7 @@ - id: 14 repo_id: 51 - index: 0 + index: 1 poster_id: 2 name: issue in archived repo content: we'll be testing github issue 13171 with this. diff --git a/models/fixtures/issue_index.yml b/models/fixtures/issue_index.yml new file mode 100644 index 0000000000..49d95c57ab --- /dev/null +++ b/models/fixtures/issue_index.yml @@ -0,0 +1,24 @@ +- + group_id: 1 + max_index: 5 +- + group_id: 2 + max_index: 2 +- + group_id: 3 + max_index: 2 +- + group_id: 10 + max_index: 1 +- + group_id: 48 + max_index: 1 +- + group_id: 42 + max_index: 1 +- + group_id: 50 + max_index: 1 +- + group_id: 51 + max_index: 1
\ No newline at end of file diff --git a/models/index.go b/models/index.go new file mode 100644 index 0000000000..18db13c490 --- /dev/null +++ b/models/index.go @@ -0,0 +1,113 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package models + +import ( + "errors" + "fmt" + + "code.gitea.io/gitea/modules/setting" +) + +// ResourceIndex represents a resource index which could be used as issue/release and others +// We can create different tables i.e. issue_index, release_index and etc. +type ResourceIndex struct { + GroupID int64 `xorm:"unique"` + MaxIndex int64 `xorm:"index"` +} + +// IssueIndex represents the issue index table +type IssueIndex ResourceIndex + +// upsertResourceIndex the function will not return until it acquires the lock or receives an error. +func upsertResourceIndex(e Engine, tableName string, groupID int64) (err error) { + // An atomic UPSERT operation (INSERT/UPDATE) is the only operation + // that ensures that the key is actually locked. + switch { + case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL: + _, err = e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+ + "VALUES (?,1) ON CONFLICT (group_id) DO UPDATE SET max_index = %s.max_index+1", + tableName, tableName), groupID) + case setting.Database.UseMySQL: + _, err = e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+ + "VALUES (?,1) ON DUPLICATE KEY UPDATE max_index = max_index+1", tableName), + groupID) + case setting.Database.UseMSSQL: + // https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/ + _, err = e.Exec(fmt.Sprintf("MERGE %s WITH (HOLDLOCK) as target "+ + "USING (SELECT ? AS group_id) AS src "+ + "ON src.group_id = target.group_id "+ + "WHEN MATCHED THEN UPDATE SET target.max_index = target.max_index+1 "+ + "WHEN NOT MATCHED THEN INSERT (group_id, max_index) "+ + "VALUES (src.group_id, 1);", tableName), + groupID) + default: + return fmt.Errorf("database type not supported") + } + return +} + +var ( + // ErrResouceOutdated represents an error when request resource outdated + ErrResouceOutdated = errors.New("resource outdated") + // ErrGetResourceIndexFailed represents an error when resource index retries 3 times + ErrGetResourceIndexFailed = errors.New("get resource index failed") +) + +const ( + maxDupIndexAttempts = 3 +) + +// GetNextResourceIndex retried 3 times to generate a resource index +func GetNextResourceIndex(tableName string, groupID int64) (int64, error) { + for i := 0; i < maxDupIndexAttempts; i++ { + idx, err := getNextResourceIndex(tableName, groupID) + if err == ErrResouceOutdated { + continue + } + if err != nil { + return 0, err + } + return idx, nil + } + return 0, ErrGetResourceIndexFailed +} + +// deleteResouceIndex delete resource index +func deleteResouceIndex(e Engine, tableName string, groupID int64) error { + _, err := e.Exec(fmt.Sprintf("DELETE FROM %s WHERE group_id=?", tableName), groupID) + return err +} + +// getNextResourceIndex return the next index +func getNextResourceIndex(tableName string, groupID int64) (int64, error) { + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return 0, err + } + var preIdx int64 + _, err := sess.SQL(fmt.Sprintf("SELECT max_index FROM %s WHERE group_id = ?", tableName), groupID).Get(&preIdx) + if err != nil { + return 0, err + } + + if err := upsertResourceIndex(sess, tableName, groupID); err != nil { + return 0, err + } + + var curIdx int64 + has, err := sess.SQL(fmt.Sprintf("SELECT max_index FROM %s WHERE group_id = ? AND max_index=?", tableName), groupID, preIdx+1).Get(&curIdx) + if err != nil { + return 0, err + } + if !has { + return 0, ErrResouceOutdated + } + if err := sess.Commit(); err != nil { + return 0, err + } + return curIdx, nil +} diff --git a/models/index_test.go b/models/index_test.go new file mode 100644 index 0000000000..40e570ad9f --- /dev/null +++ b/models/index_test.go @@ -0,0 +1,27 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package models + +import ( + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestResourceIndex(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func(i int) { + testInsertIssue(t, fmt.Sprintf("issue %d", i+1), "my issue", 0) + wg.Done() + }(i) + } + wg.Wait() +} diff --git a/models/issue.go b/models/issue.go index 760aaaab09..769988795a 100644 --- a/models/issue.go +++ b/models/issue.go @@ -78,9 +78,8 @@ var ( ) const ( - issueTasksRegexpStr = `(^\s*[-*]\s\[[\sxX]\]\s.)|(\n\s*[-*]\s\[[\sxX]\]\s.)` - issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[xX]\]\s.)|(\n\s*[-*]\s\[[xX]\]\s.)` - issueMaxDupIndexAttempts = 3 + issueTasksRegexpStr = `(^\s*[-*]\s\[[\sxX]\]\s.)|(\n\s*[-*]\s\[[\sxX]\]\s.)` + issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[xX]\]\s.)|(\n\s*[-*]\s\[[xX]\]\s.)` ) func init() { @@ -896,21 +895,17 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { } } - // Milestone validation should happen before insert actual object. - if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1"). - Where("repo_id=?", opts.Issue.RepoID). - Insert(opts.Issue); err != nil { - return ErrNewIssueInsert{err} + if opts.Issue.Index <= 0 { + return fmt.Errorf("no issue index provided") + } + if opts.Issue.ID > 0 { + return fmt.Errorf("issue exist") } - inserted, err := getIssueByID(e, opts.Issue.ID) - if err != nil { + if _, err := e.Insert(opts.Issue); err != nil { return err } - // Patch Index with the value calculated by the database - opts.Issue.Index = inserted.Index - if opts.Issue.MilestoneID > 0 { if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil { return err @@ -987,24 +982,13 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { // NewIssue creates new issue with labels for repository. func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) { - // Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887 - i := 0 - for { - if err = newIssueAttempt(repo, issue, labelIDs, uuids); err == nil { - return nil - } - if !IsErrNewIssueInsert(err) { - return err - } - if i++; i == issueMaxDupIndexAttempts { - break - } - log.Error("NewIssue: error attempting to insert the new issue; will retry. Original error: %v", err) + idx, err := GetNextResourceIndex("issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate issue index failed: %v", err) } - return fmt.Errorf("NewIssue: too many errors attempting to insert the new issue. Last error was: %v", err) -} -func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) { + issue.Index = idx + sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { diff --git a/models/issue_test.go b/models/issue_test.go index b612ab267b..f2c9b7a68f 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -345,37 +345,45 @@ func TestGetRepoIDsForIssuesOptions(t *testing.T) { } } -func testInsertIssue(t *testing.T, title, content string) { - repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) - user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) - - issue := Issue{ - RepoID: repo.ID, - PosterID: user.ID, - Title: title, - Content: content, - } - err := NewIssue(repo, &issue, nil, nil) - assert.NoError(t, err) - +func testInsertIssue(t *testing.T, title, content string, expectIndex int64) *Issue { var newIssue Issue - has, err := x.ID(issue.ID).Get(&newIssue) - assert.NoError(t, err) - assert.True(t, has) - assert.EqualValues(t, issue.Title, newIssue.Title) - assert.EqualValues(t, issue.Content, newIssue.Content) - // there are 5 issues and max index is 5 on repository 1, so this one should 6 - assert.EqualValues(t, 6, newIssue.Index) + t.Run(title, func(t *testing.T) { + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + + issue := Issue{ + RepoID: repo.ID, + PosterID: user.ID, + Title: title, + Content: content, + } + err := NewIssue(repo, &issue, nil, nil) + assert.NoError(t, err) - _, err = x.ID(issue.ID).Delete(new(Issue)) - assert.NoError(t, err) + has, err := x.ID(issue.ID).Get(&newIssue) + assert.NoError(t, err) + assert.True(t, has) + assert.EqualValues(t, issue.Title, newIssue.Title) + assert.EqualValues(t, issue.Content, newIssue.Content) + if expectIndex > 0 { + assert.EqualValues(t, expectIndex, newIssue.Index) + } + }) + return &newIssue } func TestIssue_InsertIssue(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - testInsertIssue(t, "my issue1", "special issue's comments?") - testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?") + // there are 5 issues and max index is 5 on repository 1, so this one should 6 + issue := testInsertIssue(t, "my issue1", "special issue's comments?", 6) + _, err := x.ID(issue.ID).Delete(new(Issue)) + assert.NoError(t, err) + + issue = testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7) + _, err = x.ID(issue.ID).Delete(new(Issue)) + assert.NoError(t, err) + } func TestIssue_ResolveMentions(t *testing.T) { diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index f7a1adb083..a2d1a4b11e 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -125,12 +125,27 @@ func TestXRef_ResolveCrossReferences(t *testing.T) { func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispull bool) *Issue { r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository) d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User) - i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: ispull} + + idx, err := GetNextResourceIndex("issue_index", r.ID) + assert.NoError(t, err) + i := &Issue{ + RepoID: r.ID, + PosterID: d.ID, + Poster: d, + Title: title, + Content: content, + IsPull: ispull, + Index: idx, + } sess := x.NewSession() defer sess.Close() + assert.NoError(t, sess.Begin()) - _, err := sess.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").Where("repo_id=?", repo).Insert(i) + err = newIssue(sess, d, NewIssueOptions{ + Repo: r, + Issue: i, + }) assert.NoError(t, err) i, err = getIssueByID(sess, i.ID) assert.NoError(t, err) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index df1bac4a13..4c07db0a0f 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -313,6 +313,8 @@ var migrations = []Migration{ NewMigration("Delete credentials from past migrations", deleteMigrationCredentials), // v181 -> v182 NewMigration("Always save primary email on email address table", addPrimaryEmail2EmailAddress), + // v182 -> v183 + NewMigration("Add issue resource index table", addIssueResourceIndexTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v182.go b/models/migrations/v182.go new file mode 100644 index 0000000000..dd9a04f27e --- /dev/null +++ b/models/migrations/v182.go @@ -0,0 +1,42 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addIssueResourceIndexTable(x *xorm.Engine) error { + type ResourceIndex struct { + GroupID int64 `xorm:"index unique(s)"` + MaxIndex int64 `xorm:"index unique(s)"` + } + + sess := x.NewSession() + defer sess.Close() + + if err := sess.Begin(); err != nil { + return err + } + + if err := sess.Table("issue_index").Sync2(new(ResourceIndex)); err != nil { + return err + } + + // Remove data we're goint to rebuild + if _, err := sess.Table("issue_index").Where("1=1").Delete(&ResourceIndex{}); err != nil { + return err + } + + // Create current data for all repositories with issues and PRs + if _, err := sess.Exec("INSERT INTO issue_index (group_id, max_index) " + + "SELECT max_data.repo_id, max_data.max_index " + + "FROM ( SELECT issue.repo_id AS repo_id, max(issue.`index`) AS max_index " + + "FROM issue GROUP BY issue.repo_id) AS max_data"); err != nil { + return err + } + + return sess.Commit() +} diff --git a/models/migrations/v182_test.go b/models/migrations/v182_test.go new file mode 100644 index 0000000000..6f418f7794 --- /dev/null +++ b/models/migrations/v182_test.go @@ -0,0 +1,59 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_addIssueResourceIndexTable(t *testing.T) { + // Create the models used in the migration + type Issue struct { + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + Index int64 `xorm:"UNIQUE(s)"` + } + + // Prepare and load the testing database + x, deferable := prepareTestEnv(t, 0, new(Issue)) + if x == nil || t.Failed() { + defer deferable() + return + } + defer deferable() + + // Run the migration + if err := addIssueResourceIndexTable(x); err != nil { + assert.NoError(t, err) + return + } + + type ResourceIndex struct { + GroupID int64 `xorm:"index unique(s)"` + MaxIndex int64 `xorm:"index unique(s)"` + } + + var start = 0 + const batchSize = 1000 + for { + var indexes = make([]ResourceIndex, 0, batchSize) + err := x.Table("issue_index").Limit(batchSize, start).Find(&indexes) + assert.NoError(t, err) + + for _, idx := range indexes { + var maxIndex int + has, err := x.SQL("SELECT max(`index`) FROM issue WHERE repo_id = ?", idx.GroupID).Get(&maxIndex) + assert.NoError(t, err) + assert.True(t, has) + assert.EqualValues(t, maxIndex, idx.MaxIndex) + } + if len(indexes) < batchSize { + break + } + start += len(indexes) + } +} diff --git a/models/models.go b/models/models.go index b0a9062566..2b3203ecca 100644 --- a/models/models.go +++ b/models/models.go @@ -134,6 +134,7 @@ func init() { new(ProjectIssue), new(Session), new(RepoTransfer), + new(IssueIndex), ) gonicNames := []string{"SSL", "UID"} @@ -171,6 +172,10 @@ func GetNewEngine() (*xorm.Engine, error) { return engine, nil } +func syncTables() error { + return x.StoreEngine("InnoDB").Sync2(tables...) +} + // NewTestEngine sets a new test xorm.Engine func NewTestEngine() (err error) { x, err = GetNewEngine() @@ -181,7 +186,7 @@ func NewTestEngine() (err error) { x.SetMapper(names.GonicMapper{}) x.SetLogger(NewXORMLogger(!setting.IsProd())) x.ShowSQL(!setting.IsProd()) - return x.StoreEngine("InnoDB").Sync2(tables...) + return syncTables() } // SetEngine sets the xorm.Engine @@ -222,7 +227,7 @@ func NewEngine(ctx context.Context, migrateFunc func(*xorm.Engine) error) (err e return fmt.Errorf("migrate: %v", err) } - if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil { + if err = syncTables(); err != nil { return fmt.Errorf("sync database struct error: %v", err) } diff --git a/models/pull.go b/models/pull.go index a1fd7c3e41..1abe9fcce7 100644 --- a/models/pull.go +++ b/models/pull.go @@ -427,34 +427,23 @@ func (pr *PullRequest) SetMerged() (bool, error) { } // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { - // Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887 - i := 0 - for { - if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr); err == nil { - return nil - } - if !IsErrNewIssueInsert(err) { - return err - } - if i++; i == issueMaxDupIndexAttempts { - break - } - log.Error("NewPullRequest: error attempting to insert the new issue; will retry. Original error: %v", err) +func NewPullRequest(repo *Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { + idx, err := GetNextResourceIndex("issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate issue index failed: %v", err) } - return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err) -} -func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { + issue.Index = idx + sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { return err } - if err = newIssue(sess, pull.Poster, NewIssueOptions{ + if err = newIssue(sess, issue.Poster, NewIssueOptions{ Repo: repo, - Issue: pull, + Issue: issue, LabelIDs: labelIDs, Attachments: uuids, IsPull: true, @@ -465,10 +454,9 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid return fmt.Errorf("newIssue: %v", err) } - pr.Index = pull.Index + pr.Index = issue.Index pr.BaseRepo = repo - - pr.IssueID = pull.ID + pr.IssueID = issue.ID if _, err = sess.Insert(pr); err != nil { return fmt.Errorf("insert pull repo: %v", err) } diff --git a/models/repo.go b/models/repo.go index 58a393ae70..532b7ae1f5 100644 --- a/models/repo.go +++ b/models/repo.go @@ -1510,6 +1510,11 @@ func DeleteRepository(doer *User, uid, repoID int64) error { return err } + // Delete issue index + if err := deleteResouceIndex(sess, "issue_index", repoID); err != nil { + return err + } + if repo.IsFork { if _, err := sess.Exec("UPDATE `repository` SET num_forks=num_forks-1 WHERE id=?", repo.ForkID); err != nil { return fmt.Errorf("decrease fork count: %v", err) diff --git a/models/unit_tests.go b/models/unit_tests.go index cefdae2cd6..5a145fa2c0 100644 --- a/models/unit_tests.go +++ b/models/unit_tests.go @@ -103,7 +103,7 @@ func CreateTestEngine(fixturesDir string) error { return err } x.SetMapper(names.GonicMapper{}) - if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil { + if err = syncTables(); err != nil { return err } switch os.Getenv("GITEA_UNIT_TESTS_VERBOSE") { |