Check ignored errors for issue and milestone count (#3213)

3 files changed, 57 insertions, 21 deletions

diff --git a/models/issue_milestone.go b/models/issue_milestone.go
index 6ec8291233..761b598e92 100644
--- a/models/issue_milestone.go
+++ b/models/issue_milestone.go
@@ -165,31 +165,33 @@ func UpdateMilestone(m *Milestone) error {
 	return updateMilestone(x, m)
 }
 
-func countRepoMilestones(e Engine, repoID int64) int64 {
-	count, _ := e.
+func countRepoMilestones(e Engine, repoID int64) (int64, error) {
+	return e.
 		Where("repo_id=?", repoID).
 		Count(new(Milestone))
-	return count
 }
 
-func countRepoClosedMilestones(e Engine, repoID int64) int64 {
-	closed, _ := e.
+func countRepoClosedMilestones(e Engine, repoID int64) (int64, error) {
+	return e.
 		Where("repo_id=? AND is_closed=?", repoID, true).
 		Count(new(Milestone))
-	return closed
 }
 
 // CountRepoClosedMilestones returns number of closed milestones in given repository.
-func CountRepoClosedMilestones(repoID int64) int64 {
+func CountRepoClosedMilestones(repoID int64) (int64, error) {
 	return countRepoClosedMilestones(x, repoID)
 }
 
 // MilestoneStats returns number of open and closed milestones of given repository.
-func MilestoneStats(repoID int64) (open int64, closed int64) {
-	open, _ = x.
+func MilestoneStats(repoID int64) (open int64, closed int64, err error) {
+	open, err = x.
 		Where("repo_id=? AND is_closed=?", repoID, false).
 		Count(new(Milestone))
-	return open, CountRepoClosedMilestones(repoID)
+	if err != nil {
+		return 0, 0, nil
+	}
+	closed, err = CountRepoClosedMilestones(repoID)
+	return open, closed, err
 }
 
 // ChangeMilestoneStatus changes the milestone open/closed status.
@@ -210,8 +212,17 @@ func ChangeMilestoneStatus(m *Milestone, isClosed bool) (err error) {
 		return err
 	}
 
-	repo.NumMilestones = int(countRepoMilestones(sess, repo.ID))
-	repo.NumClosedMilestones = int(countRepoClosedMilestones(sess, repo.ID))
+	numMilestones, err := countRepoMilestones(sess, repo.ID)
+	if err != nil {
+		return err
+	}
+	numClosedMilestones, err := countRepoClosedMilestones(sess, repo.ID)
+	if err != nil {
+		return err
+	}
+	repo.NumMilestones = int(numMilestones)
+	repo.NumClosedMilestones = int(numClosedMilestones)
+
 	if _, err = sess.ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil {
 		return err
 	}
@@ -324,8 +335,17 @@ func DeleteMilestoneByRepoID(repoID, id int64) error {
 		return err
 	}
 
-	repo.NumMilestones = int(countRepoMilestones(sess, repo.ID))
-	repo.NumClosedMilestones = int(countRepoClosedMilestones(sess, repo.ID))
+	numMilestones, err := countRepoMilestones(sess, repo.ID)
+	if err != nil {
+		return err
+	}
+	numClosedMilestones, err := countRepoClosedMilestones(sess, repo.ID)
+	if err != nil {
+		return err
+	}
+	repo.NumMilestones = int(numMilestones)
+	repo.NumClosedMilestones = int(numClosedMilestones)
+
 	if _, err = sess.ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil {
 		return err
 	}
diff --git a/models/issue_milestone_test.go b/models/issue_milestone_test.go
index e0bfe11df1..f7987d45ad 100644
--- a/models/issue_milestone_test.go
+++ b/models/issue_milestone_test.go
@@ -146,31 +146,42 @@ func TestCountRepoMilestones(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	test := func(repoID int64) {
 		repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository)
-		assert.EqualValues(t, repo.NumMilestones, countRepoMilestones(x, repoID))
+		count, err := countRepoMilestones(x, repoID)
+		assert.NoError(t, err)
+		assert.EqualValues(t, repo.NumMilestones, count)
 	}
 	test(1)
 	test(2)
 	test(3)
-	assert.EqualValues(t, 0, countRepoMilestones(x, NonexistentID))
+
+	count, err := countRepoMilestones(x, NonexistentID)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 0, count)
 }
 
 func TestCountRepoClosedMilestones(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	test := func(repoID int64) {
 		repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository)
-		assert.EqualValues(t, repo.NumClosedMilestones, CountRepoClosedMilestones(repoID))
+		count, err := CountRepoClosedMilestones(repoID)
+		assert.NoError(t, err)
+		assert.EqualValues(t, repo.NumClosedMilestones, count)
 	}
 	test(1)
 	test(2)
 	test(3)
-	assert.EqualValues(t, 0, countRepoMilestones(x, NonexistentID))
+
+	count, err := CountRepoClosedMilestones(NonexistentID)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 0, count)
 }
 
 func TestMilestoneStats(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	test := func(repoID int64) {
 		repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository)
-		open, closed := MilestoneStats(repoID)
+		open, closed, err := MilestoneStats(repoID)
+		assert.NoError(t, err)
 		assert.EqualValues(t, repo.NumMilestones-repo.NumClosedMilestones, open)
 		assert.EqualValues(t, repo.NumClosedMilestones, closed)
 	}
@@ -178,7 +189,8 @@ func TestMilestoneStats(t *testing.T) {
 	test(2)
 	test(3)
 
-	open, closed := MilestoneStats(NonexistentID)
+	open, closed, err := MilestoneStats(NonexistentID)
+	assert.NoError(t, err)
 	assert.EqualValues(t, 0, open)
 	assert.EqualValues(t, 0, closed)
 }
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index ec8fda8f7c..578ead1343 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1067,7 +1067,11 @@ func Milestones(ctx *context.Context) {
 	ctx.Data["PageIsMilestones"] = true
 
 	isShowClosed := ctx.Query("state") == "closed"
-	openCount, closedCount := models.MilestoneStats(ctx.Repo.Repository.ID)
+	openCount, closedCount, err := models.MilestoneStats(ctx.Repo.Repository.ID)
+	if err != nil {
+		ctx.Handle(500, "MilestoneStats", err)
+		return
+	}
 	ctx.Data["OpenCount"] = openCount
 	ctx.Data["ClosedCount"] = closedCount