From 9828aca7330dbb7c50645bd06a221773447a19e7 Mon Sep 17 00:00:00 2001 From: BtbN Date: Sat, 30 Aug 2025 03:29:23 +0200 Subject: [PATCH] feat: github compatability for removing label from issue API (#8831) On GitHub, `DELETE /repos/{owner}/{repo}/issues/{index}/labels/{id}` takes the label name, not id: https://docs.github.com/en/rest/issues/labels?apiVersion=2022-11-28#remove-a-label-from-an-issue This breaks workflows and actions that interact with labels and delete them. It also makes the API quite difficult to use, always having to query the ID first before deleting a label from an issue, potentially with two API calls, because it could be a repo or org label. For backwards compatibility, if no label with the given name is found, and the name converts to an int without error, it'll still be looked up by ID. The API on GitHub also does not return 204, but 200, with the label it just removed from the issue as content. So this is returned when `application/vnd.github+json` is set in the `Accept` request header. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8831 Reviewed-by: Gusted Co-authored-by: BtbN Co-committed-by: BtbN --- routers/api/v1/api.go | 2 +- routers/api/v1/repo/issue_label.go | 34 +++++++++++++----- services/actions/notifier.go | 2 +- templates/swagger/v1_json.tmpl | 9 +++-- tests/integration/api_issue_label_test.go | 35 +++++++++++++++++++ .../issue_label.yml | 9 +++++ 6 files changed, 75 insertions(+), 16 deletions(-) create mode 100644 tests/integration/fixtures/TestAPIRemoveIssueLabelByName/issue_label.yml diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 26a2c0ffe3..ca65148a35 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1431,7 +1431,7 @@ func Routes() *web.Route { Post(reqToken(), bind(api.IssueLabelsOption{}), repo.AddIssueLabels). Put(reqToken(), bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). Delete(reqToken(), bind(api.DeleteLabelsOption{}), repo.ClearIssueLabels) - m.Delete("/{id}", reqToken(), bind(api.DeleteLabelsOption{}), repo.DeleteIssueLabel) + m.Delete("/{identifier}", reqToken(), bind(api.DeleteLabelsOption{}), repo.DeleteIssueLabel) }) m.Group("/times", func() { m.Combo(""). diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go index f2e79ea417..afab033907 100644 --- a/routers/api/v1/repo/issue_label.go +++ b/routers/api/v1/repo/issue_label.go @@ -8,6 +8,7 @@ import ( "errors" "net/http" "reflect" + "strconv" issues_model "forgejo.org/models/issues" api "forgejo.org/modules/structs" @@ -125,7 +126,7 @@ func AddIssueLabels(ctx *context.APIContext) { // DeleteIssueLabel delete a label for an issue func DeleteIssueLabel(ctx *context.APIContext) { - // swagger:operation DELETE /repos/{owner}/{repo}/issues/{index}/labels/{id} issue issueRemoveLabel + // swagger:operation DELETE /repos/{owner}/{repo}/issues/{index}/labels/{identifier} issue issueRemoveLabel // --- // summary: Remove a label from an issue // produces: @@ -147,11 +148,10 @@ func DeleteIssueLabel(ctx *context.APIContext) { // type: integer // format: int64 // required: true - // - name: id + // - name: identifier // in: path - // description: id of the label to remove - // type: integer - // format: int64 + // description: name or id of the label to remove + // type: string // required: true // - name: body // in: body @@ -188,12 +188,24 @@ func DeleteIssueLabel(ctx *context.APIContext) { return } - label, err := issues_model.GetLabelByID(ctx, ctx.ParamsInt64(":id")) + labelName := ctx.Params(":identifier") + label, err := issues_model.GetLabelInRepoByName(ctx, ctx.Repo.Repository.ID, labelName) + if err != nil && issues_model.IsErrRepoLabelNotExist(err) && ctx.Repo.Owner.IsOrganization() { + label, err = issues_model.GetLabelInOrgByName(ctx, ctx.Repo.Owner.ID, labelName) + } + if err != nil && (issues_model.IsErrRepoLabelNotExist(err) || issues_model.IsErrOrgLabelNotExist(err)) { + if labelID, parseErr := strconv.ParseInt(labelName, 10, 64); parseErr == nil { + label, err = issues_model.GetLabelByID(ctx, labelID) + } + } + if err != nil { - if issues_model.IsErrLabelNotExist(err) { + if issues_model.IsErrRepoLabelNotExist(err) || + issues_model.IsErrOrgLabelNotExist(err) || + issues_model.IsErrLabelNotExist(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { - ctx.Error(http.StatusInternalServerError, "GetLabelByID", err) + ctx.Error(http.StatusInternalServerError, "GetLabel", err) } return } @@ -203,7 +215,11 @@ func DeleteIssueLabel(ctx *context.APIContext) { return } - ctx.Status(http.StatusNoContent) + if ctx.Req.Header.Get("Accept") == "application/vnd.github+json" { + ctx.JSON(http.StatusOK, convert.ToLabelList([]*issues_model.Label{label}, ctx.Repo.Repository, ctx.Repo.Owner)) + } else { + ctx.Status(http.StatusNoContent) + } } // ReplaceIssueLabels replace labels for an issue diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 33ac675fcc..7b291d491b 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -227,7 +227,7 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues var apiLabel *api.Label if action == api.HookIssueLabelUpdated || action == api.HookIssueLabelCleared { - apiLabel = convert.ToLabel(label, issue.Repo, nil) + apiLabel = convert.ToLabel(label, issue.Repo, issue.Repo.Owner) } if issue.IsPull { diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 353abe76c0..ede509409c 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -10998,7 +10998,7 @@ } } }, - "/repos/{owner}/{repo}/issues/{index}/labels/{id}": { + "/repos/{owner}/{repo}/issues/{index}/labels/{identifier}": { "delete": { "produces": [ "application/json" @@ -11032,10 +11032,9 @@ "required": true }, { - "type": "integer", - "format": "int64", - "description": "id of the label to remove", - "name": "id", + "type": "string", + "description": "name or id of the label to remove", + "name": "identifier", "in": "path", "required": true }, diff --git a/tests/integration/api_issue_label_test.go b/tests/integration/api_issue_label_test.go index 665e4b2f2a..c6706d6ce2 100644 --- a/tests/integration/api_issue_label_test.go +++ b/tests/integration/api_issue_label_test.go @@ -172,6 +172,41 @@ func TestAPIRemoveIssueLabel(t *testing.T) { unittest.AssertCount(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: issueLabel.LabelID}, 0) } +func TestAPIRemoveIssueLabelByName(t *testing.T) { + defer unittest.OverrideFixtures("tests/integration/fixtures/TestAPIRemoveIssueLabelByName")() + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: repo.ID, ID: 12}) + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + repoIssueLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: 10}) + orgIssueLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: 3}) + repoLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: repoIssueLabel.LabelID, RepoID: repo.ID}) + orgLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: orgIssueLabel.LabelID, OrgID: owner.ID}) + + assert.Equal(t, int64(0), orgLabel.RepoID) + assert.Equal(t, int64(0), repoLabel.OrgID) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47}) + task.RepoID = repo.ID + task.OwnerID = repo.OwnerID + require.NoError(t, task.GenerateToken()) + actions_model.UpdateTask(t.Context(), task) + + deleteURL := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels/%s", owner.Name, repo.Name, issue.Index, repoLabel.Name) + req := NewRequest(t, "DELETE", deleteURL). + AddTokenAuth(task.Token) + MakeRequest(t, req, http.StatusNoContent) + + deleteURL = fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels/%s", owner.Name, repo.Name, issue.Index, orgLabel.Name) + req = NewRequest(t, "DELETE", deleteURL). + AddTokenAuth(task.Token) + MakeRequest(t, req, http.StatusNoContent) + + unittest.AssertCount(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: repoIssueLabel.ID}, 0) + unittest.AssertCount(t, &issues_model.IssueLabel{IssueID: issue.ID, LabelID: orgIssueLabel.ID}, 0) +} + func TestAPIAddIssueLabelsAutoDate(t *testing.T) { defer tests.PrepareTestEnv(t)() diff --git a/tests/integration/fixtures/TestAPIRemoveIssueLabelByName/issue_label.yml b/tests/integration/fixtures/TestAPIRemoveIssueLabelByName/issue_label.yml new file mode 100644 index 0000000000..2ee2186b04 --- /dev/null +++ b/tests/integration/fixtures/TestAPIRemoveIssueLabelByName/issue_label.yml @@ -0,0 +1,9 @@ +- + id: 1001 + issue_id: 12 + label_id: 3 + +- + id: 1002 + issue_id: 12 + label_id: 10