Improve attachments deletion

models/db/file_status.go

@@ -0,0 +1,12 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package db
+
+// FileStatus represents the status of a file in the disk.
+type FileStatus int
+
+const (
+	FileStatusNormal      FileStatus = iota // FileStatusNormal indicates the file is normal and exists on disk.
+	FileStatusToBeDeleted                   // FileStatusToBeDeleted indicates the file is marked for deletion but still exists on disk.
+)

models/fixtures/attachment.yml

@@ -153,3 +153,16 @@
   download_count: 0
   size: 0
   created_unix: 946684800
+
+-
+  id: 13
+  uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a23
+  repo_id: 1
+  issue_id: 3
+  release_id: 0
+  uploader_id: 0
+  comment_id: 7
+  name: code_comment_uploaded_attachment.png
+  download_count: 0
+  size: 0
+  created_unix: 946684812

models/fixtures/comment.yml

@@ -102,3 +102,12 @@
   review_id: 22
   assignee_id: 5
   created_unix: 946684817
+
+-
+  id: 12
+  type: 22 # review
+  poster_id: 100
+  issue_id: 3
+  content: ""
+  review_id: 10
+  created_unix: 946684812

models/issues/comment.go

@@ -390,18 +390,6 @@ func (c *Comment) LoadPoster(ctx context.Context) (err error) {
 	return err
 }
 
-// AfterDelete is invoked from XORM after the object is deleted.
-func (c *Comment) AfterDelete(ctx context.Context) {
-	if c.ID <= 0 {
-		return
-	}
-
-	_, err := repo_model.DeleteAttachmentsByComment(ctx, c.ID, true)
-	if err != nil {
-		log.Info("Could not delete files for comment %d on issue #%d: %s", c.ID, c.IssueID, err)
-	}
-}
-
 // HTMLURL formats a URL-string to the issue-comment
 func (c *Comment) HTMLURL(ctx context.Context) string {
 	err := c.LoadIssue(ctx)
@@ -611,6 +599,9 @@ func UpdateCommentAttachments(ctx context.Context, c *Comment, uuids []string) e
 			return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
 		}
 		for i := range attachments {
+			if attachments[i].IssueID != 0 || attachments[i].CommentID != 0 {
+				return util.NewPermissionDeniedErrorf("update comment attachments permission denied")
+			}
 			attachments[i].IssueID = c.IssueID
 			attachments[i].CommentID = c.ID
 			if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
@@ -1122,36 +1113,65 @@ func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *us
 }
 
 // DeleteComment deletes the comment
-func DeleteComment(ctx context.Context, comment *Comment) error {
-	e := db.GetEngine(ctx)
-	if _, err := e.ID(comment.ID).NoAutoCondition().Delete(comment); err != nil {
-		return err
+func DeleteComment(ctx context.Context, comment *Comment) (*Comment, error) {
+	if _, err := db.GetEngine(ctx).ID(comment.ID).NoAutoCondition().Delete(comment); err != nil {
+		return nil, err
 	}
 
 	if _, err := db.DeleteByBean(ctx, &ContentHistory{
 		CommentID: comment.ID,
 	}); err != nil {
-		return err
+		return nil, err
 	}
 
 	if comment.Type.CountedAsConversation() {
 		if err := UpdateIssueNumComments(ctx, comment.IssueID); err != nil {
-			return err
+			return nil, err
 		}
 	}
-	if _, err := e.Table("action").
+	if _, err := db.GetEngine(ctx).Table("action").
 		Where("comment_id = ?", comment.ID).
 		Update(map[string]any{
 			"is_deleted": true,
 		}); err != nil {
-		return err
+		return nil, err
+	}
+
+	var deletedReviewComment *Comment
+
+	// delete review & review comment if the code comment is the last comment of the review
+	if comment.Type == CommentTypeCode && comment.ReviewID > 0 {
+		res, err := db.GetEngine(ctx).ID(comment.ReviewID).
+			Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ? AND `type` = ?)", comment.ReviewID, CommentTypeCode).
+			Delete(new(Review))
+		if err != nil {
+			return nil, err
+		}
+		if res > 0 {
+			var reviewComment Comment
+			has, err := db.GetEngine(ctx).Where("review_id = ?", comment.ReviewID).
+				And("type = ?", CommentTypeReview).Get(&reviewComment)
+			if err != nil {
+				return nil, err
+			}
+			if has && reviewComment.Content == "" {
+				if _, err := db.GetEngine(ctx).ID(reviewComment.ID).Delete(new(Comment)); err != nil {
+					return nil, err
+				}
+				deletedReviewComment = &reviewComment
+			}
+			comment.ReviewID = 0 // reset review ID to 0 for the notification
+		}
 	}
 
 	if err := comment.neuterCrossReferences(ctx); err != nil {
-		return err
+		return nil, err
 	}
 
-	return DeleteReaction(ctx, &ReactionOptions{CommentID: comment.ID})
+	if err := DeleteReaction(ctx, &ReactionOptions{CommentID: comment.ID}); err != nil {
+		return nil, err
+	}
+	return deletedReviewComment, nil
 }
 
 // UpdateCommentsMigrationsByType updates comments' migrations information via given git service type and original id and poster id

models/issues/issue_update.go

@@ -305,6 +305,9 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string)
 		return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
 	}
 	for i := range attachments {
+		if attachments[i].IssueID != 0 {
+			return util.NewPermissionDeniedErrorf("update issue attachments permission denied")
+		}
 		attachments[i].IssueID = issueID
 		if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
 			return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)

models/issues/pull_list_test.go

@@ -39,9 +39,8 @@ func TestPullRequestList_LoadReviewCommentsCounts(t *testing.T) {
 	reviewComments, err := prs.LoadReviewCommentsCounts(db.DefaultContext)
 	assert.NoError(t, err)
 	assert.Len(t, reviewComments, 2)
-	for _, pr := range prs {
-		assert.Equal(t, 1, reviewComments[pr.IssueID])
-	}
+	assert.Equal(t, 1, reviewComments[prs[0].IssueID])
+	assert.Equal(t, 2, reviewComments[prs[1].IssueID])
 }
 
 func TestPullRequestList_LoadReviews(t *testing.T) {

models/migrations/v1_25/main_test.go

@@ -0,0 +1,14 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_25
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/migrations/base"
+)
+
+func TestMain(m *testing.M) {
+	base.MainTest(m)
+}

models/migrations/v1_25/v321.go

@@ -0,0 +1,41 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_25
+
+import (
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+func AddFileStatusToAttachment(x *xorm.Engine) error {
+	type Attachment struct {
+		ID                   int64  `xorm:"pk autoincr"`
+		UUID                 string `xorm:"uuid UNIQUE"`
+		RepoID               int64  `xorm:"INDEX"`           // this should not be zero
+		IssueID              int64  `xorm:"INDEX"`           // maybe zero when creating
+		ReleaseID            int64  `xorm:"INDEX"`           // maybe zero when creating
+		UploaderID           int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
+		CommentID            int64  `xorm:"INDEX"`
+		Name                 string
+		DownloadCount        int64              `xorm:"DEFAULT 0"`
+		Status               db.FileStatus      `xorm:"INDEX DEFAULT 0"`
+		DeleteFailedCount    int                `xorm:"DEFAULT 0"` // Number of times the deletion failed, used to prevent infinite loop
+		LastDeleteFailedTime timeutil.TimeStamp // Last time the deletion failed, used to prevent infinite loop
+		Size                 int64              `xorm:"DEFAULT 0"`
+		CreatedUnix          timeutil.TimeStamp `xorm:"created"`
+		CustomDownloadURL    string             `xorm:"-"`
+	}
+
+	if err := x.Sync(new(Attachment)); err != nil {
+		return err
+	}
+
+	if _, err := x.Exec("UPDATE `attachment` SET status = ? WHERE status IS NULL", db.FileStatusNormal); err != nil {
+		return err
+	}
+
+	return nil
+}

models/migrations/v1_25/v321_test.go

@@ -0,0 +1,36 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_25
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/migrations/base"
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_AddFileStatusToAttachment(t *testing.T) {
+	type Attachment struct {
+		ID                int64  `xorm:"pk autoincr"`
+		UUID              string `xorm:"uuid UNIQUE"`
+		RepoID            int64  `xorm:"INDEX"`           // this should not be zero
+		IssueID           int64  `xorm:"INDEX"`           // maybe zero when creating
+		ReleaseID         int64  `xorm:"INDEX"`           // maybe zero when creating
+		UploaderID        int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
+		CommentID         int64  `xorm:"INDEX"`
+		Name              string
+		DownloadCount     int64              `xorm:"DEFAULT 0"`
+		Size              int64              `xorm:"DEFAULT 0"`
+		CreatedUnix       timeutil.TimeStamp `xorm:"created"`
+		CustomDownloadURL string             `xorm:"-"`
+	}
+
+	// Prepare and load the testing database
+	x, deferable := base.PrepareTestEnv(t, 0, new(Attachment))
+	defer deferable()
+
+	assert.NoError(t, AddFileStatusToAttachment(x))
+}