Skip to content

Commit 0393a57

Browse files
lunny6543lafriks
authored
Add a new table issue_index to store the max issue index so that issue could be deleted with no duplicated index (#15599)
* Add a new table issue_index to store the max issue index so that issue could be deleted with no duplicated index * Fix pull index * Add tests for concurrent creating issues * Fix lint * Fix tests * Fix postgres test * Add test for migration v180 * Rename wrong test file name Co-authored-by: 6543 <6543@obermui.de> Co-authored-by: Lauris BH <lauris@nix.lv>
1 parent a005265 commit 0393a57

14 files changed

+354
-82
lines changed

models/fixtures/issue.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@
152152
-
153153
id: 13
154154
repo_id: 50
155-
index: 0
155+
index: 1
156156
poster_id: 2
157157
name: issue in active repo
158158
content: we'll be testing github issue 13171 with this.
@@ -164,7 +164,7 @@
164164
-
165165
id: 14
166166
repo_id: 51
167-
index: 0
167+
index: 1
168168
poster_id: 2
169169
name: issue in archived repo
170170
content: we'll be testing github issue 13171 with this.

models/fixtures/issue_index.yml

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
-
2+
group_id: 1
3+
max_index: 5
4+
-
5+
group_id: 2
6+
max_index: 2
7+
-
8+
group_id: 3
9+
max_index: 2
10+
-
11+
group_id: 10
12+
max_index: 1
13+
-
14+
group_id: 48
15+
max_index: 1
16+
-
17+
group_id: 42
18+
max_index: 1
19+
-
20+
group_id: 50
21+
max_index: 1
22+
-
23+
group_id: 51
24+
max_index: 1

models/index.go

+113
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
// Copyright 2021 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package models
6+
7+
import (
8+
"errors"
9+
"fmt"
10+
11+
"code.gitea.io/gitea/modules/setting"
12+
)
13+
14+
// ResourceIndex represents a resource index which could be used as issue/release and others
15+
// We can create different tables i.e. issue_index, release_index and etc.
16+
type ResourceIndex struct {
17+
GroupID int64 `xorm:"unique"`
18+
MaxIndex int64 `xorm:"index"`
19+
}
20+
21+
// IssueIndex represents the issue index table
22+
type IssueIndex ResourceIndex
23+
24+
// upsertResourceIndex the function will not return until it acquires the lock or receives an error.
25+
func upsertResourceIndex(e Engine, tableName string, groupID int64) (err error) {
26+
// An atomic UPSERT operation (INSERT/UPDATE) is the only operation
27+
// that ensures that the key is actually locked.
28+
switch {
29+
case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL:
30+
_, err = e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+
31+
"VALUES (?,1) ON CONFLICT (group_id) DO UPDATE SET max_index = %s.max_index+1",
32+
tableName, tableName), groupID)
33+
case setting.Database.UseMySQL:
34+
_, err = e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+
35+
"VALUES (?,1) ON DUPLICATE KEY UPDATE max_index = max_index+1", tableName),
36+
groupID)
37+
case setting.Database.UseMSSQL:
38+
// https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/
39+
_, err = e.Exec(fmt.Sprintf("MERGE %s WITH (HOLDLOCK) as target "+
40+
"USING (SELECT ? AS group_id) AS src "+
41+
"ON src.group_id = target.group_id "+
42+
"WHEN MATCHED THEN UPDATE SET target.max_index = target.max_index+1 "+
43+
"WHEN NOT MATCHED THEN INSERT (group_id, max_index) "+
44+
"VALUES (src.group_id, 1);", tableName),
45+
groupID)
46+
default:
47+
return fmt.Errorf("database type not supported")
48+
}
49+
return
50+
}
51+
52+
var (
53+
// ErrResouceOutdated represents an error when request resource outdated
54+
ErrResouceOutdated = errors.New("resource outdated")
55+
// ErrGetResourceIndexFailed represents an error when resource index retries 3 times
56+
ErrGetResourceIndexFailed = errors.New("get resource index failed")
57+
)
58+
59+
const (
60+
maxDupIndexAttempts = 3
61+
)
62+
63+
// GetNextResourceIndex retried 3 times to generate a resource index
64+
func GetNextResourceIndex(tableName string, groupID int64) (int64, error) {
65+
for i := 0; i < maxDupIndexAttempts; i++ {
66+
idx, err := getNextResourceIndex(tableName, groupID)
67+
if err == ErrResouceOutdated {
68+
continue
69+
}
70+
if err != nil {
71+
return 0, err
72+
}
73+
return idx, nil
74+
}
75+
return 0, ErrGetResourceIndexFailed
76+
}
77+
78+
// deleteResouceIndex delete resource index
79+
func deleteResouceIndex(e Engine, tableName string, groupID int64) error {
80+
_, err := e.Exec(fmt.Sprintf("DELETE FROM %s WHERE group_id=?", tableName), groupID)
81+
return err
82+
}
83+
84+
// getNextResourceIndex return the next index
85+
func getNextResourceIndex(tableName string, groupID int64) (int64, error) {
86+
sess := x.NewSession()
87+
defer sess.Close()
88+
if err := sess.Begin(); err != nil {
89+
return 0, err
90+
}
91+
var preIdx int64
92+
_, err := sess.SQL(fmt.Sprintf("SELECT max_index FROM %s WHERE group_id = ?", tableName), groupID).Get(&preIdx)
93+
if err != nil {
94+
return 0, err
95+
}
96+
97+
if err := upsertResourceIndex(sess, tableName, groupID); err != nil {
98+
return 0, err
99+
}
100+
101+
var curIdx int64
102+
has, err := sess.SQL(fmt.Sprintf("SELECT max_index FROM %s WHERE group_id = ? AND max_index=?", tableName), groupID, preIdx+1).Get(&curIdx)
103+
if err != nil {
104+
return 0, err
105+
}
106+
if !has {
107+
return 0, ErrResouceOutdated
108+
}
109+
if err := sess.Commit(); err != nil {
110+
return 0, err
111+
}
112+
return curIdx, nil
113+
}

models/index_test.go

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Copyright 2021 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package models
6+
7+
import (
8+
"fmt"
9+
"sync"
10+
"testing"
11+
12+
"github.com/stretchr/testify/assert"
13+
)
14+
15+
func TestResourceIndex(t *testing.T) {
16+
assert.NoError(t, PrepareTestDatabase())
17+
18+
var wg sync.WaitGroup
19+
for i := 0; i < 100; i++ {
20+
wg.Add(1)
21+
go func(i int) {
22+
testInsertIssue(t, fmt.Sprintf("issue %d", i+1), "my issue", 0)
23+
wg.Done()
24+
}(i)
25+
}
26+
wg.Wait()
27+
}

models/issue.go

+13-29
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,8 @@ var (
7878
)
7979

8080
const (
81-
issueTasksRegexpStr = `(^\s*[-*]\s\[[\sxX]\]\s.)|(\n\s*[-*]\s\[[\sxX]\]\s.)`
82-
issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[xX]\]\s.)|(\n\s*[-*]\s\[[xX]\]\s.)`
83-
issueMaxDupIndexAttempts = 3
81+
issueTasksRegexpStr = `(^\s*[-*]\s\[[\sxX]\]\s.)|(\n\s*[-*]\s\[[\sxX]\]\s.)`
82+
issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[xX]\]\s.)|(\n\s*[-*]\s\[[xX]\]\s.)`
8483
)
8584

8685
func init() {
@@ -896,21 +895,17 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
896895
}
897896
}
898897

899-
// Milestone validation should happen before insert actual object.
900-
if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").
901-
Where("repo_id=?", opts.Issue.RepoID).
902-
Insert(opts.Issue); err != nil {
903-
return ErrNewIssueInsert{err}
898+
if opts.Issue.Index <= 0 {
899+
return fmt.Errorf("no issue index provided")
900+
}
901+
if opts.Issue.ID > 0 {
902+
return fmt.Errorf("issue exist")
904903
}
905904

906-
inserted, err := getIssueByID(e, opts.Issue.ID)
907-
if err != nil {
905+
if _, err := e.Insert(opts.Issue); err != nil {
908906
return err
909907
}
910908

911-
// Patch Index with the value calculated by the database
912-
opts.Issue.Index = inserted.Index
913-
914909
if opts.Issue.MilestoneID > 0 {
915910
if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
916911
return err
@@ -987,24 +982,13 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
987982

988983
// NewIssue creates new issue with labels for repository.
989984
func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
990-
// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
991-
i := 0
992-
for {
993-
if err = newIssueAttempt(repo, issue, labelIDs, uuids); err == nil {
994-
return nil
995-
}
996-
if !IsErrNewIssueInsert(err) {
997-
return err
998-
}
999-
if i++; i == issueMaxDupIndexAttempts {
1000-
break
1001-
}
1002-
log.Error("NewIssue: error attempting to insert the new issue; will retry. Original error: %v", err)
985+
idx, err := GetNextResourceIndex("issue_index", repo.ID)
986+
if err != nil {
987+
return fmt.Errorf("generate issue index failed: %v", err)
1003988
}
1004-
return fmt.Errorf("NewIssue: too many errors attempting to insert the new issue. Last error was: %v", err)
1005-
}
1006989

1007-
func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
990+
issue.Index = idx
991+
1008992
sess := x.NewSession()
1009993
defer sess.Close()
1010994
if err = sess.Begin(); err != nil {

models/issue_test.go

+32-24
Original file line numberDiff line numberDiff line change
@@ -345,37 +345,45 @@ func TestGetRepoIDsForIssuesOptions(t *testing.T) {
345345
}
346346
}
347347

348-
func testInsertIssue(t *testing.T, title, content string) {
349-
repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
350-
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
351-
352-
issue := Issue{
353-
RepoID: repo.ID,
354-
PosterID: user.ID,
355-
Title: title,
356-
Content: content,
357-
}
358-
err := NewIssue(repo, &issue, nil, nil)
359-
assert.NoError(t, err)
360-
348+
func testInsertIssue(t *testing.T, title, content string, expectIndex int64) *Issue {
361349
var newIssue Issue
362-
has, err := x.ID(issue.ID).Get(&newIssue)
363-
assert.NoError(t, err)
364-
assert.True(t, has)
365-
assert.EqualValues(t, issue.Title, newIssue.Title)
366-
assert.EqualValues(t, issue.Content, newIssue.Content)
367-
// there are 5 issues and max index is 5 on repository 1, so this one should 6
368-
assert.EqualValues(t, 6, newIssue.Index)
350+
t.Run(title, func(t *testing.T) {
351+
repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
352+
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
353+
354+
issue := Issue{
355+
RepoID: repo.ID,
356+
PosterID: user.ID,
357+
Title: title,
358+
Content: content,
359+
}
360+
err := NewIssue(repo, &issue, nil, nil)
361+
assert.NoError(t, err)
369362

370-
_, err = x.ID(issue.ID).Delete(new(Issue))
371-
assert.NoError(t, err)
363+
has, err := x.ID(issue.ID).Get(&newIssue)
364+
assert.NoError(t, err)
365+
assert.True(t, has)
366+
assert.EqualValues(t, issue.Title, newIssue.Title)
367+
assert.EqualValues(t, issue.Content, newIssue.Content)
368+
if expectIndex > 0 {
369+
assert.EqualValues(t, expectIndex, newIssue.Index)
370+
}
371+
})
372+
return &newIssue
372373
}
373374

374375
func TestIssue_InsertIssue(t *testing.T) {
375376
assert.NoError(t, PrepareTestDatabase())
376377

377-
testInsertIssue(t, "my issue1", "special issue's comments?")
378-
testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?")
378+
// there are 5 issues and max index is 5 on repository 1, so this one should 6
379+
issue := testInsertIssue(t, "my issue1", "special issue's comments?", 6)
380+
_, err := x.ID(issue.ID).Delete(new(Issue))
381+
assert.NoError(t, err)
382+
383+
issue = testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7)
384+
_, err = x.ID(issue.ID).Delete(new(Issue))
385+
assert.NoError(t, err)
386+
379387
}
380388

381389
func TestIssue_ResolveMentions(t *testing.T) {

models/issue_xref_test.go

+17-2
Original file line numberDiff line numberDiff line change
@@ -125,12 +125,27 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
125125
func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispull bool) *Issue {
126126
r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository)
127127
d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User)
128-
i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: ispull}
128+
129+
idx, err := GetNextResourceIndex("issue_index", r.ID)
130+
assert.NoError(t, err)
131+
i := &Issue{
132+
RepoID: r.ID,
133+
PosterID: d.ID,
134+
Poster: d,
135+
Title: title,
136+
Content: content,
137+
IsPull: ispull,
138+
Index: idx,
139+
}
129140

130141
sess := x.NewSession()
131142
defer sess.Close()
143+
132144
assert.NoError(t, sess.Begin())
133-
_, err := sess.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").Where("repo_id=?", repo).Insert(i)
145+
err = newIssue(sess, d, NewIssueOptions{
146+
Repo: r,
147+
Issue: i,
148+
})
134149
assert.NoError(t, err)
135150
i, err = getIssueByID(sess, i.ID)
136151
assert.NoError(t, err)

models/migrations/migrations.go

+2
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,8 @@ var migrations = []Migration{
313313
NewMigration("Delete credentials from past migrations", deleteMigrationCredentials),
314314
// v181 -> v182
315315
NewMigration("Always save primary email on email address table", addPrimaryEmail2EmailAddress),
316+
// v182 -> v183
317+
NewMigration("Add issue resource index table", addIssueResourceIndexTable),
316318
}
317319

318320
// GetCurrentDBVersion returns the current db version

0 commit comments

Comments
 (0)