From 9f1302f6850a840ae2a9faa536fdd2222de0789c Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 25 Jul 2024 15:16:44 +0200 Subject: [PATCH] fix(api): issue state change is not idempotent The PATCH if issue & pull request switched to use the service functions instead. However, the service function changing the state is not idempotent. Instead of doing nothing which changing from open to open or close to close, it will fail with an error like: Issue [2472] 0 was already closed Regression of: 6a4bc0289db5d5d791864f45ed9bb47b6bc8d2fe Fixes: https://codeberg.org/forgejo/forgejo/issues/4686 (cherry picked from commit e9e3b8c0f389e7a3674152e18a124b1967873935) --- routers/api/v1/repo/issue.go | 13 ++++++++----- routers/api/v1/repo/pull.go | 13 ++++++++----- tests/integration/api_issue_test.go | 15 +++++++++++++++ tests/integration/api_pull_test.go | 15 +++++++++++++-- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 0e977d563a..ba711763d0 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -889,13 +889,16 @@ func EditIssue(ctx *context.APIContext) { return } } - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + isClosed := api.StateClosed == api.StateType(*form.State) + if issue.IsClosed != isClosed { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) - return } } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 62d71875bf..2a89d2f55f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -690,13 +690,16 @@ func EditPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged") return } - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + isClosed := api.StateClosed == api.StateType(*form.State) + if issue.IsClosed != isClosed { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) - return } } diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index e6fb62a3a9..a8df0250df 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -215,6 +215,21 @@ func TestAPIEditIssue(t *testing.T) { assert.Equal(t, int64(0), int64(issueAfter.DeadlineUnix)) assert.Equal(t, body, issueAfter.Content) assert.Equal(t, title, issueAfter.Title) + + // verify the idempotency of state, milestone, body and title changes + req = NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{ + State: &issueState, + Milestone: &milestone, + Body: &body, + Title: title, + }).AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusCreated) + var apiIssueIdempotent api.Issue + DecodeJSON(t, resp, &apiIssueIdempotent) + assert.Equal(t, apiIssue.State, apiIssueIdempotent.State) + assert.Equal(t, apiIssue.Milestone.Title, apiIssueIdempotent.Milestone.Title) + assert.Equal(t, apiIssue.Body, apiIssueIdempotent.Body) + assert.Equal(t, apiIssue.Title, apiIssueIdempotent.Title) } func TestAPIEditIssueAutoDate(t *testing.T) { diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 4f068502f7..b1bd0aba85 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -236,7 +236,8 @@ func TestAPIEditPull(t *testing.T) { newTitle := "edit a this pr" newBody := "edited body" - req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, apiPull.Index), &api.EditPullRequestOption{ + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, apiPull.Index) + req = NewRequestWithJSON(t, http.MethodPatch, urlStr, &api.EditPullRequestOption{ Base: "feature/1", Title: newTitle, Body: &newBody, @@ -251,7 +252,17 @@ func TestAPIEditPull(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: pull.Issue.ID, OldTitle: title, NewTitle: newTitle}) unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{IssueID: pull.Issue.ID, ContentText: newBody, IsFirstCreated: false}) - req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, pull.Index), &api.EditPullRequestOption{ + // verify the idempotency of a state change + pullState := string(apiPull.State) + req = NewRequestWithJSON(t, http.MethodPatch, urlStr, &api.EditPullRequestOption{ + State: &pullState, + }).AddTokenAuth(token) + apiPullIdempotent := new(api.PullRequest) + resp = MakeRequest(t, req, http.StatusCreated) + DecodeJSON(t, resp, apiPullIdempotent) + assert.EqualValues(t, apiPull.State, apiPullIdempotent.State) + + req = NewRequestWithJSON(t, http.MethodPatch, urlStr, &api.EditPullRequestOption{ Base: "not-exist", }).AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound)