Skip to content

Commit 8dfd601

Browse files
lunnylunny
committed
improvements
1 parent 0d4d4bf commit 8dfd601

File tree

11 files changed

+177
-85
lines changed

11 files changed

+177
-85
lines changed

‎modules/util/cleanup.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Copyright 2025 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package util
5+
6+
type CleanUpFunc func()
7+
8+
func NewCleanUpFunc() CleanUpFunc {
9+
return func() {}
10+
}
11+
12+
func (f CleanUpFunc) Append(newF CleanUpFunc) CleanUpFunc {
13+
return func() {
14+
f()
15+
newF()
16+
}
17+
}

‎routers/api/v1/repo/issue_comment.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ func deleteIssueComment(ctx *context.APIContext) {
726726
return
727727
}
728728

729-
if _, err = issue_service.DeleteComment(ctx, ctx.Doer, comment, true); err != nil {
729+
if _, err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil {
730730
ctx.APIErrorInternal(err)
731731
return
732732
}

‎routers/web/repo/issue_comment.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ func DeleteComment(ctx *context.Context) {
325325
return
326326
}
327327

328-
deletedReviewComment, err := issue_service.DeleteComment(ctx, ctx.Doer, comment, true)
328+
deletedReviewComment, err := issue_service.DeleteComment(ctx, ctx.Doer, comment)
329329
if err != nil {
330330
ctx.ServerError("DeleteComment", err)
331331
return

‎services/issue/comments.go

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"context"
88
"errors"
99
"fmt"
10+
"os"
1011

1112
"code.gitea.io/gitea/models/db"
1213
issues_model "code.gitea.io/gitea/models/issues"
@@ -15,8 +16,10 @@ import (
1516
user_model "code.gitea.io/gitea/models/user"
1617
"code.gitea.io/gitea/modules/gitrepo"
1718
"code.gitea.io/gitea/modules/json"
19+
"code.gitea.io/gitea/modules/log"
20+
"code.gitea.io/gitea/modules/storage"
1821
"code.gitea.io/gitea/modules/timeutil"
19-
attachment_service "code.gitea.io/gitea/services/attachment"
22+
"code.gitea.io/gitea/modules/util"
2023
git_service "code.gitea.io/gitea/services/git"
2124
notify_service "code.gitea.io/gitea/services/notify"
2225
)
@@ -132,9 +135,11 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
132135
}
133136

134137
// deleteComment deletes the comment
135-
func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAttachments bool) (*issues_model.Comment, error) {
136-
return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
138+
func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAttachments bool) (*issues_model.Comment, func(), error) {
139+
storageCleanup := util.NewCleanUpFunc()
140+
deletedReviewComment, err := db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
137141
if removeAttachments {
142+
// load attachments before deleting the comment
138143
if err := comment.LoadAttachments(ctx); err != nil {
139144
return nil, err
140145
}
@@ -147,20 +152,42 @@ func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAtt
147152

148153
if removeAttachments {
149154
// delete comment attachments
150-
if _, err := attachment_service.DeleteAttachments(ctx, comment.Attachments); err != nil {
155+
_, err := db.GetEngine(ctx).Where("comment_id = ?", comment.ID).NoAutoCondition().Delete(&repo_model.Attachment{})
156+
if err != nil {
151157
return nil, err
152158
}
153-
}
154159

160+
// the storage cleanup function to remove attachments could be called after all transactions are committed
161+
storageCleanup = storageCleanup.Append(func() {
162+
for _, a := range comment.Attachments {
163+
if err := storage.Attachments.Delete(a.RelativePath()); err != nil {
164+
if !errors.Is(err, os.ErrNotExist) {
165+
// Even delete files failed, but the attachments has been removed from database, so we
166+
// should not return error but only record the error on logs.
167+
// users have to delete this attachments manually or we should have a
168+
// synchronize between database attachment table and attachment storage
169+
log.Error("delete attachment[uuid: %s] failed: %v", a.UUID, err)
170+
} else {
171+
log.Warn("Attachment file not found when deleting: %s", a.RelativePath())
172+
}
173+
}
174+
}
175+
})
176+
}
155177
return deletedReviewComment, nil
156178
})
179+
if err != nil {
180+
return nil, nil, err
181+
}
182+
return deletedReviewComment, storageCleanup, nil
157183
}
158184

159-
func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment, removeAttachments bool) (*issues_model.Comment, error) {
160-
deletedReviewComment, err := deleteComment(ctx, comment, removeAttachments)
185+
func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) (*issues_model.Comment, error) {
186+
deletedReviewComment, cleanup, err := deleteComment(ctx, comment, false)
161187
if err != nil {
162188
return nil, err
163189
}
190+
cleanup()
164191

165192
notify_service.DeleteComment(ctx, doer, comment)
166193

‎services/issue/comments_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func Test_DeleteCommentWithReview(t *testing.T) {
2727
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
2828

2929
// since this is the last comment of the review, it should be deleted when the comment is deleted
30-
deletedReviewComment, err := DeleteComment(db.DefaultContext, user1, comment, true)
30+
deletedReviewComment, err := DeleteComment(db.DefaultContext, user1, comment)
3131
assert.NoError(t, err)
3232
assert.NotNil(t, deletedReviewComment)
3333

‎services/issue/issue.go

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"code.gitea.io/gitea/modules/git"
2020
"code.gitea.io/gitea/modules/log"
2121
"code.gitea.io/gitea/modules/storage"
22+
"code.gitea.io/gitea/modules/util"
2223
notify_service "code.gitea.io/gitea/services/notify"
2324
)
2425

@@ -190,9 +191,11 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
190191
}
191192

192193
// delete entries in database
193-
if err := deleteIssue(ctx, issue, true); err != nil {
194+
cleanup, err := deleteIssue(ctx, issue, true)
195+
if err != nil {
194196
return err
195197
}
198+
cleanup()
196199

197200
// delete pull request related git data
198201
if issue.IsPull && gitRepo != nil {
@@ -256,8 +259,9 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i
256259
}
257260

258261
// deleteIssue deletes the issue
259-
func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachments bool) error {
260-
return db.WithTx(ctx, func(ctx context.Context) error {
262+
func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachments bool) (util.CleanUpFunc, error) {
263+
cleanup := util.NewCleanUpFunc()
264+
if err := db.WithTx(ctx, func(ctx context.Context) error {
261265
if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil {
262266
return err
263267
}
@@ -302,7 +306,6 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachmen
302306
&issues_model.Stopwatch{IssueID: issue.ID},
303307
&issues_model.TrackedTime{IssueID: issue.ID},
304308
&project_model.ProjectIssue{IssueID: issue.ID},
305-
&repo_model.Attachment{IssueID: issue.ID},
306309
&issues_model.PullRequest{IssueID: issue.ID},
307310
&issues_model.Comment{RefIssueID: issue.ID},
308311
&issues_model.IssueDependency{DependencyID: issue.ID},
@@ -313,19 +316,32 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachmen
313316
}
314317

315318
for _, comment := range issue.Comments {
316-
if _, err := deleteComment(ctx, comment, deleteAttachments); err != nil {
319+
_, cleanupDeleteComment, err := deleteComment(ctx, comment, deleteAttachments)
320+
if err != nil {
317321
return fmt.Errorf("deleteComment [comment_id: %d]: %w", comment.ID, err)
318322
}
323+
cleanup = cleanup.Append(cleanupDeleteComment)
319324
}
320325

321326
if deleteAttachments {
322-
// Remove issue attachment files.
323-
for i := range issue.Attachments {
324-
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete issue attachment", issue.Attachments[i].RelativePath())
327+
// delete issue attachments
328+
_, err := db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issue.ID).NoAutoCondition().Delete(&issues_model.Issue{})
329+
if err != nil {
330+
return err
325331
}
332+
// the storage cleanup function to remove attachments could be called after all transactions are committed
333+
cleanup = cleanup.Append(func() {
334+
// Remove issue attachment files.
335+
for i := range issue.Attachments {
336+
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete issue attachment", issue.Attachments[i].RelativePath())
337+
}
338+
})
326339
}
327340
return nil
328-
})
341+
}); err != nil {
342+
return nil, err
343+
}
344+
return cleanup, nil
329345
}
330346

331347
// DeleteOrphanedIssues delete issues without a repo
@@ -361,9 +377,11 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64, deleteAttachments b
361377
}
362378

363379
for _, issue := range issues {
364-
if err := deleteIssue(ctx, issue, deleteAttachments); err != nil {
380+
cleanup, err := deleteIssue(ctx, issue, deleteAttachments)
381+
if err != nil {
365382
return fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err)
366383
}
384+
cleanup()
367385
}
368386
}
369387

‎services/issue/issue_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,9 @@ func TestIssue_DeleteIssue(t *testing.T) {
4444
ID: issueIDs[2],
4545
}
4646

47-
err = deleteIssue(db.DefaultContext, issue, true)
47+
cleanup, err := deleteIssue(db.DefaultContext, issue, true)
4848
assert.NoError(t, err)
49+
cleanup()
4950
issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1)
5051
assert.NoError(t, err)
5152
assert.Len(t, issueIDs, 4)
@@ -55,8 +56,9 @@ func TestIssue_DeleteIssue(t *testing.T) {
5556
assert.NoError(t, err)
5657
issue, err = issues_model.GetIssueByID(db.DefaultContext, 4)
5758
assert.NoError(t, err)
58-
err = deleteIssue(db.DefaultContext, issue, true)
59+
cleanup, err = deleteIssue(db.DefaultContext, issue, true)
5960
assert.NoError(t, err)
61+
cleanup()
6062
assert.Len(t, attachments, 2)
6163
for i := range attachments {
6264
attachment, err := repo_model.GetAttachmentByUUID(db.DefaultContext, attachments[i].UUID)
@@ -78,8 +80,9 @@ func TestIssue_DeleteIssue(t *testing.T) {
7880
assert.NoError(t, err)
7981
assert.False(t, left)
8082

81-
err = deleteIssue(db.DefaultContext, issue2, true)
83+
cleanup, err = deleteIssue(db.DefaultContext, issue2, true)
8284
assert.NoError(t, err)
85+
cleanup()
8386
left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)
8487
assert.NoError(t, err)
8588
assert.True(t, left)

‎services/release/release.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"code.gitea.io/gitea/modules/storage"
2323
"code.gitea.io/gitea/modules/timeutil"
2424
"code.gitea.io/gitea/modules/util"
25-
attachment_service "code.gitea.io/gitea/services/attachment"
2625
notify_service "code.gitea.io/gitea/services/notify"
2726
)
2827

@@ -302,8 +301,9 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
302301
deletedUUIDs.Add(attach.UUID)
303302
}
304303

305-
if _, err := attachment_service.DeleteAttachments(ctx, attachments); err != nil {
306-
return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", delAttachmentUUIDs, err)
304+
_, err = db.GetEngine(ctx).In("uuid", deletedUUIDs.Values()).NoAutoCondition().Delete(&repo_model.Attachment{})
305+
if err != nil {
306+
return err
307307
}
308308
}
309309

@@ -339,7 +339,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
339339
return err
340340
}
341341

342-
for _, uuid := range delAttachmentUUIDs {
342+
for _, uuid := range deletedUUIDs.Values() {
343343
if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(uuid)); err != nil {
344344
// Even delete files failed, but the attachments has been removed from database, so we
345345
// should not return error but only record the error on logs.

‎services/repository/delete.go

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,22 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
115115
}
116116
}
117117

118-
attachments := make([]*repo_model.Attachment, 0, 20)
119-
if err = sess.Join("INNER", "`release`", "`release`.id = `attachment`.release_id").
118+
// some attachments have release_id but repo_id = 0
119+
releaseAttachments := make([]*repo_model.Attachment, 0, 20)
120+
if err = db.GetEngine(ctx).Join("INNER", "`release`", "`release`.id = `attachment`.release_id").
120121
Where("`release`.repo_id = ?", repoID).
121-
Find(&attachments); err != nil {
122+
Find(&releaseAttachments); err != nil {
122123
return err
123124
}
124-
releaseAttachments := make([]string, 0, len(attachments))
125-
for i := 0; i < len(attachments); i++ {
126-
releaseAttachments = append(releaseAttachments, attachments[i].RelativePath())
125+
// Delete attachments with release_id but repo_id = 0
126+
if len(releaseAttachments) > 0 {
127+
ids := make([]int64, 0, len(releaseAttachments))
128+
for _, attach := range releaseAttachments {
129+
ids = append(ids, attach.ID)
130+
}
131+
if _, err := db.GetEngine(ctx).In("id", ids).Delete(&repo_model.Attachment{}); err != nil {
132+
return fmt.Errorf("delete release attachments failed: %w", err)
133+
}
127134
}
128135

129136
if _, err := db.Exec(ctx, "UPDATE `user` SET num_stars=num_stars-1 WHERE id IN (SELECT `uid` FROM `star` WHERE repo_id = ?)", repo.ID); err != nil {
@@ -267,21 +274,14 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
267274
}
268275
}
269276

270-
// Get all attachments with both issue_id and release_id are zero
271-
var newAttachments []*repo_model.Attachment
277+
// Get all attachments with repo_id = repo.ID. some release attachments have repo_id = 0 should be deleted before
278+
var repoAttachments []*repo_model.Attachment
272279
if err := sess.Where(builder.Eq{
273-
"repo_id": repo.ID,
274-
"issue_id": 0,
275-
"release_id": 0,
276-
}).Find(&newAttachments); err != nil {
280+
"repo_id": repo.ID,
281+
}).Find(&repoAttachments); err != nil {
277282
return err
278283
}
279284

280-
newAttachmentPaths := make([]string, 0, len(newAttachments))
281-
for _, attach := range newAttachments {
282-
newAttachmentPaths = append(newAttachmentPaths, attach.RelativePath())
283-
}
284-
285285
if _, err := sess.Where("repo_id=?", repo.ID).Delete(new(repo_model.Attachment)); err != nil {
286286
return err
287287
}
@@ -330,14 +330,13 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID
330330
system_model.RemoveStorageWithNotice(ctx, storage.LFS, "Delete orphaned LFS file", lfsObj)
331331
}
332332

333-
// Remove release attachment files.
334-
for _, releaseAttachment := range releaseAttachments {
335-
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete release attachment", releaseAttachment)
333+
// Remove release attachments
334+
for _, attachment := range releaseAttachments {
335+
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete release attachment", attachment.RelativePath())
336336
}
337-
338-
// Remove attachment with no issue_id and release_id.
339-
for _, newAttachment := range newAttachmentPaths {
340-
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete issue attachment", newAttachment)
337+
// Remove attachment with repo_id = repo.ID.
338+
for _, attachment := range repoAttachments {
339+
system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete repo attachment", attachment.RelativePath())
341340
}
342341

343342
if len(repo.Avatar) > 0 {

0 commit comments

Comments
 (0)