diff --git a/models/db/file_status.go b/models/db/file_status.go new file mode 100644 index 00000000000..6def378a0e5 --- /dev/null +++ b/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. +) diff --git a/models/issues/comment.go b/models/issues/comment.go index 603c6ad7ca2..e73ac4cf095 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -599,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 { diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 9b99787e3bc..bb49470dd56 100644 --- a/models/issues/issue_update.go +++ b/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) diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go new file mode 100644 index 00000000000..4dae05114a9 --- /dev/null +++ b/models/migrations/v1_25/v321.go @@ -0,0 +1,41 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_24 + +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 +} diff --git a/models/repo/attachment.go b/models/repo/attachment.go index d0690a1319a..3d3e931fe35 100644 --- a/models/repo/attachment.go +++ b/models/repo/attachment.go @@ -18,18 +18,22 @@ import ( // Attachment represent a attachment of issue/comment/release. 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:"-"` + 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 + LastDeleteFailedReason string `xorm:"TEXT"` // Last reason 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:"-"` } func init() { @@ -88,7 +92,9 @@ func (err ErrAttachmentNotExist) Unwrap() error { // GetAttachmentByID returns attachment by given id func GetAttachmentByID(ctx context.Context, id int64) (*Attachment, error) { attach := &Attachment{} - if has, err := db.GetEngine(ctx).ID(id).Get(attach); err != nil { + if has, err := db.GetEngine(ctx).ID(id). + And("status = ?", db.FileStatusNormal). + Get(attach); err != nil { return nil, err } else if !has { return nil, ErrAttachmentNotExist{ID: id, UUID: ""} @@ -99,7 +105,9 @@ func GetAttachmentByID(ctx context.Context, id int64) (*Attachment, error) { // GetAttachmentByUUID returns attachment by given UUID. func GetAttachmentByUUID(ctx context.Context, uuid string) (*Attachment, error) { attach := &Attachment{} - has, err := db.GetEngine(ctx).Where("uuid=?", uuid).Get(attach) + has, err := db.GetEngine(ctx).Where("uuid=?", uuid). + And("status = ?", db.FileStatusNormal). + Get(attach) if err != nil { return nil, err } else if !has { @@ -116,18 +124,24 @@ func GetAttachmentsByUUIDs(ctx context.Context, uuids []string) ([]*Attachment, // Silently drop invalid uuids. attachments := make([]*Attachment, 0, len(uuids)) - return attachments, db.GetEngine(ctx).In("uuid", uuids).Find(&attachments) + return attachments, db.GetEngine(ctx).In("uuid", uuids). + And("status = ?", db.FileStatusNormal). + Find(&attachments) } // ExistAttachmentsByUUID returns true if attachment exists with the given UUID func ExistAttachmentsByUUID(ctx context.Context, uuid string) (bool, error) { - return db.GetEngine(ctx).Where("`uuid`=?", uuid).Exist(new(Attachment)) + return db.GetEngine(ctx).Where("`uuid`=?", uuid). + And("status = ?", db.FileStatusNormal). + Exist(new(Attachment)) } // GetAttachmentsByIssueID returns all attachments of an issue. func GetAttachmentsByIssueID(ctx context.Context, issueID int64) ([]*Attachment, error) { attachments := make([]*Attachment, 0, 10) - return attachments, db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issueID).Find(&attachments) + return attachments, db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issueID). + And("status = ?", db.FileStatusNormal). + Find(&attachments) } // GetAttachmentsByIssueIDImagesLatest returns the latest image attachments of an issue. @@ -142,19 +156,23 @@ func GetAttachmentsByIssueIDImagesLatest(ctx context.Context, issueID int64) ([] OR name like '%.jxl' OR name like '%.png' OR name like '%.svg' - OR name like '%.webp')`, issueID).Desc("comment_id").Limit(5).Find(&attachments) + OR name like '%.webp')`, issueID). + And("status = ?", db.FileStatusNormal). + Desc("comment_id").Limit(5).Find(&attachments) } // GetAttachmentsByCommentID returns all attachments if comment by given ID. func GetAttachmentsByCommentID(ctx context.Context, commentID int64) ([]*Attachment, error) { attachments := make([]*Attachment, 0, 10) - return attachments, db.GetEngine(ctx).Where("comment_id=?", commentID).Find(&attachments) + return attachments, db.GetEngine(ctx).Where("comment_id=?", commentID). + And("status = ?", db.FileStatusNormal). + Find(&attachments) } // GetAttachmentByReleaseIDFileName returns attachment by given releaseId and fileName. func GetAttachmentByReleaseIDFileName(ctx context.Context, releaseID int64, fileName string) (*Attachment, error) { attach := &Attachment{ReleaseID: releaseID, Name: fileName} - has, err := db.GetEngine(ctx).Get(attach) + has, err := db.GetEngine(ctx).Where("status = ?", db.FileStatusNormal).Get(attach) if err != nil { return nil, err } else if !has { @@ -185,7 +203,8 @@ func UpdateAttachment(ctx context.Context, atta *Attachment) error { return err } -func DeleteAttachments(ctx context.Context, attachments []*Attachment) (int64, error) { +// MarkAttachmentsDeleted marks the given attachments as deleted +func MarkAttachmentsDeleted(ctx context.Context, attachments []*Attachment) (int64, error) { if len(attachments) == 0 { return 0, nil } @@ -195,15 +214,41 @@ func DeleteAttachments(ctx context.Context, attachments []*Attachment) (int64, e ids = append(ids, a.ID) } - return db.GetEngine(ctx).In("id", ids).NoAutoCondition().Delete(attachments[0]) + return db.GetEngine(ctx).Table("attachment").In("id", ids).Update(map[string]any{ + "status": db.FileStatusToBeDeleted, + }) } -// DeleteAttachmentsByRelease deletes all attachments associated with the given release. -func DeleteAttachmentsByRelease(ctx context.Context, releaseID int64) error { - _, err := db.GetEngine(ctx).Where("release_id = ?", releaseID).Delete(&Attachment{}) +// MarkAttachmentsDeletedByRelease marks all attachments associated with the given release as deleted. +func MarkAttachmentsDeletedByRelease(ctx context.Context, releaseID int64) error { + _, err := db.GetEngine(ctx).Table("attachment").Where("release_id = ?", releaseID).Update(map[string]any{ + "status": db.FileStatusToBeDeleted, + }) return err } +// DeleteAttachmentByID deletes the attachment which has been marked as deleted by given id +func DeleteAttachmentByID(ctx context.Context, id int64) error { + cnt, err := db.GetEngine(ctx).ID(id).Where("status = ?", db.FileStatusToBeDeleted).Delete(new(Attachment)) + if err != nil { + return fmt.Errorf("delete attachment by id: %w", err) + } + if cnt != 1 { + return fmt.Errorf("the attachment with id %d was not found or is not marked for deletion", id) + } + return nil +} + +func UpdateAttachmentFailure(ctx context.Context, attachment *Attachment, err error) error { + attachment.DeleteFailedCount++ + _, updateErr := db.GetEngine(ctx).Table("attachment").ID(attachment.ID).Update(map[string]any{ + "delete_failed_count": attachment.DeleteFailedCount, + "last_delete_failed_reason": err.Error(), + "last_delete_failed_time": timeutil.TimeStampNow(), + }) + return updateErr +} + // 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`))"). diff --git a/modules/util/post_tx_action.go b/modules/util/post_tx_action.go deleted file mode 100644 index b5b0e636173..00000000000 --- a/modules/util/post_tx_action.go +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package util - -// PostTxAction is a function that is executed after a database transaction -type PostTxAction func() - -func NewPostTxAction() PostTxAction { - return func() {} -} - -func (f PostTxAction) Append(appendF PostTxAction) PostTxAction { - return func() { - f() - appendF() - } -} diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 550abf4a7b3..aab68443046 100644 --- a/routers/api/v1/repo/issue_attachment.go +++ b/routers/api/v1/repo/issue_attachment.go @@ -6,6 +6,7 @@ package repo import ( "net/http" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/log" @@ -360,6 +361,10 @@ func getIssueAttachmentSafeRead(ctx *context.APIContext, issue *issues_model.Iss if !attachmentBelongsToRepoOrIssue(ctx, attachment, issue) { return nil } + if attachment.Status != db.FileStatusNormal { + ctx.APIErrorNotFound() + return nil + } return attachment } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 704db1c7a3a..5385275c127 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -7,6 +7,7 @@ import ( "errors" "net/http" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -391,6 +392,10 @@ func getIssueCommentAttachmentSafeRead(ctx *context.APIContext, comment *issues_ if !attachmentBelongsToRepoOrComment(ctx, attachment, comment) { return nil } + if attachment.Status != db.FileStatusNormal { + ctx.APIErrorNotFound() + return nil + } return attachment } diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index ab47cd4fd35..fa25c0cdfdd 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/routers/api/v1/repo/release_attachment.go @@ -393,7 +393,6 @@ func DeleteReleaseAttachment(ctx *context.APIContext) { return } // FIXME Should prove the existence of the given repo, but results in unnecessary database requests - if err := attachment_service.DeleteAttachment(ctx, attach); err != nil { ctx.APIErrorInternal(err) return diff --git a/routers/init.go b/routers/init.go index 744feee2f0d..b8bcd937bf9 100644 --- a/routers/init.go +++ b/routers/init.go @@ -36,6 +36,7 @@ import ( web_routers "code.gitea.io/gitea/routers/web" actions_service "code.gitea.io/gitea/services/actions" asymkey_service "code.gitea.io/gitea/services/asymkey" + attachment_service "code.gitea.io/gitea/services/attachment" "code.gitea.io/gitea/services/auth" "code.gitea.io/gitea/services/auth/source/oauth2" "code.gitea.io/gitea/services/automerge" @@ -174,6 +175,7 @@ func InitWebInstalled(ctx context.Context) { mustInitCtx(ctx, actions_service.Init) mustInit(repo_service.InitLicenseClassifier) + mustInit(attachment_service.Init) // Finally start up the cron cron.NewContext(ctx) diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index 9b7be58875c..ff8d098c1f2 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -70,7 +70,11 @@ func DeleteAttachment(ctx *context.Context) { file := ctx.FormString("file") attach, err := repo_model.GetAttachmentByUUID(ctx, file) if err != nil { - ctx.HTTPError(http.StatusBadRequest, err.Error()) + if repo_model.IsErrAttachmentNotExist(err) { + ctx.HTTPError(http.StatusNotFound) + } else { + ctx.ServerError("GetAttachmentByUUID", err) + } return } if !ctx.IsSigned || (ctx.Doer.ID != attach.UploaderID) { diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go index d430819357c..a41c8d87292 100644 --- a/services/attachment/attachment.go +++ b/services/attachment/attachment.go @@ -13,7 +13,9 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/context/upload" @@ -71,23 +73,96 @@ func DeleteAttachment(ctx context.Context, a *repo_model.Attachment) error { // DeleteAttachments deletes the given attachments and optionally the associated files. func DeleteAttachments(ctx context.Context, attachments []*repo_model.Attachment) (int, error) { - cnt, err := repo_model.DeleteAttachments(ctx, attachments) + cnt, err := repo_model.MarkAttachmentsDeleted(ctx, attachments) if err != nil { return 0, err } - for _, a := range attachments { - if err := storage.Attachments.Delete(a.RelativePath()); err != nil { - if !errors.Is(err, os.ErrNotExist) { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", a.UUID, err) - } else { - log.Warn("Attachment file not found when deleting: %s", a.RelativePath()) - } - } - } + CleanAttachments(ctx, attachments) + return int(cnt), nil } + +var cleanQueue *queue.WorkerPoolQueue[int64] + +func Init() error { + cleanQueue = queue.CreateSimpleQueue(graceful.GetManager().ShutdownContext(), "attachments-clean", handler) + if cleanQueue == nil { + return errors.New("Unable to create attachments-clean queue") + } + return nil +} + +// CleanAttachments adds the attachments to the clean queue for deletion. +func CleanAttachments(ctx context.Context, attachments []*repo_model.Attachment) { + for _, a := range attachments { + if err := cleanQueue.Push(a.ID); err != nil { + log.Error("Failed to push attachment ID %d to clean queue: %v", a.ID, err) + continue + } + } +} + +func handler(attachmentIDs ...int64) []int64 { + return cleanAttachments(graceful.GetManager().ShutdownContext(), attachmentIDs) +} + +func cleanAttachments(ctx context.Context, attachmentIDs []int64) []int64 { + var failed []int64 + for _, attachmentID := range attachmentIDs { + attachment, exist, err := db.GetByID[repo_model.Attachment](ctx, attachmentID) + if err != nil { + log.Error("Failed to get attachment by ID %d: %v", attachmentID, err) + continue + } + if !exist { + continue + } + if attachment.Status != db.FileStatusToBeDeleted { + log.Trace("Attachment %s is not marked for deletion, skipping", attachment.RelativePath()) + continue + } + + if err := storage.Attachments.Delete(attachment.RelativePath()); err != nil { + if !errors.Is(err, os.ErrNotExist) { + log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) + failed = append(failed, attachment.ID) + if err := repo_model.UpdateAttachmentFailure(ctx, attachment, err); err != nil { + log.Error("Failed to update attachment failure for ID %d: %v", attachment.ID, err) + } + continue + } + } + if err := repo_model.DeleteAttachmentByID(ctx, attachment.ID); err != nil { + log.Error("Failed to delete attachment by ID %d(will be tried later): %v", attachment.ID, err) + failed = append(failed, attachment.ID) + } else { + log.Trace("Attachment %s deleted from database", attachment.RelativePath()) + } + } + return failed +} + +// ScanTobeDeletedAttachments scans for attachments that are marked as to be deleted and send to +// clean queue +func ScanTobeDeletedAttachments(ctx context.Context) error { + attachments := make([]*repo_model.Attachment, 0, 10) + lastID := int64(0) + for { + if err := db.GetEngine(ctx). + Where("id > ? AND status = ?", lastID, db.FileStatusToBeDeleted). + Limit(100). + Find(&attachments); err != nil { + return fmt.Errorf("scan to-be-deleted attachments: %w", err) + } + + if len(attachments) == 0 { + log.Trace("No more attachments to be deleted") + break + } + CleanAttachments(ctx, attachments) + lastID = attachments[len(attachments)-1].ID + } + + return nil +} diff --git a/services/cron/tasks_extended.go b/services/cron/tasks_extended.go index 0018c5facc5..7c41ff399f4 100644 --- a/services/cron/tasks_extended.go +++ b/services/cron/tasks_extended.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/updatechecker" asymkey_service "code.gitea.io/gitea/services/asymkey" + attachment_service "code.gitea.io/gitea/services/attachment" repo_service "code.gitea.io/gitea/services/repository" archiver_service "code.gitea.io/gitea/services/repository/archiver" user_service "code.gitea.io/gitea/services/user" @@ -223,6 +224,16 @@ func registerRebuildIssueIndexer() { }) } +func registerCleanAttachments() { + RegisterTaskFatal("clean_attachments", &BaseConfig{ + Enabled: false, + RunAtStart: false, + Schedule: "@every 24h", + }, func(ctx context.Context, _ *user_model.User, _ Config) error { + return attachment_service.ScanTobeDeletedAttachments(ctx) + }) +} + func initExtendedTasks() { registerDeleteInactiveUsers() registerDeleteRepositoryArchives() @@ -238,4 +249,5 @@ func initExtendedTasks() { registerDeleteOldSystemNotices() registerGCLFS() registerRebuildIssueIndexer() + registerCleanAttachments() } diff --git a/services/issue/comments.go b/services/issue/comments.go index e5d67c2afd7..a2c5e28d234 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "os" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" @@ -16,10 +15,8 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" - "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" - "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/attachment" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" ) @@ -135,8 +132,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion } // deleteComment deletes the comment -func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAttachments bool) (*issues_model.Comment, util.PostTxAction, error) { - postTxActions := util.NewPostTxAction() +func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAttachments bool) (*issues_model.Comment, error) { deletedReviewComment, err := db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { if removeAttachments { // load attachments before deleting the comment @@ -151,42 +147,26 @@ func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAtt } if removeAttachments { - // delete comment attachments - if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments); err != nil { + // mark comment attachments as deleted + if _, err := repo_model.MarkAttachmentsDeleted(ctx, comment.Attachments); err != nil { return nil, err } - - // the storage cleanup function to remove attachments could be called after all transactions are committed - postTxActions = postTxActions.Append(func() { - for _, a := range comment.Attachments { - if err := storage.Attachments.Delete(a.RelativePath()); err != nil { - if !errors.Is(err, os.ErrNotExist) { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", a.UUID, err) - } else { - log.Warn("Attachment file not found when deleting: %s", a.RelativePath()) - } - } - } - }) } return deletedReviewComment, nil }) if err != nil { - return nil, nil, err + return nil, err } - return deletedReviewComment, postTxActions, nil + return deletedReviewComment, nil } func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) (*issues_model.Comment, error) { - deletedReviewComment, postTxActions, err := deleteComment(ctx, comment, true) + deletedReviewComment, err := deleteComment(ctx, comment, true) if err != nil { return nil, err } - postTxActions() + + attachment.CleanAttachments(ctx, comment.Attachments) notify_service.DeleteComment(ctx, doer, comment) diff --git a/services/issue/issue.go b/services/issue/issue.go index 04b3e3f1793..54f5fd9e1a4 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -13,13 +13,11 @@ import ( access_model "code.gitea.io/gitea/models/perm/access" project_model "code.gitea.io/gitea/models/project" repo_model "code.gitea.io/gitea/models/repo" - system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/storage" - "code.gitea.io/gitea/modules/util" + attachment_service "code.gitea.io/gitea/services/attachment" notify_service "code.gitea.io/gitea/services/notify" ) @@ -191,11 +189,12 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } // delete entries in database - postTxActions, err := deleteIssue(ctx, issue, true) + toBeCleanedAttachments, err := deleteIssue(ctx, issue, true) if err != nil { return err } - postTxActions() + + attachment_service.CleanAttachments(ctx, toBeCleanedAttachments) // delete pull request related git data if issue.IsPull && gitRepo != nil { @@ -259,8 +258,8 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i } // deleteIssue deletes the issue -func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachments bool) (util.PostTxAction, error) { - postTxActions := util.NewPostTxAction() +func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachments bool) ([]*repo_model.Attachment, error) { + toBeCleanedAttachments := make([]*repo_model.Attachment, 0) if err := db.WithTx(ctx, func(ctx context.Context) error { if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { return err @@ -316,60 +315,56 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue, deleteAttachmen } for _, comment := range issue.Comments { - _, postTxActionsDeleteComment, err := deleteComment(ctx, comment, deleteAttachments) + _, err := deleteComment(ctx, comment, deleteAttachments) if err != nil { return fmt.Errorf("deleteComment [comment_id: %d]: %w", comment.ID, err) } - postTxActions = postTxActions.Append(postTxActionsDeleteComment) + toBeCleanedAttachments = append(toBeCleanedAttachments, comment.Attachments...) } if deleteAttachments { // delete issue attachments - _, err := db.GetEngine(ctx).Where("issue_id = ? AND comment_id = 0", issue.ID).NoAutoCondition().Delete(&repo_model.Attachment{}) - if err != nil { + if err := issue.LoadAttachments(ctx); err != nil { return err } - // the storage cleanup function to remove attachments could be called after all transactions are committed - postTxActions = postTxActions.Append(func() { - // Remove issue attachment files. - for i := range issue.Attachments { - system_model.RemoveStorageWithNotice(ctx, storage.Attachments, "Delete issue attachment", issue.Attachments[i].RelativePath()) - } - }) + if _, err := repo_model.MarkAttachmentsDeleted(ctx, issue.Attachments); err != nil { + return err + } + toBeCleanedAttachments = append(toBeCleanedAttachments, issue.Attachments...) } return nil }); err != nil { return nil, err } - return postTxActions, nil + return toBeCleanedAttachments, nil } // DeleteOrphanedIssues delete issues without a repo func DeleteOrphanedIssues(ctx context.Context) error { - postTxActions := util.NewPostTxAction() + toBeCleanedAttachments := make([]*repo_model.Attachment, 0) if err := db.WithTx(ctx, func(ctx context.Context) error { repoIDs, err := issues_model.GetOrphanedIssueRepoIDs(ctx) if err != nil { return err } for i := range repoIDs { - postTxActionsDeleteIssues, err := DeleteIssuesByRepoID(ctx, repoIDs[i], true) + toBeCleanedIssueAttachments, err := DeleteIssuesByRepoID(ctx, repoIDs[i], true) if err != nil { return err } - postTxActions = postTxActions.Append(postTxActionsDeleteIssues) + toBeCleanedAttachments = append(toBeCleanedAttachments, toBeCleanedIssueAttachments...) } return nil }); err != nil { return err } - postTxActions() + attachment_service.CleanAttachments(ctx, toBeCleanedAttachments) return nil } // DeleteIssuesByRepoID deletes issues by repositories id -func DeleteIssuesByRepoID(ctx context.Context, repoID int64, deleteAttachments bool) (util.PostTxAction, error) { - postTxActions := util.NewPostTxAction() +func DeleteIssuesByRepoID(ctx context.Context, repoID int64, deleteAttachments bool) ([]*repo_model.Attachment, error) { + toBeCleanedAttachments := make([]*repo_model.Attachment, 0) for { issues := make([]*issues_model.Issue, 0, db.DefaultMaxInSize) if err := db.GetEngine(ctx). @@ -385,13 +380,13 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64, deleteAttachments b } for _, issue := range issues { - postTxActionsDeleteIssue, err := deleteIssue(ctx, issue, deleteAttachments) + toBeCleanedIssueAttachments, err := deleteIssue(ctx, issue, deleteAttachments) if err != nil { return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err) } - postTxActions = postTxActions.Append(postTxActionsDeleteIssue) + toBeCleanedAttachments = append(toBeCleanedAttachments, toBeCleanedIssueAttachments...) } } - return postTxActions, nil + return toBeCleanedAttachments, nil } diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index 27a10ce9de4..ec15c0a5e01 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -11,6 +11,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + attachment_service "code.gitea.io/gitea/services/attachment" "github.com/stretchr/testify/assert" ) @@ -44,9 +45,9 @@ func TestIssue_DeleteIssue(t *testing.T) { ID: issueIDs[2], } - postTxActions, err := deleteIssue(db.DefaultContext, issue, true) + toBeCleanedAttachments, err := deleteIssue(db.DefaultContext, issue, true) assert.NoError(t, err) - postTxActions() + attachment_service.CleanAttachments(db.DefaultContext, toBeCleanedAttachments) issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1) assert.NoError(t, err) assert.Len(t, issueIDs, 4) @@ -56,9 +57,9 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) issue, err = issues_model.GetIssueByID(db.DefaultContext, 4) assert.NoError(t, err) - postTxActions, err = deleteIssue(db.DefaultContext, issue, true) + toBeCleanedAttachments, err = deleteIssue(db.DefaultContext, issue, true) assert.NoError(t, err) - postTxActions() + attachment_service.CleanAttachments(db.DefaultContext, toBeCleanedAttachments) assert.Len(t, attachments, 2) for i := range attachments { attachment, err := repo_model.GetAttachmentByUUID(db.DefaultContext, attachments[i].UUID) @@ -80,9 +81,9 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) assert.False(t, left) - postTxActions, err = deleteIssue(db.DefaultContext, issue2, true) + toBeCleanedAttachments, err = deleteIssue(db.DefaultContext, issue2, true) assert.NoError(t, err) - postTxActions() + attachment_service.CleanAttachments(db.DefaultContext, toBeCleanedAttachments) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) assert.True(t, left) diff --git a/services/release/release.go b/services/release/release.go index 42af8e85187..ae840836d25 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -19,9 +19,9 @@ import ( "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/repository" - "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" + attachment_service "code.gitea.io/gitea/services/attachment" notify_service "code.gitea.io/gitea/services/notify" ) @@ -288,6 +288,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } deletedUUIDs := make(container.Set[string]) + deletedAttachments := make([]*repo_model.Attachment, 0, len(delAttachmentUUIDs)) if len(delAttachmentUUIDs) > 0 { // Check attachments attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, delAttachmentUUIDs) @@ -299,12 +300,13 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo return util.NewPermissionDeniedErrorf("delete attachment of release permission denied") } deletedUUIDs.Add(attach.UUID) + deletedAttachments = append(deletedAttachments, attach) } - _, err = db.GetEngine(ctx).In("uuid", deletedUUIDs.Values()).NoAutoCondition().Delete(&repo_model.Attachment{}) - if err != nil { - return err + if _, err := repo_model.MarkAttachmentsDeleted(ctx, deletedAttachments); err != nil { + return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", deletedUUIDs.Values(), err) } + // files will be deleted after database transaction is committed successfully } if len(editAttachments) > 0 { @@ -339,15 +341,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo return err } - for _, uuid := range deletedUUIDs.Values() { - if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(uuid)); err != nil { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", uuid, err) - } - } + attachment_service.CleanAttachments(ctx, deletedAttachments) if !rel.IsDraft { if !isTagCreated && !isConvertedFromTag { @@ -361,64 +355,64 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo // DeleteReleaseByID deletes a release and corresponding Git tag by given ID. func DeleteReleaseByID(ctx context.Context, repo *repo_model.Repository, rel *repo_model.Release, doer *user_model.User, delTag bool) error { - if delTag { - protectedTags, err := git_model.GetProtectedTags(ctx, rel.RepoID) - if err != nil { - return fmt.Errorf("GetProtectedTags: %w", err) - } - isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, rel.PublisherID) - if err != nil { - return err - } - if !isAllowed { - return ErrProtectedTagName{ - TagName: rel.TagName, + if err := db.WithTx(ctx, func(ctx context.Context) error { + if delTag { + protectedTags, err := git_model.GetProtectedTags(ctx, rel.RepoID) + if err != nil { + return fmt.Errorf("GetProtectedTags: %w", err) + } + isAllowed, err := git_model.IsUserAllowedToControlTag(ctx, protectedTags, rel.TagName, rel.PublisherID) + if err != nil { + return err + } + if !isAllowed { + return ErrProtectedTagName{ + TagName: rel.TagName, + } + } + + if stdout, _, err := git.NewCommand("tag", "-d").AddDashesAndList(rel.TagName). + RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil && !strings.Contains(err.Error(), "not found") { + log.Error("DeleteReleaseByID (git tag -d): %d in %v Failed:\nStdout: %s\nError: %v", rel.ID, repo, stdout, err) + return fmt.Errorf("git tag -d: %w", err) + } + + refName := git.RefNameFromTag(rel.TagName) + objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName) + notify_service.PushCommits( + ctx, doer, repo, + &repository.PushUpdateOptions{ + RefFullName: refName, + OldCommitID: rel.Sha1, + NewCommitID: objectFormat.EmptyObjectID().String(), + }, repository.NewPushCommits()) + notify_service.DeleteRef(ctx, doer, repo, refName) + + if _, err := db.DeleteByID[repo_model.Release](ctx, rel.ID); err != nil { + return fmt.Errorf("DeleteReleaseByID: %w", err) + } + } else { + rel.IsTag = true + + if err := repo_model.UpdateRelease(ctx, rel); err != nil { + return fmt.Errorf("Update: %w", err) } } - if stdout, _, err := git.NewCommand("tag", "-d").AddDashesAndList(rel.TagName). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil && !strings.Contains(err.Error(), "not found") { - log.Error("DeleteReleaseByID (git tag -d): %d in %v Failed:\nStdout: %s\nError: %v", rel.ID, repo, stdout, err) - return fmt.Errorf("git tag -d: %w", err) + rel.Repo = repo + if err := rel.LoadAttributes(ctx); err != nil { + return fmt.Errorf("LoadAttributes: %w", err) } - refName := git.RefNameFromTag(rel.TagName) - objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName) - notify_service.PushCommits( - ctx, doer, repo, - &repository.PushUpdateOptions{ - RefFullName: refName, - OldCommitID: rel.Sha1, - NewCommitID: objectFormat.EmptyObjectID().String(), - }, repository.NewPushCommits()) - notify_service.DeleteRef(ctx, doer, repo, refName) - - if _, err := db.DeleteByID[repo_model.Release](ctx, rel.ID); err != nil { - return fmt.Errorf("DeleteReleaseByID: %w", err) - } - } else { - rel.IsTag = true - - if err := repo_model.UpdateRelease(ctx, rel); err != nil { - return fmt.Errorf("Update: %w", err) + if err := repo_model.MarkAttachmentsDeletedByRelease(ctx, rel.ID); err != nil { + return fmt.Errorf("DeleteAttachments: %w", err) } + return nil + }); err != nil { + return err } - rel.Repo = repo - if err := rel.LoadAttributes(ctx); err != nil { - return fmt.Errorf("LoadAttributes: %w", err) - } - - if err := repo_model.DeleteAttachmentsByRelease(ctx, rel.ID); err != nil { - return fmt.Errorf("DeleteAttachments: %w", err) - } - - for i := range rel.Attachments { - attachment := rel.Attachments[i] - if err := storage.Attachments.Delete(attachment.RelativePath()); err != nil { - log.Error("Delete attachment %s of release %s failed: %v", attachment.UUID, rel.ID, err) - } - } + attachment_service.CleanAttachments(ctx, rel.Attachments) if !rel.IsDraft { notify_service.DeleteRelease(ctx, doer, rel) diff --git a/services/repository/delete.go b/services/repository/delete.go index f6bb58222dc..84f7fecbc73 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -29,6 +29,7 @@ import ( "code.gitea.io/gitea/modules/storage" actions_service "code.gitea.io/gitea/services/actions" asymkey_service "code.gitea.io/gitea/services/asymkey" + attachment_service "code.gitea.io/gitea/services/attachment" issue_service "code.gitea.io/gitea/services/issue" "xorm.io/builder" @@ -122,15 +123,9 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams Find(&releaseAttachments); err != nil { return err } - // Delete attachments with release_id but repo_id = 0 - if len(releaseAttachments) > 0 { - ids := make([]int64, 0, len(releaseAttachments)) - for _, attach := range releaseAttachments { - ids = append(ids, attach.ID) - } - if _, err := db.GetEngine(ctx).In("id", ids).Delete(&repo_model.Attachment{}); err != nil { - return fmt.Errorf("delete release attachments failed: %w", err) - } + + if _, err := repo_model.MarkAttachmentsDeleted(ctx, releaseAttachments); err != nil { + return fmt.Errorf("delete release attachments: %w", err) } 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 { @@ -200,9 +195,8 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams } // Delete Issues and related objects - // attachments will be deleted later with repo_id - postTxActions, err := issue_service.DeleteIssuesByRepoID(ctx, repoID, false) - if err != nil { + // attachments will be deleted later with repo_id, so we don't need to delete them here + if _, err := issue_service.DeleteIssuesByRepoID(ctx, repoID, false); err != nil { return err } @@ -282,8 +276,7 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams }).Find(&repoAttachments); err != nil { return err } - - if _, err := sess.Where("repo_id=?", repo.ID).Delete(new(repo_model.Attachment)); err != nil { + if _, err := repo_model.MarkAttachmentsDeleted(ctx, repoAttachments); err != nil { return err } @@ -298,7 +291,8 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams committer.Close() - postTxActions() + attachment_service.CleanAttachments(ctx, releaseAttachments) + attachment_service.CleanAttachments(ctx, repoAttachments) if needRewriteKeysFile { if err := asymkey_service.RewriteAllPublicKeys(ctx); err != nil { diff --git a/services/user/delete.go b/services/user/delete.go index b88d1fa54b7..af287a074a3 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -5,9 +5,7 @@ package user import ( "context" - "errors" "fmt" - "os" "time" _ "image/jpeg" // Needed for jpeg support @@ -24,17 +22,14 @@ import ( pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/storage" - "code.gitea.io/gitea/modules/util" "xorm.io/builder" ) // deleteUser deletes models associated to an user. -func deleteUser(ctx context.Context, u *user_model.User, purge bool) (postTxActions util.PostTxAction, err error) { - postTxActions = util.NewPostTxAction() +func deleteUser(ctx context.Context, u *user_model.User, purge bool) (toBeCleanedAttachments []*repo_model.Attachment, err error) { + toBeCleanedAttachments = make([]*repo_model.Attachment, 0) // ***** START: Watch ***** watchedRepoIDs, err := db.FindIDs(ctx, "watch", "watch.repo_id", @@ -131,25 +126,10 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (postTxActi return nil, err } - if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments); err != nil { + if _, err := repo_model.MarkAttachmentsDeleted(ctx, comment.Attachments); err != nil { return nil, err } - - postTxActions = postTxActions.Append(func() { - for _, a := range comment.Attachments { - if err := storage.Attachments.Delete(a.RelativePath()); err != nil { - if !errors.Is(err, os.ErrNotExist) { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", a.UUID, err) - } else { - log.Warn("Attachment file not found when deleting: %s", a.RelativePath()) - } - } - } - }) + toBeCleanedAttachments = append(toBeCleanedAttachments, comment.Attachments...) } } @@ -227,5 +207,5 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (postTxActi return nil, fmt.Errorf("delete: %w", err) } - return postTxActions, nil + return toBeCleanedAttachments, nil } diff --git a/services/user/user.go b/services/user/user.go index 26d7b11cdcc..7f43d3aa6d5 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/agit" asymkey_service "code.gitea.io/gitea/services/asymkey" + attachment_service "code.gitea.io/gitea/services/attachment" org_service "code.gitea.io/gitea/services/org" "code.gitea.io/gitea/services/packages" container_service "code.gitea.io/gitea/services/packages/container" @@ -243,7 +244,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { return packages_model.ErrUserOwnPackages{UID: u.ID} } - postTxActions, err := deleteUser(ctx, u, purge) + toBeCleanedAttachments, err := deleteUser(ctx, u, purge) if err != nil { return fmt.Errorf("DeleteUser: %w", err) } @@ -253,7 +254,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { } _ = committer.Close() - postTxActions() + attachment_service.CleanAttachments(ctx, toBeCleanedAttachments) if err = asymkey_service.RewriteAllPublicKeys(ctx); err != nil { return err