diff --git a/modules/structs/issue.go b/modules/structs/issue.go index 1efe3334ca..a34e4b0693 100644 --- a/modules/structs/issue.go +++ b/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 diff --git a/modules/structs/pull.go b/modules/structs/pull.go index 3ad2f78bd3..ad320e2b82 100644 --- a/modules/structs/pull.go +++ b/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 diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index db205380e4..20ccd099a4 100644 --- a/routers/api/v1/repo/issue.go +++ b/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 } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index a045bba49c..ef86f413b7 100644 --- a/routers/api/v1/repo/pull.go +++ b/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 } diff --git a/services/convert/issue.go b/services/convert/issue.go index acd67fece4..61f11d8f19 100644 --- a/services/convert/issue.go +++ b/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 { diff --git a/services/convert/pull.go b/services/convert/pull.go index bb675811f2..5c7c99f2ce 100644 --- a/services/convert/pull.go +++ b/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, diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index e01ff1112b..5ae0f197df 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -10362,6 +10362,7 @@ } }, "patch": { + "description": "Pass `content_version` to enable optimistic locking on body edits.\nIf the version doesn't match the current value, the request fails with 409 Conflict.\n", "consumes": [ "application/json" ], @@ -24766,6 +24767,11 @@ "type": "string", "x-go-name": "Body" }, + "content_version": { + "type": "integer", + "format": "int64", + "x-go-name": "ContentVersion" + }, "due_date": { "type": "string", "format": "date-time", @@ -24938,6 +24944,11 @@ "type": "string", "x-go-name": "Body" }, + "content_version": { + "type": "integer", + "format": "int64", + "x-go-name": "ContentVersion" + }, "due_date": { "type": "string", "format": "date-time", @@ -26223,6 +26234,11 @@ "format": "int64", "x-go-name": "Comments" }, + "content_version": { + "type": "integer", + "format": "int64", + "x-go-name": "ContentVersion" + }, "created_at": { "type": "string", "format": "date-time", @@ -27725,6 +27741,11 @@ "format": "int64", "x-go-name": "Comments" }, + "content_version": { + "type": "integer", + "format": "int64", + "x-go-name": "ContentVersion" + }, "created_at": { "type": "string", "format": "date-time", diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 8d85543dc8..c3e96059de 100644 --- a/tests/integration/api_issue_test.go +++ b/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,9 +149,7 @@ 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. @@ -152,7 +157,7 @@ func TestAPICreateIssueParallel(t *testing.T) { // 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) + }) +}