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 <gusted@noreply.codeberg.org>
Co-authored-by: BtbN <btbn@btbn.de>
Co-committed-by: BtbN <btbn@btbn.de>

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("").

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
+	//   description: name or id of the label to remove
-	//   type: integer
+	//   type: string
-	//   format: int64
 	//   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,8 +215,12 @@ func DeleteIssueLabel(ctx *context.APIContext) {
 		return
 	}
 
+	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
 func ReplaceIssueLabels(ctx *context.APIContext) {

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 {

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",
+            "type": "string",
-            "format": "int64",
+            "description": "name or id of the label to remove",
-            "description": "id of the label to remove",
+            "name": "identifier",
-            "name": "id",
             "in": "path",
             "required": true
           },

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

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