From dcb3ddb5c5e092f49ed0e495e4973a6205ebb177 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Fri, 27 Sep 2019 18:56:17 -0300 Subject: [PATCH 1/6] Revert #7898 --- models/issue.go | 52 +++++++++++++++++++++---------------- routers/api/v1/repo/pull.go | 7 +++++ routers/repo/pull.go | 7 +++++ 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/models/issue.go b/models/issue.go index 77712c0fec72..eea39fb2c5ba 100644 --- a/models/issue.go +++ b/models/issue.go @@ -5,6 +5,7 @@ package models import ( + "errors" "fmt" "path" "regexp" @@ -73,7 +74,6 @@ var ( const issueTasksRegexpStr = `(^\s*[-*]\s\[[\sx]\]\s.)|(\n\s*[-*]\s\[[\sx]\]\s.)` const issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[x]\]\s.)|(\n\s*[-*]\s\[[x]\]\s.)` -const issueMaxDupIndexAttempts = 3 func init() { issueTasksPat = regexp.MustCompile(issueTasksRegexpStr) @@ -1053,9 +1053,36 @@ type NewIssueOptions struct { IsPull bool } +// GetMaxIndexOfIssue returns the max index on issue +func GetMaxIndexOfIssue(repoID int64) (int64, error) { + return getMaxIndexOfIssue(x, repoID) +} + +func getMaxIndexOfIssue(e Engine, repoID int64) (int64, error) { + var ( + maxIndex int64 + has bool + err error + ) + + has, err = e.SQL("SELECT COALESCE((SELECT MAX(`index`) FROM issue WHERE repo_id = ?),0)", repoID).Get(&maxIndex) + if err != nil { + return 0, err + } else if !has { + return 0, errors.New("Retrieve Max index from issue failed") + } + return maxIndex, nil +} + func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { opts.Issue.Title = strings.TrimSpace(opts.Issue.Title) + maxIndex, err := getMaxIndexOfIssue(e, opts.Issue.RepoID) + if err != nil { + return err + } + opts.Issue.Index = maxIndex + 1 + if opts.Issue.MilestoneID > 0 { milestone, err := getMilestoneByRepoID(e, opts.Issue.RepoID, opts.Issue.MilestoneID) if err != nil && !IsErrMilestoneNotExist(err) { @@ -1104,31 +1131,10 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { } // Milestone and assignee validation should happen before insert actual object. - - // There's no good way to identify a duplicate key error in database/sql; brute force some retries - dupIndexAttempts := issueMaxDupIndexAttempts - for { - _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1"). - Where("repo_id=?", opts.Issue.RepoID). - Insert(opts.Issue) - if err == nil { - break - } - - dupIndexAttempts-- - if dupIndexAttempts <= 0 { - return err - } - } - - 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 = changeMilestoneAssign(e, doer, opts.Issue, -1); err != nil { return err diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 0f9eab2f5029..370a95eabff1 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -254,8 +254,15 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) } + maxIndex, err := models.GetMaxIndexOfIssue(repo.ID) + if err != nil { + ctx.ServerError("GetPatch", err) + return + } + prIssue := &models.Issue{ RepoID: repo.ID, + Index: maxIndex + 1, Title: form.Title, PosterID: ctx.User.ID, Poster: ctx.User, diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 180d592e3dbf..f6332d0d5d20 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -766,8 +766,15 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } + maxIndex, err := models.GetMaxIndexOfIssue(repo.ID) + if err != nil { + ctx.ServerError("GetPatch", err) + return + } + pullIssue := &models.Issue{ RepoID: repo.ID, + Index: maxIndex + 1, Title: form.Title, PosterID: ctx.User.ID, Poster: ctx.User, From c786930c12ea83f7706f56c94794f4eed07f143b Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 28 Sep 2019 04:46:01 -0300 Subject: [PATCH 2/6] Transaction-aware retry create issue to cope with duplicate keys --- models/error.go | 15 +++++++++++++++ models/issue.go | 32 ++++++++++++++++++++++++++++---- models/pull.go | 18 +++++++++++++++++- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/models/error.go b/models/error.go index 9974287a0aef..995617e83b8a 100644 --- a/models/error.go +++ b/models/error.go @@ -1089,6 +1089,21 @@ func (err ErrIssueLabelTemplateLoad) Error() string { return fmt.Sprintf("Failed to load label template file '%s': %v", err.TemplateFile, err.OriginalError) } +// ErrNewIssueInsert is used when the INSERT statement in newIssue fails +type ErrNewIssueInsert struct { + OriginalError error +} + +// IsErrNewIssueInsert checks if an error is a ErrNewIssueInsert. +func IsErrNewIssueInsert(err error) bool { + _, ok := err.(ErrNewIssueInsert) + return ok +} + +func (err ErrNewIssueInsert) Error() string { + return err.OriginalError.Error() +} + // __________ .__ .__ __________ __ // \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_ // | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\ diff --git a/models/issue.go b/models/issue.go index eea39fb2c5ba..cc81a52b5411 100644 --- a/models/issue.go +++ b/models/issue.go @@ -74,6 +74,7 @@ var ( const issueTasksRegexpStr = `(^\s*[-*]\s\[[\sx]\]\s.)|(\n\s*[-*]\s\[[\sx]\]\s.)` const issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[x]\]\s.)|(\n\s*[-*]\s\[[x]\]\s.)` +const issueMaxDupIndexAttempts = 3 func init() { issueTasksPat = regexp.MustCompile(issueTasksRegexpStr) @@ -1132,7 +1133,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { // Milestone and assignee validation should happen before insert actual object. if _, err = e.Insert(opts.Issue); err != nil { - return err + return ErrNewIssueInsert{err} } if opts.Issue.MilestoneID > 0 { @@ -1207,6 +1208,24 @@ 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, assigneeIDs []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, assigneeIDs, uuids); err == nil { + return newIssuePostInsert(issue, repo) + } + 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) + } + return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err) +} + +func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { @@ -1220,7 +1239,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in Attachments: uuids, AssigneeIDs: assigneeIDs, }); err != nil { - if IsErrUserDoesNotHaveAccessToRepo(err) { + if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) { return err } return fmt.Errorf("newIssue: %v", err) @@ -1231,7 +1250,12 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in } sess.Close() - if err = NotifyWatchers(&Action{ + return nil +} + +// newIssuePostInsert performs issue creation operations that need to be separate transactions +func newIssuePostInsert(issue *Issue, repo *Repository) error { + if err := NotifyWatchers(&Action{ ActUserID: issue.Poster.ID, ActUser: issue.Poster, OpType: ActionCreateIssue, @@ -1244,7 +1268,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in } mode, _ := AccessLevel(issue.Poster, issue.Repo) - if err = PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{ + if err := PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{ Action: api.HookIssueOpened, Index: issue.Index, Issue: issue.APIFormat(), diff --git a/models/pull.go b/models/pull.go index 60ccb144f510..e35509ddf285 100644 --- a/models/pull.go +++ b/models/pull.go @@ -654,6 +654,22 @@ func (pr *PullRequest) testPatch(e Engine) (err error) { // NewPullRequest creates new pull request with labels for repository. func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte, assigneeIDs []int64) (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, patch, assigneeIDs); !IsErrNewIssueInsert(err) { + // Usually err == nil + return err + } + if i++; i == issueMaxDupIndexAttempts { + break + } + log.Error("NewPullRequest: error attempting to insert the new issue; will retry. Original error: %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, patch []byte, assigneeIDs []int64) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { @@ -668,7 +684,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str IsPull: true, AssigneeIDs: assigneeIDs, }); err != nil { - if IsErrUserDoesNotHaveAccessToRepo(err) { + if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) { return err } return fmt.Errorf("newIssue: %v", err) From e6e777fe2f7c83f72397a823cf43496489ea1288 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 29 Sep 2019 20:24:01 -0300 Subject: [PATCH 3/6] Restore INSERT ... WHERE usage --- models/issue.go | 42 +++++++++++-------------------------- routers/api/v1/repo/pull.go | 7 ------- routers/repo/pull.go | 7 ------- 3 files changed, 12 insertions(+), 44 deletions(-) diff --git a/models/issue.go b/models/issue.go index cc81a52b5411..b666f6d729df 100644 --- a/models/issue.go +++ b/models/issue.go @@ -5,7 +5,6 @@ package models import ( - "errors" "fmt" "path" "regexp" @@ -1054,36 +1053,9 @@ type NewIssueOptions struct { IsPull bool } -// GetMaxIndexOfIssue returns the max index on issue -func GetMaxIndexOfIssue(repoID int64) (int64, error) { - return getMaxIndexOfIssue(x, repoID) -} - -func getMaxIndexOfIssue(e Engine, repoID int64) (int64, error) { - var ( - maxIndex int64 - has bool - err error - ) - - has, err = e.SQL("SELECT COALESCE((SELECT MAX(`index`) FROM issue WHERE repo_id = ?),0)", repoID).Get(&maxIndex) - if err != nil { - return 0, err - } else if !has { - return 0, errors.New("Retrieve Max index from issue failed") - } - return maxIndex, nil -} - func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { opts.Issue.Title = strings.TrimSpace(opts.Issue.Title) - maxIndex, err := getMaxIndexOfIssue(e, opts.Issue.RepoID) - if err != nil { - return err - } - opts.Issue.Index = maxIndex + 1 - if opts.Issue.MilestoneID > 0 { milestone, err := getMilestoneByRepoID(e, opts.Issue.RepoID, opts.Issue.MilestoneID) if err != nil && !IsErrMilestoneNotExist(err) { @@ -1132,10 +1104,20 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { } // Milestone and assignee validation should happen before insert actual object. - if _, err = e.Insert(opts.Issue); err != nil { - return ErrNewIssueInsert{err} + if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1"). + Where("repo_id=?", opts.Issue.RepoID). + Insert(opts.Issue); err != nil { + return err } + inserted, err := getIssueByID(e, opts.Issue.ID) + if 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 = changeMilestoneAssign(e, doer, opts.Issue, -1); err != nil { return err diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 290ef8fd40d5..978c8a3f1f49 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -253,15 +253,8 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption deadlineUnix = timeutil.TimeStamp(form.Deadline.Unix()) } - maxIndex, err := models.GetMaxIndexOfIssue(repo.ID) - if err != nil { - ctx.ServerError("GetPatch", err) - return - } - prIssue := &models.Issue{ RepoID: repo.ID, - Index: maxIndex + 1, Title: form.Title, PosterID: ctx.User.ID, Poster: ctx.User, diff --git a/routers/repo/pull.go b/routers/repo/pull.go index af82d8dcb0e6..72d2ffcaa7d6 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -765,15 +765,8 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - maxIndex, err := models.GetMaxIndexOfIssue(repo.ID) - if err != nil { - ctx.ServerError("GetPatch", err) - return - } - pullIssue := &models.Issue{ RepoID: repo.ID, - Index: maxIndex + 1, Title: form.Title, PosterID: ctx.User.ID, Poster: ctx.User, From 07f5e8e4b2c8ea763a7742f04eeb98a3730523d9 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 1 Oct 2019 12:45:32 -0300 Subject: [PATCH 4/6] Rearrange code for clarity --- models/issue.go | 6 ++++-- models/pull.go | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/models/issue.go b/models/issue.go index fbf8f95c3a58..d2d7610c3c59 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1193,8 +1193,10 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in // 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, assigneeIDs, uuids); !IsErrNewIssueInsert(err) { - // Usually err == nil + if err = newIssueAttempt(repo, issue, labelIDs, assigneeIDs, uuids); err == nil { + return nil + } + if !IsErrNewIssueInsert(err) { return err } if i++; i == issueMaxDupIndexAttempts { diff --git a/models/pull.go b/models/pull.go index 517dffd3c540..ff1f7b773b2b 100644 --- a/models/pull.go +++ b/models/pull.go @@ -660,8 +660,10 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str // 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, patch, assigneeIDs); !IsErrNewIssueInsert(err) { - // Usually err == nil + if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch, assigneeIDs); err == nil { + return nil + } + if !IsErrNewIssueInsert(err) { return err } if i++; i == issueMaxDupIndexAttempts { From 073a9813cb6e519bcb2348d3fd936a126b59d2af Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 1 Oct 2019 13:12:00 -0300 Subject: [PATCH 5/6] Fix error return in newIssue() --- models/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue.go b/models/issue.go index d2d7610c3c59..4389dc107f2e 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1107,7 +1107,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1"). Where("repo_id=?", opts.Issue.RepoID). Insert(opts.Issue); err != nil { - return err + return ErrNewIssueInsert{err} } inserted, err := getIssueByID(e, opts.Issue.ID) From ceec120e5bd551876967cd8904bd93ce1b909c2e Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Tue, 1 Oct 2019 13:30:05 -0300 Subject: [PATCH 6/6] Fix error message --- models/issue.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issue.go b/models/issue.go index 4389dc107f2e..09d443384b90 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1202,9 +1202,9 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in if i++; i == issueMaxDupIndexAttempts { break } - log.Error("NewPullRequest: error attempting to insert the new issue; will retry. Original error: %v", err) + log.Error("NewIssue: error attempting to insert the new issue; will retry. Original error: %v", err) } - return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %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, assigneeIDs []int64, uuids []string) (err error) {