From 70a5b583020a2a94e68aff4c335eccbed721c83a Mon Sep 17 00:00:00 2001 From: Gregorius Bima Kharisma Wicaksana <51526537+bimakw@users.noreply.github.com> Date: Thu, 18 Dec 2025 17:11:39 +0700 Subject: [PATCH 1/4] refactor: extract helper functions from SearchIssues - Add ErrOwnerOrgRequired custom error type to replace string comparison - Refactor error handling to use proper Go error pattern --- routers/api/v1/repo/issue.go | 183 +++++++++++++++++++---------------- 1 file changed, 102 insertions(+), 81 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 3a85054e61b49..f932f71961833 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -32,6 +32,100 @@ import ( issue_service "code.gitea.io/gitea/services/issue" ) +// ErrOwnerOrgRequired represents an error when owner organisation is required for filtering on team +type ErrOwnerOrgRequired struct{} + +// IsErrOwnerOrgRequired checks if an error is an ErrOwnerOrgRequired +func IsErrOwnerOrgRequired(err error) bool { + _, ok := err.(ErrOwnerOrgRequired) + return ok +} + +func (err ErrOwnerOrgRequired) Error() string { + return "owner organisation is required for filtering on team" +} + +// parseIssueIsClosed parses the "state" query parameter and returns the corresponding isClosed option +func parseIssueIsClosed(ctx *context.APIContext) optional.Option[bool] { + switch ctx.FormString("state") { + case "closed": + return optional.Some(true) + case "all": + return optional.None[bool]() + default: + return optional.Some(false) + } +} + +// parseIssueIsPull parses the "type" query parameter and returns the corresponding isPull option +func parseIssueIsPull(ctx *context.APIContext) optional.Option[bool] { + switch ctx.FormString("type") { + case "pulls": + return optional.Some(true) + case "issues": + return optional.Some(false) + default: + return optional.None[bool]() + } +} + +// 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) ([]int64, bool, error) { + var repoIDs []int64 + var allPublic bool + + 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, ErrOwnerOrgRequired{} + } + 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 @@ -136,81 +230,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) - } - - 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 - } + isClosed := parseIssueIsClosed(ctx) - 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 user_model.IsErrUserNotExist(err) || organization.IsErrTeamNotExist(err) || IsErrOwnerOrgRequired(err) { + 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 +247,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 := parseIssueIsPull(ctx) var includedAnyLabels []int64 { From 1d28fc26b4dc8d89f19b3129390a564e0ea43530 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 19 Dec 2025 12:46:33 +0800 Subject: [PATCH 2/4] use common function for issue filter --- routers/api/v1/repo/issue.go | 57 ++++---------------------------- routers/api/v1/repo/milestone.go | 8 +---- routers/common/issue_filter.go | 25 ++++++++++++++ routers/web/repo/issue_list.go | 34 ++++--------------- 4 files changed, 38 insertions(+), 86 deletions(-) create mode 100644 routers/common/issue_filter.go diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index f932f71961833..25848f74a9dcd 100644 --- 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,43 +33,6 @@ import ( issue_service "code.gitea.io/gitea/services/issue" ) -// ErrOwnerOrgRequired represents an error when owner organisation is required for filtering on team -type ErrOwnerOrgRequired struct{} - -// IsErrOwnerOrgRequired checks if an error is an ErrOwnerOrgRequired -func IsErrOwnerOrgRequired(err error) bool { - _, ok := err.(ErrOwnerOrgRequired) - return ok -} - -func (err ErrOwnerOrgRequired) Error() string { - return "owner organisation is required for filtering on team" -} - -// parseIssueIsClosed parses the "state" query parameter and returns the corresponding isClosed option -func parseIssueIsClosed(ctx *context.APIContext) optional.Option[bool] { - switch ctx.FormString("state") { - case "closed": - return optional.Some(true) - case "all": - return optional.None[bool]() - default: - return optional.Some(false) - } -} - -// parseIssueIsPull parses the "type" query parameter and returns the corresponding isPull option -func parseIssueIsPull(ctx *context.APIContext) optional.Option[bool] { - switch ctx.FormString("type") { - case "pulls": - return optional.Some(true) - case "issues": - return optional.Some(false) - default: - return optional.None[bool]() - } -} - // 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) ([]int64, bool, error) { @@ -101,7 +65,7 @@ func buildSearchIssuesRepoIDs(ctx *context.APIContext) ([]int64, bool, error) { } if ctx.FormString("team") != "" { if ctx.FormString("owner") == "" { - return nil, false, ErrOwnerOrgRequired{} + 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 { @@ -230,11 +194,11 @@ func SearchIssues(ctx *context.APIContext) { return } - isClosed := parseIssueIsClosed(ctx) + isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state")) repoIDs, allPublic, err := buildSearchIssuesRepoIDs(ctx) if err != nil { - if user_model.IsErrUserNotExist(err) || organization.IsErrTeamNotExist(err) || IsErrOwnerOrgRequired(err) { + if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) { ctx.APIError(http.StatusBadRequest, err) } else { ctx.APIErrorInternal(err) @@ -247,7 +211,7 @@ func SearchIssues(ctx *context.APIContext) { keyword = "" } - isPull := parseIssueIsPull(ctx) + isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type")) var includedAnyLabels []int64 { @@ -430,16 +394,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 = "" diff --git a/routers/api/v1/repo/milestone.go b/routers/api/v1/repo/milestone.go index 33fa7c4b1644c..2cd91b7caf961 100644 --- 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), diff --git a/routers/common/issue_filter.go b/routers/common/issue_filter.go new file mode 100644 index 0000000000000..caf1fefef54ab --- /dev/null +++ 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) +} diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index a11d35da1eb85..da0ba6c407db3 100644 --- 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]() } From 7e9d99a30a26629336c4aadc7b5f6bc52aaf7cb5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 19 Dec 2025 19:32:05 +0800 Subject: [PATCH 3/4] simplify --- routers/api/v1/repo/issue.go | 25 +++---------------------- templates/swagger/v1_json.tmpl | 7 ------- 2 files changed, 3 insertions(+), 29 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 25848f74a9dcd..41076fd99c8fd 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -35,10 +35,7 @@ import ( // 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) ([]int64, bool, error) { - var repoIDs []int64 - var allPublic bool - +func buildSearchIssuesRepoIDs(ctx *context.APIContext) (repoIDs []int64, allPublic bool, err error) { opts := repo_model.SearchRepoOptions{ Private: false, AllPublic: true, @@ -78,7 +75,7 @@ func buildSearchIssuesRepoIDs(ctx *context.APIContext) ([]int64, bool, error) { 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) + repoIDs, _, err = repo_model.SearchRepositoryIDs(ctx, opts) if err != nil { return nil, false, err } @@ -116,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 @@ -241,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{ @@ -291,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) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 056e05ae4d4b7..be6c4bdfd3119 100644 --- 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", From 601e0db8601bd725e879c87755bea9cfee33c119 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 20 Dec 2025 09:05:32 +0800 Subject: [PATCH 4/4] fix test --- tests/integration/api_issue_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 4f751032f8455..56bed7db0d3b0 100644 --- 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)