From e9e3b8c0f389e7a3674152e18a124b1967873935 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 --- 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 8f9b598c0b..afcfbc00e3 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -893,13 +893,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 725a33929f..d5bed1f640 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -711,13 +711,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)