Fix issue comment number (#30556)

5 files changed, 89 insertions, 19 deletions

diff --git a/models/issues/comment.go b/models/issues/comment.go
index 880a6ce8c5..a7ec8f57fc 100644
--- a/models/issues/comment.go
+++ b/models/issues/comment.go
@@ -197,6 +197,20 @@ func (t CommentType) HasMailReplySupport() bool {
 	return false
 }
 
+func (t CommentType) CountedAsConversation() bool {
+	for _, ct := range ConversationCountedCommentType() {
+		if t == ct {
+			return true
+		}
+	}
+	return false
+}
+
+// ConversationCountedCommentType returns the comment types that are counted as a conversation
+func ConversationCountedCommentType() []CommentType {
+	return []CommentType{CommentTypeComment, CommentTypeReview}
+}
+
 // RoleInRepo presents the user's participation in the repo
 type RoleInRepo string
 
@@ -893,7 +907,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 		}
 		fallthrough
 	case CommentTypeComment:
-		if _, err = db.Exec(ctx, "UPDATE `issue` SET num_comments=num_comments+1 WHERE id=?", opts.Issue.ID); err != nil {
+		if err := UpdateIssueNumComments(ctx, opts.Issue.ID); err != nil {
 			return err
 		}
 		fallthrough
@@ -1165,8 +1179,8 @@ func DeleteComment(ctx context.Context, comment *Comment) error {
 		return err
 	}
 
-	if comment.Type == CommentTypeComment {
-		if _, err := e.ID(comment.IssueID).Decr("num_comments").Update(new(Issue)); err != nil {
+	if comment.Type.CountedAsConversation() {
+		if err := UpdateIssueNumComments(ctx, comment.IssueID); err != nil {
 			return err
 		}
 	}
@@ -1283,6 +1297,21 @@ func (c *Comment) HasOriginalAuthor() bool {
 	return c.OriginalAuthor != "" && c.OriginalAuthorID != 0
 }
 
+func UpdateIssueNumCommentsBuilder(issueID int64) *builder.Builder {
+	subQuery := builder.Select("COUNT(*)").From("`comment`").Where(
+		builder.Eq{"issue_id": issueID}.And(
+			builder.In("`type`", ConversationCountedCommentType()),
+		))
+
+	return builder.Update(builder.Eq{"num_comments": subQuery}).
+		From("`issue`").Where(builder.Eq{"id": issueID})
+}
+
+func UpdateIssueNumComments(ctx context.Context, issueID int64) error {
+	_, err := db.GetEngine(ctx).Exec(UpdateIssueNumCommentsBuilder(issueID))
+	return err
+}
+
 // InsertIssueComments inserts many comments of issues.
 func InsertIssueComments(ctx context.Context, comments []*Comment) error {
 	if len(comments) == 0 {
@@ -1315,8 +1344,7 @@ func InsertIssueComments(ctx context.Context, comments []*Comment) error {
 	}
 
 	for _, issueID := range issueIDs {
-		if _, err := db.Exec(ctx, "UPDATE issue set num_comments = (SELECT count(*) FROM comment WHERE issue_id = ? AND `type`=?) WHERE id = ?",
-			issueID, CommentTypeComment, issueID); err != nil {
+		if err := UpdateIssueNumComments(ctx, issueID); err != nil {
 			return err
 		}
 	}
diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go
index 32b86891e8..ae0bc3ce17 100644
--- a/models/issues/comment_test.go
+++ b/models/issues/comment_test.go
@@ -115,3 +115,12 @@ func TestMigrate_InsertIssueComments(t *testing.T) {
 
 	unittest.CheckConsistencyFor(t, &issues_model.Issue{})
 }
+
+func Test_UpdateIssueNumComments(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+
+	assert.NoError(t, issues_model.UpdateIssueNumComments(db.DefaultContext, issue2.ID))
+	issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	assert.EqualValues(t, 1, issue2.NumComments)
+}
diff --git a/models/issues/review.go b/models/issues/review.go
index 8b345e5fd8..3e787273be 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -639,6 +639,10 @@ func InsertReviews(ctx context.Context, reviews []*Review) error {
 				return err
 			}
 		}
+
+		if err := UpdateIssueNumComments(ctx, review.IssueID); err != nil {
+			return err
+		}
 	}
 
 	return committer.Commit()
diff --git a/models/repo.go b/models/repo.go
index 3e9c52fdd9..9bc67079a9 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -16,6 +16,8 @@ import (
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/log"
+
+	"xorm.io/builder"
 )
 
 // Init initialize model
@@ -24,7 +26,7 @@ func Init(ctx context.Context) error {
 }
 
 type repoChecker struct {
-	querySQL   func(ctx context.Context) ([]map[string][]byte, error)
+	querySQL   func(ctx context.Context) ([]int64, error)
 	correctSQL func(ctx context.Context, id int64) error
 	desc       string
 }
@@ -35,8 +37,7 @@ func repoStatsCheck(ctx context.Context, checker *repoChecker) {
 		log.Error("Select %s: %v", checker.desc, err)
 		return
 	}
-	for _, result := range results {
-		id, _ := strconv.ParseInt(string(result["id"]), 10, 64)
+	for _, id := range results {
 		select {
 		case <-ctx.Done():
 			log.Warn("CheckRepoStats: Cancelled before checking %s for with id=%d", checker.desc, id)
@@ -51,21 +52,23 @@ func repoStatsCheck(ctx context.Context, checker *repoChecker) {
 	}
 }
 
-func StatsCorrectSQL(ctx context.Context, sql string, id int64) error {
-	_, err := db.GetEngine(ctx).Exec(sql, id, id)
+func StatsCorrectSQL(ctx context.Context, sql any, ids ...any) error {
+	args := []any{sql}
+	args = append(args, ids...)
+	_, err := db.GetEngine(ctx).Exec(args...)
 	return err
 }
 
 func repoStatsCorrectNumWatches(ctx context.Context, id int64) error {
-	return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", id)
+	return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", id, id)
 }
 
 func repoStatsCorrectNumStars(ctx context.Context, id int64) error {
-	return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE repo_id=?) WHERE id=?", id)
+	return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE repo_id=?) WHERE id=?", id, id)
 }
 
 func labelStatsCorrectNumIssues(ctx context.Context, id int64) error {
-	return StatsCorrectSQL(ctx, "UPDATE `label` SET num_issues=(SELECT COUNT(*) FROM `issue_label` WHERE label_id=?) WHERE id=?", id)
+	return StatsCorrectSQL(ctx, "UPDATE `label` SET num_issues=(SELECT COUNT(*) FROM `issue_label` WHERE label_id=?) WHERE id=?", id, id)
 }
 
 func labelStatsCorrectNumIssuesRepo(ctx context.Context, id int64) error {
@@ -102,11 +105,11 @@ func milestoneStatsCorrectNumIssuesRepo(ctx context.Context, id int64) error {
 }
 
 func userStatsCorrectNumRepos(ctx context.Context, id int64) error {
-	return StatsCorrectSQL(ctx, "UPDATE `user` SET num_repos=(SELECT COUNT(*) FROM `repository` WHERE owner_id=?) WHERE id=?", id)
+	return StatsCorrectSQL(ctx, "UPDATE `user` SET num_repos=(SELECT COUNT(*) FROM `repository` WHERE owner_id=?) WHERE id=?", id, id)
 }
 
 func repoStatsCorrectIssueNumComments(ctx context.Context, id int64) error {
-	return StatsCorrectSQL(ctx, "UPDATE `issue` SET num_comments=(SELECT COUNT(*) FROM `comment` WHERE issue_id=? AND type=0) WHERE id=?", id)
+	return StatsCorrectSQL(ctx, issues_model.UpdateIssueNumCommentsBuilder(id))
 }
 
 func repoStatsCorrectNumIssues(ctx context.Context, id int64) error {
@@ -125,9 +128,12 @@ func repoStatsCorrectNumClosedPulls(ctx context.Context, id int64) error {
 	return repo_model.UpdateRepoIssueNumbers(ctx, id, true, true)
 }
 
-func statsQuery(args ...any) func(context.Context) ([]map[string][]byte, error) {
-	return func(ctx context.Context) ([]map[string][]byte, error) {
-		return db.GetEngine(ctx).Query(args...)
+// statsQuery returns a function that queries the database for a list of IDs
+// sql could be a string or a *builder.Builder
+func statsQuery(sql any, args ...any) func(context.Context) ([]int64, error) {
+	return func(ctx context.Context) ([]int64, error) {
+		var ids []int64
+		return ids, db.GetEngine(ctx).SQL(sql, args...).Find(&ids)
 	}
 }
 
@@ -198,7 +204,16 @@ func CheckRepoStats(ctx context.Context) error {
 		},
 		// Issue.NumComments
 		{
-			statsQuery("SELECT `issue`.id FROM `issue` WHERE `issue`.num_comments!=(SELECT COUNT(*) FROM `comment` WHERE issue_id=`issue`.id AND type=0)"),
+			statsQuery(builder.Select("`issue`.id").From("`issue`").Where(
+				builder.Neq{
+					"`issue`.num_comments": builder.Select("COUNT(*)").From("`comment`").Where(
+						builder.Expr("issue_id = `issue`.id").And(
+							builder.In("type", issues_model.ConversationCountedCommentType()),
+						),
+					),
+				},
+			),
+			),
 			repoStatsCorrectIssueNumComments,
 			"issue count 'num_comments'",
 		},
diff --git a/models/repo_test.go b/models/repo_test.go
index 2a8a4a743e..bcf62237f0 100644
--- a/models/repo_test.go
+++ b/models/repo_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
 
 	"github.com/stretchr/testify/assert"
@@ -22,3 +23,16 @@ func TestDoctorUserStarNum(t *testing.T) {
 
 	assert.NoError(t, DoctorUserStarNum(db.DefaultContext))
 }
+
+func Test_repoStatsCorrectIssueNumComments(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	assert.NotNil(t, issue2)
+	assert.EqualValues(t, 0, issue2.NumComments) // the fixture data is wrong, but we don't fix it here
+
+	assert.NoError(t, repoStatsCorrectIssueNumComments(db.DefaultContext, 2))
+	// reload the issue
+	issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	assert.EqualValues(t, 1, issue2.NumComments)
+}