Fix CheckRepoStats and reuse it during migration (#18264)

The CheckRepoStats function missed the following counters: - label num_closed_issues & num_closed_pulls - milestone num_closed_issues & num_closed_pulls The update SQL statements for updating the repository num_closed_issues & num_closed_pulls fields were repeated in three functions (repo.CheckRepoStats, migrate.insertIssues and models.Issue.updateClosedNum) and were moved to a single helper. The UpdateRepoStats is implemented and called in the Finish migration method so that it happens immediately instead of wating for the CheckRepoStats to run. Signed-off-by: Loïc Dachary loic@dachary.org --- [source](https://lab.forgefriends.org/forgefriends/forgefriends/-/merge_requests/34)
go-gitea · Jan 17, 2022 · 076cead · 076cead
1 parent 7dde39a
commit 076cead
Show file tree

Hide file tree

Showing 10 changed files with 219 additions and 168 deletions.
diff --git a/models/issue.go b/models/issue.go
@@ -595,8 +595,8 @@ func (issue *Issue) ReadBy(userID int64) error {
 	return setIssueNotificationStatusReadIfUnread(db.GetEngine(db.DefaultContext), userID, issue.ID)
 }
 
-func updateIssueCols(e db.Engine, issue *Issue, cols ...string) error {
-	if _, err := e.ID(issue.ID).Cols(cols...).Update(issue); err != nil {
+func updateIssueCols(ctx context.Context, issue *Issue, cols ...string) error {
+	if _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue); err != nil {
 		return err
 	}
 	return nil
@@ -646,7 +646,7 @@ func (issue *Issue) doChangeStatus(ctx context.Context, doer *user_model.User, i
 		issue.ClosedUnix = 0
 	}
 
-	if err := updateIssueCols(e, issue, "is_closed", "closed_unix"); err != nil {
+	if err := updateIssueCols(ctx, issue, "is_closed", "closed_unix"); err != nil {
 		return nil, err
 	}
 
@@ -662,12 +662,12 @@ func (issue *Issue) doChangeStatus(ctx context.Context, doer *user_model.User, i
 
 	// Update issue count of milestone
 	if issue.MilestoneID > 0 {
-		if err := updateMilestoneCounters(e, issue.MilestoneID); err != nil {
+		if err := updateMilestoneCounters(ctx, issue.MilestoneID); err != nil {
 			return nil, err
 		}
 	}
 
-	if err := issue.updateClosedNum(e); err != nil {
+	if err := issue.updateClosedNum(ctx); err != nil {
 		return nil, err
 	}
 
@@ -722,7 +722,7 @@ func (issue *Issue) ChangeTitle(doer *user_model.User, oldTitle string) (err err
 	}
 	defer committer.Close()
 
-	if err = updateIssueCols(db.GetEngine(ctx), issue, "name"); err != nil {
+	if err = updateIssueCols(ctx, issue, "name"); err != nil {
 		return fmt.Errorf("updateIssueCols: %v", err)
 	}
 
@@ -756,7 +756,7 @@ func (issue *Issue) ChangeRef(doer *user_model.User, oldRef string) (err error)
 	}
 	defer committer.Close()
 
-	if err = updateIssueCols(db.GetEngine(ctx), issue, "ref"); err != nil {
+	if err = updateIssueCols(ctx, issue, "ref"); err != nil {
 		return fmt.Errorf("updateIssueCols: %v", err)
 	}
 
@@ -847,7 +847,7 @@ func (issue *Issue) ChangeContent(doer *user_model.User, content string) (err er
 
 	issue.Content = content
 
-	if err = updateIssueCols(db.GetEngine(ctx), issue, "content"); err != nil {
+	if err = updateIssueCols(ctx, issue, "content"); err != nil {
 		return fmt.Errorf("UpdateIssueCols: %v", err)
 	}
 
@@ -956,7 +956,7 @@ func newIssue(ctx context.Context, doer *user_model.User, opts NewIssueOptions)
 	}
 
 	if opts.Issue.MilestoneID > 0 {
-		if err := updateMilestoneCounters(e, opts.Issue.MilestoneID); err != nil {
+		if err := updateMilestoneCounters(ctx, opts.Issue.MilestoneID); err != nil {
 			return err
 		}
 
@@ -1970,10 +1970,9 @@ func UpdateIssueDeadline(issue *Issue, deadlineUnix timeutil.TimeStamp, doer *us
 		return err
 	}
 	defer committer.Close()
-	sess := db.GetEngine(ctx)
 
 	// Update the deadline
-	if err = updateIssueCols(sess, &Issue{ID: issue.ID, DeadlineUnix: deadlineUnix}, "deadline_unix"); err != nil {
+	if err = updateIssueCols(ctx, &Issue{ID: issue.ID, DeadlineUnix: deadlineUnix}, "deadline_unix"); err != nil {
 		return err
 	}
 
@@ -2059,21 +2058,11 @@ func (issue *Issue) BlockingDependencies() ([]*DependencyInfo, error) {
 	return issue.getBlockingDependencies(db.GetEngine(db.DefaultContext))
 }
 
-func (issue *Issue) updateClosedNum(e db.Engine) (err error) {
+func (issue *Issue) updateClosedNum(ctx context.Context) (err error) {
 	if issue.IsPull {
-		_, err = e.Exec("UPDATE `repository` SET num_closed_pulls=(SELECT count(*) FROM issue WHERE repo_id=? AND is_pull=? AND is_closed=?) WHERE id=?",
-			issue.RepoID,
-			true,
-			true,
-			issue.RepoID,
-		)
+		err = repoStatsCorrectNumClosed(ctx, issue.RepoID, true, "num_closed_pulls")
 	} else {
-		_, err = e.Exec("UPDATE `repository` SET num_closed_issues=(SELECT count(*) FROM issue WHERE repo_id=? AND is_pull=? AND is_closed=?) WHERE id=?",
-			issue.RepoID,
-			false,
-			true,
-			issue.RepoID,
-		)
+		err = repoStatsCorrectNumClosed(ctx, issue.RepoID, false, "num_closed_issues")
 	}
 	return
 }

diff --git a/models/issue_comment.go b/models/issue_comment.go
@@ -856,12 +856,12 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 			}
 		}
 	case CommentTypeReopen, CommentTypeClose:
-		if err = opts.Issue.updateClosedNum(e); err != nil {
+		if err = opts.Issue.updateClosedNum(ctx); err != nil {
 			return err
 		}
 	}
 	// update the issue's updated_unix column
-	return updateIssueCols(e, opts.Issue, "updated_unix")
+	return updateIssueCols(ctx, opts.Issue, "updated_unix")
 }
 
 func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) {

diff --git a/models/issue_lock.go b/models/issue_lock.go
@@ -46,7 +46,7 @@ func updateIssueLock(opts *IssueLockOptions, lock bool) error {
 	}
 	defer committer.Close()
 
-	if err := updateIssueCols(db.GetEngine(ctx), opts.Issue, "is_locked"); err != nil {
+	if err := updateIssueCols(ctx, opts.Issue, "is_locked"); err != nil {
 		return err
 	}
 

diff --git a/models/issue_milestone.go b/models/issue_milestone.go
@@ -158,37 +158,36 @@ func UpdateMilestone(m *Milestone, oldIsClosed bool) error {
 	}
 	defer committer.Close()
 
-	sess := db.GetEngine(ctx)
-
 	if m.IsClosed && !oldIsClosed {
 		m.ClosedDateUnix = timeutil.TimeStampNow()
 	}
 
-	if err := updateMilestone(sess, m); err != nil {
+	if err := updateMilestone(ctx, m); err != nil {
 		return err
 	}
 
 	// if IsClosed changed, update milestone numbers of repository
 	if oldIsClosed != m.IsClosed {
-		if err := updateRepoMilestoneNum(sess, m.RepoID); err != nil {
+		if err := updateRepoMilestoneNum(ctx, m.RepoID); err != nil {
 			return err
 		}
 	}
 
 	return committer.Commit()
 }
 
-func updateMilestone(e db.Engine, m *Milestone) error {
+func updateMilestone(ctx context.Context, m *Milestone) error {
 	m.Name = strings.TrimSpace(m.Name)
-	_, err := e.ID(m.ID).AllCols().Update(m)
+	_, err := db.GetEngine(ctx).ID(m.ID).AllCols().Update(m)
 	if err != nil {
 		return err
 	}
-	return updateMilestoneCounters(e, m.ID)
+	return updateMilestoneCounters(ctx, m.ID)
 }
 
 // updateMilestoneCounters calculates NumIssues, NumClosesIssues and Completeness
-func updateMilestoneCounters(e db.Engine, id int64) error {
+func updateMilestoneCounters(ctx context.Context, id int64) error {
+	e := db.GetEngine(ctx)
 	_, err := e.ID(id).
 		SetExpr("num_issues", builder.Select("count(*)").From("issue").Where(
 			builder.Eq{"milestone_id": id},
@@ -217,21 +216,19 @@ func ChangeMilestoneStatusByRepoIDAndID(repoID, milestoneID int64, isClosed bool
 	}
 	defer committer.Close()
 
-	sess := db.GetEngine(ctx)
-
 	m := &Milestone{
 		ID:     milestoneID,
 		RepoID: repoID,
 	}
 
-	has, err := sess.ID(milestoneID).Where("repo_id = ?", repoID).Get(m)
+	has, err := db.GetEngine(ctx).ID(milestoneID).Where("repo_id = ?", repoID).Get(m)
 	if err != nil {
 		return err
 	} else if !has {
 		return ErrMilestoneNotExist{ID: milestoneID, RepoID: repoID}
 	}
 
-	if err := changeMilestoneStatus(sess, m, isClosed); err != nil {
+	if err := changeMilestoneStatus(ctx, m, isClosed); err != nil {
 		return err
 	}
 
@@ -246,43 +243,42 @@ func ChangeMilestoneStatus(m *Milestone, isClosed bool) (err error) {
 	}
 	defer committer.Close()
 
-	if err := changeMilestoneStatus(db.GetEngine(ctx), m, isClosed); err != nil {
+	if err := changeMilestoneStatus(ctx, m, isClosed); err != nil {
 		return err
 	}
 
 	return committer.Commit()
 }
 
-func changeMilestoneStatus(e db.Engine, m *Milestone, isClosed bool) error {
+func changeMilestoneStatus(ctx context.Context, m *Milestone, isClosed bool) error {
 	m.IsClosed = isClosed
 	if isClosed {
 		m.ClosedDateUnix = timeutil.TimeStampNow()
 	}
 
-	count, err := e.ID(m.ID).Where("repo_id = ? AND is_closed = ?", m.RepoID, !isClosed).Cols("is_closed", "closed_date_unix").Update(m)
+	count, err := db.GetEngine(ctx).ID(m.ID).Where("repo_id = ? AND is_closed = ?", m.RepoID, !isClosed).Cols("is_closed", "closed_date_unix").Update(m)
 	if err != nil {
 		return err
 	}
 	if count < 1 {
 		return nil
 	}
-	return updateRepoMilestoneNum(e, m.RepoID)
+	return updateRepoMilestoneNum(ctx, m.RepoID)
 }
 
 func changeMilestoneAssign(ctx context.Context, doer *user_model.User, issue *Issue, oldMilestoneID int64) error {
-	e := db.GetEngine(ctx)
-	if err := updateIssueCols(e, issue, "milestone_id"); err != nil {
+	if err := updateIssueCols(ctx, issue, "milestone_id"); err != nil {
 		return err
 	}
 
 	if oldMilestoneID > 0 {
-		if err := updateMilestoneCounters(e, oldMilestoneID); err != nil {
+		if err := updateMilestoneCounters(ctx, oldMilestoneID); err != nil {
 			return err
 		}
 	}
 
 	if issue.MilestoneID > 0 {
-		if err := updateMilestoneCounters(e, issue.MilestoneID); err != nil {
+		if err := updateMilestoneCounters(ctx, issue.MilestoneID); err != nil {
 			return err
 		}
 	}
@@ -634,8 +630,8 @@ func CountMilestonesByRepoCondAndKw(repoCond builder.Cond, keyword string, isClo
 	return countMap, nil
 }
 
-func updateRepoMilestoneNum(e db.Engine, repoID int64) error {
-	_, err := e.Exec("UPDATE `repository` SET num_milestones=(SELECT count(*) FROM milestone WHERE repo_id=?),num_closed_milestones=(SELECT count(*) FROM milestone WHERE repo_id=? AND is_closed=?) WHERE id=?",
+func updateRepoMilestoneNum(ctx context.Context, repoID int64) error {
+	_, err := db.GetEngine(ctx).Exec("UPDATE `repository` SET num_milestones=(SELECT count(*) FROM milestone WHERE repo_id=?),num_closed_milestones=(SELECT count(*) FROM milestone WHERE repo_id=? AND is_closed=?) WHERE id=?",
 		repoID,
 		repoID,
 		true,

diff --git a/models/issue_milestone_test.go b/models/issue_milestone_test.go
@@ -228,14 +228,14 @@ func TestUpdateMilestoneCounters(t *testing.T) {
 	issue.ClosedUnix = timeutil.TimeStampNow()
 	_, err := db.GetEngine(db.DefaultContext).ID(issue.ID).Cols("is_closed", "closed_unix").Update(issue)
 	assert.NoError(t, err)
-	assert.NoError(t, updateMilestoneCounters(db.GetEngine(db.DefaultContext), issue.MilestoneID))
+	assert.NoError(t, updateMilestoneCounters(db.DefaultContext, issue.MilestoneID))
 	unittest.CheckConsistencyFor(t, &Milestone{})
 
 	issue.IsClosed = false
 	issue.ClosedUnix = 0
 	_, err = db.GetEngine(db.DefaultContext).ID(issue.ID).Cols("is_closed", "closed_unix").Update(issue)
 	assert.NoError(t, err)
-	assert.NoError(t, updateMilestoneCounters(db.GetEngine(db.DefaultContext), issue.MilestoneID))
+	assert.NoError(t, updateMilestoneCounters(db.DefaultContext, issue.MilestoneID))
 	unittest.CheckConsistencyFor(t, &Milestone{})
 }
 

diff --git a/models/issue_test.go b/models/issue_test.go
@@ -129,7 +129,7 @@ func TestUpdateIssueCols(t *testing.T) {
 	issue.Content = "This should have no effect"
 
 	now := time.Now().Unix()
-	assert.NoError(t, updateIssueCols(db.GetEngine(db.DefaultContext), issue, "name"))
+	assert.NoError(t, updateIssueCols(db.DefaultContext, issue, "name"))
 	then := time.Now().Unix()
 
 	updatedIssue := unittest.AssertExistsAndLoadBean(t, &Issue{ID: issue.ID}).(*Issue)