From 1c117a9352e4a64cd301028403c33949ab146424 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 15 Oct 2026 03:55:30 +0800 Subject: [PATCH] refactor: clean up comment model (#39779) Try to figure out whether we can reuse the "comment" table for #36862 The legacy "CommitID" field is removed, and updated the comment. Some useless and outdated "doctor fixes" are also removed together. --- models/issues/comment.go | 128 ++++++++++++------------------- models/issues/issue_label.go | 29 ------- models/repo/attachment.go | 16 ++-- models/repo/attachment_test.go | 31 ++++++++ services/doctor/dbconsistency.go | 24 +----- 5 files changed, 90 insertions(+), 138 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index bf636d874bf..b747c51592c 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -65,7 +65,7 @@ type CommentType int const CommentTypeUndefined CommentType = -1 const ( - CommentTypeComment CommentType = iota // 0 Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0) + CommentTypeComment CommentType = iota // 0 Plain comment, can be associated with "commit_sha" and a non-zero "line" CommentTypeReopen // 1 CommentTypeClose // 2 @@ -253,46 +253,57 @@ type CommentMetaData struct { // Comment represents a comment in commit and issue page. type Comment struct { - ID int64 `xorm:"pk autoincr"` - Type CommentType `xorm:"INDEX"` - PosterID int64 `xorm:"INDEX"` - Poster *user_model.User `xorm:"-"` + ID int64 `xorm:"pk autoincr"` + Type CommentType + + PosterID int64 `xorm:"INDEX"` + Poster *user_model.User `xorm:"-"` + OriginalAuthor string OriginalAuthorID int64 - IssueID int64 `xorm:"INDEX"` - Issue *Issue `xorm:"-"` - LabelID int64 - Label *Label `xorm:"-"` - AddedLabels []*Label `xorm:"-"` - RemovedLabels []*Label `xorm:"-"` - OldProjectID int64 - ProjectID int64 - OldProject *project_model.Project `xorm:"-"` - Project *project_model.Project `xorm:"-"` - OldMilestoneID int64 - MilestoneID int64 - OldMilestone *Milestone `xorm:"-"` - Milestone *Milestone `xorm:"-"` - TimeID int64 - Time *TrackedTime `xorm:"-"` - AssigneeID int64 - RemovedAssignee bool - Assignee *user_model.User `xorm:"-"` - AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"` - AssigneeTeam *organization.Team `xorm:"-"` - ResolveDoerID int64 - ResolveDoer *user_model.User `xorm:"-"` - OldTitle string - NewTitle string - OldRef string - NewRef string + + IssueID int64 `xorm:"INDEX"` + Issue *Issue `xorm:"-"` + + LabelID int64 + Label *Label `xorm:"-"` + AddedLabels []*Label `xorm:"-"` + RemovedLabels []*Label `xorm:"-"` + + OldProjectID int64 + ProjectID int64 + OldProject *project_model.Project `xorm:"-"` + Project *project_model.Project `xorm:"-"` + + OldMilestoneID int64 + MilestoneID int64 + OldMilestone *Milestone `xorm:"-"` + Milestone *Milestone `xorm:"-"` + + TimeID int64 + Time *TrackedTime `xorm:"-"` + + AssigneeID int64 + RemovedAssignee bool + Assignee *user_model.User `xorm:"-"` + AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"` + AssigneeTeam *organization.Team `xorm:"-"` + + ResolveDoerID int64 + ResolveDoer *user_model.User `xorm:"-"` + + OldTitle string + NewTitle string + OldRef string + NewRef string + DependentIssueID int64 `xorm:"index"` // This is used by issue_service.deleteIssue DependentIssue *Issue `xorm:"-"` - CommitID int64 - Line int64 // - previous line / + proposed line - TreePath string `xorm:"VARCHAR(4000)"` // SQLServer only supports up to 4000 - Content string `xorm:"LONGTEXT"` + Line int64 // - previous line / + proposed line + TreePath string `xorm:"VARCHAR(4000)"` // SQLServer only supports up to 4000 + Content string `xorm:"LONGTEXT"` + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` RenderedContent template.HTML `xorm:"-"` @@ -324,12 +335,12 @@ type Comment struct { RefAction references.XRefAction `xorm:"SMALLINT"` // What happens if RefIssueID resolves RefIsPull bool - CommentMetaData *CommentMetaData `xorm:"JSON TEXT"` // put all non-index metadata in a single field - RefRepo *repo_model.Repository `xorm:"-"` RefIssue *Issue `xorm:"-"` RefComment *Comment `xorm:"-"` + CommentMetaData *CommentMetaData `xorm:"JSON TEXT"` // put all non-index metadata in a single field + Commits []*git_model.SignCommitWithStatuses `xorm:"-"` OldCommit string `xorm:"-"` NewCommit string `xorm:"-"` @@ -849,7 +860,6 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, RemovedAssignee: opts.RemovedAssignee, AssigneeID: opts.AssigneeID, AssigneeTeamID: opts.AssigneeTeamID, - CommitID: opts.CommitID, CommitSHA: opts.CommitSHA, Line: opts.LineNum, Content: opts.Content, @@ -1015,7 +1025,6 @@ type CreateCommentOptions struct { NewTitle string OldRef string NewRef string - CommitID int64 CommitSHA string Patch string LineNum int64 @@ -1258,45 +1267,6 @@ func (c *Comment) GetExternalName() string { return c.OriginalAuthor } // GetExternalID ExternalUserRemappable interface func (c *Comment) GetExternalID() int64 { return c.OriginalAuthorID } -// CountCommentTypeLabelWithEmptyLabel count label comments with empty label -func CountCommentTypeLabelWithEmptyLabel(ctx context.Context) (int64, error) { - return db.GetEngine(ctx).Where(builder.Eq{"type": CommentTypeLabel, "label_id": 0}).Count(new(Comment)) -} - -// FixCommentTypeLabelWithEmptyLabel count label comments with empty label -func FixCommentTypeLabelWithEmptyLabel(ctx context.Context) (int64, error) { - return db.GetEngine(ctx).Where(builder.Eq{"type": CommentTypeLabel, "label_id": 0}).Delete(new(Comment)) -} - -// CountCommentTypeLabelWithOutsideLabels count label comments with outside label -func CountCommentTypeLabelWithOutsideLabels(ctx context.Context) (int64, error) { - return db.GetEngine(ctx).Where("comment.type = ? AND ((label.org_id = 0 AND issue.repo_id != label.repo_id) OR (label.repo_id = 0 AND label.org_id != repository.owner_id))", CommentTypeLabel). - Table("comment"). - Join("inner", "label", "label.id = comment.label_id"). - Join("inner", "issue", "issue.id = comment.issue_id "). - Join("inner", "repository", "issue.repo_id = repository.id"). - Count() -} - -// FixCommentTypeLabelWithOutsideLabels count label comments with outside label -func FixCommentTypeLabelWithOutsideLabels(ctx context.Context) (int64, error) { - res, err := db.GetEngine(ctx).Exec(`DELETE FROM comment WHERE comment.id IN ( - SELECT il_too.id FROM ( - SELECT com.id - FROM comment AS com - INNER JOIN label ON com.label_id = label.id - INNER JOIN issue on issue.id = com.issue_id - INNER JOIN repository ON issue.repo_id = repository.id - WHERE - com.type = ? AND ((label.org_id = 0 AND issue.repo_id != label.repo_id) OR (label.repo_id = 0 AND label.org_id != repository.owner_id)) - ) AS il_too)`, CommentTypeLabel) - if err != nil { - return 0, err - } - - return res.RowsAffected() -} - // HasOriginalAuthor returns if a comment was migrated and has an original author. func (c *Comment) HasOriginalAuthor() bool { return c.OriginalAuthor != "" && c.OriginalAuthorID != 0 diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index b628a25c14c..10b308ede50 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -282,35 +282,6 @@ func DeleteOrphanedIssueLabels(ctx context.Context) error { return err } -// CountIssueLabelWithOutsideLabels count label comments with outside label -func CountIssueLabelWithOutsideLabels(ctx context.Context) (int64, error) { - return db.GetEngine(ctx).Where(builder.Expr("(label.org_id = 0 AND issue.repo_id != label.repo_id) OR (label.repo_id = 0 AND label.org_id != repository.owner_id)")). - Table("issue_label"). - Join("inner", "label", "issue_label.label_id = label.id "). - Join("inner", "issue", "issue.id = issue_label.issue_id "). - Join("inner", "repository", "issue.repo_id = repository.id"). - Count(new(IssueLabel)) -} - -// FixIssueLabelWithOutsideLabels fix label comments with outside label -func FixIssueLabelWithOutsideLabels(ctx context.Context) (int64, error) { - res, err := db.GetEngine(ctx).Exec(`DELETE FROM issue_label WHERE issue_label.id IN ( - SELECT il_too.id FROM ( - SELECT il_too_too.id - FROM issue_label AS il_too_too - INNER JOIN label ON il_too_too.label_id = label.id - INNER JOIN issue on issue.id = il_too_too.issue_id - INNER JOIN repository on repository.id = issue.repo_id - WHERE - (label.org_id = 0 AND issue.repo_id != label.repo_id) OR (label.repo_id = 0 AND label.org_id != repository.owner_id) - ) AS il_too )`) - if err != nil { - return 0, err - } - - return res.RowsAffected() -} - // LoadLabels loads labels func (issue *Issue) LoadLabels(ctx context.Context) (err error) { if !issue.isLabelsLoaded && issue.Labels == nil && issue.ID != 0 { diff --git a/models/repo/attachment.go b/models/repo/attachment.go index da522d919fb..a3adc473baa 100644 --- a/models/repo/attachment.go +++ b/models/repo/attachment.go @@ -257,15 +257,17 @@ func DeleteAttachmentsByRelease(ctx context.Context, releaseID int64) error { return err } -// CountOrphanedAttachments returns the number of bad attachments -func CountOrphanedAttachments(ctx context.Context) (int64, error) { - return db.GetEngine(ctx).Where("(issue_id > 0 and issue_id not in (select id from issue)) or (release_id > 0 and release_id not in (select id from `release`))"). - Count(new(Attachment)) +func sqlOrphanedAttachmentsWhereCond() string { + return "(issue_id > 0 and issue_id not in (select id from `issue`))" + + " or (release_id > 0 and release_id not in (select id from `release`))" + + " or (comment_id > 0 and comment_id not in (select id from `comment`))" +} + +func CountOrphanedAttachments(ctx context.Context) (int64, error) { + return db.GetEngine(ctx).Where(sqlOrphanedAttachmentsWhereCond()).Count(new(Attachment)) } -// DeleteOrphanedAttachments delete all bad attachments func DeleteOrphanedAttachments(ctx context.Context) error { - _, err := db.GetEngine(ctx).Where("(issue_id > 0 and issue_id not in (select id from issue)) or (release_id > 0 and release_id not in (select id from `release`))"). - Delete(new(Attachment)) + _, err := db.GetEngine(ctx).Where(sqlOrphanedAttachmentsWhereCond()).Delete(new(Attachment)) return err } diff --git a/models/repo/attachment_test.go b/models/repo/attachment_test.go index 1ea3026010a..362092f8c46 100644 --- a/models/repo/attachment_test.go +++ b/models/repo/attachment_test.go @@ -6,10 +6,13 @@ package repo_test import ( "testing" + "gitea.dev/models/db" + issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIncreaseDownloadCount(t *testing.T) { @@ -69,6 +72,34 @@ func TestGetAttachmentByID(t *testing.T) { assert.Equal(t, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", attach.UUID) } +func TestDeleteOrphanedAttachments(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + issue := &issues_model.Issue{} + release := &repo_model.Release{} + comment := &issues_model.Comment{} + require.NoError(t, db.Insert(t.Context(), issue, release, comment)) + require.NoError(t, db.Insert(t.Context(), []*repo_model.Attachment{ + {UUID: "valid-issue", IssueID: issue.ID}, + {UUID: "valid-release", ReleaseID: release.ID}, + {UUID: "valid-comment", CommentID: comment.ID}, + {UUID: "unlinked"}, + {UUID: "orphaned-issue", IssueID: unittest.NonexistentID}, + {UUID: "orphaned-release", ReleaseID: unittest.NonexistentID}, + {UUID: "orphaned-comment", IssueID: issue.ID, CommentID: unittest.NonexistentID}, + })) + + require.NoError(t, repo_model.DeleteOrphanedAttachments(t.Context())) + + unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{UUID: "valid-issue"}) + unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{UUID: "valid-release"}) + unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{UUID: "valid-comment"}) + unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{UUID: "unlinked"}) + unittest.AssertNotExistsBean(t, &repo_model.Attachment{UUID: "orphaned-issue"}) + unittest.AssertNotExistsBean(t, &repo_model.Attachment{UUID: "orphaned-release"}) + unittest.AssertNotExistsBean(t, &repo_model.Attachment{UUID: "orphaned-comment"}) +} + func TestAttachment_DownloadURL(t *testing.T) { attach := &repo_model.Attachment{ UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index f2495e411dd..ee3225ada4e 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -108,9 +108,8 @@ func prepareDBConsistencyChecks() []consistencyCheck { // find tracked times without existing issues/pulls genericOrphanCheck("Orphaned TrackedTimes without existing issue", "tracked_time", "issue", "tracked_time.issue_id=issue.id"), - // find attachments without existing issues or releases { - Name: "Orphaned Attachments without existing issues or releases", + Name: "Orphaned Attachments without existing issues, comments or releases", Counter: repo_model.CountOrphanedAttachments, Fixer: asFixer(repo_model.DeleteOrphanedAttachments), }, @@ -121,27 +120,6 @@ func prepareDBConsistencyChecks() []consistencyCheck { Fixer: repo_model.FixNullArchivedRepository, FixedMessage: "Fixed", }, - // find label comments with empty labels - { - Name: "Label comments with empty labels", - Counter: issues_model.CountCommentTypeLabelWithEmptyLabel, - Fixer: issues_model.FixCommentTypeLabelWithEmptyLabel, - FixedMessage: "Fixed", - }, - // find label comments with labels from outside the repository - { - Name: "Label comments with labels from outside the repository", - Counter: issues_model.CountCommentTypeLabelWithOutsideLabels, - Fixer: issues_model.FixCommentTypeLabelWithOutsideLabels, - FixedMessage: "Removed", - }, - // find issue_label with labels from outside the repository - { - Name: "IssueLabels with Labels from outside the repository", - Counter: issues_model.CountIssueLabelWithOutsideLabels, - Fixer: issues_model.FixIssueLabelWithOutsideLabels, - FixedMessage: "Removed", - }, { Name: "Action with created_unix set as an empty string", Counter: activities_model.CountActionCreatedUnixString,