From 170743c8a0cdf216ee21076aadc5d905dfef0cd6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 9 Oct 2019 01:55:16 +0800 Subject: [PATCH] Revert "Fix issues/pr list broken when there are many repositories (#8409)" (#8427) This reverts commit 78438d310be42f9c5e0e2937ee54e6050cc8f381. --- models/issue.go | 54 +++++++---------- models/issue_test.go | 11 ++-- models/user.go | 59 +++++++++++------- models/user_test.go | 22 +++++++ routers/user/home.go | 140 ++++++++++++++++++++++++++++--------------- 5 files changed, 177 insertions(+), 109 deletions(-) diff --git a/models/issue.go b/models/issue.go index cfa6191b4..e4cc1291c 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1306,19 +1306,18 @@ func GetIssuesByIDs(issueIDs []int64) ([]*Issue, error) { // IssuesOptions represents options of an issue. type IssuesOptions struct { - RepoIDs []int64 // include all repos if empty - RepoSubQuery *builder.Builder - AssigneeID int64 - PosterID int64 - MentionedID int64 - MilestoneID int64 - Page int - PageSize int - IsClosed util.OptionalBool - IsPull util.OptionalBool - LabelIDs []int64 - SortType string - IssueIDs []int64 + RepoIDs []int64 // include all repos if empty + AssigneeID int64 + PosterID int64 + MentionedID int64 + MilestoneID int64 + Page int + PageSize int + IsClosed util.OptionalBool + IsPull util.OptionalBool + LabelIDs []int64 + SortType string + IssueIDs []int64 } // sortIssuesSession sort an issues-related session based on the provided @@ -1361,9 +1360,7 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) { sess.In("issue.id", opts.IssueIDs) } - if opts.RepoSubQuery != nil { - sess.In("issue.repo_id", opts.RepoSubQuery) - } else if len(opts.RepoIDs) > 0 { + if len(opts.RepoIDs) > 0 { // In case repository IDs are provided but actually no repository has issue. sess.In("issue.repo_id", opts.RepoIDs) } @@ -1630,12 +1627,12 @@ func GetIssueStats(opts *IssueStatsOptions) (*IssueStats, error) { // UserIssueStatsOptions contains parameters accepted by GetUserIssueStats. type UserIssueStatsOptions struct { - UserID int64 - RepoID int64 - RepoSubQuery *builder.Builder - FilterMode int - IsPull bool - IsClosed bool + UserID int64 + RepoID int64 + UserRepoIDs []int64 + FilterMode int + IsPull bool + IsClosed bool } // GetUserIssueStats returns issue statistic information for dashboard by given conditions. @@ -1649,23 +1646,16 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { cond = cond.And(builder.Eq{"issue.repo_id": opts.RepoID}) } - var repoCond = builder.NewCond() - if opts.RepoSubQuery != nil { - repoCond = builder.In("issue.repo_id", opts.RepoSubQuery) - } else { - repoCond = builder.Expr("0=1") - } - switch opts.FilterMode { case FilterModeAll: stats.OpenCount, err = x.Where(cond).And("is_closed = ?", false). - And(repoCond). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). Count(new(Issue)) if err != nil { return nil, err } stats.ClosedCount, err = x.Where(cond).And("is_closed = ?", true). - And(repoCond). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). Count(new(Issue)) if err != nil { return nil, err @@ -1740,7 +1730,7 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { } stats.YourRepositoriesCount, err = x.Where(cond). - And(repoCond). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). Count(new(Issue)) if err != nil { return nil, err diff --git a/models/issue_test.go b/models/issue_test.go index 65f4d6ba6..9cd9ff0ad 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -10,7 +10,6 @@ import ( "time" "github.com/stretchr/testify/assert" - "xorm.io/builder" ) func TestIssue_ReplaceLabels(t *testing.T) { @@ -267,12 +266,10 @@ func TestGetUserIssueStats(t *testing.T) { }, { UserIssueStatsOptions{ - UserID: 2, - RepoSubQuery: builder.Select("repository.id"). - From("repository"). - Where(builder.In("repository.id", []int64{1, 2})), - FilterMode: FilterModeAll, - IsClosed: true, + UserID: 2, + UserRepoIDs: []int64{1, 2}, + FilterMode: FilterModeAll, + IsClosed: true, }, IssueStats{ YourRepositoriesCount: 2, diff --git a/models/user.go b/models/user.go index 8c4befb13..030e23c38 100644 --- a/models/user.go +++ b/models/user.go @@ -615,35 +615,50 @@ func (u *User) GetRepositories(page, pageSize int) (err error) { return err } -// UnitRepositoriesSubQuery returns repositories query builder according units -func (u *User) UnitRepositoriesSubQuery(units ...UnitType) *builder.Builder { - b := builder.Select("repository.id").From("repository") +// GetRepositoryIDs returns repositories IDs where user owned and has unittypes +func (u *User) GetRepositoryIDs(units ...UnitType) ([]int64, error) { + var ids []int64 + + sess := x.Table("repository").Cols("repository.id") if len(units) > 0 { - b.Join("INNER", "repo_unit", builder.Expr("repository.id = repo_unit.repo_id"). - And(builder.In("repo_unit.type", units)), - ) + sess = sess.Join("INNER", "repo_unit", "repository.id = repo_unit.repo_id") + sess = sess.In("repo_unit.type", units) } - return b.Where(builder.Eq{"repository.owner_id": u.ID}) + + return ids, sess.Where("owner_id = ?", u.ID).Find(&ids) } -// OrgUnitRepositoriesSubQuery returns repositories query builder according orgnizations and units -func (u *User) OrgUnitRepositoriesSubQuery(userID int64, units ...UnitType) *builder.Builder { - b := builder. - Select("team_repo.repo_id"). - From("team_repo"). - Join("INNER", "team_user", builder.Eq{"team_user.uid": userID}.And( - builder.Expr("team_user.team_id = team_repo.team_id"), - )) +// GetOrgRepositoryIDs returns repositories IDs where user's team owned and has unittypes +func (u *User) GetOrgRepositoryIDs(units ...UnitType) ([]int64, error) { + var ids []int64 + + sess := x.Table("repository"). + Cols("repository.id"). + Join("INNER", "team_user", "repository.owner_id = team_user.org_id"). + Join("INNER", "team_repo", "repository.is_private != ? OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)", true) + if len(units) > 0 { - b.Join("INNER", "team_unit", builder.Eq{"team_unit.org_id": u.ID}.And( - builder.Expr("team_unit.team_id = team_repo.team_id").And( - builder.In("`type`", units), - ), - )) + sess = sess.Join("INNER", "team_unit", "team_unit.team_id = team_user.team_id") + sess = sess.In("team_unit.type", units) } - return b.Where(builder.Eq{"team_repo.org_id": u.ID}). - GroupBy("team_repo.repo_id") + + return ids, sess. + Where("team_user.uid = ?", u.ID). + GroupBy("repository.id").Find(&ids) +} + +// GetAccessRepoIDs returns all repositories IDs where user's or user is a team member organizations +func (u *User) GetAccessRepoIDs(units ...UnitType) ([]int64, error) { + ids, err := u.GetRepositoryIDs(units...) + if err != nil { + return nil, err + } + ids2, err := u.GetOrgRepositoryIDs(units...) + if err != nil { + return nil, err + } + return append(ids, ids2...), nil } // GetMirrorRepositories returns mirror repositories that user owns, including private repositories. diff --git a/models/user_test.go b/models/user_test.go index 75d806ead..bcb955817 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -275,6 +275,28 @@ func BenchmarkHashPassword(b *testing.B) { } } +func TestGetOrgRepositoryIDs(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User) + user5 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) + + accessibleRepos, err := user2.GetOrgRepositoryIDs() + assert.NoError(t, err) + // User 2's team has access to private repos 3, 5, repo 32 is a public repo of the organization + assert.Equal(t, []int64{3, 5, 23, 24, 32}, accessibleRepos) + + accessibleRepos, err = user4.GetOrgRepositoryIDs() + assert.NoError(t, err) + // User 4's team has access to private repo 3, repo 32 is a public repo of the organization + assert.Equal(t, []int64{3, 32}, accessibleRepos) + + accessibleRepos, err = user5.GetOrgRepositoryIDs() + assert.NoError(t, err) + // User 5's team has no access to any repo + assert.Len(t, accessibleRepos, 0) +} + func TestNewGitSig(t *testing.T) { users := make([]*User, 0, 20) sess := x.NewSession() diff --git a/routers/user/home.go b/routers/user/home.go index 21fc62aae..40b3bc3fc 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -14,11 +14,13 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "github.com/keybase/go-crypto/openpgp" "github.com/keybase/go-crypto/openpgp/armor" + "github.com/unknwon/com" ) const ( @@ -150,24 +152,6 @@ func Dashboard(ctx *context.Context) { // Issues render the user issues page func Issues(ctx *context.Context) { isPullList := ctx.Params(":type") == "pulls" - repoID := ctx.QueryInt64("repo") - if repoID > 0 { - repo, err := models.GetRepositoryByID(repoID) - if err != nil { - ctx.ServerError("GetRepositoryByID", err) - return - } - perm, err := models.GetUserRepoPermission(repo, ctx.User) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return - } - if !perm.CanReadIssuesOrPulls(isPullList) { - ctx.NotFound("Repository does not exist or you have no permission", nil) - return - } - } - if isPullList { ctx.Data["Title"] = ctx.Tr("pull_requests") ctx.Data["PageIsPulls"] = true @@ -210,32 +194,58 @@ func Issues(ctx *context.Context) { page = 1 } - var ( - isShowClosed = ctx.Query("state") == "closed" - err error - opts = &models.IssuesOptions{ - IsClosed: util.OptionalBoolOf(isShowClosed), - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - } - ) + repoID := ctx.QueryInt64("repo") + isShowClosed := ctx.Query("state") == "closed" // Get repositories. - if repoID > 0 { - opts.RepoIDs = []int64{repoID} + var err error + var userRepoIDs []int64 + if ctxUser.IsOrganization() { + env, err := ctxUser.AccessibleReposEnv(ctx.User.ID) + if err != nil { + ctx.ServerError("AccessibleReposEnv", err) + return + } + userRepoIDs, err = env.RepoIDs(1, ctxUser.NumRepos) + if err != nil { + ctx.ServerError("env.RepoIDs", err) + return + } } else { unitType := models.UnitTypeIssues if isPullList { unitType = models.UnitTypePullRequests } - if ctxUser.IsOrganization() { - opts.RepoSubQuery = ctxUser.OrgUnitRepositoriesSubQuery(ctx.User.ID, unitType) - } else { - opts.RepoSubQuery = ctxUser.UnitRepositoriesSubQuery(unitType) + userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) + if err != nil { + ctx.ServerError("ctxUser.GetAccessRepoIDs", err) + return } } + if len(userRepoIDs) == 0 { + userRepoIDs = []int64{-1} + } + + opts := &models.IssuesOptions{ + IsClosed: util.OptionalBoolOf(isShowClosed), + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, + } + + if repoID > 0 { + opts.RepoIDs = []int64{repoID} + } switch filterMode { + case models.FilterModeAll: + if repoID > 0 { + if !com.IsSliceContainsInt64(userRepoIDs, repoID) { + // force an empty result + opts.RepoIDs = []int64{-1} + } + } else { + opts.RepoIDs = userRepoIDs + } case models.FilterModeAssign: opts.AssigneeID = ctxUser.ID case models.FilterModeCreate: @@ -244,6 +254,14 @@ func Issues(ctx *context.Context) { opts.MentionedID = ctxUser.ID } + counts, err := models.CountIssuesByRepo(opts) + if err != nil { + ctx.ServerError("CountIssuesByRepo", err) + return + } + + opts.Page = page + opts.PageSize = setting.UI.IssuePagingNum var labelIDs []int64 selectLabels := ctx.Query("labels") if len(selectLabels) > 0 && selectLabels != "0" { @@ -255,15 +273,6 @@ func Issues(ctx *context.Context) { } opts.LabelIDs = labelIDs - counts, err := models.CountIssuesByRepo(opts) - if err != nil { - ctx.ServerError("CountIssuesByRepo", err) - return - } - - opts.Page = page - opts.PageSize = setting.UI.IssuePagingNum - issues, err := models.Issues(opts) if err != nil { ctx.ServerError("Issues", err) @@ -280,6 +289,41 @@ func Issues(ctx *context.Context) { showReposMap[repoID] = repo } + if repoID > 0 { + if _, ok := showReposMap[repoID]; !ok { + repo, err := models.GetRepositoryByID(repoID) + if models.IsErrRepoNotExist(err) { + ctx.NotFound("GetRepositoryByID", err) + return + } else if err != nil { + ctx.ServerError("GetRepositoryByID", fmt.Errorf("[%d]%v", repoID, err)) + return + } + showReposMap[repoID] = repo + } + + repo := showReposMap[repoID] + + // Check if user has access to given repository. + perm, err := models.GetUserRepoPermission(repo, ctxUser) + if err != nil { + ctx.ServerError("GetUserRepoPermission", fmt.Errorf("[%d]%v", repoID, err)) + return + } + if !perm.CanRead(models.UnitTypeIssues) { + if log.IsTrace() { + log.Trace("Permission Denied: User %-v cannot read %-v of repo %-v\n"+ + "User in repo has Permissions: %-+v", + ctxUser, + models.UnitTypeIssues, + repo, + perm) + } + ctx.Status(404) + return + } + } + showRepos := models.RepositoryListOfMap(showReposMap) sort.Sort(showRepos) if err = showRepos.LoadAttributes(); err != nil { @@ -297,12 +341,12 @@ func Issues(ctx *context.Context) { } issueStats, err := models.GetUserIssueStats(models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - RepoID: repoID, - RepoSubQuery: opts.RepoSubQuery, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, + UserID: ctxUser.ID, + RepoID: repoID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, }) if err != nil { ctx.ServerError("GetUserIssueStats", err)