From 968c04c7da20d6ab9d7cf5529562ca668a062e07 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 31 Dec 2024 07:54:40 +0800 Subject: [PATCH] Fix issue comment number (#30556) (#33055) Backport #30556 by @lunny Fix #22419 Only comments with types `CommentTypeComment` and `CommentTypeReview` will be counted as conversations of the pull request. `CommentTypeReview` was missed in the previous implementation. Co-authored-by: Lunny Xiao --- models/issues/comment.go | 38 +++++++++++++++++++++++++++---- models/issues/comment_test.go | 9 ++++++++ models/issues/review.go | 4 ++++ models/repo.go | 43 +++++++++++++++++++++++------------ models/repo_test.go | 14 ++++++++++++ 5 files changed, 89 insertions(+), 19 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index e4537aa872..729fc1b9e5 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 @@ -1182,8 +1196,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 } } @@ -1300,6 +1314,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 { @@ -1332,8 +1361,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 d81f33f953..cb31a21f92 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -97,3 +97,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 0dc8ee5df3..598f8df6a4 100644 --- a/models/repo.go +++ b/models/repo.go @@ -19,6 +19,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 @@ -27,7 +29,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 } @@ -38,8 +40,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) @@ -54,21 +55,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 { @@ -105,11 +108,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 { @@ -128,9 +131,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) } } @@ -201,7 +207,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) +}