fedi/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2024-11-26 15:02:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(api): issue state change is not idempotent

Browse source 
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
(cherry picked from commit e9e3b8c0f3)
This commit is contained in:
Earl Warren 2024-07-25 15:16:44 +02:00 committed by GitHub
parent 887a914a7d
commit 533623b659
4 changed files with 44 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
routers/api/v1/repo/issue.go
View file

@ -893,7 +893,9 @@ 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 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
@ -902,6 +904,7 @@ func EditIssue(ctx *context.APIContext) {
return
}
}
}
// Refetch from database to assign some automatic values
issue, err = issues_model.GetIssueByID(ctx, issue.ID)

5
routers/api/v1/repo/pull.go
View file

@ -711,7 +711,9 @@ 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 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
@ -720,6 +722,7 @@ func EditPullRequest(ctx *context.APIContext) {
return
}
}
}
// change pull target branch
if !pr.HasMerged && len(form.Base) != 0 && form.Base != pr.BaseBranch {

15
tests/integration/api_issue_test.go
View file

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

15
tests/integration/api_pull_test.go
View file

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