From d88c9f8f32b97f49cf978086654cec2187433292 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 17:38:38 +0800 Subject: [PATCH 01/16] Move more functions to db.Find --- models/asymkey/gpg_key.go | 22 ++++++++---------- models/asymkey/gpg_key_commit_verification.go | 5 +++- models/asymkey/ssh_key.go | 6 ++++- models/asymkey/ssh_key_principals.go | 23 ------------------- routers/api/v1/user/gpg_key.go | 9 ++++++-- routers/web/user/home.go | 5 +++- routers/web/user/setting/keys.go | 11 +++++++-- services/asymkey/sign.go | 20 ++++++++++++---- services/user/delete.go | 5 +++- 9 files changed, 59 insertions(+), 47 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 421f24d4de939..6124c2cd237e4 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -16,6 +16,7 @@ import ( "github.com/keybase/go-crypto/openpgp" "github.com/keybase/go-crypto/openpgp/packet" + "xorm.io/builder" "xorm.io/xorm" ) @@ -76,20 +77,17 @@ func PaddedKeyID(keyID string) string { return zeros[0:16-len(keyID)] + keyID } -// ListGPGKeys returns a list of public keys belongs to given user. -func ListGPGKeys(ctx context.Context, uid int64, listOptions db.ListOptions) ([]*GPGKey, error) { - sess := db.GetEngine(ctx).Table(&GPGKey{}).Where("owner_id=? AND primary_key_id=''", uid) - if listOptions.Page != 0 { - sess = db.SetSessionPagination(sess, &listOptions) - } - - keys := make([]*GPGKey, 0, 2) - return keys, sess.Find(&keys) +type FindGPGKeyOptions struct { + db.ListOptions + OwnerID int64 } -// CountUserGPGKeys return number of gpg keys a user own -func CountUserGPGKeys(ctx context.Context, userID int64) (int64, error) { - return db.GetEngine(ctx).Where("owner_id=? AND primary_key_id=''", userID).Count(&GPGKey{}) +func (opts FindGPGKeyOptions) ToConds() builder.Cond { + var cond builder.Cond = builder.Eq{"primary_key_id": ""} + if opts.OwnerID > 0 { + cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) + } + return cond } func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, error) { diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index 8ac436440453f..03f0f9a16a38c 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -166,7 +166,10 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific // Now try to associate the signature with the committer, if present if committer.ID != 0 { - keys, err := ListGPGKeys(ctx, committer.ID, db.ListOptions{}) + keys, err := db.Find[GPGKey](ctx, FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: committer.ID, + }) if err != nil { // Skipping failed to get gpg keys of user log.Error("ListGPGKeys: %v", err) return &CommitVerification{ diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index 552f2ffd69e6f..a14a45bfa72a0 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -197,7 +197,11 @@ func (opts FindPublicKeyOptions) ToConds() builder.Cond { cond = cond.And(builder.Eq{"fingerprint": opts.Fingerprint}) } if len(opts.KeyTypes) > 0 { - cond = cond.And(builder.In("type", opts.KeyTypes)) + if len(opts.KeyTypes) == 1 { + cond = cond.And(builder.Eq{"type": opts.KeyTypes[0]}) + } else { + cond = cond.And(builder.In("type", opts.KeyTypes)) + } } if opts.NotKeytype > 0 { cond = cond.And(builder.Neq{"type": opts.NotKeytype}) diff --git a/models/asymkey/ssh_key_principals.go b/models/asymkey/ssh_key_principals.go index 92789e26f8679..4e7dee2c91d06 100644 --- a/models/asymkey/ssh_key_principals.go +++ b/models/asymkey/ssh_key_principals.go @@ -15,15 +15,6 @@ import ( "code.gitea.io/gitea/modules/util" ) -// __________ .__ .__ .__ -// \______ _______|__| ____ ____ |_____________ | | ______ -// | ___\_ __ | |/ \_/ ___\| \____ \__ \ | | / ___/ -// | | | | \| | | \ \___| | |_> / __ \| |__\___ \ -// |____| |__| |__|___| /\___ |__| __(____ |____/____ > -// \/ \/ |__| \/ \/ -// -// This file contains functions related to principals - // AddPrincipalKey adds new principal to database and authorized_principals file. func AddPrincipalKey(ctx context.Context, ownerID int64, content string, authSourceID int64) (*PublicKey, error) { dbCtx, committer, err := db.TxContext(ctx) @@ -103,17 +94,3 @@ func CheckPrincipalKeyString(ctx context.Context, user *user_model.User, content return "", fmt.Errorf("didn't match allowed principals: %s", setting.SSH.AuthorizedPrincipalsAllow) } - -// ListPrincipalKeys returns a list of principals belongs to given user. -func ListPrincipalKeys(ctx context.Context, uid int64, listOptions db.ListOptions) ([]*PublicKey, error) { - sess := db.GetEngine(ctx).Where("owner_id = ? AND type = ?", uid, KeyTypePrincipal) - if listOptions.Page != 0 { - sess = db.SetSessionPagination(sess, &listOptions) - - keys := make([]*PublicKey, 0, listOptions.PageSize) - return keys, sess.Find(&keys) - } - - keys := make([]*PublicKey, 0, 5) - return keys, sess.Find(&keys) -} diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 4f8bcaca3e6d5..a6caeb4775baf 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -18,7 +18,10 @@ import ( ) func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) { - keys, err := asymkey_model.ListGPGKeys(ctx, uid, listOptions) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: listOptions, + OwnerID: uid, + }) if err != nil { ctx.Error(http.StatusInternalServerError, "ListGPGKeys", err) return @@ -29,7 +32,9 @@ func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) apiKeys[i] = convert.ToGPGKey(keys[i]) } - total, err := asymkey_model.CountUserGPGKeys(ctx, uid) + total, err := db.Count[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + OwnerID: uid, + }) if err != nil { ctx.InternalServerError(err) return diff --git a/routers/web/user/home.go b/routers/web/user/home.go index 204d4adbd4900..debbb9753c29b 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -663,7 +663,10 @@ func ShowSSHKeys(ctx *context.Context) { // ShowGPGKeys output all the public GPG keys of user by uid func ShowGPGKeys(ctx *context.Context) { - keys, err := asymkey_model.ListGPGKeys(ctx, ctx.ContextUser.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: ctx.ContextUser.ID, + }) if err != nil { ctx.ServerError("ListGPGKeys", err) return diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 0dfb506fa9676..1b2405fd3925d 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -277,7 +277,10 @@ func loadKeysData(ctx *context.Context) { } ctx.Data["ExternalKeys"] = externalKeys - gpgkeys, err := asymkey_model.ListGPGKeys(ctx, ctx.Doer.ID, db.ListOptions{}) + gpgkeys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: ctx.Doer.ID, + }) if err != nil { ctx.ServerError("ListGPGKeys", err) return @@ -288,7 +291,11 @@ func loadKeysData(ctx *context.Context) { // generate a new aes cipher using the csrfToken ctx.Data["TokenToSign"] = tokenToSign - principals, err := asymkey_model.ListPrincipalKeys(ctx, ctx.Doer.ID, db.ListOptions{}) + principals, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: ctx.Doer.ID, + KeyTypes: []asymkey_model.KeyType{asymkey_model.KeyTypePrincipal}, + }) if err != nil { ctx.ServerError("ListPrincipalKeys", err) return diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index 1598f32165349..7c882b0d2ca71 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -143,7 +143,10 @@ Loop: case always: break Loop case pubkey: - keys, err := asymkey_model.ListGPGKeys(ctx, u.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: u.ID, + }) if err != nil { return false, "", nil, err } @@ -179,7 +182,10 @@ Loop: case always: break Loop case pubkey: - keys, err := asymkey_model.ListGPGKeys(ctx, u.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: u.ID, + }) if err != nil { return false, "", nil, err } @@ -232,7 +238,10 @@ Loop: case always: break Loop case pubkey: - keys, err := asymkey_model.ListGPGKeys(ctx, u.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: u.ID, + }) if err != nil { return false, "", nil, err } @@ -294,7 +303,10 @@ Loop: case always: break Loop case pubkey: - keys, err := asymkey_model.ListGPGKeys(ctx, u.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: u.ID, + }) if err != nil { return false, "", nil, err } diff --git a/services/user/delete.go b/services/user/delete.go index c4617e064e782..09f15cd330847 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -159,7 +159,10 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) // ***** END: PublicKey ***** // ***** START: GPGPublicKey ***** - keys, err := asymkey_model.ListGPGKeys(ctx, u.ID, db.ListOptions{}) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + ListOptions: db.ListOptionsAll, + OwnerID: u.ID, + }) if err != nil { return fmt.Errorf("ListGPGKeys: %w", err) } From 2ce7fd03f96788496ffac0b69575ddb1e09da99c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 17:51:33 +0800 Subject: [PATCH 02/16] More refactor --- models/asymkey/gpg_key.go | 10 ++++------ models/asymkey/gpg_key_commit_verification.go | 8 ++++++-- routers/api/v1/user/gpg_key.go | 6 ++++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 6124c2cd237e4..ca86251fd705d 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -80,6 +80,7 @@ func PaddedKeyID(keyID string) string { type FindGPGKeyOptions struct { db.ListOptions OwnerID int64 + KeyID string } func (opts FindGPGKeyOptions) ToConds() builder.Cond { @@ -87,6 +88,9 @@ func (opts FindGPGKeyOptions) ToConds() builder.Cond { if opts.OwnerID > 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } + if opts.KeyID != "" { + cond = cond.And(builder.Eq{"key_id": opts.KeyID}) + } return cond } @@ -101,12 +105,6 @@ func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, e return key, nil } -// GetGPGKeysByKeyID returns public key by given ID. -func GetGPGKeysByKeyID(ctx context.Context, keyID string) ([]*GPGKey, error) { - keys := make([]*GPGKey, 0, 1) - return keys, db.GetEngine(ctx).Where("key_id=?", keyID).Find(&keys) -} - // GPGKeyToEntity retrieve the imported key and the traducted entity func GPGKeyToEntity(ctx context.Context, k *GPGKey) (*openpgp.Entity, error) { impKey, err := GetGPGImportByKeyID(ctx, k.KeyID) diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index 03f0f9a16a38c..bd42f688969ad 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -395,7 +395,9 @@ func hashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s if keyID == "" { return nil } - keys, err := GetGPGKeysByKeyID(ctx, keyID) + keys, err := db.Find[GPGKey](ctx, FindGPGKeyOptions{ + KeyID: keyID, + }) if err != nil { log.Error("GetGPGKeysByKeyID: %v", err) return &CommitVerification{ @@ -410,7 +412,9 @@ func hashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s for _, key := range keys { var primaryKeys []*GPGKey if key.PrimaryKeyID != "" { - primaryKeys, err = GetGPGKeysByKeyID(ctx, key.PrimaryKeyID) + primaryKeys, err = db.Find[GPGKey](ctx, FindGPGKeyOptions{ + KeyID: key.PrimaryKeyID, + }) if err != nil { log.Error("GetGPGKeysByKeyID: %v", err) return &CommitVerification{ diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index a6caeb4775baf..75ef93d3536fa 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -203,7 +203,9 @@ func VerifyUserGPGKey(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "VerifyUserGPGKey", err) } - key, err := asymkey_model.GetGPGKeysByKeyID(ctx, form.KeyID) + keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ + KeyID: form.KeyID, + }) if err != nil { if asymkey_model.IsErrGPGKeyNotExist(err) { ctx.NotFound() @@ -212,7 +214,7 @@ func VerifyUserGPGKey(ctx *context.APIContext) { } return } - ctx.JSON(http.StatusOK, convert.ToGPGKey(key[0])) + ctx.JSON(http.StatusOK, convert.ToGPGKey(keys[0])) } // swagger:parameters userCurrentPostGPGKey From bb5042f32572842ed8b4f92d2594e0d86414bb44 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 21:30:47 +0800 Subject: [PATCH 03/16] more refactor --- models/db/list.go | 43 +++++++++++++++--------- models/issues/tracked_time.go | 20 ++++++++--- models/repo/collaboration.go | 50 ++++++++++++---------------- models/repo/collaboration_test.go | 12 +++++-- models/repo/fork.go | 11 +++--- routers/api/v1/repo/collaborators.go | 5 ++- 6 files changed, 85 insertions(+), 56 deletions(-) diff --git a/models/db/list.go b/models/db/list.go index b2f932e89bf8e..8c1ca868ebde6 100644 --- a/models/db/list.go +++ b/models/db/list.go @@ -25,13 +25,6 @@ type Paginator interface { IsListAll() bool } -// GetPaginatedSession creates a paginated database session -func GetPaginatedSession(p Paginator) *xorm.Session { - skip, take := p.GetSkipTake() - - return x.Limit(take, skip) -} - // SetSessionPagination sets pagination for a database session func SetSessionPagination(sess Engine, p Paginator) *xorm.Session { skip, take := p.GetSkipTake() @@ -39,13 +32,6 @@ func SetSessionPagination(sess Engine, p Paginator) *xorm.Session { return sess.Limit(take, skip) } -// SetEnginePagination sets pagination for a database engine -func SetEnginePagination(e Engine, p Paginator) Engine { - skip, take := p.GetSkipTake() - - return e.Limit(take, skip) -} - // ListOptions options to paginate results type ListOptions struct { PageSize int @@ -148,13 +134,29 @@ type FindOptions interface { ToConds() builder.Cond } +type JoinFunc func(sess Engine) error + +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()) + sess := GetEngine(ctx) + + if joinOpt, ok := opts.(FindOptionsJoin); ok && len(joinOpt.ToJoins()) > 0 { + for _, joinFunc := range joinOpt.ToJoins() { + if err := joinFunc(sess); err != nil { + return nil, err + } + } + } + + sess = sess.Where(opts.ToConds()) page, pageSize := opts.GetPage(), opts.GetPageSize() if !opts.IsListAll() && pageSize > 0 && page >= 1 { sess.Limit(pageSize, (page-1)*pageSize) @@ -176,8 +178,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) + if joinOpt, ok := opts.(FindOptionsJoin); ok && len(joinOpt.ToJoins()) > 0 { + for _, joinFunc := range joinOpt.ToJoins() { + if err := joinFunc(sess); err != nil { + return 0, err + } + } + } + var object T - return GetEngine(ctx).Where(opts.ToConds()).Count(&object) + return sess.Where(opts.ToConds()).Count(&object) } // FindAndCount represents a common findandcount function which accept an options interface diff --git a/models/issues/tracked_time.go b/models/issues/tracked_time.go index 795bddeb347b8..521873bb7e20b 100644 --- a/models/issues/tracked_time.go +++ b/models/issues/tracked_time.go @@ -94,7 +94,7 @@ type FindTrackedTimesOptions struct { } // toCond will convert each condition into a xorm-Cond -func (opts *FindTrackedTimesOptions) toCond() builder.Cond { +func (opts *FindTrackedTimesOptions) ToConds() builder.Cond { cond := builder.NewCond().And(builder.Eq{"tracked_time.deleted": false}) if opts.IssueID != 0 { cond = cond.And(builder.Eq{"issue_id": opts.IssueID}) @@ -117,6 +117,18 @@ func (opts *FindTrackedTimesOptions) toCond() builder.Cond { return cond } +func (opts FindTrackedTimesOptions) ToJoins() []db.JoinFunc { + if opts.RepositoryID > 0 || opts.MilestoneID > 0 { + return []db.JoinFunc{ + func(e db.Engine) error { + e.Join("INNER", "issue", "issue.id = tracked_time.issue_id") + return nil + }, + } + } + return nil +} + // toSession will convert the given options to a xorm Session by using the conditions from toCond and joining with issue table if required func (opts *FindTrackedTimesOptions) toSession(e db.Engine) db.Engine { sess := e @@ -124,10 +136,10 @@ func (opts *FindTrackedTimesOptions) toSession(e db.Engine) db.Engine { sess = e.Join("INNER", "issue", "issue.id = tracked_time.issue_id") } - sess = sess.Where(opts.toCond()) + sess = sess.Where(opts.ToConds()) if opts.Page != 0 { - sess = db.SetEnginePagination(sess, opts) + sess = db.SetSessionPagination(sess, opts) } return sess @@ -141,7 +153,7 @@ func GetTrackedTimes(ctx context.Context, options *FindTrackedTimesOptions) (tra // CountTrackedTimes returns count of tracked times that fit to the given options. func CountTrackedTimes(ctx context.Context, opts *FindTrackedTimesOptions) (int64, error) { - sess := db.GetEngine(ctx).Where(opts.toCond()) + sess := db.GetEngine(ctx).Where(opts.ToConds()) if opts.RepositoryID > 0 || opts.MilestoneID > 0 { sess = sess.Join("INNER", "issue", "issue.id = tracked_time.issue_id") } diff --git a/models/repo/collaboration.go b/models/repo/collaboration.go index 2018ae2a7db15..e08bfc797a3a3 100644 --- a/models/repo/collaboration.go +++ b/models/repo/collaboration.go @@ -11,8 +11,8 @@ import ( "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" + "xorm.io/builder" ) // Collaboration represent the relation between an individual and a repository. @@ -37,35 +37,34 @@ type Collaborator struct { // GetCollaborators returns the collaborators for a repository func GetCollaborators(ctx context.Context, repoID int64, listOptions db.ListOptions) ([]*Collaborator, error) { - collaborations, err := getCollaborations(ctx, repoID, listOptions) + collaborations, err := db.Find[Collaboration](ctx, FindCollaborationOptions{ + ListOptions: listOptions, + RepoID: repoID, + }) if err != nil { return nil, fmt.Errorf("getCollaborations: %w", err) } collaborators := make([]*Collaborator, 0, len(collaborations)) + userIDs := make([]int64, 0, len(collaborations)) + for _, c := range collaborations { + userIDs = append(userIDs, c.UserID) + } + + usersMap := make(map[int64]*user_model.User) + if err := db.GetEngine(ctx).In("id", userIDs).Find(&usersMap); err != nil { + return nil, fmt.Errorf("getCollaborations: %w", err) + } + for _, c := range collaborations { - user, err := user_model.GetUserByID(ctx, c.UserID) - if err != nil { - if user_model.IsErrUserNotExist(err) { - log.Warn("Inconsistent DB: User: %d is listed as collaborator of %-v but does not exist", c.UserID, repoID) - user = user_model.NewGhostUser() - } else { - return nil, err - } - } collaborators = append(collaborators, &Collaborator{ - User: user, + User: usersMap[c.UserID], Collaboration: c, }) } return collaborators, nil } -// CountCollaborators returns total number of collaborators for a repository -func CountCollaborators(ctx context.Context, repoID int64) (int64, error) { - return db.GetEngine(ctx).Where("repo_id = ? ", repoID).Count(&Collaboration{}) -} - // GetCollaboration get collaboration for a repository id with a user id func GetCollaboration(ctx context.Context, repoID, uid int64) (*Collaboration, error) { collaboration := &Collaboration{ @@ -84,18 +83,13 @@ func IsCollaborator(ctx context.Context, repoID, userID int64) (bool, error) { return db.GetEngine(ctx).Get(&Collaboration{RepoID: repoID, UserID: userID}) } -func getCollaborations(ctx context.Context, repoID int64, listOptions db.ListOptions) ([]*Collaboration, error) { - if listOptions.Page == 0 { - collaborations := make([]*Collaboration, 0, 8) - return collaborations, db.GetEngine(ctx).Find(&collaborations, &Collaboration{RepoID: repoID}) - } - - e := db.GetEngine(ctx) - - e = db.SetEnginePagination(e, &listOptions) +type FindCollaborationOptions struct { + db.ListOptions + RepoID int64 +} - collaborations := make([]*Collaboration, 0, listOptions.PageSize) - return collaborations, e.Find(&collaborations, &Collaboration{RepoID: repoID}) +func (opts FindCollaborationOptions) ToConds() builder.Cond { + return builder.And(builder.Eq{"repo_id": opts.RepoID}) } // ChangeCollaborationAccessMode sets new access mode for the collaboration. diff --git a/models/repo/collaboration_test.go b/models/repo/collaboration_test.go index 38114c307f6e1..21a99dd5573e6 100644 --- a/models/repo/collaboration_test.go +++ b/models/repo/collaboration_test.go @@ -89,17 +89,23 @@ func TestRepository_CountCollaborators(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) - count, err := repo_model.CountCollaborators(db.DefaultContext, repo1.ID) + count, err := db.Count[repo_model.Collaboration](db.DefaultContext, repo_model.FindCollaborationOptions{ + RepoID: repo1.ID, + }) assert.NoError(t, err) assert.EqualValues(t, 2, count) repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 22}) - count, err = repo_model.CountCollaborators(db.DefaultContext, repo2.ID) + count, err = db.Count[repo_model.Collaboration](db.DefaultContext, repo_model.FindCollaborationOptions{ + RepoID: repo2.ID, + }) assert.NoError(t, err) assert.EqualValues(t, 2, count) // Non-existent repository. - count, err = repo_model.CountCollaborators(db.DefaultContext, unittest.NonexistentID) + count, err = db.Count[repo_model.Collaboration](db.DefaultContext, repo_model.FindCollaborationOptions{ + RepoID: unittest.NonexistentID, + }) assert.NoError(t, err) assert.EqualValues(t, 0, count) } diff --git a/models/repo/fork.go b/models/repo/fork.go index 6be6ebc3f523c..07cd31c2690a9 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -56,13 +56,16 @@ func GetUserFork(ctx context.Context, repoID, userID int64) (*Repository, error) // GetForks returns all the forks of the repository func GetForks(ctx context.Context, repo *Repository, listOptions db.ListOptions) ([]*Repository, error) { + sess := db.GetEngine(ctx) + + var forks []*Repository if listOptions.Page == 0 { - forks := make([]*Repository, 0, repo.NumForks) - return forks, db.GetEngine(ctx).Find(&forks, &Repository{ForkID: repo.ID}) + forks = make([]*Repository, 0, repo.NumForks) + } else { + forks = make([]*Repository, 0, listOptions.PageSize) + sess = db.SetSessionPagination(sess, &listOptions) } - sess := db.GetPaginatedSession(&listOptions) - forks := make([]*Repository, 0, listOptions.PageSize) return forks, sess.Find(&forks, &Repository{ForkID: repo.ID}) } diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 2538bcdbc628d..a222e50a5e537 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -8,6 +8,7 @@ import ( "errors" "net/http" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -53,7 +54,9 @@ func ListCollaborators(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - count, err := repo_model.CountCollaborators(ctx, ctx.Repo.Repository.ID) + count, err := db.Count[repo_model.Collaboration](ctx, repo_model.FindCollaborationOptions{ + RepoID: ctx.Repo.Repository.ID, + }) if err != nil { ctx.InternalServerError(err) return From 841c59b8db559cfda75f8b328f3a445ec8761196 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 21:40:28 +0800 Subject: [PATCH 04/16] fix import order --- models/repo/collaboration.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/repo/collaboration.go b/models/repo/collaboration.go index e08bfc797a3a3..254537f62002c 100644 --- a/models/repo/collaboration.go +++ b/models/repo/collaboration.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/timeutil" + "xorm.io/builder" ) From 8d78985bcd43014957a6697e3601643991946173 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 11 Dec 2023 22:52:45 +0800 Subject: [PATCH 05/16] Some refactors --- cmd/admin.go | 4 +-- models/repo/release.go | 42 ++++------------------ modules/context/repo.go | 6 ++-- modules/repository/repo.go | 3 +- routers/api/v1/repo/release.go | 6 ++-- routers/web/feed/release.go | 4 ++- routers/web/repo/release.go | 6 ++-- routers/web/repo/release_test.go | 4 ++- routers/web/repo/repo.go | 5 ++- services/convert/repository.go | 7 +++- services/migrations/gitea_uploader_test.go | 6 ++-- services/repository/push.go | 5 ++- tests/integration/mirror_pull_test.go | 12 ++++--- tests/integration/repo_tag_test.go | 3 +- 14 files changed, 57 insertions(+), 56 deletions(-) diff --git a/cmd/admin.go b/cmd/admin.go index 49d0e4ef74aca..f1799b24e495a 100644 --- a/cmd/admin.go +++ b/cmd/admin.go @@ -157,10 +157,10 @@ func runRepoSyncReleases(_ *cli.Context) error { } func getReleaseCount(ctx context.Context, id int64) (int64, error) { - return repo_model.GetReleaseCountByRepoID( + return db.Count[repo_model.Release]( ctx, - id, repo_model.FindReleasesOptions{ + RepoID: id, IncludeTags: true, }, ) diff --git a/models/repo/release.go b/models/repo/release.go index 223d3f2501922..cae4605ca51b2 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -225,6 +225,7 @@ func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, err // FindReleasesOptions describes the conditions to Find releases type FindReleasesOptions struct { db.ListOptions + RepoID int64 IncludeDrafts bool IncludeTags bool IsPreRelease util.OptionalBool @@ -233,9 +234,8 @@ type FindReleasesOptions struct { HasSha1 util.OptionalBool // useful to find draft releases which are created with existing tags } -func (opts *FindReleasesOptions) toConds(repoID int64) builder.Cond { - cond := builder.NewCond() - cond = cond.And(builder.Eq{"repo_id": repoID}) +func (opts FindReleasesOptions) ToConds() builder.Cond { + var cond builder.Cond = builder.Eq{"repo_id": opts.RepoID} if !opts.IncludeDrafts { cond = cond.And(builder.Eq{"is_draft": false}) @@ -262,18 +262,8 @@ func (opts *FindReleasesOptions) toConds(repoID int64) builder.Cond { return cond } -// GetReleasesByRepoID returns a list of releases of repository. -func GetReleasesByRepoID(ctx context.Context, repoID int64, opts FindReleasesOptions) ([]*Release, error) { - sess := db.GetEngine(ctx). - Desc("created_unix", "id"). - Where(opts.toConds(repoID)) - - if opts.PageSize != 0 { - sess = db.SetSessionPagination(sess, &opts.ListOptions) - } - - rels := make([]*Release, 0, opts.PageSize) - return rels, sess.Find(&rels) +func (opts FindReleasesOptions) ToOrders() string { + return "created_unix DESC, id DESC" } // GetTagNamesByRepoID returns a list of release tag names of repository. @@ -286,23 +276,19 @@ func GetTagNamesByRepoID(ctx context.Context, repoID int64) ([]string, error) { IncludeDrafts: true, IncludeTags: true, HasSha1: util.OptionalBoolTrue, + RepoID: repoID, } tags := make([]string, 0) sess := db.GetEngine(ctx). Table("release"). Desc("created_unix", "id"). - Where(opts.toConds(repoID)). + Where(opts.ToConds()). Cols("tag_name") return tags, sess.Find(&tags) } -// CountReleasesByRepoID returns a number of releases matching FindReleaseOptions and RepoID. -func CountReleasesByRepoID(ctx context.Context, repoID int64, opts FindReleasesOptions) (int64, error) { - return db.GetEngine(ctx).Where(opts.toConds(repoID)).Count(new(Release)) -} - // GetLatestReleaseByRepoID returns the latest release for a repository func GetLatestReleaseByRepoID(ctx context.Context, repoID int64) (*Release, error) { cond := builder.NewCond(). @@ -325,20 +311,6 @@ func GetLatestReleaseByRepoID(ctx context.Context, repoID int64) (*Release, erro return rel, nil } -// GetReleasesByRepoIDAndNames returns a list of releases of repository according repoID and tagNames. -func GetReleasesByRepoIDAndNames(ctx context.Context, repoID int64, tagNames []string) (rels []*Release, err error) { - err = db.GetEngine(ctx). - In("tag_name", tagNames). - Desc("created_unix"). - Find(&rels, Release{RepoID: repoID}) - return rels, err -} - -// GetReleaseCountByRepoID returns the count of releases of repository -func GetReleaseCountByRepoID(ctx context.Context, repoID int64, opts FindReleasesOptions) (int64, error) { - return db.GetEngine(ctx).Where(opts.toConds(repoID)).Count(&Release{}) -} - type releaseMetaSearch struct { ID []int64 Rel []*Release diff --git a/modules/context/repo.go b/modules/context/repo.go index a18dc873b6444..8911d9fe1785b 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -536,18 +536,20 @@ func RepoAssignment(ctx *Context) context.CancelFunc { ctx.Data["RepoExternalIssuesLink"] = unit.ExternalTrackerConfig().ExternalTrackerURL } - ctx.Data["NumTags"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{ + ctx.Data["NumTags"], err = db.Count[repo_model.Release](ctx, repo_model.FindReleasesOptions{ IncludeDrafts: true, IncludeTags: true, HasSha1: util.OptionalBoolTrue, // only draft releases which are created with existing tags + RepoID: ctx.Repo.Repository.ID, }) if err != nil { ctx.ServerError("GetReleaseCountByRepoID", err) return nil } - ctx.Data["NumReleases"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{ + ctx.Data["NumReleases"], err = db.Count[repo_model.Release](ctx, repo_model.FindReleasesOptions{ // only show draft releases for users who can write, read-only users shouldn't see draft releases. IncludeDrafts: ctx.Repo.CanWrite(unit_model.TypeReleases), + RepoID: ctx.Repo.Repository.ID, }) if err != nil { ctx.ServerError("GetReleaseCountByRepoID", err) diff --git a/modules/repository/repo.go b/modules/repository/repo.go index a9a2773501090..33363e4689bd1 100644 --- a/modules/repository/repo.go +++ b/modules/repository/repo.go @@ -299,10 +299,11 @@ func SyncReleasesWithTags(ctx context.Context, repo *repo_model.Repository, gitR IncludeDrafts: true, IncludeTags: true, ListOptions: db.ListOptions{PageSize: 50}, + RepoID: repo.ID, } for page := 1; ; page++ { opts.Page = page - rels, err := repo_model.GetReleasesByRepoID(gitRepo.Ctx, repo.ID, opts) + rels, err := db.Find[repo_model.Release](gitRepo.Ctx, opts) if err != nil { return fmt.Errorf("unable to GetReleasesByRepoID in Repo[%d:%s/%s]: %w", repo.ID, repo.OwnerName, repo.Name, err) } diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index b1d3b5f45717d..a41c5ba7d8550 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -7,6 +7,7 @@ import ( "net/http" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -154,9 +155,10 @@ func ListReleases(ctx *context.APIContext) { IncludeTags: false, IsDraft: ctx.FormOptionalBool("draft"), IsPreRelease: ctx.FormOptionalBool("pre-release"), + RepoID: ctx.Repo.Repository.ID, } - releases, err := repo_model.GetReleasesByRepoID(ctx, ctx.Repo.Repository.ID, opts) + releases, err := db.Find[repo_model.Release](ctx, opts) if err != nil { ctx.Error(http.StatusInternalServerError, "GetReleasesByRepoID", err) return @@ -170,7 +172,7 @@ func ListReleases(ctx *context.APIContext) { rels[i] = convert.ToAPIRelease(ctx, ctx.Repo.Repository, release) } - filteredCount, err := repo_model.CountReleasesByRepoID(ctx, ctx.Repo.Repository.ID, opts) + filteredCount, err := db.Count[repo_model.Release](ctx, opts) if err != nil { ctx.InternalServerError(err) return diff --git a/routers/web/feed/release.go b/routers/web/feed/release.go index fbfa11c63ecff..57b0c927665d0 100644 --- a/routers/web/feed/release.go +++ b/routers/web/feed/release.go @@ -6,6 +6,7 @@ package feed import ( "time" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/context" @@ -14,8 +15,9 @@ import ( // shows tags and/or releases on the repo as RSS / Atom feed func ShowReleaseFeed(ctx *context.Context, repo *repo_model.Repository, isReleasesOnly bool, formatType string) { - releases, err := repo_model.GetReleasesByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{ + releases, err := db.Find[repo_model.Release](ctx, repo_model.FindReleasesOptions{ IncludeTags: !isReleasesOnly, + RepoID: ctx.Repo.Repository.ID, }) if err != nil { ctx.ServerError("GetReleasesByRepoID", err) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 595d599fe1312..86cb93e49b07e 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -95,9 +95,10 @@ func Releases(ctx *context.Context) { ListOptions: listOptions, // only show draft releases for users who can write, read-only users shouldn't see draft releases. IncludeDrafts: writeAccess, + RepoID: ctx.Repo.Repository.ID, } - releases, err := repo_model.GetReleasesByRepoID(ctx, ctx.Repo.Repository.ID, opts) + releases, err := db.Find[repo_model.Release](ctx, opts) if err != nil { ctx.ServerError("GetReleasesByRepoID", err) return @@ -194,9 +195,10 @@ func TagsList(ctx *context.Context) { IncludeDrafts: true, IncludeTags: true, HasSha1: util.OptionalBoolTrue, + RepoID: ctx.Repo.Repository.ID, } - releases, err := repo_model.GetReleasesByRepoID(ctx, ctx.Repo.Repository.ID, opts) + releases, err := db.Find[repo_model.Release](ctx, opts) if err != nil { ctx.ServerError("GetReleasesByRepoID", err) return diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index a5a923d464ab9..c4a2c1904e3e0 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -6,6 +6,7 @@ package repo import ( "testing" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" @@ -74,8 +75,9 @@ func TestCalReleaseNumCommitsBehind(t *testing.T) { contexttest.LoadGitRepo(t, ctx) t.Cleanup(func() { ctx.Repo.GitRepo.Close() }) - releases, err := repo_model.GetReleasesByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{ + releases, err := db.Find[repo_model.Release](ctx, repo_model.FindReleasesOptions{ IncludeDrafts: ctx.Repo.CanWrite(unit.TypeReleases), + RepoID: ctx.Repo.Repository.ID, }) assert.NoError(t, err) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 482fbaac2bc57..1e9f3e14c46e7 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -377,7 +377,10 @@ func RedirectDownload(ctx *context.Context) { ) tagNames := []string{vTag} curRepo := ctx.Repo.Repository - releases, err := repo_model.GetReleasesByRepoIDAndNames(ctx, curRepo.ID, tagNames) + releases, err := db.Find[repo_model.Release](ctx, repo_model.FindReleasesOptions{ + RepoID: curRepo.ID, + TagNames: tagNames, + }) if err != nil { ctx.ServerError("RedirectDownload", err) return diff --git a/services/convert/repository.go b/services/convert/repository.go index 71038cd0629cb..c16180c0afc30 100644 --- a/services/convert/repository.go +++ b/services/convert/repository.go @@ -8,6 +8,7 @@ import ( "time" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -133,7 +134,11 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR return nil } - numReleases, _ := repo_model.GetReleaseCountByRepoID(ctx, repo.ID, repo_model.FindReleasesOptions{IncludeDrafts: false, IncludeTags: false}) + numReleases, _ := db.Count[repo_model.Release](ctx, repo_model.FindReleasesOptions{ + IncludeDrafts: false, + IncludeTags: false, + RepoID: repo.ID, + }) mirrorInterval := "" var mirrorUpdated time.Time diff --git a/services/migrations/gitea_uploader_test.go b/services/migrations/gitea_uploader_test.go index 7c291eabafa62..07efad2892992 100644 --- a/services/migrations/gitea_uploader_test.go +++ b/services/migrations/gitea_uploader_test.go @@ -83,22 +83,24 @@ func TestGiteaUploadRepo(t *testing.T) { assert.NoError(t, err) assert.Len(t, labels, 12) - releases, err := repo_model.GetReleasesByRepoID(db.DefaultContext, repo.ID, repo_model.FindReleasesOptions{ + releases, err := db.Find[repo_model.Release](db.DefaultContext, repo_model.FindReleasesOptions{ ListOptions: db.ListOptions{ PageSize: 10, Page: 0, }, IncludeTags: true, + RepoID: repo.ID, }) assert.NoError(t, err) assert.Len(t, releases, 8) - releases, err = repo_model.GetReleasesByRepoID(db.DefaultContext, repo.ID, repo_model.FindReleasesOptions{ + releases, err = db.Find[repo_model.Release](db.DefaultContext, repo_model.FindReleasesOptions{ ListOptions: db.ListOptions{ PageSize: 10, Page: 0, }, IncludeTags: false, + RepoID: repo.ID, }) assert.NoError(t, err) assert.Len(t, releases, 1) diff --git a/services/repository/push.go b/services/repository/push.go index b5388834c0bd1..52050d322f8e4 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -322,7 +322,10 @@ func pushUpdateAddTags(ctx context.Context, repo *repo_model.Repository, gitRepo lowerTags = append(lowerTags, strings.ToLower(tag)) } - releases, err := repo_model.GetReleasesByRepoIDAndNames(ctx, repo.ID, lowerTags) + releases, err := db.Find[repo_model.Release](ctx, repo_model.FindReleasesOptions{ + RepoID: repo.ID, + TagNames: lowerTags, + }) if err != nil { return fmt.Errorf("GetReleasesByRepoIDAndNames: %w", err) } diff --git a/tests/integration/mirror_pull_test.go b/tests/integration/mirror_pull_test.go index c02e16bfc0b67..1e0edd9a2dd07 100644 --- a/tests/integration/mirror_pull_test.go +++ b/tests/integration/mirror_pull_test.go @@ -58,8 +58,12 @@ func TestMirrorPull(t *testing.T) { assert.NoError(t, err) defer gitRepo.Close() - findOptions := repo_model.FindReleasesOptions{IncludeDrafts: true, IncludeTags: true} - initCount, err := repo_model.GetReleaseCountByRepoID(db.DefaultContext, mirror.ID, findOptions) + findOptions := repo_model.FindReleasesOptions{ + IncludeDrafts: true, + IncludeTags: true, + RepoID: mirror.ID, + } + initCount, err := db.Count[repo_model.Release](db.DefaultContext, findOptions) assert.NoError(t, err) assert.NoError(t, release_service.CreateRelease(gitRepo, &repo_model.Release{ @@ -82,7 +86,7 @@ func TestMirrorPull(t *testing.T) { ok := mirror_service.SyncPullMirror(ctx, mirror.ID) assert.True(t, ok) - count, err := repo_model.GetReleaseCountByRepoID(db.DefaultContext, mirror.ID, findOptions) + count, err := db.Count[repo_model.Release](db.DefaultContext, findOptions) assert.NoError(t, err) assert.EqualValues(t, initCount+1, count) @@ -93,7 +97,7 @@ func TestMirrorPull(t *testing.T) { ok = mirror_service.SyncPullMirror(ctx, mirror.ID) assert.True(t, ok) - count, err = repo_model.GetReleaseCountByRepoID(db.DefaultContext, mirror.ID, findOptions) + count, err = db.Count[repo_model.Release](db.DefaultContext, findOptions) assert.NoError(t, err) assert.EqualValues(t, initCount, count) } diff --git a/tests/integration/repo_tag_test.go b/tests/integration/repo_tag_test.go index 8667a6d6e9f51..5678dea048b3e 100644 --- a/tests/integration/repo_tag_test.go +++ b/tests/integration/repo_tag_test.go @@ -74,9 +74,10 @@ func TestCreateNewTagProtected(t *testing.T) { }) // Cleanup - releases, err := repo_model.GetReleasesByRepoID(db.DefaultContext, repo.ID, repo_model.FindReleasesOptions{ + releases, err := db.Find[repo_model.Release](db.DefaultContext, repo_model.FindReleasesOptions{ IncludeTags: true, TagNames: []string{"v-1", "v-1.1"}, + RepoID: repo.ID, }) assert.NoError(t, err) From 3ee36bea1c519b3afa590a461b684377004a96ba Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 13 Dec 2023 16:33:40 +0800 Subject: [PATCH 06/16] Fix test --- models/db/list.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/models/db/list.go b/models/db/list.go index 8c1ca868ebde6..ce5d03212927c 100644 --- a/models/db/list.go +++ b/models/db/list.go @@ -158,7 +158,10 @@ func Find[T any](ctx context.Context, opts FindOptions) ([]*T, error) { sess = sess.Where(opts.ToConds()) page, pageSize := opts.GetPage(), opts.GetPageSize() - if !opts.IsListAll() && pageSize > 0 && page >= 1 { + if !opts.IsListAll() && pageSize > 0 { + if page == 0 { + page = 1 + } sess.Limit(pageSize, (page-1)*pageSize) } if newOpt, ok := opts.(FindOptionsOrder); ok && newOpt.ToOrders() != "" { From 15288e3fe0299e64fda09534c1492d0343de2476 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 13 Dec 2023 16:42:08 +0800 Subject: [PATCH 07/16] Remove unused interface method --- models/db/list.go | 13 ------------- models/db/paginator/paginator_test.go | 3 --- models/repo/archiver.go | 13 +++---------- routers/api/v1/repo/pull.go | 28 ++++++++++++++------------- 4 files changed, 18 insertions(+), 39 deletions(-) diff --git a/models/db/list.go b/models/db/list.go index ce5d03212927c..4aeaf3e084508 100644 --- a/models/db/list.go +++ b/models/db/list.go @@ -21,7 +21,6 @@ const ( // Paginator is the base for different ListOptions types type Paginator interface { GetSkipTake() (skip, take int) - GetStartEnd() (start, end int) IsListAll() bool } @@ -52,13 +51,6 @@ func (opts *ListOptions) GetSkipTake() (skip, take int) { return (opts.Page - 1) * opts.PageSize, opts.PageSize } -// GetStartEnd returns the start and end of the ListOptions -func (opts *ListOptions) GetStartEnd() (start, end int) { - start, take := opts.GetSkipTake() - end = start + take - return start, end -} - func (opts ListOptions) GetPage() int { return opts.Page } @@ -121,11 +113,6 @@ func (opts *AbsoluteListOptions) GetSkipTake() (skip, take int) { return opts.skip, opts.take } -// GetStartEnd returns the start and end values -func (opts *AbsoluteListOptions) GetStartEnd() (start, end int) { - return opts.skip, opts.skip + opts.take -} - // FindOptions represents a find options type FindOptions interface { GetPage() int diff --git a/models/db/paginator/paginator_test.go b/models/db/paginator/paginator_test.go index a1117fc7a4455..20602212d9876 100644 --- a/models/db/paginator/paginator_test.go +++ b/models/db/paginator/paginator_test.go @@ -52,11 +52,8 @@ func TestPaginator(t *testing.T) { for _, c := range cases { skip, take := c.Paginator.GetSkipTake() - start, end := c.Paginator.GetStartEnd() assert.Equal(t, c.Skip, skip) assert.Equal(t, c.Take, take) - assert.Equal(t, c.Start, start) - assert.Equal(t, c.End, end) } } diff --git a/models/repo/archiver.go b/models/repo/archiver.go index 6d0ed42877f53..2b71b75b55beb 100644 --- a/models/repo/archiver.go +++ b/models/repo/archiver.go @@ -124,7 +124,7 @@ type FindRepoArchiversOption struct { OlderThan time.Duration } -func (opts FindRepoArchiversOption) toConds() builder.Cond { +func (opts FindRepoArchiversOption) ToConds() builder.Cond { cond := builder.NewCond() if opts.OlderThan > 0 { cond = cond.And(builder.Lt{"created_unix": time.Now().Add(-opts.OlderThan).Unix()}) @@ -132,15 +132,8 @@ func (opts FindRepoArchiversOption) toConds() builder.Cond { return cond } -// FindRepoArchives find repo archivers -func FindRepoArchives(ctx context.Context, opts FindRepoArchiversOption) ([]*RepoArchiver, error) { - archivers := make([]*RepoArchiver, 0, opts.PageSize) - start, limit := opts.GetSkipTake() - err := db.GetEngine(ctx).Where(opts.toConds()). - Asc("created_unix"). - Limit(limit, start). - Find(&archivers) - return archivers, err +func (opts FindRepoArchiversOption) ToOrders() string { + return "created_unix ASC" } // SetArchiveRepoState sets if a repo is archived diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index d6b9dddd9d7ba..4a4ca8372f1b9 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1329,17 +1329,20 @@ func GetPullRequestCommits(ctx *context.APIContext) { userCache := make(map[string]*user_model.User) - start, end := listOptions.GetStartEnd() + start, limit := listOptions.GetSkipTake() - if end > totalNumberOfCommits { - end = totalNumberOfCommits + if start+limit > totalNumberOfCommits { + limit = totalNumberOfCommits - start + } + if limit < 0 { + limit = 0 } verification := ctx.FormString("verification") == "" || ctx.FormBool("verification") files := ctx.FormString("files") == "" || ctx.FormBool("files") - apiCommits := make([]*api.Commit, 0, end-start) - for i := start; i < end; i++ { + apiCommits := make([]*api.Commit, 0, limit) + for i := start; i < start+limit; i++ { apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, baseGitRepo, commits[i], userCache, convert.ToCommitOptions{ Stat: true, @@ -1477,19 +1480,18 @@ func GetPullRequestFiles(ctx *context.APIContext) { totalNumberOfFiles := diff.NumFiles totalNumberOfPages := int(math.Ceil(float64(totalNumberOfFiles) / float64(listOptions.PageSize))) - start, end := listOptions.GetStartEnd() + start, limit := listOptions.GetSkipTake() - if end > totalNumberOfFiles { - end = totalNumberOfFiles + if start+limit > totalNumberOfFiles { + limit = totalNumberOfFiles - start } - lenFiles := end - start - if lenFiles < 0 { - lenFiles = 0 + if limit < 0 { + limit = 0 } - apiFiles := make([]*api.ChangedFile, 0, lenFiles) - for i := start; i < end; i++ { + apiFiles := make([]*api.ChangedFile, 0, limit) + for i := start; i < start+limit; i++ { apiFiles = append(apiFiles, convert.ToChangedFile(diff.Files[i], pr.HeadRepo, endCommitID)) } From 54ad0e7a0fe888be73f292f0b711706c4edf0e40 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 13 Dec 2023 17:27:37 +0800 Subject: [PATCH 08/16] fix lint --- services/repository/archiver/archiver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/repository/archiver/archiver.go b/services/repository/archiver/archiver.go index 9f1ea48dca400..15b39d7cdfa6f 100644 --- a/services/repository/archiver/archiver.go +++ b/services/repository/archiver/archiver.go @@ -346,7 +346,7 @@ func DeleteOldRepositoryArchives(ctx context.Context, olderThan time.Duration) e log.Trace("Doing: ArchiveCleanup") for { - archivers, err := repo_model.FindRepoArchives(ctx, repo_model.FindRepoArchiversOption{ + archivers, err := db.Find[repo_model.RepoArchiver](ctx, repo_model.FindRepoArchiversOption{ ListOptions: db.ListOptions{ PageSize: 100, Page: 1, From ade38602ce1c76234b10fcd2840b292c2caabbd7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 26 Dec 2023 13:49:07 +0800 Subject: [PATCH 09/16] Fix bug --- models/asymkey/gpg_key.go | 10 +++++++--- models/asymkey/gpg_key_commit_verification.go | 4 ++-- routers/api/v1/user/gpg_key.go | 18 ++++++------------ routers/web/user/home.go | 5 +++-- routers/web/user/setting/keys.go | 5 +++-- services/asymkey/sign.go | 16 ++++++++-------- services/user/delete.go | 4 ++-- 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index ca86251fd705d..995f7110c159f 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -79,12 +79,16 @@ func PaddedKeyID(keyID string) string { type FindGPGKeyOptions struct { db.ListOptions - OwnerID int64 - KeyID string + OwnerID int64 + KeyID string + IsPrimaryKeyIDEmpty bool } func (opts FindGPGKeyOptions) ToConds() builder.Cond { - var cond builder.Cond = builder.Eq{"primary_key_id": ""} + var cond builder.Cond = builder.NewCond() + if opts.IsPrimaryKeyIDEmpty { + cond = cond.And(builder.Eq{"primary_key_id": ""}) + } if opts.OwnerID > 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index bd42f688969ad..c16c132651e11 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -167,8 +167,8 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific // Now try to associate the signature with the committer, if present if committer.ID != 0 { keys, err := db.Find[GPGKey](ctx, FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: committer.ID, + OwnerID: committer.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { // Skipping failed to get gpg keys of user log.Error("ListGPGKeys: %v", err) diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 75ef93d3536fa..78edd4b39f193 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -18,10 +18,12 @@ import ( ) func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) { - keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: listOptions, - OwnerID: uid, - }) + opts := asymkey_model.FindGPGKeyOptions{ + ListOptions: listOptions, + OwnerID: uid, + IsPrimaryKeyIDEmpty: true, + } + keys, total, err := db.FindAndCount[asymkey_model.GPGKey](ctx, opts) if err != nil { ctx.Error(http.StatusInternalServerError, "ListGPGKeys", err) return @@ -32,14 +34,6 @@ func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) apiKeys[i] = convert.ToGPGKey(keys[i]) } - total, err := db.Count[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: uid, - }) - if err != nil { - ctx.InternalServerError(err) - return - } - ctx.SetTotalCountHeader(total) ctx.JSON(http.StatusOK, &apiKeys) } diff --git a/routers/web/user/home.go b/routers/web/user/home.go index debbb9753c29b..b3f41e998371d 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -664,8 +664,9 @@ func ShowSSHKeys(ctx *context.Context) { // ShowGPGKeys output all the public GPG keys of user by uid func ShowGPGKeys(ctx *context.Context) { keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: ctx.ContextUser.ID, + ListOptions: db.ListOptionsAll, + OwnerID: ctx.ContextUser.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { ctx.ServerError("ListGPGKeys", err) diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 1b2405fd3925d..14cbc80a485a1 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -278,8 +278,9 @@ func loadKeysData(ctx *context.Context) { ctx.Data["ExternalKeys"] = externalKeys gpgkeys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: ctx.Doer.ID, + ListOptions: db.ListOptionsAll, + OwnerID: ctx.Doer.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { ctx.ServerError("ListGPGKeys", err) diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index 7c882b0d2ca71..828603fc57b1e 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -144,8 +144,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: u.ID, + OwnerID: u.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { return false, "", nil, err @@ -183,8 +183,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: u.ID, + OwnerID: u.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { return false, "", nil, err @@ -239,8 +239,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: u.ID, + OwnerID: u.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { return false, "", nil, err @@ -304,8 +304,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: u.ID, + OwnerID: u.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { return false, "", nil, err diff --git a/services/user/delete.go b/services/user/delete.go index e920aa84ac0c3..2b6667a6a1ba0 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -160,8 +160,8 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) // ***** START: GPGPublicKey ***** keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: u.ID, + OwnerID: u.ID, + IsPrimaryKeyIDEmpty: true, }) if err != nil { return fmt.Errorf("ListGPGKeys: %w", err) From 680e8ab0b27d91352b8a228f0ed6fae30e68567d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 26 Dec 2023 15:13:46 +0800 Subject: [PATCH 10/16] Fix lint --- models/asymkey/gpg_key.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 995f7110c159f..c80637a566e66 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -85,7 +85,7 @@ type FindGPGKeyOptions struct { } func (opts FindGPGKeyOptions) ToConds() builder.Cond { - var cond builder.Cond = builder.NewCond() + cond := builder.NewCond() if opts.IsPrimaryKeyIDEmpty { cond = cond.And(builder.Eq{"primary_key_id": ""}) } From a9654a3f2941c3848cd5c6024740b020fb25cf5c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 31 Dec 2023 16:07:58 +0800 Subject: [PATCH 11/16] Fix GPGList loading --- models/asymkey/gpg_key.go | 18 ++------- models/asymkey/gpg_key_commit_verification.go | 9 +++++ models/asymkey/gpg_key_list.go | 38 +++++++++++++++++++ routers/api/v1/user/gpg_key.go | 9 +++++ routers/web/user/home.go | 5 +++ routers/web/user/setting/keys.go | 4 ++ 6 files changed, 69 insertions(+), 14 deletions(-) create mode 100644 models/asymkey/gpg_key_list.go diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index c80637a566e66..35a9d44487292 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -11,22 +11,13 @@ import ( "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "github.com/keybase/go-crypto/openpgp" "github.com/keybase/go-crypto/openpgp/packet" "xorm.io/builder" - "xorm.io/xorm" ) -// __________________ ________ ____ __. -// / _____/\______ \/ _____/ | |/ _|____ ___.__. -// / \ ___ | ___/ \ ___ | <_/ __ < | | -// \ \_\ \| | \ \_\ \ | | \ ___/\___ | -// \______ /|____| \______ / |____|__ \___ > ____| -// \/ \/ \/ \/\/ - // GPGKey represents a GPG key. type GPGKey struct { ID int64 `xorm:"pk autoincr"` @@ -55,12 +46,11 @@ func (key *GPGKey) BeforeInsert() { key.AddedUnix = timeutil.TimeStampNow() } -// AfterLoad is invoked from XORM after setting the values of all fields of this object. -func (key *GPGKey) AfterLoad(session *xorm.Session) { - err := session.Where("primary_key_id=?", key.KeyID).Find(&key.SubsKey) - if err != nil { - log.Error("Find Sub GPGkeys[%s]: %v", key.KeyID, err) +func (key *GPGKey) LoadSubKeys(ctx context.Context) error { + if err := db.GetEngine(ctx).Where("primary_key_id=?", key.KeyID).Find(&key.SubsKey); err != nil { + return fmt.Errorf("find Sub GPGkeys[%s]: %v", key.KeyID, err) } + return nil } // PaddedKeyID show KeyID padded to 16 characters diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index c16c132651e11..4370666e890c6 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -179,6 +179,15 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific } } + if err := GPGKeyList(keys).LoadSubKeys(ctx); err != nil { + log.Error("LoadSubKeys: %v", err) + return &CommitVerification{ + CommittingUser: committer, + Verified: false, + Reason: "gpg.error.failed_retrieval_gpg_keys", + } + } + committerEmailAddresses, _ := user_model.GetEmailAddresses(ctx, committer.ID) activated := false for _, e := range committerEmailAddresses { diff --git a/models/asymkey/gpg_key_list.go b/models/asymkey/gpg_key_list.go new file mode 100644 index 0000000000000..89548e495ec3f --- /dev/null +++ b/models/asymkey/gpg_key_list.go @@ -0,0 +1,38 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package asymkey + +import ( + "context" + + "code.gitea.io/gitea/models/db" +) + +type GPGKeyList []*GPGKey + +func (keys GPGKeyList) keyIDs() []string { + ids := make([]string, len(keys)) + for i, key := range keys { + ids[i] = key.KeyID + } + return ids +} + +func (keys GPGKeyList) LoadSubKeys(ctx context.Context) error { + subKeys := make([]*GPGKey, 0, len(keys)) + if err := db.GetEngine(ctx).In("primary_key_id", keys.keyIDs()).Find(&subKeys); err != nil { + return err + } + subKeysMap := make(map[string][]*GPGKey, len(subKeys)) + for _, key := range subKeys { + subKeysMap[key.PrimaryKeyID] = append(subKeysMap[key.PrimaryKeyID], key) + } + + for _, key := range keys { + if subKeys, ok := subKeysMap[key.KeyID]; ok { + key.SubsKey = subKeys + } + } + return nil +} diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 78edd4b39f193..522d1506b76d4 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -29,6 +29,11 @@ func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) return } + if err := asymkey_model.GPGKeyList(keys).LoadSubKeys(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "ListGPGKeys", err) + return + } + apiKeys := make([]*api.GPGKey, len(keys)) for i := range keys { apiKeys[i] = convert.ToGPGKey(keys[i]) @@ -120,6 +125,10 @@ func GetGPGKey(ctx *context.APIContext) { } return } + if err := key.LoadSubKeys(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadSubKeys", err) + return + } ctx.JSON(http.StatusOK, convert.ToGPGKey(key)) } diff --git a/routers/web/user/home.go b/routers/web/user/home.go index b3f41e998371d..a7b3c06c6db8a 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -673,6 +673,11 @@ func ShowGPGKeys(ctx *context.Context) { return } + if err := asymkey_model.GPGKeyList(keys).LoadSubKeys(ctx); err != nil { + ctx.ServerError("LoadSubKeys", err) + return + } + entities := make([]*openpgp.Entity, 0) failedEntitiesID := make([]string, 0) for _, k := range keys { diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 14cbc80a485a1..a7435c0ee8482 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -286,6 +286,10 @@ func loadKeysData(ctx *context.Context) { ctx.ServerError("ListGPGKeys", err) return } + if err := asymkey_model.GPGKeyList(gpgkeys).LoadSubKeys(ctx); err != nil { + ctx.ServerError("LoadSubKeys", err) + return + } ctx.Data["GPGKeys"] = gpgkeys tokenToSign := asymkey_model.VerificationToken(ctx.Doer, 1) From 54e026e9b8f3200bdb8314c4ded1a46c2a716050 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 31 Dec 2023 16:48:24 +0800 Subject: [PATCH 12/16] remove unnsecessary parameter for AfterLoad --- models/auth/webauthn.go | 3 +-- models/issues/comment.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/models/auth/webauthn.go b/models/auth/webauthn.go index d12713bd37c89..a65d2e1e343db 100644 --- a/models/auth/webauthn.go +++ b/models/auth/webauthn.go @@ -13,7 +13,6 @@ import ( "code.gitea.io/gitea/modules/util" "github.com/go-webauthn/webauthn/webauthn" - "xorm.io/xorm" ) // ErrWebAuthnCredentialNotExist represents a "ErrWebAuthnCRedentialNotExist" kind of error. @@ -83,7 +82,7 @@ func (cred *WebAuthnCredential) BeforeUpdate() { } // AfterLoad is invoked from XORM after setting the values of all fields of this object. -func (cred *WebAuthnCredential) AfterLoad(session *xorm.Session) { +func (cred *WebAuthnCredential) AfterLoad() { cred.LowerName = strings.ToLower(cred.Name) } diff --git a/models/issues/comment.go b/models/issues/comment.go index 7b068d49831ac..a1698d48246a0 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -28,7 +28,6 @@ import ( "code.gitea.io/gitea/modules/util" "xorm.io/builder" - "xorm.io/xorm" ) // ErrCommentNotExist represents a "CommentNotExist" kind of error. @@ -338,7 +337,7 @@ func (c *Comment) BeforeUpdate() { } // AfterLoad is invoked from XORM after setting the values of all fields of this object. -func (c *Comment) AfterLoad(session *xorm.Session) { +func (c *Comment) AfterLoad() { c.Patch = c.PatchQuoted if len(c.PatchQuoted) > 0 && c.PatchQuoted[0] == '"' { unquoted, err := strconv.Unquote(c.PatchQuoted) From bba80e68fa51bfb91915892db7bb129ddf94ff8d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 2 Jan 2024 15:37:45 +0800 Subject: [PATCH 13/16] Apply suggestions from code review Co-authored-by: delvh --- routers/api/v1/repo/pull.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 4a4ca8372f1b9..2b3eacbd3fdbc 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1331,12 +1331,8 @@ func GetPullRequestCommits(ctx *context.APIContext) { start, limit := listOptions.GetSkipTake() - if start+limit > totalNumberOfCommits { - limit = totalNumberOfCommits - start - } - if limit < 0 { - limit = 0 - } + limit = min(limit, totalNumberOfCommits-start) + limit = max(limit, 0) verification := ctx.FormString("verification") == "" || ctx.FormBool("verification") files := ctx.FormString("files") == "" || ctx.FormBool("files") @@ -1482,13 +1478,9 @@ func GetPullRequestFiles(ctx *context.APIContext) { start, limit := listOptions.GetSkipTake() - if start+limit > totalNumberOfFiles { - limit = totalNumberOfFiles - start - } + limit = min(limit, totalNumberOfFiles - start) - if limit < 0 { - limit = 0 - } + limit = max(limit, 0) apiFiles := make([]*api.ChangedFile, 0, limit) for i := start; i < start+limit; i++ { From f8a31cc795cb6f336ac21c4f110f55146a924417 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 2 Jan 2024 20:39:05 +0800 Subject: [PATCH 14/16] Fix lint --- routers/api/v1/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 2b3eacbd3fdbc..34129ad5958c8 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1478,7 +1478,7 @@ func GetPullRequestFiles(ctx *context.APIContext) { start, limit := listOptions.GetSkipTake() - limit = min(limit, totalNumberOfFiles - start) + limit = min(limit, totalNumberOfFiles-start) limit = max(limit, 0) From a59919fe28e508dd833768eaa77b7d8ac3f03db0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Jan 2024 12:13:47 +0800 Subject: [PATCH 15/16] Follow KN4CK3R's reviews --- models/asymkey/gpg_key.go | 9 +++++---- models/asymkey/gpg_key_commit_verification.go | 9 +++++---- models/asymkey/ssh_key.go | 8 ++------ models/issues/tracked_time.go | 2 +- models/repo/collaboration.go | 6 +++++- routers/api/v1/user/gpg_key.go | 8 ++++---- routers/web/user/home.go | 10 ++-------- routers/web/user/setting/keys.go | 5 ++--- services/asymkey/sign.go | 16 ++++++++-------- services/repository/push.go | 2 +- services/user/delete.go | 3 +-- 11 files changed, 36 insertions(+), 42 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 35a9d44487292..5236b2d4500e4 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -69,16 +69,17 @@ func PaddedKeyID(keyID string) string { type FindGPGKeyOptions struct { db.ListOptions - OwnerID int64 - KeyID string - IsPrimaryKeyIDEmpty bool + OwnerID int64 + KeyID string + IncludeSubKeys bool } func (opts FindGPGKeyOptions) ToConds() builder.Cond { cond := builder.NewCond() - if opts.IsPrimaryKeyIDEmpty { + if !opts.IncludeSubKeys { cond = cond.And(builder.Eq{"primary_key_id": ""}) } + if opts.OwnerID > 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index 4370666e890c6..83fbab5d36c8a 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -167,8 +167,7 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific // Now try to associate the signature with the committer, if present if committer.ID != 0 { keys, err := db.Find[GPGKey](ctx, FindGPGKeyOptions{ - OwnerID: committer.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: committer.ID, }) if err != nil { // Skipping failed to get gpg keys of user log.Error("ListGPGKeys: %v", err) @@ -405,7 +404,8 @@ func hashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s return nil } keys, err := db.Find[GPGKey](ctx, FindGPGKeyOptions{ - KeyID: keyID, + KeyID: keyID, + IncludeSubKeys: true, }) if err != nil { log.Error("GetGPGKeysByKeyID: %v", err) @@ -422,7 +422,8 @@ func hashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s var primaryKeys []*GPGKey if key.PrimaryKeyID != "" { primaryKeys, err = db.Find[GPGKey](ctx, FindGPGKeyOptions{ - KeyID: key.PrimaryKeyID, + KeyID: key.PrimaryKeyID, + IncludeSubKeys: true, }) if err != nil { log.Error("GetGPGKeysByKeyID: %v", err) diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index 1c37ab36e00c9..a409d8e841926 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -197,14 +197,10 @@ func (opts FindPublicKeyOptions) ToConds() builder.Cond { cond = cond.And(builder.Eq{"fingerprint": opts.Fingerprint}) } if len(opts.KeyTypes) > 0 { - if len(opts.KeyTypes) == 1 { - cond = cond.And(builder.Eq{"type": opts.KeyTypes[0]}) - } else { - cond = cond.And(builder.In("type", opts.KeyTypes)) - } + cond = cond.And(builder.In("`type`", opts.KeyTypes)) } if opts.NotKeytype > 0 { - cond = cond.And(builder.Neq{"type": opts.NotKeytype}) + cond = cond.And(builder.Neq{"`type`": opts.NotKeytype}) } if opts.LoginSourceID > 0 { cond = cond.And(builder.Eq{"login_source_id": opts.LoginSourceID}) diff --git a/models/issues/tracked_time.go b/models/issues/tracked_time.go index 521873bb7e20b..884a445d265cb 100644 --- a/models/issues/tracked_time.go +++ b/models/issues/tracked_time.go @@ -117,7 +117,7 @@ func (opts *FindTrackedTimesOptions) ToConds() builder.Cond { return cond } -func (opts FindTrackedTimesOptions) ToJoins() []db.JoinFunc { +func (opts *FindTrackedTimesOptions) ToJoins() []db.JoinFunc { if opts.RepositoryID > 0 || opts.MilestoneID > 0 { return []db.JoinFunc{ func(e db.Engine) error { diff --git a/models/repo/collaboration.go b/models/repo/collaboration.go index 254537f62002c..0471803d3817d 100644 --- a/models/repo/collaboration.go +++ b/models/repo/collaboration.go @@ -58,8 +58,12 @@ func GetCollaborators(ctx context.Context, repoID int64, listOptions db.ListOpti } for _, c := range collaborations { + u := usersMap[c.UserID] + if u == nil { + u = user_model.NewGhostUser() + } collaborators = append(collaborators, &Collaborator{ - User: usersMap[c.UserID], + User: u, Collaboration: c, }) } diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 522d1506b76d4..b6f94115a7efc 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -19,9 +19,8 @@ import ( func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) { opts := asymkey_model.FindGPGKeyOptions{ - ListOptions: listOptions, - OwnerID: uid, - IsPrimaryKeyIDEmpty: true, + ListOptions: listOptions, + OwnerID: uid, } keys, total, err := db.FindAndCount[asymkey_model.GPGKey](ctx, opts) if err != nil { @@ -207,7 +206,8 @@ func VerifyUserGPGKey(ctx *context.APIContext) { } keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - KeyID: form.KeyID, + KeyID: form.KeyID, + IncludeSubKeys: true, }) if err != nil { if asymkey_model.IsErrGPGKeyNotExist(err) { diff --git a/routers/web/user/home.go b/routers/web/user/home.go index a7b3c06c6db8a..debbb9753c29b 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -664,20 +664,14 @@ func ShowSSHKeys(ctx *context.Context) { // ShowGPGKeys output all the public GPG keys of user by uid func ShowGPGKeys(ctx *context.Context) { keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: ctx.ContextUser.ID, - IsPrimaryKeyIDEmpty: true, + ListOptions: db.ListOptionsAll, + OwnerID: ctx.ContextUser.ID, }) if err != nil { ctx.ServerError("ListGPGKeys", err) return } - if err := asymkey_model.GPGKeyList(keys).LoadSubKeys(ctx); err != nil { - ctx.ServerError("LoadSubKeys", err) - return - } - entities := make([]*openpgp.Entity, 0) failedEntitiesID := make([]string, 0) for _, k := range keys { diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index a7435c0ee8482..16410d06ff02f 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -278,9 +278,8 @@ func loadKeysData(ctx *context.Context) { ctx.Data["ExternalKeys"] = externalKeys gpgkeys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - ListOptions: db.ListOptionsAll, - OwnerID: ctx.Doer.ID, - IsPrimaryKeyIDEmpty: true, + ListOptions: db.ListOptionsAll, + OwnerID: ctx.Doer.ID, }) if err != nil { ctx.ServerError("ListGPGKeys", err) diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index 828603fc57b1e..0c4aac8156934 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -144,8 +144,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: u.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: u.ID, + IncludeSubKeys: true, }) if err != nil { return false, "", nil, err @@ -183,8 +183,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: u.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: u.ID, + IncludeSubKeys: true, }) if err != nil { return false, "", nil, err @@ -239,8 +239,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: u.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: u.ID, + IncludeSubKeys: true, }) if err != nil { return false, "", nil, err @@ -304,8 +304,8 @@ Loop: break Loop case pubkey: keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: u.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: u.ID, + IncludeSubKeys: true, }) if err != nil { return false, "", nil, err diff --git a/services/repository/push.go b/services/repository/push.go index 31e11dd1b8d6d..e86eebde8110b 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -332,7 +332,7 @@ func pushUpdateAddTags(ctx context.Context, repo *repo_model.Repository, gitRepo TagNames: lowerTags, }) if err != nil { - return fmt.Errorf("GetReleasesByRepoIDAndNames: %w", err) + return fmt.Errorf("db.Find[repo_model.Release]: %w", err) } relMap := make(map[string]*repo_model.Release) for _, rel := range releases { diff --git a/services/user/delete.go b/services/user/delete.go index 2b6667a6a1ba0..0e9c86617146e 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -160,8 +160,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) // ***** START: GPGPublicKey ***** keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ - OwnerID: u.ID, - IsPrimaryKeyIDEmpty: true, + OwnerID: u.ID, }) if err != nil { return fmt.Errorf("ListGPGKeys: %w", err) From 14289faf405989d5b1788f30554f6a8227429cc9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Jan 2024 09:43:52 +0800 Subject: [PATCH 16/16] Some improvements --- models/repo/collaboration.go | 4 ++-- routers/api/v1/user/gpg_key.go | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/models/repo/collaboration.go b/models/repo/collaboration.go index 0471803d3817d..7288082614738 100644 --- a/models/repo/collaboration.go +++ b/models/repo/collaboration.go @@ -43,7 +43,7 @@ func GetCollaborators(ctx context.Context, repoID int64, listOptions db.ListOpti RepoID: repoID, }) if err != nil { - return nil, fmt.Errorf("getCollaborations: %w", err) + return nil, fmt.Errorf("db.Find[Collaboration]: %w", err) } collaborators := make([]*Collaborator, 0, len(collaborations)) @@ -54,7 +54,7 @@ func GetCollaborators(ctx context.Context, repoID int64, listOptions db.ListOpti usersMap := make(map[int64]*user_model.User) if err := db.GetEngine(ctx).In("id", userIDs).Find(&usersMap); err != nil { - return nil, fmt.Errorf("getCollaborations: %w", err) + return nil, fmt.Errorf("Find users map by user ids: %w", err) } for _, c := range collaborations { diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index b6f94115a7efc..234da5dfdc490 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -18,11 +18,10 @@ import ( ) func listGPGKeys(ctx *context.APIContext, uid int64, listOptions db.ListOptions) { - opts := asymkey_model.FindGPGKeyOptions{ + keys, total, err := db.FindAndCount[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ ListOptions: listOptions, OwnerID: uid, - } - keys, total, err := db.FindAndCount[asymkey_model.GPGKey](ctx, opts) + }) if err != nil { ctx.Error(http.StatusInternalServerError, "ListGPGKeys", err) return