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: 6a4bc0289d

Fixes: https://codeberg.org/forgejo/forgejo/issues/4686

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)