fix(db): make paginated database reads always require "order" option (#39017)

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

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()

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.

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 {

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()
 

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 {

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 {

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

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() {

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)
+}

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 {

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
 }

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 {

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()

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 {

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 {

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 {

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

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()
 

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

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 {

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")
 }

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}

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 {

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})

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{},

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)
 	})
 }

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,

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 {

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)