refactor(api): load assignees list correctly (#39917)

Correctly handle empty names, upper&lower cases, duplicate names.

Fixes https://github.com/go-gitea/gitea/issues/39916

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/issues/assignees.go

@@ -9,7 +9,6 @@ import (
 
 	"gitea.dev/models/db"
 	user_model "gitea.dev/models/user"
-	"gitea.dev/modules/util"
 
 	"xorm.io/builder"
 )
@@ -172,21 +171,3 @@ func toggleUserAssignee(ctx context.Context, issue *Issue, assigneeID int64) (re
 
 	return found, nil
 }
-
-// MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs
-func MakeIDsFromAPIAssigneesToAdd(ctx context.Context, oneAssignee string, multipleAssignees []string) ([]int64, error) {
-	var requestAssignees []string
-
-	// Keeping the old assigning method for compatibility reasons
-	if oneAssignee != "" && !util.SliceContainsString(multipleAssignees, oneAssignee) {
-		requestAssignees = append(requestAssignees, oneAssignee)
-	}
-
-	// Prevent empty assignees
-	if len(multipleAssignees) > 0 && multipleAssignees[0] != "" {
-		requestAssignees = append(requestAssignees, multipleAssignees...)
-	}
-
-	// Get the IDs of all assignees
-	return user_model.GetDoerUserIDsByNames(ctx, requestAssignees)
-}

models/issues/assignees_test.go

@@ -65,29 +65,3 @@ func TestUpdateAssignee(t *testing.T) {
 	assert.NoError(t, err)
 	assert.False(t, isAssigned)
 }
-
-func TestMakeIDsFromAPIAssigneesToAdd(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	_ = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
-	_ = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-
-	IDs, err := issues_model.MakeIDsFromAPIAssigneesToAdd(t.Context(), "", []string{""})
-	assert.NoError(t, err)
-	assert.Equal(t, []int64{}, IDs)
-
-	_, err = issues_model.MakeIDsFromAPIAssigneesToAdd(t.Context(), "", []string{"none_existing_user"})
-	assert.Error(t, err)
-
-	IDs, err = issues_model.MakeIDsFromAPIAssigneesToAdd(t.Context(), "user1", []string{"user1"})
-	assert.NoError(t, err)
-	assert.Equal(t, []int64{1}, IDs)
-
-	IDs, err = issues_model.MakeIDsFromAPIAssigneesToAdd(t.Context(), "user2", []string{""})
-	assert.NoError(t, err)
-	assert.Equal(t, []int64{2}, IDs)
-
-	IDs, err = issues_model.MakeIDsFromAPIAssigneesToAdd(t.Context(), "", []string{"user1", "user2"})
-	assert.NoError(t, err)
-	assert.Equal(t, []int64{1, 2}, IDs)
-}

models/user/user.go

@@ -1180,6 +1180,15 @@ func GetMailableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([]
 }
 
 func getDoerUserIDsByNames(ctx context.Context, names []string, allowSystemDoer bool) ([]int64, error) {
+	users, err := getDoerUsersByNames(ctx, names, allowSystemDoer)
+	ids := make([]int64, 0, len(users))
+	for _, u := range users {
+		ids = append(ids, u.ID)
+	}
+	return ids, err
+}
+
+func getDoerUsersByNames(ctx context.Context, names []string, allowSystemDoer bool) ([]*User, error) {
 	namesLower := make([]string, 0, len(names))
 	namesMap := make(map[string]*User, len(names))
 	for _, name := range names {
@@ -1193,16 +1202,16 @@ func getDoerUserIDsByNames(ctx context.Context, names []string, allowSystemDoer
 		}
 	}
 
-	var users []*User
-	err := db.GetEngine(ctx).In("lower_name", namesLower).Where("`type` IN (?,?)", UserTypeIndividual, UserTypeBot).Find(&users)
+	var dbUsers []*User
+	err := db.GetEngine(ctx).In("lower_name", namesLower).Where("`type` IN (?,?)", UserTypeIndividual, UserTypeBot).Find(&dbUsers)
 	if err != nil {
 		return nil, err
 	}
-	for _, user := range users {
+	for _, user := range dbUsers {
 		namesMap[user.LowerName] = user
 	}
 
-	ids := make([]int64, 0, len(names))
+	users := make([]*User, 0, len(names))
 	var errFinal error
 	for _, nameLower := range namesLower {
 		u := namesMap[nameLower]
@@ -1210,9 +1219,13 @@ func getDoerUserIDsByNames(ctx context.Context, names []string, allowSystemDoer
 			errFinal = ErrUserNotExist{Name: nameLower}
 			continue
 		}
-		ids = append(ids, u.ID)
+		users = append(users, u)
 	}
-	return ids, errFinal
+	return users, errFinal
+}
+
+func GetDoerUsersByNames(ctx context.Context, names []string) ([]*User, error) {
+	return getDoerUsersByNames(ctx, names, false)
 }
 
 func GetDoerUserIDsByNamesWithSystem(ctx context.Context, names []string) ([]int64, error) {

routers/api/v1/repo/issue.go

@@ -14,9 +14,9 @@ import (
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
 	access_model "gitea.dev/models/perm/access"
-	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
 	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/container"
 	issue_indexer "gitea.dev/modules/indexer/issues"
 	"gitea.dev/modules/optional"
 	"gitea.dev/modules/setting"
@@ -526,6 +526,39 @@ func GetIssue(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, ctx.Doer, issue))
 }
 
+func loadAssigneeUserIDs(ctx *context.APIContext, oneAssignee string, multipleAssignees []string) []int64 {
+	assigneeNames := container.Set[string]{}
+	assigneeNames.Add(strings.ToLower(oneAssignee))
+	for _, assignee := range multipleAssignees {
+		assigneeNames.Add(strings.ToLower(assignee))
+	}
+	assigneeNames.Remove("")
+	assignees, err := user_model.GetDoerUsersByNames(ctx, assigneeNames.Values())
+	if err != nil {
+		if errors.Is(err, util.ErrNotExist) {
+			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: %s", err))
+		} else {
+			ctx.APIErrorInternal(err)
+		}
+		return nil
+	}
+
+	// Check if the passed assignees is assignable
+	assigneeIDs := make([]int64, 0, len(assignees))
+	for _, assignee := range assignees {
+		valid, err := access_model.CanBeAssigned(ctx, assignee, ctx.Repo.Repository)
+		if err != nil {
+			ctx.APIErrorAuto(err)
+			return nil
+		} else if !valid {
+			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("User %s cannot be assigned to this issue", assignee.Name))
+			return nil
+		}
+		assigneeIDs = append(assigneeIDs, assignee.ID)
+	}
+	return assigneeIDs
+}
+
 // CreateIssue create an issue of a repository
 func CreateIssue(ctx *context.APIContext) {
 	// swagger:operation POST /repos/{owner}/{repo}/issues issue issueCreateIssue
@@ -583,34 +616,10 @@ func CreateIssue(ctx *context.APIContext) {
 			return
 		}
 		issue.MilestoneID = form.Milestone
-		assigneeIDs, err = issues_model.MakeIDsFromAPIAssigneesToAdd(ctx, form.Assignee, form.Assignees)
-		if err != nil {
-			if errors.Is(err, util.ErrNotExist) {
-				ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: %s", err))
-			} else {
-				ctx.APIErrorInternal(err)
-			}
+		assigneeIDs = loadAssigneeUserIDs(ctx, form.Assignee, form.Assignees)
+		if ctx.Written() {
 			return
 		}
-
-		// Check if the passed assignees is assignable
-		for _, aID := range assigneeIDs {
-			assignee, err := user_model.GetUserByID(ctx, aID)
-			if err != nil {
-				ctx.APIErrorInternal(err)
-				return
-			}
-
-			valid, err := access_model.CanBeAssigned(ctx, assignee, ctx.Repo.Repository)
-			if err != nil {
-				ctx.APIErrorInternal(err)
-				return
-			}
-			if !valid {
-				ctx.APIError(http.StatusUnprocessableEntity, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: ctx.Repo.Repository.Name}.Error())
-				return
-			}
-		}
 	} else {
 		// setting labels is not allowed if user is not a writer
 		form.Labels = make([]int64, 0)

routers/api/v1/repo/issue_test.go

@@ -0,0 +1,40 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package repo
+
+import (
+	"net/http"
+	"testing"
+
+	"gitea.dev/models/unittest"
+	"gitea.dev/services/contexttest"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLoadAssigneeUserIDs(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	for _, test := range []struct {
+		name              string
+		oneAssignee       string
+		multipleAssignees []string
+		wantIDs           []int64
+		wantStatus        int
+	}{
+		{name: "empty", wantIDs: []int64{}, wantStatus: http.StatusOK},
+		{name: "combined", oneAssignee: "User2", multipleAssignees: []string{"user2", "USER2", "user1", ""}, wantIDs: []int64{1, 2}, wantStatus: http.StatusOK},
+		{name: "unknown user", oneAssignee: "does-not-exist", wantStatus: http.StatusUnprocessableEntity},
+		{name: "no access", oneAssignee: "user4", wantStatus: http.StatusUnprocessableEntity},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			ctx, resp := contexttest.MockAPIContext(t, "user2/repo2")
+			contexttest.LoadRepo(t, ctx, 2)
+
+			ids := loadAssigneeUserIDs(ctx, test.oneAssignee, test.multipleAssignees)
+			assert.Equal(t, test.wantStatus, resp.Code)
+			assert.ElementsMatch(t, test.wantIDs, ids)
+		})
+	}
+}

routers/api/v1/repo/pull.go

@@ -524,33 +524,10 @@ func CreatePullRequest(ctx *context.APIContext) {
 	pr.AllowMaintainerEdit = optional.FromPtr(form.AllowMaintainerEdit).ValueOrDefault(unitPullRequest.PullRequestsConfig().DefaultAllowMaintainerEdit)
 
 	// Get all assignee IDs
-	assigneeIDs, err := issues_model.MakeIDsFromAPIAssigneesToAdd(ctx, form.Assignee, form.Assignees)
-	if err != nil {
-		if errors.Is(err, util.ErrNotExist) {
-			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: [name: %s]", err))
-		} else {
-			ctx.APIErrorInternal(err)
-		}
+	assigneeIDs := loadAssigneeUserIDs(ctx, form.Assignee, form.Assignees)
+	if ctx.Written() {
 		return
 	}
-	// Check if the passed assignees is assignable
-	for _, aID := range assigneeIDs {
-		assignee, err := user_model.GetUserByID(ctx, aID)
-		if err != nil {
-			ctx.APIErrorInternal(err)
-			return
-		}
-
-		valid, err := access_model.CanBeAssigned(ctx, assignee, repo)
-		if err != nil {
-			ctx.APIErrorInternal(err)
-			return
-		}
-		if !valid {
-			ctx.APIError(http.StatusUnprocessableEntity, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name}.Error())
-			return
-		}
-	}
 
 	prOpts := &pull_service.NewPullRequestOptions{
 		Repo:        repo,

routers/web/repo/issue.go

@@ -462,7 +462,7 @@ func UpdateIssueAssignee(ctx *context.Context) {
 				return
 			}
 			if !valid {
-				ctx.ServerError("canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name})
+				ctx.HTTPError(http.StatusForbidden, fmt.Sprintf("User %d cannot be assigned to this issue", assigneeID))
 				return
 			}