From eee37e05608f0c4032bc98290e09416f6af47fcf Mon Sep 17 00:00:00 2001 From: Giteabot Date: Thu, 20 Aug 2026 10:51:20 -0700 Subject: [PATCH] fix: avoid enumerating every public repository in issue search (#38992) (#39000) Backport #38992 by @lunny Both issue search endpoints resolve their repository filter with `SearchRepositoryIDs` and pass the result to the indexer as `RepoIDs`. They mean to leave public repositories to the indexer, but `SearchRepoOptions.AllPublic` is only read when `OwnerID > 0`, so without an `owner` filter the flag does nothing and every public repository is enumerated, without a `LIMIT`, into `repo_id IN (...)`. Those IDs are redundant, as `allPublic` is passed to the indexer, which already matches every public repository. On a large instance this binds tens of thousands of parameters and can fail in the driver, making the endpoint return 500 for every filter. Admins are worst hit, as `SearchRepositoryCondition` skips their accessible-repository condition and enumerates the whole table. Restrict the enumeration to private repositories. The result set is unchanged, as the dropped IDs are a subset of what `allPublic` matches. Both endpoints held copies of this block, so it moves to `routers/common`. Co-authored-by: Lunny Xiao Co-authored-by: silverwind Co-authored-by: wxiaoguang --- models/repo/repo_list.go | 9 +++ routers/api/v1/repo/issue.go | 78 ++------------------ routers/common/issue_filter.go | 62 ++++++++++++++++ routers/common/issue_filter_test.go | 93 ++++++++++++++++++++++++ routers/web/repo/issue_list.go | 109 +++++----------------------- 5 files changed, 189 insertions(+), 162 deletions(-) create mode 100644 routers/common/issue_filter_test.go diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index 95eca362350..077bbe0ac83 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -771,6 +771,15 @@ func PublicRepoUnderPublicOwnerCond() builder.Cond { ) } +// NotPublicRepoUnderPublicOwnerCond complements PublicRepoUnderPublicOwnerCond. Spelled positively so +// the owner subquery hashes the limited/private minority, not every public user. +func NotPublicRepoUnderPublicOwnerCond() builder.Cond { + return builder.Or( + builder.Eq{"`repository`.is_private": true}, + builder.In("`repository`.owner_id", builder.Select("id").From("`user`").Where(builder.Neq{"visibility": structs.VisibleTypePublic})), + ) +} + // UserActionsAccessibleOwnerRepoCond selects the repos owned by ownerID whose Actions `user` may read. // It is used to list an org/user's Actions runs and jobs (see the callers in routers/api/v1/shared). // - owner_id = ownerID: only that owner's repos. diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index fe5772fa469..7e06f7a29d3 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -14,7 +14,6 @@ import ( "gitea.dev/models/db" issues_model "gitea.dev/models/issues" - "gitea.dev/models/organization" access_model "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" @@ -33,61 +32,6 @@ import ( issue_service "gitea.dev/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 = true - opts.AllLimited = true - } - opts.ApplyPublicOnly(ctx.PublicOnly) - 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 @@ -193,7 +137,12 @@ func SearchIssues(ctx *context.APIContext) { isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state")) - repoIDs, allPublic, err := buildSearchIssuesRepoIDs(ctx) + repoIDs, allPublic, err := common.SearchIssuesRepoIDs(ctx, common.SearchIssuesRepoIDsOptions{ + Doer: ctx.Doer, + PublicOnly: ctx.PublicOnly, + OwnerName: ctx.FormString("owner"), + TeamName: ctx.FormString("team"), + }) if err != nil { if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) { ctx.APIError(http.StatusBadRequest, err.Error()) @@ -204,10 +153,6 @@ func SearchIssues(ctx *context.APIContext) { } keyword := ctx.FormTrim("q") - if strings.IndexByte(keyword, 0) >= 0 { - keyword = "" - } - isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type")) var includedAnyLabels []int64 @@ -390,9 +335,6 @@ func ListIssues(ctx *context.APIContext) { isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state")) keyword := ctx.FormTrim("q") - if strings.IndexByte(keyword, 0) >= 0 { - keyword = "" - } var labelIDs []int64 if splitted := strings.Split(ctx.FormString("labels"), ","); len(splitted) > 0 { @@ -435,13 +377,7 @@ func ListIssues(ctx *context.APIContext) { listOptions := utils.GetListOptions(ctx) - isPull := optional.None[bool]() - switch ctx.FormString("type") { - case "pulls": - isPull = optional.Some(true) - case "issues": - isPull = optional.Some(false) - } + isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type")) if isPull.Has() && !ctx.Repo.Permission.CanReadIssuesOrPulls(isPull.Value()) { ctx.APIErrorNotFound() diff --git a/routers/common/issue_filter.go b/routers/common/issue_filter.go index 4d93f2bd1ca..b11d5bc9b16 100644 --- a/routers/common/issue_filter.go +++ b/routers/common/issue_filter.go @@ -4,7 +4,13 @@ package common import ( + "context" + + "gitea.dev/models/organization" + repo_model "gitea.dev/models/repo" + user_model "gitea.dev/models/user" "gitea.dev/modules/optional" + "gitea.dev/modules/util" ) func ParseIssueFilterStateIsClosed(state string) optional.Option[bool] { @@ -23,3 +29,59 @@ func ParseIssueFilterStateIsClosed(state string) optional.Option[bool] { func ParseIssueFilterTypeIsPull(typ string) optional.Option[bool] { return optional.FromMapLookup(map[string]bool{"pulls": true, "issues": false}, typ) } + +type SearchIssuesRepoIDsOptions struct { + Doer *user_model.User + PublicOnly bool + OwnerName string + TeamName string +} + +// SearchIssuesRepoIDs resolves the repository filter of an issue search. allPublic makes the indexer +// match everything its own is_public covers (modules/indexer/issues/util.go), so repoIDs omits those. +func SearchIssuesRepoIDs(ctx context.Context, opts SearchIssuesRepoIDsOptions) (repoIDs []int64, allPublic bool, err error) { + searchOpts := repo_model.SearchRepoOptions{ + Private: opts.Doer != nil, + Collaborate: optional.None[bool](), + Actor: opts.Doer, + } + searchOpts.ApplyPublicOnly(opts.PublicOnly) + if opts.OwnerName != "" { + owner, err := user_model.GetUserByName(ctx, opts.OwnerName) + if err != nil { + return nil, false, err + } + searchOpts.OwnerID = owner.ID + searchOpts.Collaborate = optional.Some(false) + } + if opts.TeamName != "" { + if opts.OwnerName == "" { + return nil, false, util.NewInvalidArgumentErrorf("owner organisation is required for filtering on team") + } + team, err := organization.GetTeam(ctx, searchOpts.OwnerID, opts.TeamName) + if err != nil { + return nil, false, err + } + searchOpts.TeamID = team.ID + } + + // SearchRepoOptions.AllPublic and AllLimited only apply under an owner filter, so the indexer covers them + allPublic = opts.OwnerName == "" + cond := repo_model.SearchRepositoryCondition(searchOpts) + if allPublic { + if !searchOpts.Private { + return []int64{0}, allPublic, nil // sees nothing beyond is_public, so skip the query + } + cond = cond.And(repo_model.NotPublicRepoUnderPublicOwnerCond()) // enumerating them scales with the instance + } + repoIDs, err = repo_model.SearchRepositoryIDsByCondition(ctx, cond) + 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 +} diff --git a/routers/common/issue_filter_test.go b/routers/common/issue_filter_test.go new file mode 100644 index 00000000000..6cffeef45f4 --- /dev/null +++ b/routers/common/issue_filter_test.go @@ -0,0 +1,93 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "testing" + + "gitea.dev/models/unittest" + user_model "gitea.dev/models/user" + "gitea.dev/modules/util" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSearchIssuesRepoIDs(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + // the indexer's is_public covers repo 1 (public under a public owner) but misses repo 38 (public + // under a limited org) and repo 40 (public under a private org) + cases := []struct { + name string + doerID int64 + opts SearchIssuesRepoIDsOptions + allPublic bool + want []int64 + wantErr error + }{ + { + name: "site admin", // admins skip the accessible repository condition entirely + doerID: 1, + allPublic: true, + want: []int64{2, 38, 40}, + }, + { + name: "regular user", + doerID: 2, + allPublic: true, + want: []int64{2, 38}, + }, + { + name: "private org member", + doerID: 5, + allPublic: true, + want: []int64{38, 40}, + }, + { + name: "anonymous", + allPublic: true, + want: []int64{0}, // the placeholder keeps the indexer off "every repository" + }, + { + name: "public-only token", + doerID: 2, + opts: SearchIssuesRepoIDsOptions{PublicOnly: true}, + allPublic: true, + want: []int64{0}, + }, + { + name: "owner filter", // turns allPublic off, so public repos must still be enumerated + doerID: 2, + opts: SearchIssuesRepoIDsOptions{OwnerName: "user2"}, + want: []int64{1, 2}, + }, + { + name: "team without owner", + opts: SearchIssuesRepoIDsOptions{TeamName: "team1"}, + wantErr: util.ErrInvalidArgument, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + opts := tc.opts + if tc.doerID != 0 { + opts.Doer = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.doerID}) + } + + repoIDs, allPublic, err := SearchIssuesRepoIDs(t.Context(), opts) + if tc.wantErr != nil { + assert.ErrorIs(t, err, tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.allPublic, allPublic) + assert.Subset(t, repoIDs, tc.want) + if allPublic { + assert.NotContains(t, repoIDs, int64(1), "already matched by the indexer's is_public") + } + }) + } +} diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index 5ccef292a20..cc1bdc0e6d3 100644 --- a/routers/web/repo/issue_list.go +++ b/routers/web/repo/issue_list.go @@ -5,6 +5,7 @@ package repo import ( "bytes" + "errors" "maps" "net/http" "slices" @@ -15,7 +16,6 @@ import ( "gitea.dev/models/db" git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" - "gitea.dev/models/organization" repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" user_model "gitea.dev/models/user" @@ -56,85 +56,22 @@ func SearchIssues(ctx *context.Context) { 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 = true - 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.HTTPError(http.StatusBadRequest, "Owner not found", err.Error()) - } else { - ctx.HTTPError(http.StatusInternalServerError, "GetUserByName", err.Error()) - } - 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.HTTPError(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.HTTPError(http.StatusBadRequest, "Team not found", err.Error()) - } else { - ctx.HTTPError(http.StatusInternalServerError, "GetUserByName", err.Error()) - } - 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 { - ctx.HTTPError(http.StatusInternalServerError, "SearchRepositoryIDs", err.Error()) - return - } - if len(repoIDs) == 0 { - // no repos found, don't let the indexer return all repos - repoIDs = []int64{0} + repoIDs, allPublic, err := common.SearchIssuesRepoIDs(ctx, common.SearchIssuesRepoIDsOptions{ + Doer: ctx.Doer, + OwnerName: ctx.FormString("owner"), + TeamName: ctx.FormString("team"), + }) + if err != nil { + if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) { + ctx.HTTPError(http.StatusBadRequest, err.Error()) + } else { + ctx.ServerError("SearchIssuesRepoIDs", err) } + return } keyword := ctx.FormTrim("q") - if strings.IndexByte(keyword, 0) >= 0 { - keyword = "" - } - - isPull := optional.None[bool]() - switch ctx.FormString("type") { - case "pulls": - isPull = optional.Some(true) - case "issues": - isPull = optional.Some(false) - } + isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type")) var includedAnyLabels []int64 { @@ -145,7 +82,7 @@ func SearchIssues(ctx *context.Context) { } includedAnyLabels, err = issues_model.GetLabelIDsByNames(ctx, includedLabelNames) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "GetLabelIDsByNames", err.Error()) + ctx.ServerError("GetLabelIDsByNames", err) return } } @@ -159,7 +96,7 @@ func SearchIssues(ctx *context.Context) { } includedMilestones, err = issues_model.GetMilestoneIDsByNames(ctx, includedMilestoneNames) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "GetMilestoneIDsByNames", err.Error()) + ctx.ServerError("GetMilestoneIDsByNames", err) return } } @@ -223,12 +160,12 @@ func SearchIssues(ctx *context.Context) { ids, total, err := issue_indexer.SearchIssues(ctx, searchOpt) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "SearchIssues", err.Error()) + ctx.ServerError("SearchIssues", err) return } issues, err := issues_model.GetIssuesByIDs(ctx, ids, true) if err != nil { - ctx.HTTPError(http.StatusInternalServerError, "FindIssuesByIDs", err.Error()) + ctx.ServerError("FindIssuesByIDs", err) return } @@ -267,11 +204,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) { } isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state")) - keyword := ctx.FormTrim("q") - if strings.IndexByte(keyword, 0) >= 0 { - keyword = "" - } var mileIDs []int64 if part := strings.Split(ctx.FormString("milestones"), ","); len(part) > 0 { @@ -303,13 +236,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) { } } - isPull := optional.None[bool]() - switch ctx.FormString("type") { - case "pulls": - isPull = optional.Some(true) - case "issues": - isPull = optional.Some(false) - } + isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type")) // FIXME: we should be more efficient here createdByID := getUserIDForFilter(ctx, "created_by")