From 6d220540c15e68196404951e8b67cac8002d1a6f Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Sat, 4 Feb 2023 13:36:00 +0800 Subject: [PATCH] refactor(db): migrate methods off `user.go` (#7334) --- internal/db/user.go | 30 ------------------------------ internal/db/users.go | 34 +++++++++++++++++++++------------- internal/db/users_test.go | 2 +- internal/route/repo/commit.go | 31 ++++++++++++++++++++++++++++--- internal/route/repo/pull.go | 4 ++-- 5 files changed, 52 insertions(+), 49 deletions(-) diff --git a/internal/db/user.go b/internal/db/user.go index 17e0f66b9..5be646680 100644 --- a/internal/db/user.go +++ b/internal/db/user.go @@ -5,7 +5,6 @@ package db import ( - "context" "fmt" _ "image/jpeg" "os" @@ -14,8 +13,6 @@ import ( log "unknwon.dev/clog/v2" "xorm.io/xorm" - "github.com/gogs/git-module" - "gogs.io/gogs/internal/repoutil" "gogs.io/gogs/internal/userutil" ) @@ -200,33 +197,6 @@ func DeleteInactivateUsers() (err error) { return err } -// UserCommit represents a commit with validation of user. -type UserCommit struct { - User *User - *git.Commit -} - -// ValidateCommitsWithEmails checks if authors' e-mails of commits are corresponding to users. -func ValidateCommitsWithEmails(oldCommits []*git.Commit) []*UserCommit { - emails := make(map[string]*User) - newCommits := make([]*UserCommit, len(oldCommits)) - for i := range oldCommits { - var u *User - if v, ok := emails[oldCommits[i].Author.Email]; !ok { - u, _ = Users.GetByEmail(context.TODO(), oldCommits[i].Author.Email) - emails[oldCommits[i].Author.Email] = u - } else { - u = v - } - - newCommits[i] = &UserCommit{ - User: u, - Commit: oldCommits[i], - } - } - return newCommits -} - // GetRepositoryAccesses finds all repositories with their access mode where a user has access but does not own. func (u *User) GetRepositoryAccesses() (map[*Repository]AccessMode, error) { accesses := make([]*Access, 0, 10) diff --git a/internal/db/users.go b/internal/db/users.go index 7e986a472..7b2b550ec 100644 --- a/internal/db/users.go +++ b/internal/db/users.go @@ -450,27 +450,35 @@ func (db *users) GetByEmail(ctx context.Context, email string) (*User, error) { } email = strings.ToLower(email) - // First try to find the user by primary email + /* + Equivalent SQL for PostgreSQL: + + SELECT * FROM "user" + LEFT JOIN email_address ON email_address.uid = "user".id + WHERE + "user".type = @userType + AND ( + "user".email = @email AND "user".is_active = TRUE + OR email_address.email = @email AND email_address.is_activated = TRUE + ) + */ user := new(User) err := db.WithContext(ctx). - Where("email = ? AND type = ? AND is_active = ?", email, UserTypeIndividual, true). - First(user). + Joins(dbutil.Quote("LEFT JOIN email_address ON email_address.uid = %s.id", "user"), true). + Where(dbutil.Quote("%s.type = ?", "user"), UserTypeIndividual). + Where(db. + Where(dbutil.Quote("%[1]s.email = ? AND %[1]s.is_active = ?", "user"), email, true). + Or("email_address.email = ? AND email_address.is_activated = ?", email, true), + ). + First(&user). Error - if err == nil { - return user, nil - } else if err != gorm.ErrRecordNotFound { - return nil, err - } - - // Otherwise, check activated email addresses - emailAddress, err := NewEmailAddressesStore(db.DB).GetByEmail(ctx, email, true) if err != nil { - if IsErrEmailAddressNotExist(err) { + if err == gorm.ErrRecordNotFound { return nil, ErrUserNotExist{args: errutil.Args{"email": email}} } return nil, err } - return db.GetByID(ctx, emailAddress.UserID) + return user, nil } func (db *users) GetByID(ctx context.Context, id int64) (*User, error) { diff --git a/internal/db/users_test.go b/internal/db/users_test.go index 10f30b811..171b3a88d 100644 --- a/internal/db/users_test.go +++ b/internal/db/users_test.go @@ -593,7 +593,7 @@ func usersGetMailableEmailsByUsernames(t *testing.T, db *users) { _, err = db.Create(ctx, "cindy", "cindy@exmaple.com", CreateUserOptions{Activated: true}) require.NoError(t, err) - got, err := db.GetMailableEmailsByUsernames(ctx, []string{alice.Name, bob.Name, "404"}) + got, err := db.GetMailableEmailsByUsernames(ctx, []string{alice.Name, bob.Name, "ignore-non-exist"}) require.NoError(t, err) want := []string{bob.Email} assert.Equal(t, want, got) diff --git a/internal/route/repo/commit.go b/internal/route/repo/commit.go index da8c5e48b..f82336d97 100644 --- a/internal/route/repo/commit.go +++ b/internal/route/repo/commit.go @@ -61,7 +61,7 @@ func renderCommits(c *context.Context, filename string) { } commits = RenderIssueLinks(commits, c.Repo.RepoLink) - c.Data["Commits"] = db.ValidateCommitsWithEmails(commits) + c.Data["Commits"] = matchUsersWithCommitEmails(c.Req.Context(), commits) if page > 1 { c.Data["HasPrevious"] = true @@ -98,7 +98,7 @@ func SearchCommits(c *context.Context) { } commits = RenderIssueLinks(commits, c.Repo.RepoLink) - c.Data["Commits"] = db.ValidateCommitsWithEmails(commits) + c.Data["Commits"] = matchUsersWithCommitEmails(c.Req.Context(), commits) c.Data["Keyword"] = keyword c.Data["Username"] = c.Repo.Owner.Name @@ -188,6 +188,31 @@ func RawDiff(c *context.Context) { } } +type userCommit struct { + User *db.User + *git.Commit +} + +// matchUsersWithCommitEmails matches existing users using commit author emails. +func matchUsersWithCommitEmails(ctx gocontext.Context, oldCommits []*git.Commit) []*userCommit { + emailToUsers := make(map[string]*db.User) + newCommits := make([]*userCommit, len(oldCommits)) + for i := range oldCommits { + var u *db.User + if v, ok := emailToUsers[oldCommits[i].Author.Email]; !ok { + emailToUsers[oldCommits[i].Author.Email], _ = db.Users.GetByEmail(ctx, oldCommits[i].Author.Email) + } else { + u = v + } + + newCommits[i] = &userCommit{ + User: u, + Commit: oldCommits[i], + } + } + return newCommits +} + func CompareDiff(c *context.Context) { c.Data["IsDiffCompare"] = true userName := c.Repo.Owner.Name @@ -218,7 +243,7 @@ func CompareDiff(c *context.Context) { c.Data["IsSplitStyle"] = c.Query("style") == "split" c.Data["CommitRepoLink"] = c.Repo.RepoLink - c.Data["Commits"] = db.ValidateCommitsWithEmails(commits) + c.Data["Commits"] = matchUsersWithCommitEmails(c.Req.Context(), commits) c.Data["CommitsCount"] = len(commits) c.Data["BeforeCommitID"] = beforeCommitID c.Data["AfterCommitID"] = afterCommitID diff --git a/internal/route/repo/pull.go b/internal/route/repo/pull.go index 2429ff8bd..7eedcb3a9 100644 --- a/internal/route/repo/pull.go +++ b/internal/route/repo/pull.go @@ -298,7 +298,7 @@ func ViewPullCommits(c *context.Context) { commits = prInfo.Commits } - c.Data["Commits"] = db.ValidateCommitsWithEmails(commits) + c.Data["Commits"] = matchUsersWithCommitEmails(c.Req.Context(), commits) c.Data["CommitsCount"] = len(commits) c.Success(PULL_COMMITS) @@ -606,7 +606,7 @@ func PrepareCompareDiff( return false } - c.Data["Commits"] = db.ValidateCommitsWithEmails(meta.Commits) + c.Data["Commits"] = matchUsersWithCommitEmails(c.Req.Context(), meta.Commits) c.Data["CommitCount"] = len(meta.Commits) c.Data["Username"] = headUser.Name c.Data["Reponame"] = headRepo.Name