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

@@ -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

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 {

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
 }

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",

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,