Skip to content

Commit 792ea5f

Browse files
metiftikcibartvdbraak
metiftikci
authored and
bartvdbraak
committed
BLENDER: Prevent simultaneous editing of comments and issues
This backports upstream 31053. This commit can be removed when upgrading to the next Gitea function that contains this.
1 parent ace3c88 commit 792ea5f

File tree

21 files changed

+176
-28
lines changed

21 files changed

+176
-28
lines changed

‎models/issues/comment.go

+11-2
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ func (err ErrCommentNotExist) Unwrap() error {
5252
return util.ErrNotExist
5353
}
5454

55+
var ErrCommentAlreadyChanged = util.NewInvalidArgumentErrorf("the comment is already changed")
56+
5557
// CommentType defines whether a comment is just a simple comment, an action (like close) or a reference.
5658
type CommentType int
5759

@@ -262,6 +264,7 @@ type Comment struct {
262264
Line int64 // - previous line / + proposed line
263265
TreePath string
264266
Content string `xorm:"LONGTEXT"`
267+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
265268
RenderedContent template.HTML `xorm:"-"`
266269

267270
// Path represents the 4 lines of code cemented by this comment
@@ -1111,17 +1114,23 @@ func UpdateCommentInvalidate(ctx context.Context, c *Comment) error {
11111114
}
11121115

11131116
// UpdateComment updates information of comment.
1114-
func UpdateComment(ctx context.Context, c *Comment, doer *user_model.User) error {
1117+
func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *user_model.User) error {
11151118
ctx, committer, err := db.TxContext(ctx)
11161119
if err != nil {
11171120
return err
11181121
}
11191122
defer committer.Close()
11201123
sess := db.GetEngine(ctx)
11211124

1122-
if _, err := sess.ID(c.ID).AllCols().Update(c); err != nil {
1125+
c.ContentVersion = contentVersion + 1
1126+
1127+
affected, err := sess.ID(c.ID).AllCols().Where("content_version = ?", contentVersion).Update(c)
1128+
if err != nil {
11231129
return err
11241130
}
1131+
if affected == 0 {
1132+
return ErrCommentAlreadyChanged
1133+
}
11251134
if err := c.LoadIssue(ctx); err != nil {
11261135
return err
11271136
}

‎models/issues/issue.go

+3
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ func (err ErrIssueWasClosed) Error() string {
9494
return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
9595
}
9696

97+
var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed")
98+
9799
// Issue represents an issue or pull request of repository.
98100
type Issue struct {
99101
ID int64 `xorm:"pk autoincr"`
@@ -107,6 +109,7 @@ type Issue struct {
107109
Title string `xorm:"name"`
108110
Content string `xorm:"LONGTEXT"`
109111
RenderedContent template.HTML `xorm:"-"`
112+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
110113
Labels []*Label `xorm:"-"`
111114
MilestoneID int64 `xorm:"INDEX"`
112115
Milestone *Milestone `xorm:"-"`

‎models/issues/issue_update.go

+8-3
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string)
235235
}
236236

237237
// ChangeIssueContent changes issue content, as the given user.
238-
func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string) (err error) {
238+
func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string, contentVersion int) (err error) {
239239
ctx, committer, err := db.TxContext(ctx)
240240
if err != nil {
241241
return err
@@ -254,9 +254,14 @@ func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User
254254
}
255255

256256
issue.Content = content
257+
issue.ContentVersion = contentVersion + 1
257258

258-
if err = UpdateIssueCols(ctx, issue, "content"); err != nil {
259-
return fmt.Errorf("UpdateIssueCols: %w", err)
259+
affected, err := db.GetEngine(ctx).ID(issue.ID).Cols("content", "content_version").Where("content_version = ?", contentVersion).Update(issue)
260+
if err != nil {
261+
return err
262+
}
263+
if affected == 0 {
264+
return ErrIssueAlreadyChanged
260265
}
261266

262267
if err = SaveIssueContentHistory(ctx, doer.ID, issue.ID, 0,

‎models/migrations/migrations.go

+8-1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"code.gitea.io/gitea/models/migrations/v1_20"
2222
"code.gitea.io/gitea/models/migrations/v1_21"
2323
"code.gitea.io/gitea/models/migrations/v1_22"
24+
"code.gitea.io/gitea/models/migrations/v1_23"
2425
"code.gitea.io/gitea/models/migrations/v1_6"
2526
"code.gitea.io/gitea/models/migrations/v1_7"
2627
"code.gitea.io/gitea/models/migrations/v1_8"
@@ -70,6 +71,12 @@ type Version struct {
7071
// Use noopMigration when there is a migration that has been no-oped
7172
var noopMigration = func(_ *xorm.Engine) error { return nil }
7273

74+
// BLENDER: hack to backport feature without going to 1.23 table
75+
func BLENDER_v122_migration(x *xorm.Engine) error {
76+
v1_22.DropWronglyCreatedTable(x)
77+
return v1_23.AddContentVersionToIssueAndComment(x)
78+
}
79+
7380
// This is a sequence of migrations. Add new migrations to the bottom of the list.
7481
// If you want to "retire" a migration, remove it from the top of the list and
7582
// update minDBVersion accordingly
@@ -584,7 +591,7 @@ var migrations = []Migration{
584591
// v297 -> v298
585592
NewMigration("Add everyone_access_mode for repo_unit", v1_22.AddRepoUnitEveryoneAccessMode),
586593
// v298 -> v299
587-
NewMigration("Drop wrongly created table o_auth2_application", v1_22.DropWronglyCreatedTable),
594+
NewMigration("Drop wrongly created table o_auth2_application", BLENDER_v122_migration),
588595

589596
// Gitea 1.22.0-rc1 ends at 299
590597
}

‎models/migrations/v1_23/v299.go

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package v1_23 //nolint
5+
6+
import "xorm.io/xorm"
7+
8+
func AddContentVersionToIssueAndComment(x *xorm.Engine) error {
9+
type Issue struct {
10+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
11+
}
12+
13+
type Comment struct {
14+
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
15+
}
16+
17+
return x.Sync(new(Comment), new(Issue))
18+
}

‎options/locale/locale_en-US.ini

+4
Original file line numberDiff line numberDiff line change
@@ -1443,6 +1443,7 @@ issues.new.clear_assignees = Clear assignees
14431443
issues.new.no_assignees = No Assignees
14441444
issues.new.no_reviewers = No reviewers
14451445
issues.new.blocked_user = Cannot create issue because you are blocked by the repository owner.
1446+
issues.edit.already_changed = Unable to save changes to the issue. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
14461447
issues.edit.blocked_user = Cannot edit content because you are blocked by the poster or repository owner.
14471448
issues.choose.get_started = Get Started
14481449
issues.choose.open_external_link = Open
@@ -1758,6 +1759,7 @@ compare.compare_head = compare
17581759
pulls.desc = Enable pull requests and code reviews.
17591760
pulls.new = New Pull Request
17601761
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
1762+
pulls.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
17611763
pulls.view = View Pull Request
17621764
pulls.compare_changes = New Pull Request
17631765
pulls.allow_edits_from_maintainers = Allow edits from maintainers
@@ -1903,6 +1905,8 @@ pulls.recently_pushed_new_branches = You pushed on branch <strong>%[1]s</strong>
19031905

19041906
pull.deleted_branch = (deleted):%s
19051907

1908+
comments.edit.already_changed = Unable to save changes to the comment. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
1909+
19061910
milestones.new = New Milestone
19071911
milestones.closed = Closed %s
19081912
milestones.update_ago = Updated %s

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

+6-1
Original file line numberDiff line numberDiff line change
@@ -810,8 +810,13 @@ func EditIssue(ctx *context.APIContext) {
810810
}
811811
}
812812
if form.Body != nil {
813-
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body)
813+
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
814814
if err != nil {
815+
if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
816+
ctx.Error(http.StatusBadRequest, "ChangeContent", err)
817+
return
818+
}
819+
815820
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
816821
return
817822
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ func CreateIssueAttachment(ctx *context.APIContext) {
198198

199199
issue.Attachments = append(issue.Attachments, attachment)
200200

201-
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, issue.Content); err != nil {
201+
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, issue.Content, issue.ContentVersion); err != nil {
202202
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
203203
return
204204
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
611611

612612
oldContent := comment.Content
613613
comment.Content = form.Body
614-
if err := issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
614+
if err := issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, oldContent); err != nil {
615615
if errors.Is(err, user_model.ErrBlockedUser) {
616616
ctx.Error(http.StatusForbidden, "UpdateComment", err)
617617
} else {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
210210
return
211211
}
212212

213-
if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, comment.Content); err != nil {
213+
if err = issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, comment.Content); err != nil {
214214
if errors.Is(err, user_model.ErrBlockedUser) {
215215
ctx.Error(http.StatusForbidden, "UpdateComment", err)
216216
} else {

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

+6-1
Original file line numberDiff line numberDiff line change
@@ -610,8 +610,13 @@ func EditPullRequest(ctx *context.APIContext) {
610610
}
611611
}
612612
if form.Body != nil {
613-
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body)
613+
err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
614614
if err != nil {
615+
if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
616+
ctx.Error(http.StatusBadRequest, "ChangeContent", err)
617+
return
618+
}
619+
615620
ctx.Error(http.StatusInternalServerError, "ChangeContent", err)
616621
return
617622
}

‎routers/web/repo/issue.go

+17-6
Original file line numberDiff line numberDiff line change
@@ -2254,9 +2254,15 @@ func UpdateIssueContent(ctx *context.Context) {
22542254
return
22552255
}
22562256

2257-
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, ctx.Req.FormValue("content")); err != nil {
2257+
if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, ctx.Req.FormValue("content"), ctx.FormInt("content_version")); err != nil {
22582258
if errors.Is(err, user_model.ErrBlockedUser) {
22592259
ctx.JSONError(ctx.Tr("repo.issues.edit.blocked_user"))
2260+
} else if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
2261+
if issue.IsPull {
2262+
ctx.JSONError(ctx.Tr("repo.pulls.edit.already_changed"))
2263+
} else {
2264+
ctx.JSONError(ctx.Tr("repo.issues.edit.already_changed"))
2265+
}
22602266
} else {
22612267
ctx.ServerError("ChangeContent", err)
22622268
}
@@ -2285,8 +2291,9 @@ func UpdateIssueContent(ctx *context.Context) {
22852291
}
22862292

22872293
ctx.JSON(http.StatusOK, map[string]any{
2288-
"content": content,
2289-
"attachments": attachmentsHTML(ctx, issue.Attachments, issue.Content),
2294+
"content": content,
2295+
"contentVersion": issue.ContentVersion,
2296+
"attachments": attachmentsHTML(ctx, issue.Attachments, issue.Content),
22902297
})
22912298
}
22922299

@@ -3160,12 +3167,15 @@ func UpdateCommentContent(ctx *context.Context) {
31603167

31613168
oldContent := comment.Content
31623169
newContent := ctx.FormString("content")
3170+
contentVersion := ctx.FormInt("content_version")
31633171

31643172
// allow to save empty content
31653173
comment.Content = newContent
3166-
if err = issue_service.UpdateComment(ctx, comment, ctx.Doer, oldContent); err != nil {
3174+
if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil {
31673175
if errors.Is(err, user_model.ErrBlockedUser) {
31683176
ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
3177+
} else if errors.Is(err, issues_model.ErrCommentAlreadyChanged) {
3178+
ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed"))
31693179
} else {
31703180
ctx.ServerError("UpdateComment", err)
31713181
}
@@ -3205,8 +3215,9 @@ func UpdateCommentContent(ctx *context.Context) {
32053215
}
32063216

32073217
ctx.JSON(http.StatusOK, map[string]any{
3208-
"content": renderedContent,
3209-
"attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content),
3218+
"content": renderedContent,
3219+
"contentVersion": comment.ContentVersion,
3220+
"attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content),
32103221
})
32113222
}
32123223

‎services/issue/comments.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
8282
}
8383

8484
// UpdateComment updates information of comment.
85-
func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_model.User, oldContent string) error {
85+
func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion int, doer *user_model.User, oldContent string) error {
8686
if err := c.LoadIssue(ctx); err != nil {
8787
return err
8888
}
@@ -110,7 +110,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_mode
110110
}
111111
}
112112

113-
if err := issues_model.UpdateComment(ctx, c, doer); err != nil {
113+
if err := issues_model.UpdateComment(ctx, c, contentVersion, doer); err != nil {
114114
return err
115115
}
116116

‎services/issue/content.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
)
1414

1515
// ChangeContent changes issue content, as the given user.
16-
func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, content string) error {
16+
func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, content string, contentVersion int) error {
1717
if err := issue.LoadRepo(ctx); err != nil {
1818
return err
1919
}
@@ -26,7 +26,7 @@ func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_mo
2626

2727
oldContent := issue.Content
2828

29-
if err := issues_model.ChangeIssueContent(ctx, issue, doer, content); err != nil {
29+
if err := issues_model.ChangeIssueContent(ctx, issue, doer, content, contentVersion); err != nil {
3030
return err
3131
}
3232

‎templates/repo/diff/comments.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
{{end}}
6262
</div>
6363
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
64-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-context="{{$.root.RepoLink}}" data-attachment-url="{{$.root.RepoLink}}/comments/{{.ID}}/attachments"></div>
64+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.root.RepoLink}}" data-attachment-url="{{$.root.RepoLink}}/comments/{{.ID}}/attachments"></div>
6565
{{if .Attachments}}
6666
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
6767
{{end}}

‎templates/repo/issue/view_content.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
{{end}}
6161
</div>
6262
<div id="issue-{{.Issue.ID}}-raw" class="raw-content tw-hidden">{{.Issue.Content}}</div>
63-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/content" data-context="{{.RepoLink}}" data-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/attachments" data-view-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/view-attachments"></div>
63+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/content" data-content-version="{{.Issue.ContentVersion}}" data-context="{{.RepoLink}}" data-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/attachments" data-view-attachment-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/view-attachments"></div>
6464
{{if .Issue.Attachments}}
6565
{{template "repo/issue/view_content/attachments" dict "Attachments" .Issue.Attachments "RenderedContent" .Issue.RenderedContent}}
6666
{{end}}

‎templates/repo/issue/view_content/comments.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@
6767
{{end}}
6868
</div>
6969
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
70-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
70+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
7171
{{if .Attachments}}
7272
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
7373
{{end}}
@@ -441,7 +441,7 @@
441441
{{end}}
442442
</div>
443443
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
444-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
444+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
445445
{{if .Attachments}}
446446
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
447447
{{end}}

‎templates/repo/issue/view_content/conversation.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@
9696
{{end}}
9797
</div>
9898
<div id="issuecomment-{{.ID}}-raw" class="raw-content tw-hidden">{{.Content}}</div>
99-
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
99+
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-content-version="{{.ContentVersion}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
100100
{{if .Attachments}}
101101
{{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}}
102102
{{end}}

0 commit comments

Comments
 (0)