From 80cea11be0b269f0ec44bc6f8f01816149176ce7 Mon Sep 17 00:00:00 2001 From: yuvrajangadsingh Date: Wed, 11 Mar 2026 00:43:33 +0530 Subject: [PATCH] refactor: drop junction table, add RepoID to Comment model - Remove commit_comment junction table, query Comment directly via repo_id + commit_sha + type filter - Add RepoID column to Comment (migration v326) - Add reqRepoCodeWriter permission checks to all commit comment routes - Fix delete auth: use CanWrite(TypeCode) instead of IsAdmin() - Fix CanComment: gate on write access, not read - Guard LoadIssue against IssueID=0 (commit comments have no issue) - Extract duplicated markdown rendering loop in commit.go - Fix template dict keys: use "root" instead of "." - Show delete button for users with code write access - Add unit tests for create, delete, get, find operations --- models/issues/comment.go | 4 + models/issues/commit_comment.go | 82 ++------- models/issues/commit_comment_test.go | 167 +++++++++++++++++++ models/migrations/migrations.go | 1 + models/migrations/v1_26/v326.go | 20 +-- models/migrations/v1_26/v331.go | 21 +++ routers/web/repo/commit.go | 19 +-- routers/web/repo/commit_comment.go | 12 +- routers/web/web.go | 6 +- templates/repo/diff/commit_comments.tmpl | 2 +- templates/repo/diff/commit_conversation.tmpl | 6 +- templates/repo/diff/section_split.tmpl | 4 +- templates/repo/diff/section_unified.tmpl | 4 +- 13 files changed, 244 insertions(+), 104 deletions(-) create mode 100644 models/issues/commit_comment_test.go create mode 100644 models/migrations/v1_26/v331.go diff --git a/models/issues/comment.go b/models/issues/comment.go index 1e326d8a21..eae61d803a 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -260,6 +260,7 @@ type Comment struct { OriginalAuthorID int64 IssueID int64 `xorm:"INDEX"` Issue *Issue `xorm:"-"` + RepoID int64 `xorm:"INDEX DEFAULT 0"` // used by commit comments (type=39) which don't have an issue LabelID int64 Label *Label `xorm:"-"` AddedLabels []*Label `xorm:"-"` @@ -351,6 +352,9 @@ func (c *Comment) LoadIssue(ctx context.Context) (err error) { if c.Issue != nil { return nil } + if c.IssueID == 0 { + return nil + } c.Issue, err = GetIssueByID(ctx, c.IssueID) return err } diff --git a/models/issues/commit_comment.go b/models/issues/commit_comment.go index 45d5b1bafb..ef39255714 100644 --- a/models/issues/commit_comment.go +++ b/models/issues/commit_comment.go @@ -9,20 +9,6 @@ import ( "code.gitea.io/gitea/models/db" ) -// CommitComment is a junction table linking a commit (repo + SHA) to -// a Comment entry. The comment content, tree_path, line, poster, etc. -// are stored in the Comment table with Type = CommentTypeCommitComment. -type CommitComment struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX NOT NULL"` - CommitSHA string `xorm:"VARCHAR(64) INDEX NOT NULL"` - CommentID int64 `xorm:"UNIQUE NOT NULL"` -} - -func init() { - db.RegisterModel(new(CommitComment)) -} - // FileCommitComments holds commit comments for a single file, // split by side (left = old, right = new) with int keys matching DiffLine indices. type FileCommitComments struct { @@ -35,20 +21,9 @@ type CommitCommentsForDiff map[string]*FileCommitComments // FindCommitCommentsByCommitSHA returns all comments for a given commit in a repo. func FindCommitCommentsByCommitSHA(ctx context.Context, repoID int64, commitSHA string) ([]*Comment, error) { - var commentIDs []int64 - if err := db.GetEngine(ctx).Cols("comment_id").Table("commit_comment"). - Where("repo_id = ? AND commit_sha = ?", repoID, commitSHA). - Find(&commentIDs); err != nil { - return nil, err - } - - if len(commentIDs) == 0 { - return nil, nil - } - - comments := make([]*Comment, 0, len(commentIDs)) + comments := make([]*Comment, 0) if err := db.GetEngine(ctx). - In("id", commentIDs). + Where("repo_id = ? AND commit_sha = ? AND type = ?", repoID, commitSHA, CommentTypeCommitComment). OrderBy("created_unix ASC"). Find(&comments); err != nil { return nil, err @@ -91,50 +66,27 @@ func FindCommitCommentsForDiff(ctx context.Context, repoID int64, commitSHA stri return result, nil } -// CreateCommitComment creates a Comment with type CommitComment and a -// corresponding CommitComment junction record, within a transaction. -func CreateCommitComment(ctx context.Context, repoID int64, commitSHA string, comment *Comment) error { - return db.WithTx(ctx, func(ctx context.Context) error { - if _, err := db.GetEngine(ctx).Insert(comment); err != nil { - return err - } - - ref := &CommitComment{ - RepoID: repoID, - CommitSHA: commitSHA, - CommentID: comment.ID, - } - _, err := db.GetEngine(ctx).Insert(ref) - return err - }) +// CreateCommitComment creates a Comment with type CommitComment. +func CreateCommitComment(ctx context.Context, comment *Comment) error { + comment.Type = CommentTypeCommitComment + _, err := db.GetEngine(ctx).Insert(comment) + return err } -// DeleteCommitComment deletes both the junction record and the Comment entry. -func DeleteCommitComment(ctx context.Context, commentID int64) error { - return db.WithTx(ctx, func(ctx context.Context) error { - if _, err := db.GetEngine(ctx).Where("comment_id = ?", commentID).Delete(&CommitComment{}); err != nil { - return err - } - _, err := db.GetEngine(ctx).ID(commentID).Delete(&Comment{}) - return err - }) +// DeleteCommitComment deletes a commit comment by ID, verifying it belongs to the given repo. +func DeleteCommitComment(ctx context.Context, repoID, commentID int64) error { + _, err := db.GetEngine(ctx). + Where("id = ? AND repo_id = ? AND type = ?", commentID, repoID, CommentTypeCommitComment). + Delete(&Comment{}) + return err } -// GetCommitCommentByID returns a commit comment by loading the Comment entry, -// verifying it belongs to the given repository via the junction table. +// GetCommitCommentByID returns a commit comment by ID, verifying it belongs to the given repo. func GetCommitCommentByID(ctx context.Context, repoID, commentID int64) (*Comment, error) { - exists, err := db.GetEngine(ctx).Table("commit_comment"). - Where("repo_id = ? AND comment_id = ?", repoID, commentID). - Exist() - if err != nil { - return nil, err - } - if !exists { - return nil, db.ErrNotExist{Resource: "CommitComment", ID: commentID} - } - c := &Comment{} - has, err := db.GetEngine(ctx).ID(commentID).Get(c) + has, err := db.GetEngine(ctx). + Where("id = ? AND repo_id = ? AND type = ?", commentID, repoID, CommentTypeCommitComment). + Get(c) if err != nil { return nil, err } diff --git a/models/issues/commit_comment_test.go b/models/issues/commit_comment_test.go new file mode 100644 index 0000000000..af2e0e78d7 --- /dev/null +++ b/models/issues/commit_comment_test.go @@ -0,0 +1,167 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issues_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreateCommitComment(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + comment := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: "abc123def456", + TreePath: "README.md", + Line: 10, + Content: "looks good", + } + + err := issues_model.CreateCommitComment(t.Context(), comment) + require.NoError(t, err) + assert.Greater(t, comment.ID, int64(0)) + assert.Equal(t, issues_model.CommentTypeCommitComment, comment.Type) + + // Verify it's in the DB + loaded, err := issues_model.GetCommitCommentByID(t.Context(), 1, comment.ID) + require.NoError(t, err) + assert.Equal(t, "README.md", loaded.TreePath) + assert.Equal(t, int64(10), loaded.Line) + assert.Equal(t, "looks good", loaded.Content) + assert.Equal(t, int64(1), loaded.RepoID) +} + +func TestGetCommitCommentByID_WrongRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + comment := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: "abc123def456", + TreePath: "main.go", + Line: 5, + Content: "test", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), comment)) + + // Trying to load with wrong repo ID should fail + _, err := issues_model.GetCommitCommentByID(t.Context(), 999, comment.ID) + assert.True(t, db.IsErrNotExist(err)) +} + +func TestDeleteCommitComment(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + comment := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: "abc123def456", + TreePath: "README.md", + Line: 10, + Content: "to be deleted", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), comment)) + + err := issues_model.DeleteCommitComment(t.Context(), 1, comment.ID) + require.NoError(t, err) + + // Verify it's gone + _, err = issues_model.GetCommitCommentByID(t.Context(), 1, comment.ID) + assert.True(t, db.IsErrNotExist(err)) +} + +func TestDeleteCommitComment_WrongRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + comment := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: "abc123def456", + TreePath: "README.md", + Line: 10, + Content: "should not be deleted", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), comment)) + + // Delete with wrong repo should not actually delete + err := issues_model.DeleteCommitComment(t.Context(), 999, comment.ID) + require.NoError(t, err) + + // Verify it's still there + loaded, err := issues_model.GetCommitCommentByID(t.Context(), 1, comment.ID) + require.NoError(t, err) + assert.Equal(t, comment.ID, loaded.ID) +} + +func TestFindCommitCommentsByCommitSHA(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + sha := "deadbeef1234" + // Create two comments on same commit, different lines + for _, line := range []int64{5, -10} { + c := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: sha, + TreePath: "main.go", + Line: line, + Content: "comment", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), c)) + } + + comments, err := issues_model.FindCommitCommentsByCommitSHA(t.Context(), 1, sha) + require.NoError(t, err) + assert.Len(t, comments, 2) + + // Different repo should return empty + comments, err = issues_model.FindCommitCommentsByCommitSHA(t.Context(), 999, sha) + require.NoError(t, err) + assert.Empty(t, comments) +} + +func TestFindCommitCommentsForDiff(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + sha := "cafebabe5678" + // Left side comment (negative line = old/previous side) + c1 := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: sha, + TreePath: "file.go", + Line: -3, + Content: "old side", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), c1)) + + // Right side comment (positive line = new/proposed side) + c2 := &issues_model.Comment{ + PosterID: 2, + RepoID: 1, + CommitSHA: sha, + TreePath: "file.go", + Line: 7, + Content: "new side", + } + require.NoError(t, issues_model.CreateCommitComment(t.Context(), c2)) + + result, err := issues_model.FindCommitCommentsForDiff(t.Context(), 1, sha) + require.NoError(t, err) + + fcc, ok := result["file.go"] + require.True(t, ok) + assert.Len(t, fcc.Left[3], 1) + assert.Equal(t, "old side", fcc.Left[3][0].Content) + assert.Len(t, fcc.Right[7], 1) + assert.Equal(t, "new side", fcc.Right[7][0].Content) +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index cad4156dee..0c6ea60418 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -405,6 +405,7 @@ func prepareMigrationTasks() []*migration { newMigration(328, "Add TokenPermissions column to ActionRunJob", v1_26.AddTokenPermissionsToActionRunJob), newMigration(329, "Add unique constraint for user badge", v1_26.AddUniqueIndexForUserBadge), newMigration(330, "Add name column to webhook", v1_26.AddNameToWebhook), + newMigration(331, "Add repo_id column to comment table for commit comments", v1_26.AddCommitCommentColumns), } return preparedMigrations } diff --git a/models/migrations/v1_26/v326.go b/models/migrations/v1_26/v326.go index 3c3a751aa7..5fdd62758f 100644 --- a/models/migrations/v1_26/v326.go +++ b/models/migrations/v1_26/v326.go @@ -7,17 +7,15 @@ import ( "xorm.io/xorm" ) -func AddCommitCommentTable(x *xorm.Engine) error { - // CommitComment is a junction table that maps commit-specific context - // (repo, commit SHA) to a Comment entry. The actual comment content, - // tree_path, line, poster, etc. live in the Comment table with - // type = CommentTypeCommitComment (39). - type CommitComment struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX NOT NULL"` - CommitSHA string `xorm:"VARCHAR(64) INDEX NOT NULL"` - CommentID int64 `xorm:"UNIQUE NOT NULL"` +func AddCommitCommentColumns(x *xorm.Engine) error { + // Add RepoID column to the comment table for commit comments. + // Commit comments (type=39) store repo_id directly on the comment + // instead of deriving it through issue_id, since they don't belong + // to any issue. Combined with the existing commit_sha column, this + // allows querying commit comments without a junction table. + type Comment struct { + RepoID int64 `xorm:"INDEX DEFAULT 0"` } - return x.Sync(new(CommitComment)) + return x.Sync(new(Comment)) } diff --git a/models/migrations/v1_26/v331.go b/models/migrations/v1_26/v331.go new file mode 100644 index 0000000000..5fdd62758f --- /dev/null +++ b/models/migrations/v1_26/v331.go @@ -0,0 +1,21 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_26 + +import ( + "xorm.io/xorm" +) + +func AddCommitCommentColumns(x *xorm.Engine) error { + // Add RepoID column to the comment table for commit comments. + // Commit comments (type=39) store repo_id directly on the comment + // instead of deriving it through issue_id, since they don't belong + // to any issue. Combined with the existing commit_sha column, this + // allows querying commit comments without a junction table. + type Comment struct { + RepoID int64 `xorm:"INDEX DEFAULT 0"` + } + + return x.Sync(new(Comment)) +} diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 4a9a1b2b76..d33c6da5bc 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -431,17 +431,8 @@ func Diff(ctx *context.Context) { log.Error("FindCommitCommentsForDiff: %v", err) } // Render markdown content for each commit comment - for _, fcc := range commitComments { - for _, comments := range fcc.Left { - for _, c := range comments { - rctx := renderhelper.NewRenderContextRepoComment(ctx, ctx.Repo.Repository, renderhelper.RepoCommentOptions{}) - c.RenderedContent, err = markdown.RenderString(rctx, c.Content) - if err != nil { - log.Error("RenderString for commit comment %d: %v", c.ID, err) - } - } - } - for _, comments := range fcc.Right { + renderCommitComments := func(commentsByLine map[int][]*issues_model.Comment) { + for _, comments := range commentsByLine { for _, c := range comments { rctx := renderhelper.NewRenderContextRepoComment(ctx, ctx.Repo.Repository, renderhelper.RepoCommentOptions{}) c.RenderedContent, err = markdown.RenderString(rctx, c.Content) @@ -451,8 +442,12 @@ func Diff(ctx *context.Context) { } } } + for _, fcc := range commitComments { + renderCommitComments(fcc.Left) + renderCommitComments(fcc.Right) + } ctx.Data["CommitComments"] = commitComments - ctx.Data["CanComment"] = ctx.Doer != nil && ctx.Repo.CanRead(unit_model.TypeCode) + ctx.Data["CanComment"] = ctx.Doer != nil && ctx.Repo.CanWrite(unit_model.TypeCode) ctx.HTML(http.StatusOK, tplCommitPage) } diff --git a/routers/web/repo/commit_comment.go b/routers/web/repo/commit_comment.go index a3761daaf3..5b17341597 100644 --- a/routers/web/repo/commit_comment.go +++ b/routers/web/repo/commit_comment.go @@ -8,6 +8,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/renderhelper" + unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup/markdown" @@ -94,9 +95,9 @@ func CreateCommitComment(ctx *context.Context) { } comment := &issues_model.Comment{ - Type: issues_model.CommentTypeCommitComment, PosterID: ctx.Doer.ID, Poster: ctx.Doer, + RepoID: ctx.Repo.Repository.ID, CommitSHA: fullSHA, TreePath: treePath, Line: line, @@ -104,7 +105,7 @@ func CreateCommitComment(ctx *context.Context) { Patch: patch, } - if err := issues_model.CreateCommitComment(ctx, ctx.Repo.Repository.ID, fullSHA, comment); err != nil { + if err := issues_model.CreateCommitComment(ctx, comment); err != nil { ctx.ServerError("CreateCommitComment", err) return } @@ -135,12 +136,13 @@ func DeleteCommitComment(ctx *context.Context) { return } - if comment.PosterID != ctx.Doer.ID && !ctx.Repo.IsAdmin() { - ctx.JSONError("permission denied") + // Allow deletion by the comment author or anyone with write access to code + if comment.PosterID != ctx.Doer.ID && !ctx.Repo.CanWrite(unit_model.TypeCode) { + ctx.HTTPError(http.StatusForbidden) return } - if err := issues_model.DeleteCommitComment(ctx, commentID); err != nil { + if err := issues_model.DeleteCommitComment(ctx, ctx.Repo.Repository.ID, commentID); err != nil { ctx.ServerError("DeleteCommitComment", err) return } diff --git a/routers/web/web.go b/routers/web/web.go index 01cc33814f..f7086f3fbc 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1685,9 +1685,9 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Get("/graph", repo.Graph) m.Get("/commit/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) m.Get("/commit/{sha:([a-f0-9]{7,64})$}/load-branches-and-tags", repo.LoadBranchesAndTags) - m.Get("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, repo.RenderNewCommitCommentForm) - m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, context.RepoMustNotBeArchived(), repo.CreateCommitComment) - m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment/{id}/delete", reqSignIn, context.RepoMustNotBeArchived(), repo.DeleteCommitComment) + m.Get("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, reqRepoCodeWriter, repo.RenderNewCommitCommentForm) + m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, reqRepoCodeWriter, context.RepoMustNotBeArchived(), repo.CreateCommitComment) + m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment/{id}/delete", reqSignIn, reqRepoCodeWriter, context.RepoMustNotBeArchived(), repo.DeleteCommitComment) // FIXME: this route `/cherry-pick/{sha}` doesn't seem useful or right, the new code always uses `/_cherrypick/` which could handle branch name correctly m.Get("/cherry-pick/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, context.RepoRefByDefaultBranch(), repo.CherryPick) diff --git a/templates/repo/diff/commit_comments.tmpl b/templates/repo/diff/commit_comments.tmpl index 64dc7a1763..49763cd442 100644 --- a/templates/repo/diff/commit_comments.tmpl +++ b/templates/repo/diff/commit_comments.tmpl @@ -14,7 +14,7 @@
- {{if and $.root.IsSigned (eq $.root.SignedUserID .PosterID)}} + {{if and $.root.IsSigned (or (eq $.root.SignedUserID .PosterID) $.root.CanWriteCode)}}
{{svg "octicon-trash" 14}}
diff --git a/templates/repo/diff/commit_conversation.tmpl b/templates/repo/diff/commit_conversation.tmpl index 9577091693..3ace4c70ea 100644 --- a/templates/repo/diff/commit_conversation.tmpl +++ b/templates/repo/diff/commit_conversation.tmpl @@ -4,7 +4,7 @@
- {{template "repo/diff/commit_comments" dict "root" $ "comments" .comments}} + {{template "repo/diff/commit_comments" dict "root" $.root "comments" .comments}}
@@ -17,8 +17,8 @@
- {{if and $.SignedUserID (not $.Repository.IsArchived)}} - {{template "repo/diff/commit_comment_form" dict "hidden" true "root" $}} + {{if and $.root.SignedUserID (not $.root.Repository.IsArchived)}} + {{template "repo/diff/commit_comment_form" dict "hidden" true "root" $.root}} {{end}}
diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index f892a817b7..d53a8577e6 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -150,12 +150,12 @@ {{if $leftCC}} - {{template "repo/diff/commit_conversation" dict "." $.root "comments" $leftCC}} + {{template "repo/diff/commit_conversation" dict "root" $.root "comments" $leftCC}} {{end}} {{if $rightCC}} - {{template "repo/diff/commit_conversation" dict "." $.root "comments" $rightCC}} + {{template "repo/diff/commit_conversation" dict "root" $.root "comments" $rightCC}} {{end}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index cda07c5de0..4787143f1d 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -62,10 +62,10 @@ {{if $rightCC}} - {{template "repo/diff/commit_conversation" dict "." $.root "comments" $rightCC}} + {{template "repo/diff/commit_conversation" dict "root" $.root "comments" $rightCC}} {{end}} {{if $leftCC}} - {{template "repo/diff/commit_conversation" dict "." $.root "comments" $leftCC}} + {{template "repo/diff/commit_conversation" dict "root" $.root "comments" $leftCC}} {{end}}