Fix uploading comment attachments haven't updated the database columns issue_id and comment_id

models/issues/comment.go

@@ -577,28 +577,6 @@ func (c *Comment) LoadAttachments(ctx context.Context) error {
 	return nil
 }
 
-// UpdateAttachments update attachments by UUIDs for the comment
-func (c *Comment) UpdateAttachments(ctx context.Context, uuids []string) error {
-	ctx, committer, err := db.TxContext(ctx)
-	if err != nil {
-		return err
-	}
-	defer committer.Close()
-
-	attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids)
-	if err != nil {
-		return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
-	}
-	for i := 0; i < len(attachments); i++ {
-		attachments[i].IssueID = c.IssueID
-		attachments[i].CommentID = c.ID
-		if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
-			return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)
-		}
-	}
-	return committer.Commit()
-}
-
 // LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees
 func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) error {
 	var err error
@@ -872,6 +850,9 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 		}
 		fallthrough
 	case CommentTypeComment:
+		if err = updateAttachments(ctx, opts, comment); err != nil {
+			return err
+		}
 		if _, err = db.Exec(ctx, "UPDATE `issue` SET num_comments=num_comments+1 WHERE id=?", opts.Issue.ID); err != nil {
 			return err
 		}
@@ -897,8 +878,7 @@ func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment
 	for i := range attachments {
 		attachments[i].IssueID = opts.Issue.ID
 		attachments[i].CommentID = comment.ID
-		// No assign value could be 0, so ignore AllCols().
-		if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil {
+		if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("issue_id", "comment_id").Update(attachments[i]); err != nil {
 			return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err)
 		}
 	}
@@ -1126,6 +1106,7 @@ func UpdateComment(ctx context.Context, c *Comment, doer *user_model.User) error
 	if _, err := sess.ID(c.ID).AllCols().Update(c); err != nil {
 		return err
 	}
+
 	if err := c.LoadIssue(ctx); err != nil {
 		return err
 	}

models/issues/comment_attach.go

@@ -0,0 +1,44 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package issues
+
+import (
+	"context"
+	"fmt"
+
+	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
+)
+
+// UpdateAttachments update attachments by UUIDs for the comment
+func UpdateAttachments(ctx context.Context, c *Comment, uuids []string) error {
+	ctx, committer, err := db.TxContext(ctx)
+	if err != nil {
+		return err
+	}
+	defer committer.Close()
+
+	attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids)
+	if err != nil {
+		return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
+	}
+	for i := 0; i < len(attachments); i++ {
+		attachments[i].IssueID = c.IssueID
+		attachments[i].CommentID = c.ID
+		if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
+			return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)
+		}
+	}
+	return committer.Commit()
+}
+
+// ClearCommentAttaches clear attachments' comment information by UUIDs for the comment
+func ClearCommentAttaches(ctx context.Context, comment *Comment, uuids []string) error {
+	_, err := db.GetEngine(ctx).
+		Where("issue_id = ?", comment.IssueID).
+		And("comment_id = ?", comment.ID).
+		In("uuid", uuids).
+		Cols("issue_id", "comment_id").Update(&repo_model.Attachment{})
+	return err
+}

models/issues/issue_update.go

@@ -282,7 +282,6 @@ type NewIssueOptions struct {
 
 // NewIssueWithIndex creates issue with given index
 func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssueOptions) (err error) {
-	e := db.GetEngine(ctx)
 	opts.Issue.Title = strings.TrimSpace(opts.Issue.Title)
 
 	if opts.Issue.MilestoneID > 0 {
@@ -306,7 +305,7 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue
 		return fmt.Errorf("issue exist")
 	}
 
-	if _, err := e.Insert(opts.Issue); err != nil {
+	if err := db.Insert(ctx, opts.Issue); err != nil {
 		return err
 	}
 
@@ -336,7 +335,7 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue
 		// During the session, SQLite3 driver cannot handle retrieve objects after update something.
 		// So we have to get all needed labels first.
 		labels := make([]*Label, 0, len(opts.LabelIDs))
-		if err = e.In("id", opts.LabelIDs).Find(&labels); err != nil {
+		if err = db.GetEngine(ctx).In("id", opts.LabelIDs).Find(&labels); err != nil {
 			return fmt.Errorf("find all labels [label_ids: %v]: %w", opts.LabelIDs, err)
 		}
 
@@ -368,8 +367,8 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue
 
 		for i := 0; i < len(attachments); i++ {
 			attachments[i].IssueID = opts.Issue.ID
-			if _, err = e.ID(attachments[i].ID).Update(attachments[i]); err != nil {
-				return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)
+			if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Cols("issue_id").Update(attachments[i]); err != nil {
+				return fmt.Errorf("update attachment issue_id [id: %d]: %w", attachments[i].ID, err)
 			}
 		}
 	}

modules/markup/markdown/attachment.go

@@ -0,0 +1,65 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package markdown
+
+import (
+	"strings"
+
+	"code.gitea.io/gitea/modules/container"
+
+	"github.com/yuin/goldmark"
+	"github.com/yuin/goldmark/ast"
+	"github.com/yuin/goldmark/text"
+	"golang.org/x/net/html"
+)
+
+func ParseAttachments(content string) (container.Set[string], error) {
+	parser := goldmark.DefaultParser()
+	rd := text.NewReader([]byte(content))
+	doc := parser.Parse(rd)
+	var attachments []string
+	if err := ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
+		if !entering {
+			return ast.WalkContinue, nil
+		}
+		switch b := n.(type) {
+		case *ast.HTMLBlock:
+			var tag string
+			for i := 0; i < b.Lines().Len(); i++ {
+				p := content[b.Lines().At(i).Start:b.Lines().At(i).Stop]
+				tag += p
+			}
+			doc, err := html.Parse(strings.NewReader(strings.TrimSpace(tag)))
+			if err != nil {
+				return ast.WalkStop, err
+			}
+
+			var processAllImgs func(*html.Node)
+			processAllImgs = func(n *html.Node) {
+				if n.Type == html.ElementNode && n.Data == "img" {
+					for _, a := range n.Attr {
+						if a.Key == "src" && strings.HasPrefix(a.Val, "/attachments/") {
+							attachments = append(attachments, strings.TrimPrefix(a.Val, "/attachments/"))
+							return
+						}
+					}
+				}
+				// traverse the child nodes
+				for c := n.FirstChild; c != nil; c = c.NextSibling {
+					processAllImgs(c)
+				}
+			}
+			// make a recursive call to your function
+			processAllImgs(doc)
+		case *ast.Image:
+			if strings.HasPrefix(string(b.Destination), "/attachments/") {
+				attachments = append(attachments, strings.TrimPrefix(string(b.Destination), "/attachments/"))
+			}
+		}
+		return ast.WalkContinue, nil
+	}); err != nil {
+		return nil, err
+	}
+	return container.SetOf(attachments...), nil
+}

modules/markup/markdown/attachment_test.go

@@ -0,0 +1,30 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package markdown
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/modules/container"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_ParseAttachments(t *testing.T) {
+	attachments, err := ParseAttachments(`
+# This is a test
+![test](/attachments/08058e33-d916-432a-88b9-5f616cfe9f01)
+
+## free
+
+<img width="318" alt="image" src="/attachments/08058e33-d916-432a-88b9-5f616cfe9f00">
+`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.EqualValues(t, container.Set[string]{
+		"08058e33-d916-432a-88b9-5f616cfe9f01": struct{}{},
+		"08058e33-d916-432a-88b9-5f616cfe9f00": struct{}{},
+	}, attachments)
+}

routers/api/v1/repo/issue_comment.go

@@ -615,7 +615,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
 
 	oldContent := comment.Content
 	comment.Content = form.Body
-	if err := issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
+	if err := issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent, nil, true); err != nil {
 		if errors.Is(err, user_model.ErrBlockedUser) {
 			ctx.Error(http.StatusForbidden, "UpdateComment", err)
 		} else {

routers/api/v1/repo/issue_comment_attachment.go

@@ -202,7 +202,7 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
 		return
 	}
 
-	if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, comment.Content); err != nil {
+	if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, comment.Content, []string{attachment.UUID}, false); err != nil {
 		if errors.Is(err, user_model.ErrBlockedUser) {
 			ctx.Error(http.StatusForbidden, "UpdateComment", err)
 		} else {

routers/web/repo/issue.go

@@ -3162,7 +3162,7 @@ func UpdateCommentContent(ctx *context.Context) {
 		})
 		return
 	}
-	if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
+	if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent, ctx.FormStrings("files[]"), ctx.FormBool("ignore_attachments")); err != nil {
 		if errors.Is(err, user_model.ErrBlockedUser) {
 			ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
 		} else {
@@ -3176,14 +3176,6 @@ func UpdateCommentContent(ctx *context.Context) {
 		return
 	}
 
-	// when the update request doesn't intend to update attachments (eg: change checkbox state), ignore attachment updates
-	if !ctx.FormBool("ignore_attachments") {
-		if err := updateAttachments(ctx, comment, ctx.FormStrings("files[]")); err != nil {
-			ctx.ServerError("UpdateAttachments", err)
-			return
-		}
-	}
-
 	content, err := markdown.RenderString(&markup.RenderContext{
 		Links: markup.Links{
 			Base: ctx.FormString("context"), // FIXME: <- IS THIS SAFE ?
@@ -3550,7 +3542,7 @@ func updateAttachments(ctx *context.Context, item any, files []string) error {
 		case *issues_model.Issue:
 			err = issues_model.UpdateIssueAttachments(ctx, content.ID, files)
 		case *issues_model.Comment:
-			err = content.UpdateAttachments(ctx, files)
+			err = issues_model.UpdateAttachments(ctx, content, files)
 		default:
 			return fmt.Errorf("unknown Type: %T", content)
 		}

services/issue/comments.go

@@ -12,6 +12,8 @@ import (
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/container"
+	"code.gitea.io/gitea/modules/markup/markdown"
 	"code.gitea.io/gitea/modules/timeutil"
 	notify_service "code.gitea.io/gitea/services/notify"
 )
@@ -81,8 +83,22 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
 	return comment, nil
 }
 
+func getDiffAttaches(oldAttaches, newAttaches container.Set[string]) (addedAttaches, delAttaches container.Set[string]) {
+	for attach := range newAttaches {
+		if !oldAttaches.Contains(attach) {
+			addedAttaches.Add(attach)
+		}
+	}
+	for attach := range oldAttaches {
+		if !newAttaches.Contains(attach) {
+			delAttaches.Add(attach)
+		}
+	}
+	return addedAttaches, delAttaches
+}
+
 // UpdateComment updates information of comment.
-func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_model.User, oldContent string) error {
+func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_model.User, oldContent string, attachments []string, ignoreUpdateAttachments bool) error {
 	if err := c.LoadIssue(ctx); err != nil {
 		return err
 	}
@@ -110,15 +126,54 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_mode
 		}
 	}
 
-	if err := issues_model.UpdateComment(ctx, c, doer); err != nil {
-		return err
-	}
-
-	if needsContentHistory {
-		err := issues_model.SaveIssueContentHistory(ctx, doer.ID, c.IssueID, c.ID, timeutil.TimeStampNow(), c.Content, false)
-		if err != nil {
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		if err := issues_model.UpdateComment(ctx, c, doer); err != nil {
 			return err
 		}
+
+		if ignoreUpdateAttachments {
+			oldAttaches, err := markdown.ParseAttachments(oldContent)
+			if err != nil {
+				return err
+			}
+
+			newAttaches, err := markdown.ParseAttachments(c.Content)
+			if err != nil {
+				return err
+			}
+
+			addedAttaches, delAttaches := getDiffAttaches(oldAttaches, newAttaches)
+			for attach := range addedAttaches {
+				attachments = append(attachments, attach)
+			}
+
+			if len(delAttaches) > 0 {
+				if err := issues_model.ClearCommentAttaches(ctx, c, delAttaches.Values()); err != nil {
+					return err
+				}
+			}
+
+			// update all these attachments's issue_id or comment_id if it's zero
+			// warning don't update those attachments with issue_id and comment_id not zero
+			// because those maybe copied from other respositories.
+			if _, err := db.GetEngine(ctx).Where("issue_id = 0 AND comment_id = 0").
+				In("uuid", attachments).Cols("issue_id, comment_id").Update(&repo_model.Attachment{
+				IssueID:   c.IssueID,
+				CommentID: c.ID,
+			}); err != nil {
+				return err
+			}
+		}
+
+		if needsContentHistory {
+			err := issues_model.SaveIssueContentHistory(ctx, doer.ID, c.IssueID, c.ID, timeutil.TimeStampNow(), c.Content, false)
+			if err != nil {
+				return err
+			}
+		}
+		return nil
+	}); err != nil {
+		return err
 	}
 
 	notify_service.UpdateComment(ctx, doer, c, oldContent)