refactor: extract helper functions from SearchIssues (#36158)

## Summary This PR refactors the `SearchIssues` function in `routers/api/v1/repo/issue.go` by extracting common logic into reusable helper functions: - `parseIssueIsClosed()`: Parses the "state" query parameter and returns the corresponding `isClosed` option - `parseIssueIsPull()`: Parses the "type" query parameter and returns the corresponding `isPull` option - `buildSearchIssuesRepoIDs()`: Builds the list of repository IDs for issue search based on query parameters ### Benefits: - Improved code readability - Smaller, more focused functions - Easier to test individual components - Potential for reuse in other handlers ### Changes: - Extracted 3 helper functions from the ~292 line `SearchIssues` function - No functional changes - behavior remains the same - Proper error handling preserved ## Test plan - [ ] Verify existing API tests pass - [ ] Manual testing of `/repos/issues/search` endpoint Ref: #35015 --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
2025-12-22 02:17:10 +00:00 · 2025-12-21 08:57:41 +07:00
parent bf0b377879
commit b6ffe0e4e9
6 changed files with 100 additions and 153 deletions
--- a/routers/api/v1/repo/issue.go
+++ b/routers/api/v1/repo/issue.go
@@ -24,6 +24,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	"code.gitea.io/gitea/routers/common"
@@ -32,6 +33,60 @@ import (
 	issue_service "code.gitea.io/gitea/services/issue"
 )

+// buildSearchIssuesRepoIDs builds the list of repository IDs for issue search based on query parameters.
+// It returns repoIDs, allPublic flag, and any error that occurred.
+func buildSearchIssuesRepoIDs(ctx *context.APIContext) (repoIDs []int64, allPublic bool, err error) {
+	opts := repo_model.SearchRepoOptions{
+		Private:     false,
+		AllPublic:   true,
+		TopicOnly:   false,
+		Collaborate: optional.None[bool](),
+		// This needs to be a column that is not nil in fixtures or
+		// MySQL will return different results when sorting by null in some cases
+		OrderBy: db.SearchOrderByAlphabetically,
+		Actor:   ctx.Doer,
+	}
+	if ctx.IsSigned {
+		opts.Private = !ctx.PublicOnly
+		opts.AllLimited = true
+	}
+	if ctx.FormString("owner") != "" {
+		owner, err := user_model.GetUserByName(ctx, ctx.FormString("owner"))
+		if err != nil {
+			return nil, false, err
+		}
+		opts.OwnerID = owner.ID
+		opts.AllLimited = false
+		opts.AllPublic = false
+		opts.Collaborate = optional.Some(false)
+	}
+	if ctx.FormString("team") != "" {
+		if ctx.FormString("owner") == "" {
+			return nil, false, util.NewInvalidArgumentErrorf("owner organisation is required for filtering on team")
+		}
+		team, err := organization.GetTeam(ctx, opts.OwnerID, ctx.FormString("team"))
+		if err != nil {
+			return nil, false, err
+		}
+		opts.TeamID = team.ID
+	}
+
+	if opts.AllPublic {
+		allPublic = true
+		opts.AllPublic = false // set it false to avoid returning too many repos, we could filter by indexer
+	}
+	repoIDs, _, err = repo_model.SearchRepositoryIDs(ctx, opts)
+	if err != nil {
+		return nil, false, err
+	}
+	if len(repoIDs) == 0 {
+		// no repos found, don't let the indexer return all repos
+		repoIDs = []int64{0}
+	}
+
+	return repoIDs, allPublic, nil
+}
+
 // SearchIssues searches for issues across the repositories that the user has access to
 func SearchIssues(ctx *context.APIContext) {
 	// swagger:operation GET /repos/issues/search issue issueSearchIssues
@@ -58,11 +113,6 @@ func SearchIssues(ctx *context.APIContext) {
 	//   in: query
 	//   description: Search string
 	//   type: string
-	// - name: priority_repo_id
-	//   in: query
-	//   description: Repository ID to prioritize in the results
-	//   type: integer
-	//   format: int64
 	// - name: type
 	//   in: query
 	//   description: Filter by issue type
@@ -136,81 +186,16 @@ func SearchIssues(ctx *context.APIContext) {
 		return
 	}

-	var isClosed optional.Option[bool]
-	switch ctx.FormString("state") {
-	case "closed":
-		isClosed = optional.Some(true)
-	case "all":
-		isClosed = optional.None[bool]()
-	default:
-		isClosed = optional.Some(false)
-	}
+	isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))

-	var (
-		repoIDs   []int64
-		allPublic bool
-	)
-	{
-		// find repos user can access (for issue search)
-		opts := repo_model.SearchRepoOptions{
-			Private:     false,
-			AllPublic:   true,
-			TopicOnly:   false,
-			Collaborate: optional.None[bool](),
-			// This needs to be a column that is not nil in fixtures or
-			// MySQL will return different results when sorting by null in some cases
-			OrderBy: db.SearchOrderByAlphabetically,
-			Actor:   ctx.Doer,
-		}
-		if ctx.IsSigned {
-			opts.Private = !ctx.PublicOnly
-			opts.AllLimited = true
-		}
-		if ctx.FormString("owner") != "" {
-			owner, err := user_model.GetUserByName(ctx, ctx.FormString("owner"))
-			if err != nil {
-				if user_model.IsErrUserNotExist(err) {
-					ctx.APIError(http.StatusBadRequest, err)
-				} else {
-					ctx.APIErrorInternal(err)
-				}
-				return
-			}
-			opts.OwnerID = owner.ID
-			opts.AllLimited = false
-			opts.AllPublic = false
-			opts.Collaborate = optional.Some(false)
-		}
-		if ctx.FormString("team") != "" {
-			if ctx.FormString("owner") == "" {
-				ctx.APIError(http.StatusBadRequest, "Owner organisation is required for filtering on team")
-				return
-			}
-			team, err := organization.GetTeam(ctx, opts.OwnerID, ctx.FormString("team"))
-			if err != nil {
-				if organization.IsErrTeamNotExist(err) {
-					ctx.APIError(http.StatusBadRequest, err)
-				} else {
-					ctx.APIErrorInternal(err)
-				}
-				return
-			}
-			opts.TeamID = team.ID
-		}
-
-		if opts.AllPublic {
-			allPublic = true
-			opts.AllPublic = false // set it false to avoid returning too many repos, we could filter by indexer
-		}
-		repoIDs, _, err = repo_model.SearchRepositoryIDs(ctx, opts)
-		if err != nil {
+	repoIDs, allPublic, err := buildSearchIssuesRepoIDs(ctx)
+	if err != nil {
+		if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) {
+			ctx.APIError(http.StatusBadRequest, err)
+		} else {
 			ctx.APIErrorInternal(err)
-			return
-		}
-		if len(repoIDs) == 0 {
-			// no repos found, don't let the indexer return all repos
-			repoIDs = []int64{0}
 		}
+		return
 	}

 	keyword := ctx.FormTrim("q")
@@ -218,15 +203,7 @@ func SearchIssues(ctx *context.APIContext) {
 		keyword = ""
 	}

-	var isPull optional.Option[bool]
-	switch ctx.FormString("type") {
-	case "pulls":
-		isPull = optional.Some(true)
-	case "issues":
-		isPull = optional.Some(false)
-	default:
-		isPull = optional.None[bool]()
-	}
+	isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type"))

 	var includedAnyLabels []int64
 	{
@@ -256,14 +233,7 @@ func SearchIssues(ctx *context.APIContext) {
 		}
 	}

-	// this api is also used in UI,
-	// so the default limit is set to fit UI needs
-	limit := ctx.FormInt("limit")
-	if limit == 0 {
-		limit = setting.UI.IssuePagingNum
-	} else if limit > setting.API.MaxResponseItems {
-		limit = setting.API.MaxResponseItems
-	}
+	limit := util.IfZero(ctx.FormInt("limit"), setting.API.DefaultPagingNum)

 	searchOpt := &issue_indexer.SearchOptions{
 		Paginator: &db.ListOptions{
@@ -306,10 +276,6 @@ func SearchIssues(ctx *context.APIContext) {
 		}
 	}

-	// FIXME: It's unsupported to sort by priority repo when searching by indexer,
-	//        it's indeed an regression, but I think it is worth to support filtering by indexer first.
-	_ = ctx.FormInt64("priority_repo_id")
-
 	ids, total, err := issue_indexer.SearchIssues(ctx, searchOpt)
 	if err != nil {
 		ctx.APIErrorInternal(err)
@@ -409,16 +375,7 @@ func ListIssues(ctx *context.APIContext) {
 		return
 	}

-	var isClosed optional.Option[bool]
-	switch ctx.FormString("state") {
-	case "closed":
-		isClosed = optional.Some(true)
-	case "all":
-		isClosed = optional.None[bool]()
-	default:
-		isClosed = optional.Some(false)
-	}
-
+	isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
 	keyword := ctx.FormTrim("q")
 	if strings.IndexByte(keyword, 0) >= 0 {
 		keyword = ""
--- a/routers/api/v1/repo/milestone.go
+++ b/routers/api/v1/repo/milestone.go
@@ -10,7 +10,6 @@ import (

 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
-	"code.gitea.io/gitea/modules/optional"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/web"
@@ -60,12 +59,7 @@ func ListMilestones(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"

-	state := api.StateType(ctx.FormString("state"))
-	var isClosed optional.Option[bool]
-	switch state {
-	case api.StateClosed, api.StateOpen:
-		isClosed = optional.Some(state == api.StateClosed)
-	}
+	isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))

 	milestones, total, err := db.FindAndCount[issues_model.Milestone](ctx, issues_model.FindMilestoneOptions{
 		ListOptions: utils.GetListOptions(ctx),
--- a/routers/common/issue_filter.go
+++ b/routers/common/issue_filter.go
@@ -0,0 +1,25 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package common
+
+import (
+	"code.gitea.io/gitea/modules/optional"
+)
+
+func ParseIssueFilterStateIsClosed(state string) optional.Option[bool] {
+	switch state {
+	case "all":
+		return optional.None[bool]()
+	case "closed":
+		return optional.Some(true)
+	case "", "open":
+		return optional.Some(false)
+	default:
+		return optional.Some(false) // unknown state, undefined behavior
+	}
+}
+
+func ParseIssueFilterTypeIsPull(typ string) optional.Option[bool] {
+	return optional.FromMapLookup(map[string]bool{"pulls": true, "issues": false}, typ)
+}
--- a/routers/web/repo/issue_list.go
+++ b/routers/web/repo/issue_list.go
@@ -25,6 +25,7 @@ import (
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
+	"code.gitea.io/gitea/routers/common"
 	"code.gitea.io/gitea/routers/web/shared/issue"
 	shared_user "code.gitea.io/gitea/routers/web/shared/user"
 	"code.gitea.io/gitea/services/context"
@@ -45,15 +46,7 @@ func SearchIssues(ctx *context.Context) {
 		return
 	}

-	var isClosed optional.Option[bool]
-	switch ctx.FormString("state") {
-	case "closed":
-		isClosed = optional.Some(true)
-	case "all":
-		isClosed = optional.None[bool]()
-	default:
-		isClosed = optional.Some(false)
-	}
+	isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))

 	var (
 		repoIDs   []int64
@@ -268,15 +261,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) {
 		return
 	}

-	var isClosed optional.Option[bool]
-	switch ctx.FormString("state") {
-	case "closed":
-		isClosed = optional.Some(true)
-	case "all":
-		isClosed = optional.None[bool]()
-	default:
-		isClosed = optional.Some(false)
-	}
+	isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))

 	keyword := ctx.FormTrim("q")
 	if strings.IndexByte(keyword, 0) >= 0 {
@@ -580,17 +565,10 @@ func prepareIssueFilterAndList(ctx *context.Context, milestoneID, projectID int6
 		}
 	}

-	var isShowClosed optional.Option[bool]
-	switch ctx.FormString("state") {
-	case "closed":
-		isShowClosed = optional.Some(true)
-	case "all":
-		isShowClosed = optional.None[bool]()
-	default:
-		isShowClosed = optional.Some(false)
-	}
+	isShowClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
+
 	// if there are closed issues and no open issues, default to showing all issues
-	if len(ctx.FormString("state")) == 0 && issueStats.OpenCount == 0 && issueStats.ClosedCount != 0 {
+	if ctx.FormString("state") == "" && issueStats.OpenCount == 0 && issueStats.ClosedCount != 0 {
 		isShowClosed = optional.None[bool]()
 	}

--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -4235,13 +4235,6 @@
            "name": "q",
            "in": "query"
          },
-          {
-            "type": "integer",
-            "format": "int64",
-            "description": "Repository ID to prioritize in the results",
-            "name": "priority_repo_id",
-            "in": "query"
-          },
          {
            "enum": [
              "issues",
--- a/tests/integration/api_issue_test.go
+++ b/tests/integration/api_issue_test.go
@@ -19,6 +19,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/tests"

 	"github.com/stretchr/testify/assert"
@@ -264,9 +265,8 @@ func TestAPIEditIssue(t *testing.T) {

 func TestAPISearchIssues(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-
-	// as this API was used in the frontend, it uses UI page size
-	expectedIssueCount := min(20, setting.UI.IssuePagingNum) // 20 is from the fixtures
+	defer test.MockVariableValue(&setting.API.DefaultPagingNum, 20)()
+	expectedIssueCount := 20 // 20 is from the fixtures

 	link, _ := url.Parse("/api/v1/repos/issues/search")
 	token := getUserToken(t, "user1", auth_model.AccessTokenScopeReadIssue)