Merge pull request 'fix(api): issue state change is not idempotent' (#4687) from earl-warren/forgejo:wip-issue-state-idempotency into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4687
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

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 {
+		isClosed := api.StateClosed == api.StateType(*form.State)
-			if issues_model.IsErrDependenciesLeft(err) {
+		if issue.IsClosed != isClosed {
-				ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")
+			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
 		}
 	}
 

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 {
+		isClosed := api.StateClosed == api.StateType(*form.State)
-			if issues_model.IsErrDependenciesLeft(err) {
+		if issue.IsClosed != isClosed {
-				ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies")
+			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
 		}
 	}
 

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) {

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)