fix(api): adding issue labels by name attaches both the repo and the org label (#39873) (#39951)

Backport #39873 by @nazem-droubi

Fixes #39651

When an org-owned repo has a repo label and an org label with the same
name, `POST`/`PUT /repos/{owner}/{repo}/issues/{index}/labels` with
label names attached both of them.

With this change the repo label is used when the names collide, and org
labels are only looked up for the names that didn't match a repo label.
Names that only exist at the org level still resolve to the org label as
before. I documented the behavior on `IssueLabelsOption.Labels` and
regenerated the swagger files.

Picking the repo label seemed the least surprising choice since it's the
more specific one, but I'm happy to change it (for example to return a
422 on ambiguous names) if you'd prefer that.

Tests: added `TestAPIAddIssueLabelsWithSharedRepoAndOrgLabelName`, which
covers both POST and PUT. It fails on `main` (3 labels attached instead
of 2) and passes with this change. I ran the existing add/replace label
integration tests locally on sqlite, plus `go vet` and gofmt.

AI disclosure: the code and test in this PR were written primarily by an
AI coding agent working on my behalf. I'm happy to walk through it or
revise anything during review.

Signed-off-by: Nazem <214302554+nazem-droubi@users.noreply.github.com>
Co-authored-by: Nazem <nazem98@gmail.com>
Co-authored-by: Nazem <214302554+nazem-droubi@users.noreply.github.com>
Co-authored-by: silverwind <me@silverwind.io>

modules/structs/issue_label.go

@@ -56,7 +56,9 @@ type EditLabelOption struct {
 // IssueLabelsOption a collection of labels
 type IssueLabelsOption struct {
 	// Labels can be a list of integers representing label IDs
-	// or a list of strings representing label names
+	// or a list of strings representing label names.
+	// If a repository label and an organization label share a name,
+	// the repository label is used.
 	Labels []any `json:"labels"`
 }
 

routers/api/v1/repo/issue_label.go

@@ -10,6 +10,7 @@ import (
 	"reflect"
 
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/modules/container"
 	api "gitea.dev/modules/structs"
 	"gitea.dev/modules/web"
 	"gitea.dev/services/context"
@@ -339,12 +340,23 @@ func prepareForReplaceOrAdd(ctx *context.APIContext, form api.IssueLabelsOption)
 		}
 		labelIDs = append(labelIDs, repoLabelIDs...)
 		if ctx.Repo.Owner.IsOrganization() {
-			orgLabelIDs, err := issues_model.GetLabelIDsInOrgByNames(ctx, ctx.Repo.Owner.ID, labelNames)
+			repoLabels, err := issues_model.GetLabelsByIDs(ctx, repoLabelIDs, "name")
 			if err != nil {
 				ctx.APIErrorInternal(err)
 				return nil, nil, err
 			}
-			labelIDs = append(labelIDs, orgLabelIDs...)
+			orgLabelNames := container.SetOf(labelNames...)
+			for _, label := range repoLabels {
+				orgLabelNames.Remove(label.Name)
+			}
+			if len(orgLabelNames) > 0 {
+				orgLabelIDs, err := issues_model.GetLabelIDsInOrgByNames(ctx, ctx.Repo.Owner.ID, orgLabelNames.Values())
+				if err != nil {
+					ctx.APIErrorInternal(err)
+					return nil, nil, err
+				}
+				labelIDs = append(labelIDs, orgLabelIDs...)
+			}
 		}
 	}
 

templates/swagger/v1-openapi3.generated.json

@@ -7416,7 +7416,7 @@
         "description": "IssueLabelsOption a collection of labels",
         "properties": {
           "labels": {
-            "description": "Labels can be a list of integers representing label IDs\nor a list of strings representing label names",
+            "description": "Labels can be a list of integers representing label IDs\nor a list of strings representing label names.\nIf a repository label and an organization label share a name,\nthe repository label is used.",
             "items": {},
             "type": "array",
             "x-go-name": "Labels"

templates/swagger/v1-swagger.generated.json

@@ -30525,7 +30525,7 @@
       "type": "object",
       "properties": {
         "labels": {
-          "description": "Labels can be a list of integers representing label IDs\nor a list of strings representing label names",
+          "description": "Labels can be a list of integers representing label IDs\nor a list of strings representing label names.\nIf a repository label and an organization label share a name,\nthe repository label is used.",
           "type": "array",
           "items": {},
           "x-go-name": "Labels"

tests/integration/api_issue_label_test.go

@@ -175,6 +175,40 @@ func TestAPIAddIssueLabelsWithLabelNames(t *testing.T) {
 	MakeRequest(t, req, http.StatusNoContent)
 }
 
+func TestAPIAddIssueLabelsWithSharedRepoAndOrgLabelName(t *testing.T) {
+	assert.NoError(t, unittest.LoadFixtures())
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 6, RepoID: repo.ID})
+	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	repoLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 10, RepoID: repo.ID})
+	orgOnlyLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 4, OrgID: owner.ID})
+
+	assert.NoError(t, issues_model.NewLabel(t.Context(), &issues_model.Label{OrgID: owner.ID, Name: repoLabel.Name, Color: "#abcdef"}))
+
+	token := getTokenForLoggedInUser(t, loginUser(t, "user1"), auth_model.AccessTokenScopeWriteIssue)
+	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels", owner.Name, repo.Name, issue.Index)
+
+	for _, method := range []string{"POST", "PUT"} {
+		t.Run(method, func(t *testing.T) {
+			req := NewRequest(t, "DELETE", urlStr).AddTokenAuth(token)
+			MakeRequest(t, req, http.StatusNoContent)
+
+			req = NewRequestWithJSON(t, method, urlStr, &api.IssueLabelsOption{
+				Labels: []any{repoLabel.Name, orgOnlyLabel.Name},
+			}).AddTokenAuth(token)
+			resp := MakeRequest(t, req, http.StatusOK)
+			apiLabels := DecodeJSON(t, resp, []*api.Label{})
+
+			var apiLabelIDs []int64
+			for _, label := range apiLabels {
+				apiLabelIDs = append(apiLabelIDs, label.ID)
+			}
+			assert.ElementsMatch(t, []int64{repoLabel.ID, orgOnlyLabel.ID}, apiLabelIDs)
+		})
+	}
+}
+
 func TestAPIReplaceIssueLabels(t *testing.T) {
 	assert.NoError(t, unittest.LoadFixtures())