Expose content_version for optimistic locking on issue and PR edits

modules/structs/issue.go

@@ -80,7 +80,8 @@ type Issue struct {
 	PullRequest *PullRequestMeta `json:"pull_request"`
 	Repo        *RepositoryMeta  `json:"repository"`
 
-	PinOrder int `json:"pin_order"`
+	PinOrder       int `json:"pin_order"`
+	ContentVersion int `json:"content_version"`
 }
 
 // CreateIssueOption options to create one issue
@@ -114,6 +115,7 @@ type EditIssueOption struct {
 	// swagger:strfmt date-time
 	Deadline       *time.Time `json:"due_date"`
 	RemoveDeadline *bool      `json:"unset_due_date"`
+	ContentVersion *int       `json:"content_version"`
 }
 
 // EditDeadlineOption options for creating a deadline

modules/structs/pull.go

@@ -90,7 +90,8 @@ type PullRequest struct {
 	Closed *time.Time `json:"closed_at"`
 
 	// The pin order for the pull request
-	PinOrder int `json:"pin_order"`
+	PinOrder       int `json:"pin_order"`
+	ContentVersion int `json:"content_version"`
 }
 
 // PRBranchInfo information about a branch
@@ -168,6 +169,7 @@ type EditPullRequestOption struct {
 	RemoveDeadline *bool `json:"unset_due_date"`
 	// Whether to allow maintainer edits
 	AllowMaintainerEdit *bool `json:"allow_maintainer_edit"`
+	ContentVersion      *int  `json:"content_version"`
 }
 
 // ChangedFile store information about files affected by the pull request

routers/api/v1/repo/issue.go

@@ -726,6 +726,9 @@ func EditIssue(ctx *context.APIContext) {
 	// swagger:operation PATCH /repos/{owner}/{repo}/issues/{index} issue issueEditIssue
 	// ---
 	// summary: Edit an issue. If using deadline only the date will be taken into account, and time of day ignored.
+	// description: |
+	//   Pass `content_version` to enable optimistic locking on body edits.
+	//   If the version doesn't match the current value, the request fails with 409 Conflict.
 	// consumes:
 	// - application/json
 	// produces:
@@ -785,6 +788,15 @@ func EditIssue(ctx *context.APIContext) {
 		return
 	}
 
+	// Fail fast: if content_version is provided and already stale, reject
+	// before any mutations. The DB-level check in ChangeContent still
+	// handles concurrent requests.
+	// TODO: wrap all mutations in a transaction to fully prevent partial writes.
+	if form.ContentVersion != nil && *form.ContentVersion != issue.ContentVersion {
+		ctx.APIError(http.StatusConflict, issues_model.ErrIssueAlreadyChanged)
+		return
+	}
+
 	if len(form.Title) > 0 {
 		err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title)
 		if err != nil {
@@ -793,10 +805,14 @@ func EditIssue(ctx *context.APIContext) {
 		}
 	}
 	if form.Body != nil {
-		err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
+		contentVersion := issue.ContentVersion
+		if form.ContentVersion != nil {
+			contentVersion = *form.ContentVersion
+		}
+		err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, contentVersion)
 		if err != nil {
 			if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
-				ctx.APIError(http.StatusBadRequest, err)
+				ctx.APIError(http.StatusConflict, err)
 				return
 			}
 

routers/api/v1/repo/pull.go

@@ -657,6 +657,15 @@ func EditPullRequest(ctx *context.APIContext) {
 		return
 	}
 
+	// Fail fast: if content_version is provided and already stale, reject
+	// before any mutations. The DB-level check in ChangeContent still
+	// handles concurrent requests.
+	// TODO: wrap all mutations in a transaction to fully prevent partial writes.
+	if form.ContentVersion != nil && *form.ContentVersion != issue.ContentVersion {
+		ctx.APIError(http.StatusConflict, issues_model.ErrIssueAlreadyChanged)
+		return
+	}
+
 	if len(form.Title) > 0 {
 		err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title)
 		if err != nil {
@@ -665,10 +674,14 @@ func EditPullRequest(ctx *context.APIContext) {
 		}
 	}
 	if form.Body != nil {
-		err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion)
+		contentVersion := issue.ContentVersion
+		if form.ContentVersion != nil {
+			contentVersion = *form.ContentVersion
+		}
+		err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, contentVersion)
 		if err != nil {
 			if errors.Is(err, issues_model.ErrIssueAlreadyChanged) {
-				ctx.APIError(http.StatusBadRequest, err)
+				ctx.APIError(http.StatusConflict, err)
 				return
 			}
 

services/convert/issue.go

@@ -62,7 +62,8 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss
 		Updated:     issue.UpdatedUnix.AsTime(),
 		PinOrder:    util.Iif(issue.PinOrder == -1, 0, issue.PinOrder), // -1 means loaded with no pin order
 
-		TimeEstimate: issue.TimeEstimate,
+		TimeEstimate:   issue.TimeEstimate,
+		ContentVersion: issue.ContentVersion,
 	}
 
 	if issue.Repo != nil {

services/convert/pull.go

@@ -97,6 +97,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 		Created:        pr.Issue.CreatedUnix.AsTimePtr(),
 		Updated:        pr.Issue.UpdatedUnix.AsTimePtr(),
 		PinOrder:       util.Iif(apiIssue.PinOrder == -1, 0, apiIssue.PinOrder),
+		ContentVersion: apiIssue.ContentVersion,
 
 		// output "[]" rather than null to align to github outputs
 		RequestedReviewers:      []*api.User{},
@@ -372,6 +373,7 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
 			Created:        pr.Issue.CreatedUnix.AsTimePtr(),
 			Updated:        pr.Issue.UpdatedUnix.AsTimePtr(),
 			PinOrder:       util.Iif(apiIssue.PinOrder == -1, 0, apiIssue.PinOrder),
+			ContentVersion: apiIssue.ContentVersion,
 
 			AllowMaintainerEdit: pr.AllowMaintainerEdit,
 

tests/integration/api_issue_test.go

@@ -25,9 +25,19 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAPIListIssues(t *testing.T) {
+func TestAPIIssue(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
+	t.Run("ListIssues", testAPIListIssues)
+	t.Run("ListIssuesPublicOnly", testAPIListIssuesPublicOnly)
+	t.Run("SearchIssues", testAPISearchIssues)
+	t.Run("SearchIssuesWithLabels", testAPISearchIssuesWithLabels)
+	t.Run("EditIssue", testAPIEditIssue)
+	t.Run("IssueContentVersion", testAPIIssueContentVersion)
+	t.Run("CreateIssue", testAPICreateIssue)
+	t.Run("CreateIssueParallel", testAPICreateIssueParallel)
+}
 
+func testAPIListIssues(t *testing.T) {
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
 
@@ -75,9 +85,7 @@ func TestAPIListIssues(t *testing.T) {
 	}
 }
 
-func TestAPIListIssuesPublicOnly(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
+func testAPIListIssuesPublicOnly(t *testing.T) {
 	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	owner1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo1.OwnerID})
 
@@ -103,8 +111,7 @@ func TestAPIListIssuesPublicOnly(t *testing.T) {
 	MakeRequest(t, req, http.StatusForbidden)
 }
 
-func TestAPICreateIssue(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAPICreateIssue(t *testing.T) {
 	const body, title = "apiTestBody", "apiTestTitle"
 
 	repoBefore := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
@@ -142,17 +149,15 @@ func TestAPICreateIssue(t *testing.T) {
 	MakeRequest(t, req, http.StatusForbidden)
 }
 
-func TestAPICreateIssueParallel(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
+func testAPICreateIssueParallel(t *testing.T) {
 	// FIXME: There seems to be a bug in github.com/mattn/go-sqlite3 with sqlite_unlock_notify, when doing concurrent writes to the same database,
 	// some requests may get stuck in "go-sqlite3.(*SQLiteRows).Next", "go-sqlite3.(*SQLiteStmt).exec" and "go-sqlite3.unlock_notify_wait",
 	// because the "unlock_notify_wait" never returns and the internal lock never gets releases.
 	//
 	// The trigger is: a previous test created issues and made the real issue indexer queue start processing, then this test does concurrent writing.
 	// Adding this "Sleep" makes go-sqlite3 "finish" some internal operations before concurrent writes and then won't get stuck.
 	// To reproduce: make a new test run these 2 tests enough times:
-	// > func TestBug() { for i := 0; i < 100; i++ { testAPICreateIssue(t); testAPICreateIssueParallel(t) } }
+	// > func testBug() { for i := 0; i < 100; i++ { testAPICreateIssue(t); testAPICreateIssueParallel(t) } }
 	// Usually the test gets stuck in fewer than 10 iterations without this "sleep".
 	time.Sleep(time.Second)
 
@@ -197,9 +202,7 @@ func TestAPICreateIssueParallel(t *testing.T) {
 	wg.Wait()
 }
 
-func TestAPIEditIssue(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
+func testAPIEditIssue(t *testing.T) {
 	issueBefore := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 10})
 	repoBefore := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issueBefore.RepoID})
 	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repoBefore.OwnerID})
@@ -263,8 +266,7 @@ func TestAPIEditIssue(t *testing.T) {
 	assert.Equal(t, title, issueAfter.Title)
 }
 
-func TestAPISearchIssues(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAPISearchIssues(t *testing.T) {
 	defer test.MockVariableValue(&setting.API.DefaultPagingNum, 20)()
 	expectedIssueCount := 20 // 20 is from the fixtures
 
@@ -391,9 +393,7 @@ func TestAPISearchIssues(t *testing.T) {
 	assert.Len(t, apiIssues, 3)
 }
 
-func TestAPISearchIssuesWithLabels(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
+func testAPISearchIssuesWithLabels(t *testing.T) {
 	// as this API was used in the frontend, it uses UI page size
 	expectedIssueCount := min(20, setting.UI.IssuePagingNum) // 20 is from the fixtures
 
@@ -448,3 +448,56 @@ func TestAPISearchIssuesWithLabels(t *testing.T) {
 	DecodeJSON(t, resp, &apiIssues)
 	assert.Len(t, apiIssues, 2)
 }
+
+func testAPIIssueContentVersion(t *testing.T) {
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 10})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
+	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+
+	session := loginUser(t, owner.Name)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue)
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d", owner.Name, repo.Name, issue.Index)
+
+	t.Run("ResponseIncludesContentVersion", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequest(t, "GET", urlStr).AddTokenAuth(token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		apiIssue := DecodeJSON(t, resp, &api.Issue{})
+		assert.GreaterOrEqual(t, apiIssue.ContentVersion, 0)
+	})
+
+	t.Run("EditWithCorrectVersion", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequest(t, "GET", urlStr).AddTokenAuth(token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		var before api.Issue
+		DecodeJSON(t, resp, &before)
+		req = NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
+			Body:           new("updated body with correct version"),
+			ContentVersion: new(before.ContentVersion),
+		}).AddTokenAuth(token)
+		resp = MakeRequest(t, req, http.StatusCreated)
+		after := DecodeJSON(t, resp, &api.Issue{})
+		assert.Equal(t, "updated body with correct version", after.Body)
+		assert.Greater(t, after.ContentVersion, before.ContentVersion)
+	})
+
+	t.Run("EditWithWrongVersion", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		req := NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
+			Body:           new("should fail"),
+			ContentVersion: new(99999),
+		}).AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusConflict)
+	})
+
+	t.Run("EditWithoutVersion", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		req := NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
+			Body: new("edit without version succeeds"),
+		}).AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusCreated)
+	})
+}