From 1c16f04bf5c181e39cb963c0f209e7651ca0e1c2 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sun, 23 Aug 2026 10:18:18 +0200 Subject: [PATCH] fix(db): make paginated database reads always require "order" option (#39017) Co-authored-by: wxiaoguang --- models/actions/artifact.go | 2 +- models/actions/run_job_list.go | 5 +- models/actions/scoped_workflow.go | 4 ++ models/actions/variable.go | 4 ++ models/asymkey/gpg_key.go | 4 ++ models/asymkey/ssh_key.go | 4 ++ models/asymkey/ssh_key_deploy.go | 4 ++ models/auth/source.go | 4 ++ models/db/iterate.go | 83 +++++++++++++++------- models/db/list.go | 37 ++++------ models/db/list_test.go | 4 ++ models/issues/assignees.go | 4 ++ models/issues/comment.go | 4 ++ models/project/project.go | 2 +- models/repo/collaboration.go | 4 ++ models/repo/pushmirror.go | 8 +++ models/repo/user_repo.go | 8 +++ models/secret/secret.go | 4 ++ models/user/badge.go | 2 +- models/user/block.go | 4 ++ models/user/external_login_user.go | 6 +- models/user/search.go | 4 ++ models/webhook/webhook.go | 4 ++ models/webhook/webhook_system.go | 4 ++ routers/web/devtest/devtest.go | 2 +- services/auth/source/oauth2/source_sync.go | 3 +- services/feed/feed_test.go | 2 +- services/repository/fork.go | 4 ++ tests/integration/oauth_test.go | 2 +- 29 files changed, 161 insertions(+), 65 deletions(-) diff --git a/models/actions/artifact.go b/models/actions/artifact.go index 442c36d875d..e550227d034 100644 --- a/models/actions/artifact.go +++ b/models/actions/artifact.go @@ -161,7 +161,7 @@ func (opts FindArtifactsOptions) ToOrders() string { return "id" } -var _ db.FindOptionsOrder = (*FindArtifactsOptions)(nil) +var _ db.FindOptions = (*FindArtifactsOptions)(nil) func (opts FindArtifactsOptions) ToConds() builder.Cond { cond := builder.NewCond() diff --git a/models/actions/run_job_list.go b/models/actions/run_job_list.go index 5b3db940a45..62d16334540 100644 --- a/models/actions/run_job_list.go +++ b/models/actions/run_job_list.go @@ -13,6 +13,7 @@ import ( "gitea.dev/modules/container" "gitea.dev/modules/optional" "gitea.dev/modules/timeutil" + "gitea.dev/modules/util" "xorm.io/builder" ) @@ -155,10 +156,10 @@ func (opts FindRunJobOptions) ToJoins() []db.JoinFunc { } func (opts FindRunJobOptions) ToOrders() string { - return string(opts.OrderBy) + return util.IfZero(string(opts.OrderBy), "action_run_job.id") } -var _ db.FindOptionsOrder = FindRunJobOptions{} +var _ db.FindOptions = (*FindRunJobOptions)(nil) // CountRunJobsByRunAndAttemptID counts the jobs belonging to the given run attempt. // It is used to enforce MaxJobNumPerRun when reusable-workflow expansion inserts new jobs. diff --git a/models/actions/scoped_workflow.go b/models/actions/scoped_workflow.go index 310e1a041de..a2bce65ddad 100644 --- a/models/actions/scoped_workflow.go +++ b/models/actions/scoped_workflow.go @@ -53,6 +53,10 @@ type FindScopedWorkflowSourceOpts struct { SourceRepoID int64 } +func (opts FindScopedWorkflowSourceOpts) ToOrders() string { + return "id" +} + func (opts FindScopedWorkflowSourceOpts) ToConds() builder.Cond { cond := builder.NewCond() if len(opts.OwnerIDs) > 0 { diff --git a/models/actions/variable.go b/models/actions/variable.go index 839b8129ad3..f566c805816 100644 --- a/models/actions/variable.go +++ b/models/actions/variable.go @@ -79,6 +79,10 @@ type FindVariablesOpts struct { Name string } +func (opts FindVariablesOpts) ToOrders() string { + return "name" +} + func (opts FindVariablesOpts) ToConds() builder.Cond { cond := builder.NewCond() diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 77034eb0892..64685e4cc24 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -75,6 +75,10 @@ type FindGPGKeyOptions struct { IncludeSubKeys bool } +func (opts FindGPGKeyOptions) ToOrders() string { + return "id" +} + func (opts FindGPGKeyOptions) ToConds() builder.Cond { cond := builder.NewCond() if !opts.IncludeSubKeys { diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index ff7a9b12039..1c7828dd18e 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -184,6 +184,10 @@ type FindPublicKeyOptions struct { LoginSourceID int64 } +func (opts FindPublicKeyOptions) ToOrders() string { + return "id" +} + func (opts FindPublicKeyOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.OwnerID > 0 { diff --git a/models/asymkey/ssh_key_deploy.go b/models/asymkey/ssh_key_deploy.go index 41ec6d82dce..5847fbb495b 100644 --- a/models/asymkey/ssh_key_deploy.go +++ b/models/asymkey/ssh_key_deploy.go @@ -158,6 +158,10 @@ type ListDeployKeysOptions struct { Fingerprint string } +func (opt ListDeployKeysOptions) ToOrders() string { + return "name" +} + func (opt ListDeployKeysOptions) ToConds() builder.Cond { cond := builder.NewCond() cond = cond.And(builder.Eq{"repo_id": opt.RepoID}) // repo ID must be used diff --git a/models/auth/source.go b/models/auth/source.go index 9bd6ce71f37..2533ab5040e 100644 --- a/models/auth/source.go +++ b/models/auth/source.go @@ -259,6 +259,10 @@ type FindSourcesOptions struct { LoginType Type } +func (opts FindSourcesOptions) ToOrders() string { + return "name" +} + func (opts FindSourcesOptions) ToConds() builder.Cond { conds := builder.NewCond() if opts.IsActive.Has() { diff --git a/models/db/iterate.go b/models/db/iterate.go index 171ad9f5576..bb83ffdd178 100644 --- a/models/db/iterate.go +++ b/models/db/iterate.go @@ -5,39 +5,72 @@ package db import ( "context" + "fmt" "gitea.dev/modules/setting" "xorm.io/builder" + "xorm.io/xorm/schemas" ) -// Iterate iterates all the Bean object -func Iterate[Bean any](ctx context.Context, cond builder.Cond, f func(ctx context.Context, bean *Bean) error) error { - var start int - batchSize := setting.Database.IterateBufferSize - sess := GetEngine(ctx) - for { - select { - case <-ctx.Done(): - return ctx.Err() - default: - beans := make([]*Bean, 0, batchSize) - if cond != nil { - sess = sess.Where(cond) - } - if err := sess.Limit(batchSize, start).Find(&beans); err != nil { - return err - } - if len(beans) == 0 { - return nil - } - start += len(beans) +func iterateTableByColumn[Bean any](ctx context.Context, colName string, cond builder.Cond, f func(ctx context.Context, bean *Bean) error) error { + table, err := xormEngine.TableInfo(new(Bean)) + if err != nil { + return err + } - for _, bean := range beans { - if err := f(ctx, bean); err != nil { - return err - } + var col *schemas.Column + if colName == "" { + if len(table.PrimaryKeys) != 1 { + return fmt.Errorf("table %s has %d primary keys, only the table with exactly one primary key can be iterated", table.Name, len(table.PrimaryKeys)) + } + colName = table.PrimaryKeys[0] + } + + col = table.GetColumn(colName) + batchSize := setting.Database.IterateBufferSize + var lastColValue any + for { + if ctx.Err() != nil { + return ctx.Err() + } + + beans := make([]*Bean, 0, batchSize) + query := GetEngine(ctx).Table(table.Name).Asc(colName) + + batchCond := cond + if lastColValue != nil { + batchCond = builder.And(cond, builder.Gt{col.Name: lastColValue}) + } + if batchCond != nil { + query = query.Where(batchCond) + } + + if err := query.Limit(batchSize).Find(&beans); err != nil { + return err + } + if len(beans) == 0 { + return nil + } + + reflectVal, err := col.ValueOf(beans[len(beans)-1]) + if err != nil { + return err + } + lastColValue = reflectVal.Interface() + + for _, bean := range beans { + if err := f(ctx, bean); err != nil { + return err } } } } + +func IterateByColumn[Bean any](ctx context.Context, colName string, cond builder.Cond, f func(ctx context.Context, bean *Bean) error) error { + return iterateTableByColumn(ctx, colName, cond, f) +} + +func Iterate[Bean any](ctx context.Context, cond builder.Cond, f func(ctx context.Context, bean *Bean) error) error { + return iterateTableByColumn(ctx, "", cond, f) +} diff --git a/models/db/list.go b/models/db/list.go index e91a7054112..64a823addd2 100644 --- a/models/db/list.go +++ b/models/db/list.go @@ -38,10 +38,7 @@ type ListOptions struct { var ListOptionsAll = ListOptions{ListAll: true} -var ( - _ Paginator = &ListOptions{} - _ FindOptions = ListOptions{} -) +var _ Paginator = &ListOptions{} // GetSkipTake returns the skip and take values func (opts *ListOptions) GetSkipTake() (skip, take int) { @@ -117,6 +114,7 @@ type FindOptions interface { GetPageSize() int IsListAll() bool ToConds() builder.Cond + ToOrders() string } type JoinFunc func(sess Engine) error @@ -125,10 +123,6 @@ type FindOptionsJoin interface { ToJoins() []JoinFunc } -type FindOptionsOrder interface { - ToOrders() string -} - // Find represents a common find function which accept an options interface func Find[T any](ctx context.Context, opts FindOptions) ([]*T, error) { sess := GetEngine(ctx).Where(opts.ToConds()) @@ -140,12 +134,7 @@ func Find[T any](ctx context.Context, opts FindOptions) ([]*T, error) { } } } - if orderOpt, ok := opts.(FindOptionsOrder); ok { - if order := orderOpt.ToOrders(); order != "" { - sess.OrderBy(order) - } - } - + sess.OrderBy(opts.ToOrders()) page, pageSize := opts.GetPage(), opts.GetPageSize() if !opts.IsListAll() && pageSize > 0 { if page == 0 { @@ -167,15 +156,17 @@ func Find[T any](ctx context.Context, opts FindOptions) ([]*T, error) { // Count represents a common count function which accept an options interface func Count[T any](ctx context.Context, opts FindOptions) (int64, error) { - sess := GetEngine(ctx).Where(opts.ToConds()) - if joinOpt, ok := opts.(FindOptionsJoin); ok { - for _, joinFunc := range joinOpt.ToJoins() { - if err := joinFunc(sess); err != nil { - return 0, err + sess := GetEngine(ctx) + if opts != nil { + sess.Where(opts.ToConds()) + if joinOpt, ok := opts.(FindOptionsJoin); ok { + for _, joinFunc := range joinOpt.ToJoins() { + if err := joinFunc(sess); err != nil { + return 0, err + } } } } - var object T return sess.Count(&object) } @@ -194,11 +185,7 @@ func FindAndCount[T any](ctx context.Context, opts FindOptions) ([]*T, int64, er } } } - if orderOpt, ok := opts.(FindOptionsOrder); ok { - if order := orderOpt.ToOrders(); order != "" { - sess.OrderBy(order) - } - } + sess.OrderBy(opts.ToOrders()) findPageSize := defaultFindSliceSize if pageSize > 0 { diff --git a/models/db/list_test.go b/models/db/list_test.go index 959cb91bfb6..281be54ba12 100644 --- a/models/db/list_test.go +++ b/models/db/list_test.go @@ -18,6 +18,10 @@ type mockListOptions struct { db.ListOptions } +func (opts mockListOptions) ToOrders() string { + return "id" +} + func (opts mockListOptions) IsListAll() bool { return true } diff --git a/models/issues/assignees.go b/models/issues/assignees.go index 91823b4ec45..622df6ac34d 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -74,6 +74,10 @@ type AssignedIssuesOptions struct { RepoOwnerID int64 } +func (opts *AssignedIssuesOptions) ToOrders() string { + return "id" +} + func (opts *AssignedIssuesOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.AssigneeID != 0 { diff --git a/models/issues/comment.go b/models/issues/comment.go index 0307bb42da1..bf636d874bf 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1075,6 +1075,10 @@ type FindCommentsOptions struct { IsPull optional.Option[bool] } +func (opts FindCommentsOptions) ToOrders() string { + return "id" +} + // ToConds implements FindOptions interface func (opts FindCommentsOptions) ToConds() builder.Cond { cond := builder.NewCond() diff --git a/models/project/project.go b/models/project/project.go index 546409c5d5c..97c1bee27a7 100644 --- a/models/project/project.go +++ b/models/project/project.go @@ -236,7 +236,7 @@ func (opts SearchOptions) ToConds() builder.Cond { } func (opts SearchOptions) ToOrders() string { - return opts.OrderBy.String() + return util.IfZero(opts.OrderBy.String(), "id") } func GetSearchOrderByBySortType(sortType string) db.SearchOrderBy { diff --git a/models/repo/collaboration.go b/models/repo/collaboration.go index bfbf7b63888..cb79fedde54 100644 --- a/models/repo/collaboration.go +++ b/models/repo/collaboration.go @@ -43,6 +43,10 @@ type FindCollaborationOptions struct { CollaboratorID int64 } +func (opts *FindCollaborationOptions) ToOrders() string { + return "collaboration.id" +} + func (opts *FindCollaborationOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.RepoID != 0 { diff --git a/models/repo/pushmirror.go b/models/repo/pushmirror.go index fc2b1b9e1aa..8c1dd4970b3 100644 --- a/models/repo/pushmirror.go +++ b/models/repo/pushmirror.go @@ -38,6 +38,10 @@ type PushMirrorOptions struct { RemoteName string } +func (opts PushMirrorOptions) ToOrders() string { + return "id" +} + func (opts PushMirrorOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.RepoID > 0 { @@ -100,6 +104,10 @@ type findPushMirrorOptions struct { SyncOnCommit optional.Option[bool] } +func (opts findPushMirrorOptions) ToOrders() string { + return "id" +} + func (opts findPushMirrorOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.RepoID > 0 { diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index 741626eddd8..b0e11246503 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -27,6 +27,10 @@ type StarredReposOptions struct { Actor *user_model.User } +func (opts *StarredReposOptions) ToOrders() string { + return "`repository`.id" +} + func (opts *StarredReposOptions) ApplyPublicOnly(publicOnly bool) { if publicOnly { opts.IncludePrivate = false @@ -76,6 +80,10 @@ type WatchedReposOptions struct { Actor *user_model.User } +func (opts *WatchedReposOptions) ToOrders() string { + return "`repository`.id" +} + func (opts *WatchedReposOptions) ApplyPublicOnly(publicOnly bool) { if publicOnly { opts.IncludePrivate = false diff --git a/models/secret/secret.go b/models/secret/secret.go index 8d4cfd3e646..7c69ce3003f 100644 --- a/models/secret/secret.go +++ b/models/secret/secret.go @@ -108,6 +108,10 @@ type FindSecretsOptions struct { Name string } +func (opts FindSecretsOptions) ToOrders() string { + return "name" +} + func (opts FindSecretsOptions) ToConds() builder.Cond { cond := builder.NewCond() diff --git a/models/user/badge.go b/models/user/badge.go index 0858fd67656..54e327db3b7 100644 --- a/models/user/badge.go +++ b/models/user/badge.go @@ -245,7 +245,7 @@ func (opts *SearchBadgeOptions) ToConds() builder.Cond { } func (opts *SearchBadgeOptions) ToOrders() string { - return opts.OrderBy.String() + return util.IfZero(opts.OrderBy.String(), "id") } // SearchBadges returns badges based on the provided SearchBadgeOptions options diff --git a/models/user/block.go b/models/user/block.go index 6331b754a39..e4e4756cc0f 100644 --- a/models/user/block.go +++ b/models/user/block.go @@ -66,6 +66,10 @@ type FindBlockingOptions struct { BlockeeID int64 } +func (opts *FindBlockingOptions) ToOrders() string { + return "id" +} + func (opts *FindBlockingOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.BlockerID != 0 { diff --git a/models/user/external_login_user.go b/models/user/external_login_user.go index 1069b0f2836..06397d3bc0d 100644 --- a/models/user/external_login_user.go +++ b/models/user/external_login_user.go @@ -208,9 +208,5 @@ func (opts FindExternalUserOptions) ToConds() builder.Cond { } func (opts FindExternalUserOptions) ToOrders() string { - return opts.OrderBy -} - -func IterateExternalLogin(ctx context.Context, opts FindExternalUserOptions, f func(ctx context.Context, u *ExternalLoginUser) error) error { - return db.Iterate(ctx, opts.ToConds(), f) + return util.IfZero(opts.OrderBy, "external_id") } diff --git a/models/user/search.go b/models/user/search.go index 536b15527ec..1af5ab8274c 100644 --- a/models/user/search.go +++ b/models/user/search.go @@ -58,6 +58,10 @@ type SearchUserOptions struct { IncludeReserved bool } +func (opts *SearchUserOptions) ToOrders() string { + return "id" +} + func (opts *SearchUserOptions) ApplyPublicOnly(publicOnly bool) { if publicOnly { opts.Visible = []structs.VisibleType{structs.VisibleTypePublic} diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 893ec159279..89b74d0c62f 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -291,6 +291,10 @@ type ListWebhookOptions struct { IsActive optional.Option[bool] } +func (opts ListWebhookOptions) ToOrders() string { + return "id" +} + func (opts ListWebhookOptions) ToConds() builder.Cond { cond := builder.NewCond() if opts.RepoID != 0 { diff --git a/models/webhook/webhook_system.go b/models/webhook/webhook_system.go index 49e21bbf087..6f15892540f 100644 --- a/models/webhook/webhook_system.go +++ b/models/webhook/webhook_system.go @@ -20,6 +20,10 @@ type ListSystemWebhookOptions struct { IsSystem optional.Option[bool] } +func (opts ListSystemWebhookOptions) ToOrders() string { + return "id" +} + func (opts ListSystemWebhookOptions) ToConds() builder.Cond { cond := builder.NewCond() cond = cond.And(builder.Eq{"webhook.repo_id": 0}, builder.Eq{"webhook.owner_id": 0}) diff --git a/routers/web/devtest/devtest.go b/routers/web/devtest/devtest.go index e515545e083..e5e1ff385bf 100644 --- a/routers/web/devtest/devtest.go +++ b/routers/web/devtest/devtest.go @@ -58,7 +58,7 @@ func prepareMockDataGiteaUI(_ *context.Context) {} func prepareMockDataBadgeCommitSign(ctx *context.Context) { var commits []*asymkey.SignCommit - mockUsers, _ := db.Find[user_model.User](ctx, user_model.SearchUserOptions{ListOptions: db.ListOptions{PageSize: 1}}) + mockUsers, _ := db.Find[user_model.User](ctx, &user_model.SearchUserOptions{ListOptions: db.ListOptions{PageSize: 1}}) mockUser := mockUsers[0] commits = append(commits, &asymkey.SignCommit{ Verification: &asymkey.CommitVerification{}, diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 5b8511287e4..3827e81633f 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -40,8 +40,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { Expired: true, LoginSourceID: source.AuthSource.ID, } - - return user_model.IterateExternalLogin(ctx, opts, func(ctx context.Context, u *user_model.ExternalLoginUser) error { + return db.IterateByColumn(ctx, "external_id", opts.ToConds(), func(ctx context.Context, u *user_model.ExternalLoginUser) error { return source.refresh(ctx, provider, u) }) } diff --git a/services/feed/feed_test.go b/services/feed/feed_test.go index 9e066c4ebdc..b6dc4994e54 100644 --- a/services/feed/feed_test.go +++ b/services/feed/feed_test.go @@ -155,7 +155,7 @@ func TestRepoActions(t *testing.T) { OpType: activities_model.ActionCommentIssue, }) } - count, _ := db.Count[activities_model.Action](t.Context(), &db.ListOptions{}) + count, _ := db.Count[activities_model.Action](t.Context(), nil) assert.EqualValues(t, 3, count) actions, _, err := GetFeeds(t.Context(), activities_model.GetFeedsOptions{ RequestedRepo: repo, diff --git a/services/repository/fork.go b/services/repository/fork.go index 859f285b257..64af9b9956d 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -237,6 +237,10 @@ type findForksOptions struct { Doer *user_model.User } +func (opts findForksOptions) ToOrders() string { + return "id" +} + func (opts findForksOptions) ToConds() builder.Cond { cond := builder.Eq{"fork_id": opts.RepoID} if opts.Doer != nil && opts.Doer.IsAdmin { diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 535ef1994b3..13adb67e266 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -1395,7 +1395,7 @@ func testOAuthSourceSpecialChars(t *testing.T) { doc.Find(".external-login-link").Each(func(i int, s *goquery.Selection) { oauth2Links = append(oauth2Links, s.AttrOr("href", "")) }) - assert.Equal(t, []string{ + assert.ElementsMatch(t, []string{ "/user/oauth2/test%20space", "/user/oauth2/test+plus", }, oauth2Links)