From 4042d1f0c3c545773f81e2ca1b4eb8662bc4c425 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Mon, 15 Aug 2016 18:40:32 -0700 Subject: [PATCH] models/issue: improve quality and performance of NewIssue function --- .travis.yml | 1 + README.md | 2 +- cmd/web.go | 3 +- gogs.go | 2 +- models/issue.go | 183 +++++++++++++++++++---------------- models/models.go | 1 + models/pull.go | 8 +- models/repo.go | 38 +++++--- modules/auth/repo_form.go | 15 --- routers/api/v1/repo/issue.go | 4 +- routers/repo/branch.go | 50 ---------- routers/repo/issue.go | 10 +- templates/.VERSION | 2 +- 13 files changed, 146 insertions(+), 173 deletions(-) diff --git a/.travis.yml b/.travis.yml index 84d0c1df7..82387935a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,6 +4,7 @@ go: - 1.4 - 1.5 - 1.6 + - 1.7 before_install: - sudo apt-get update -qq diff --git a/README.md b/README.md index 50a73eb0a..669748225 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ Gogs - Go Git Service [![Build Status](https://travis-ci.org/gogits/gogs.svg?bra ![](https://github.com/gogits/gogs/blob/master/public/img/gogs-large-resize.png?raw=true) -##### Current tip version: 0.9.79 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions) +##### Current tip version: 0.9.80 (see [Releases](https://github.com/gogits/gogs/releases) for binary versions) | Web | UI | Preview | |:-------------:|:-------:|:-------:| diff --git a/cmd/web.go b/cmd/web.go index cdf42657c..b581f9583 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -504,10 +504,9 @@ func runWeb(ctx *cli.Context) error { m.Combo("/_new/*").Get(repo.NewFile). Post(bindIgnErr(auth.EditRepoFileForm{}), repo.NewFilePost) m.Post("/_preview/*", bindIgnErr(auth.EditPreviewDiffForm{}), repo.DiffPreviewPost) - m.Combo("/upload/*").Get(repo.UploadFile). + m.Combo("/_upload/*").Get(repo.UploadFile). Post(bindIgnErr(auth.UploadRepoFileForm{}), repo.UploadFilePost) m.Post("/_delete/*", bindIgnErr(auth.DeleteRepoFileForm{}), repo.DeleteFilePost) - m.Post("/branches", bindIgnErr(auth.NewBranchForm{}), repo.NewBranchPost) // m.Post("/upload-file", repo.UploadFileToServer) // m.Post("/upload-remove", bindIgnErr(auth.RemoveUploadFileForm{}), repo.RemoveUploadFileFromServer) }, reqRepoWriter, context.RepoRef(), func(ctx *context.Context) { diff --git a/gogs.go b/gogs.go index 52cf7a659..8882daa7e 100644 --- a/gogs.go +++ b/gogs.go @@ -17,7 +17,7 @@ import ( "github.com/gogits/gogs/modules/setting" ) -const APP_VER = "0.9.79.0815" +const APP_VER = "0.9.80.0815" func init() { runtime.GOMAXPROCS(runtime.NumCPU()) diff --git a/models/issue.go b/models/issue.go index 541b70068..bd236c7a8 100644 --- a/models/issue.go +++ b/models/issue.go @@ -25,7 +25,6 @@ import ( ) var ( - ErrWrongIssueCounter = errors.New("Invalid number of issues for this milestone") ErrMissingIssueNumber = errors.New("No issue number specified") ) @@ -580,80 +579,86 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) { return nil } -// It's caller's responsibility to create action. -func newIssue(e *xorm.Session, repo *Repository, issue *Issue, labelIDs []int64, uuids []string, isPull bool) (err error) { - issue.Title = strings.TrimSpace(issue.Title) - issue.Index = repo.NextIssueIndex() +type NewIssueOptions struct { + Repo *Repository + Issue *Issue + LableIDs []int64 + Attachments []string // In UUID format. + IsPull bool +} - if issue.AssigneeID > 0 { - // Silently drop invalid assignee - valid, err := hasAccess(e, &User{ID: issue.AssigneeID}, repo, ACCESS_MODE_WRITE) +func newIssue(e *xorm.Session, opts *NewIssueOptions) (err error) { + opts.Issue.Title = strings.TrimSpace(opts.Issue.Title) + opts.Issue.Index = opts.Repo.NextIssueIndex() + + if opts.Issue.AssigneeID > 0 { + // Silently drop invalid assignee. + valid, err := hasAccess(e, &User{ID: opts.Issue.AssigneeID}, opts.Repo, ACCESS_MODE_WRITE) if err != nil { - return fmt.Errorf("hasAccess: %v", err) + return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", opts.Issue.AssigneeID, opts.Repo.ID, err) } else if !valid { - issue.AssigneeID = 0 + opts.Issue.AssigneeID = 0 } } - if _, err = e.Insert(issue); err != nil { + if _, err = e.Insert(opts.Issue); err != nil { return err } - if isPull { - _, err = e.Exec("UPDATE `repository` SET num_pulls=num_pulls+1 WHERE id=?", issue.RepoID) + if opts.IsPull { + _, err = e.Exec("UPDATE `repository` SET num_pulls = num_pulls + 1 WHERE id = ?", opts.Issue.RepoID) } else { - _, err = e.Exec("UPDATE `repository` SET num_issues=num_issues+1 WHERE id=?", issue.RepoID) + _, err = e.Exec("UPDATE `repository` SET num_issues = num_issues + 1 WHERE id = ?", opts.Issue.RepoID) } if err != nil { return err } - if len(labelIDs) > 0 { + if len(opts.LableIDs) > 0 { // During the session, SQLite3 dirver cannot handle retrieve objects after update something. // So we have to get all needed labels first. - labels := make([]*Label, 0, len(labelIDs)) - if err = e.In("id", labelIDs).Find(&labels); err != nil { - return fmt.Errorf("find all labels: %v", err) + labels := make([]*Label, 0, len(opts.LableIDs)) + if err = e.In("id", opts.LableIDs).Find(&labels); err != nil { + return fmt.Errorf("find all labels [label_ids: %v]: %v", opts.LableIDs, err) } for _, label := range labels { - if label.RepoID != repo.ID { + // Silently drop invalid labels. + if label.RepoID != opts.Repo.ID { continue } - if err = issue.addLabel(e, label); err != nil { - return fmt.Errorf("addLabel: %v", err) + if err = opts.Issue.addLabel(e, label); err != nil { + return fmt.Errorf("addLabel [id: %d]: %v", label.ID, err) } } } - if issue.MilestoneID > 0 { - if err = changeMilestoneAssign(e, 0, issue); err != nil { + if opts.Issue.MilestoneID > 0 { + if err = changeMilestoneAssign(e, opts.Issue, -1); err != nil { return err } } - if err = newIssueUsers(e, repo, issue); err != nil { + if err = newIssueUsers(e, opts.Repo, opts.Issue); err != nil { return err } - // Check attachments. - for _, uuid := range uuids { - attachment, err := getAttachmentByUUID(e, uuid) + if len(opts.Attachments) > 0 { + attachments, err := getAttachmentsByUUIDs(e, opts.Attachments) if err != nil { - if IsErrAttachmentNotExist(err) { - continue - } - return fmt.Errorf("getAttachmentByUUID [%s]: %v", uuid, err) + return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %v", opts.Attachments, err) } - attachment.IssueID = issue.ID - // No assign value could be 0, so ignore AllCols(). - if _, err = e.Id(attachment.ID).Update(attachment); err != nil { - return fmt.Errorf("update attachment [%d]: %v", attachment.ID, err) + + for i := 0; i < len(attachments); i++ { + attachments[i].IssueID = opts.Issue.ID + if _, err = e.Id(attachments[i].ID).Update(attachments[i]); err != nil { + return fmt.Errorf("update attachment [id: %d]: %v", attachments[i].ID, err) + } } } - return issue.loadAttributes(e) + return opts.Issue.loadAttributes(e) } // NewIssue creates new issue with labels for repository. @@ -664,7 +669,12 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) return err } - if err = newIssue(sess, repo, issue, labelIDs, uuids, false); err != nil { + if err = newIssue(sess, &NewIssueOptions{ + Repo: repo, + Issue: issue, + LableIDs: labelIDs, + Attachments: uuids, + }); err != nil { return fmt.Errorf("newIssue: %v", err) } @@ -672,8 +682,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) return fmt.Errorf("Commit: %v", err) } - // Notify watchers. - act := &Action{ + if err = NotifyWatchers(&Action{ ActUserID: issue.Poster.ID, ActUserName: issue.Poster.Name, ActEmail: issue.Poster.Email, @@ -683,10 +692,10 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) RepoUserName: repo.Owner.Name, RepoName: repo.Name, IsPrivate: repo.IsPrivate, - } - if err = NotifyWatchers(act); err != nil { + }); err != nil { log.Error(4, "NotifyWatchers: %v", err) - } else if err = issue.MailParticipants(); err != nil { + } + if err = issue.MailParticipants(); err != nil { log.Error(4, "MailParticipants: %v", err) } @@ -855,38 +864,42 @@ type IssueUser struct { } func newIssueUsers(e *xorm.Session, repo *Repository, issue *Issue) error { - users, err := repo.GetAssignees() + assignees, err := repo.getAssignees(e) if err != nil { + return fmt.Errorf("getAssignees: %v", err) + } + + // Poster can be anyone, append later if not one of assignees. + isPosterAssignee := false + + // Leave a seat for poster itself to append later, but if poster is one of assignee + // and just waste 1 unit is cheaper than re-allocate memory once. + issueUsers := make([]*IssueUser, 0, len(assignees)+1) + for _, assignee := range assignees { + isPoster := assignee.ID == issue.PosterID + issueUsers = append(issueUsers, &IssueUser{ + IssueID: issue.ID, + RepoID: repo.ID, + UID: assignee.ID, + IsPoster: isPoster, + IsAssigned: assignee.ID == issue.AssigneeID, + }) + if !isPosterAssignee && isPoster { + isPosterAssignee = true + } + } + if !isPosterAssignee { + issueUsers = append(issueUsers, &IssueUser{ + IssueID: issue.ID, + RepoID: repo.ID, + UID: issue.PosterID, + IsPoster: true, + }) + } + + if _, err = e.Insert(issueUsers); err != nil { return err } - - iu := &IssueUser{ - IssueID: issue.ID, - RepoID: repo.ID, - } - - // Poster can be anyone. - isNeedAddPoster := true - for _, u := range users { - iu.ID = 0 - iu.UID = u.ID - iu.IsPoster = iu.UID == issue.PosterID - if isNeedAddPoster && iu.IsPoster { - isNeedAddPoster = false - } - iu.IsAssigned = iu.UID == issue.AssigneeID - if _, err = e.Insert(iu); err != nil { - return err - } - } - if isNeedAddPoster { - iu.ID = 0 - iu.UID = issue.PosterID - iu.IsPoster = true - if _, err = e.Insert(iu); err != nil { - return err - } - } return nil } @@ -1499,9 +1512,9 @@ func ChangeMilestoneIssueStats(issue *Issue) (err error) { return sess.Commit() } -func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error { - if oldMid > 0 { - m, err := getMilestoneByID(e, oldMid) +func changeMilestoneAssign(e *xorm.Session, issue *Issue, oldMilestoneID int64) error { + if oldMilestoneID > 0 { + m, err := getMilestoneByID(e, oldMilestoneID) if err != nil { return err } @@ -1513,7 +1526,7 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error { if err = updateMilestone(e, m); err != nil { return err - } else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id=0 WHERE issue_id=?", issue.ID); err != nil { + } else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id = 0 WHERE issue_id = ?", issue.ID); err != nil { return err } } @@ -1529,13 +1542,9 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error { m.NumClosedIssues++ } - if m.NumIssues == 0 { - return ErrWrongIssueCounter - } - if err = updateMilestone(e, m); err != nil { return err - } else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id=? WHERE issue_id=?", m.ID, issue.ID); err != nil { + } else if _, err = e.Exec("UPDATE `issue_user` SET milestone_id = ? WHERE issue_id = ?", m.ID, issue.ID); err != nil { return err } } @@ -1544,14 +1553,14 @@ func changeMilestoneAssign(e *xorm.Session, oldMid int64, issue *Issue) error { } // ChangeMilestoneAssign changes assignment of milestone for issue. -func ChangeMilestoneAssign(oldMid int64, issue *Issue) (err error) { +func ChangeMilestoneAssign(issue *Issue, oldMilestoneID int64) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { return err } - if err = changeMilestoneAssign(sess, oldMid, issue); err != nil { + if err = changeMilestoneAssign(sess, issue, oldMilestoneID); err != nil { return err } return sess.Commit() @@ -1677,6 +1686,16 @@ func getAttachmentByUUID(e Engine, uuid string) (*Attachment, error) { return attach, nil } +func getAttachmentsByUUIDs(e Engine, uuids []string) ([]*Attachment, error) { + if len(uuids) == 0 { + return []*Attachment{}, nil + } + + // Silently drop invalid uuids. + attachments := make([]*Attachment, 0, len(uuids)) + return attachments, e.In("uuid", uuids).Find(&attachments) +} + // GetAttachmentByUUID returns attachment by given UUID. func GetAttachmentByUUID(uuid string) (*Attachment, error) { return getAttachmentByUUID(x, uuid) diff --git a/models/models.go b/models/models.go index 98e07c50f..582c4403a 100644 --- a/models/models.go +++ b/models/models.go @@ -29,6 +29,7 @@ type Engine interface { Find(interface{}, ...interface{}) error Get(interface{}) (bool, error) Id(interface{}) *xorm.Session + In(string, ...interface{}) *xorm.Session Insert(...interface{}) (int64, error) InsertOne(interface{}) (int64, error) Iterate(interface{}, xorm.IterFunc) error diff --git a/models/pull.go b/models/pull.go index 39361595f..09c20df82 100644 --- a/models/pull.go +++ b/models/pull.go @@ -388,7 +388,13 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return err } - if err = newIssue(sess, repo, pull, labelIDs, uuids, true); err != nil { + if err = newIssue(sess, &NewIssueOptions{ + Repo: repo, + Issue: pull, + LableIDs: labelIDs, + Attachments: uuids, + IsPull: true, + }); err != nil { return fmt.Errorf("newIssue: %v", err) } diff --git a/models/repo.go b/models/repo.go index a858338b1..632f0be71 100644 --- a/models/repo.go +++ b/models/repo.go @@ -316,33 +316,43 @@ func (repo *Repository) DeleteWiki() { } } -// GetAssignees returns all users that have write access of repository. -func (repo *Repository) GetAssignees() (_ []*User, err error) { - if err = repo.GetOwner(); err != nil { +func (repo *Repository) getAssignees(e Engine) (_ []*User, err error) { + if err = repo.getOwner(e); err != nil { return nil, err } accesses := make([]*Access, 0, 10) - if err = x.Where("repo_id=? AND mode>=?", repo.ID, ACCESS_MODE_WRITE).Find(&accesses); err != nil { + if err = e.Where("repo_id = ? AND mode >= ?", repo.ID, ACCESS_MODE_WRITE).Find(&accesses); err != nil { return nil, err } + if len(accesses) == 0 { + return []*User{}, nil + } - users := make([]*User, 0, len(accesses)+1) // Just waste 1 unit does not matter. + userIDs := make([]int64, len(accesses)) + for i := 0; i < len(accesses); i++ { + userIDs[i] = accesses[i].UserID + } + + // Leave a seat for owner itself to append later, but if owner is an organization + // and just waste 1 unit is cheaper than re-allocate memory once. + users := make([]*User, 0, len(userIDs)+1) + if err = e.In("id", userIDs).Find(&users); err != nil { + return nil, err + } if !repo.Owner.IsOrganization() { users = append(users, repo.Owner) } - var u *User - for i := range accesses { - u, err = GetUserByID(accesses[i].UserID) - if err != nil { - return nil, err - } - users = append(users, u) - } return users, nil } +// GetAssignees returns all users that have write access and can be assigned to issues +// of the repository, +func (repo *Repository) GetAssignees() (_ []*User, err error) { + return repo.getAssignees(x) +} + // GetAssigneeByID returns the user that has write access of repository by given ID. func (repo *Repository) GetAssigneeByID(userID int64) (*User, error) { return GetAssigneeByID(repo, userID) @@ -420,6 +430,8 @@ func (repo *Repository) AllowsPulls() bool { return repo.CanEnablePulls() && repo.EnablePulls } +// FIXME: should have a mutex to prevent producing same index for two issues that are created +// closely enough. func (repo *Repository) NextIssueIndex() int64 { return int64(repo.NumIssues+repo.NumPulls) + 1 } diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 985fe2458..2eaefc544 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -342,18 +342,3 @@ type DeleteRepoFileForm struct { func (f *DeleteRepoFileForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors { return validate(errs, ctx.Data, f, ctx.Locale) } - -// __________ .__ -// \______ \____________ ____ ____ | |__ -// | | _/\_ __ \__ \ / \_/ ___\| | \ -// | | \ | | \// __ \| | \ \___| Y \ -// |______ / |__| (____ /___| /\___ >___| / -// \/ \/ \/ \/ \/ -type NewBranchForm struct { - OldBranchName string `binding:"Required;MaxSize(100)"` - BranchName string `binding:"Required;MaxSize(100)"` -} - -func (f *NewBranchForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors { - return validate(errs, ctx.Data, f, ctx.Locale) -} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 1440a17cd..0d5ba756b 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -149,9 +149,9 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { } if ctx.Repo.IsWriter() && form.Milestone != nil && issue.MilestoneID != *form.Milestone { - oldMid := issue.MilestoneID + oldMilestoneID := issue.MilestoneID issue.MilestoneID = *form.Milestone - if err = models.ChangeMilestoneAssign(oldMid, issue); err != nil { + if err = models.ChangeMilestoneAssign(issue, oldMilestoneID); err != nil { ctx.Error(500, "ChangeMilestoneAssign", err) return } diff --git a/routers/repo/branch.go b/routers/repo/branch.go index c8407114a..00f30b0f1 100644 --- a/routers/repo/branch.go +++ b/routers/repo/branch.go @@ -5,13 +5,8 @@ package repo import ( - "github.com/gogits/gogs/models" - "github.com/gogits/gogs/modules/auth" "github.com/gogits/gogs/modules/base" "github.com/gogits/gogs/modules/context" - "github.com/gogits/gogs/modules/log" - "net/url" - "strings" ) const ( @@ -34,48 +29,3 @@ func Branches(ctx *context.Context) { ctx.Data["Branches"] = brs ctx.HTML(200, BRANCH) } - -func NewBranchPost(ctx *context.Context, form auth.NewBranchForm) { - oldBranchName := form.OldBranchName - branchName := form.BranchName - - if ctx.HasError() || !ctx.Repo.IsWriter() || branchName == oldBranchName { - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + oldBranchName) - return - } - - branchName = url.QueryEscape(strings.Replace(strings.Trim(branchName, " "), " ", "-", -1)) - - if _, err := ctx.Repo.Repository.GetBranch(branchName); err == nil { - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + branchName) - return - } - - if err := ctx.Repo.Repository.CreateNewBranch(ctx.User, oldBranchName, branchName); err != nil { - ctx.Handle(404, "repo.Branches(CreateNewBranch)", err) - log.Error(4, "%s: %v", "EditFile", err) - return - } - - // Was successful, so now need to call models.CommitRepoAction() with the new commitID for webhooks and watchers - if branch, err := ctx.Repo.Repository.GetBranch(branchName); err != nil { - log.Error(4, "repo.Repository.GetBranch(%s): %v", branchName, err) - } else if commit, err := branch.GetCommit(); err != nil { - log.Error(4, "branch.GetCommit(): %v", err) - } else { - pc := &models.PushCommits{ - Len: 1, - Commits: []*models.PushCommit{models.CommitToPushCommit(commit)}, - } - oldCommitID := "0000000000000000000000000000000000000000" // New Branch so we use all 0s - newCommitID := commit.ID.String() - if err := models.CommitRepoAction(ctx.User.ID, ctx.Repo.Owner.ID, ctx.User.LowerName, ctx.Repo.Owner.Email, - ctx.Repo.Repository.ID, ctx.Repo.Owner.LowerName, ctx.Repo.Repository.Name, "refs/heads/"+branchName, pc, - oldCommitID, newCommitID); err != nil { - log.Error(4, "models.CommitRepoAction(branch = %s): %v", branchName, err) - } - models.HookQueue.Add(ctx.Repo.Repository.ID) - } - - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + branchName) -} diff --git a/routers/repo/issue.go b/routers/repo/issue.go index b79ded532..3b36556c0 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -754,9 +754,9 @@ func UpdateIssueMilestone(ctx *context.Context) { return } - oldMid := issue.MilestoneID - mid := ctx.QueryInt64("id") - if oldMid == mid { + oldMilestoneID := issue.MilestoneID + milestoneID := ctx.QueryInt64("id") + if oldMilestoneID == milestoneID { ctx.JSON(200, map[string]interface{}{ "ok": true, }) @@ -764,8 +764,8 @@ func UpdateIssueMilestone(ctx *context.Context) { } // Not check for invalid milestone id and give responsibility to owners. - issue.MilestoneID = mid - if err := models.ChangeMilestoneAssign(oldMid, issue); err != nil { + issue.MilestoneID = milestoneID + if err := models.ChangeMilestoneAssign(issue, oldMilestoneID); err != nil { ctx.Handle(500, "ChangeMilestoneAssign", err) return } diff --git a/templates/.VERSION b/templates/.VERSION index 4d4c0e73b..1bc4bb85d 100644 --- a/templates/.VERSION +++ b/templates/.VERSION @@ -1 +1 @@ -0.9.79.0815 \ No newline at end of file +0.9.80.0815 \ No newline at end of file