Skip to content

Commit da1b616

Browse files
ethantkoeniglunny
ethantkoenig
authored and
lunny
committed
Fix FIXME and remove superfluous queries in models/org (#749)
1 parent 691fbdf commit da1b616

File tree

3 files changed

+101
-76
lines changed

3 files changed

+101
-76
lines changed

‎models/action.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -658,17 +658,14 @@ func GetFeeds(ctxUser *User, actorID, offset int64, isProfile bool) ([]*Action,
658658
And("is_private = ?", false).
659659
And("act_user_id = ?", ctxUser.ID)
660660
} else if actorID != -1 && ctxUser.IsOrganization() {
661-
// FIXME: only need to get IDs here, not all fields of repository.
662-
repos, _, err := ctxUser.GetUserRepositories(actorID, 1, ctxUser.NumRepos)
661+
env, err := ctxUser.AccessibleReposEnv(actorID)
663662
if err != nil {
664-
return nil, fmt.Errorf("GetUserRepositories: %v", err)
663+
return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
665664
}
666-
667-
var repoIDs []int64
668-
for _, repo := range repos {
669-
repoIDs = append(repoIDs, repo.ID)
665+
repoIDs, err := env.RepoIDs(1, ctxUser.NumRepos)
666+
if err != nil {
667+
return nil, fmt.Errorf("GetUserRepositories: %v", err)
670668
}
671-
672669
if len(repoIDs) > 0 {
673670
sess.In("repo_id", repoIDs)
674671
}

‎models/org.go

Lines changed: 69 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -471,12 +471,6 @@ func RemoveOrgUser(orgID, userID int64) error {
471471
return fmt.Errorf("GetUserByID [%d]: %v", orgID, err)
472472
}
473473

474-
// FIXME: only need to get IDs here, not all fields of repository.
475-
repos, _, err := org.GetUserRepositories(user.ID, 1, org.NumRepos)
476-
if err != nil {
477-
return fmt.Errorf("GetUserRepositories [%d]: %v", user.ID, err)
478-
}
479-
480474
// Check if the user to delete is the last member in owner team.
481475
if IsOrganizationOwner(orgID, userID) {
482476
t, err := org.GetOwnerTeam()
@@ -501,10 +495,16 @@ func RemoveOrgUser(orgID, userID int64) error {
501495
}
502496

503497
// Delete all repository accesses and unwatch them.
504-
repoIDs := make([]int64, len(repos))
505-
for i := range repos {
506-
repoIDs = append(repoIDs, repos[i].ID)
507-
if err = watchRepo(sess, user.ID, repos[i].ID, false); err != nil {
498+
env, err := org.AccessibleReposEnv(user.ID)
499+
if err != nil {
500+
return fmt.Errorf("AccessibleReposEnv: %v", err)
501+
}
502+
repoIDs, err := env.RepoIDs(1, org.NumRepos)
503+
if err != nil {
504+
return fmt.Errorf("GetUserRepositories [%d]: %v", user.ID, err)
505+
}
506+
for _, repoID := range repoIDs {
507+
if err = watchRepo(sess, user.ID, repoID, false); err != nil {
508508
return err
509509
}
510510
}
@@ -577,82 +577,90 @@ func (org *User) GetUserTeams(userID int64) ([]*Team, error) {
577577
return org.getUserTeams(x, userID)
578578
}
579579

580-
// GetUserRepositories returns a range of repositories in organization
581-
// that the user with the given userID has access to,
582-
// and total number of records based on given condition.
583-
func (org *User) GetUserRepositories(userID int64, page, pageSize int) ([]*Repository, int64, error) {
584-
var cond builder.Cond = builder.Eq{
585-
"`repository`.owner_id": org.ID,
586-
"`repository`.is_private": false,
587-
}
580+
// AccessibleReposEnvironment operations involving the repositories that are
581+
// accessible to a particular user
582+
type AccessibleReposEnvironment interface {
583+
CountRepos() (int64, error)
584+
RepoIDs(page, pageSize int) ([]int64, error)
585+
Repos(page, pageSize int) ([]*Repository, error)
586+
MirrorRepos() ([]*Repository, error)
587+
}
588588

589+
type accessibleReposEnv struct {
590+
org *User
591+
userID int64
592+
teamIDs []int64
593+
}
594+
595+
// AccessibleReposEnv an AccessibleReposEnvironment for the repositories in `org`
596+
// that are accessible to the specified user.
597+
func (org *User) AccessibleReposEnv(userID int64) (AccessibleReposEnvironment, error) {
589598
teamIDs, err := org.GetUserTeamIDs(userID)
590599
if err != nil {
591-
return nil, 0, fmt.Errorf("GetUserTeamIDs: %v", err)
600+
return nil, err
592601
}
602+
return &accessibleReposEnv{org: org, userID: userID, teamIDs: teamIDs}, nil
603+
}
593604

594-
if len(teamIDs) > 0 {
595-
cond = cond.Or(builder.In("team_repo.team_id", teamIDs))
605+
func (env *accessibleReposEnv) cond() builder.Cond {
606+
var cond builder.Cond = builder.Eq{
607+
"`repository`.owner_id": env.org.ID,
608+
"`repository`.is_private": false,
596609
}
610+
if len(env.teamIDs) > 0 {
611+
cond = cond.Or(builder.In("team_repo.team_id", env.teamIDs))
612+
}
613+
return cond
614+
}
597615

616+
func (env *accessibleReposEnv) CountRepos() (int64, error) {
617+
repoCount, err := x.
618+
Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
619+
Where(env.cond()).
620+
GroupBy("`repository`.id").
621+
Count(&Repository{})
622+
if err != nil {
623+
return 0, fmt.Errorf("count user repositories in organization: %v", err)
624+
}
625+
return repoCount, nil
626+
}
627+
628+
func (env *accessibleReposEnv) RepoIDs(page, pageSize int) ([]int64, error) {
598629
if page <= 0 {
599630
page = 1
600631
}
601632

602-
repos := make([]*Repository, 0, pageSize)
603-
604-
if err := x.
605-
Select("`repository`.id").
633+
repoIDs := make([]int64, 0, pageSize)
634+
return repoIDs, x.
635+
Table("repository").
606636
Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
607-
Where(cond).
637+
Where(env.cond()).
608638
GroupBy("`repository`.id,`repository`.updated_unix").
609639
OrderBy("updated_unix DESC").
610640
Limit(pageSize, (page-1)*pageSize).
611-
Find(&repos); err != nil {
612-
return nil, 0, fmt.Errorf("get repository ids: %v", err)
613-
}
614-
615-
repoIDs := make([]int64,pageSize)
616-
for i := range repos {
617-
repoIDs[i] = repos[i].ID
618-
}
619-
620-
if err := x.
621-
Select("`repository`.*").
622-
Where(builder.In("`repository`.id",repoIDs)).
623-
Find(&repos); err!=nil {
624-
return nil, 0, fmt.Errorf("get repositories: %v", err)
625-
}
641+
Cols("`repository`.id").
642+
Find(&repoIDs)
643+
}
626644

627-
repoCount, err := x.
628-
Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
629-
Where(cond).
630-
GroupBy("`repository`.id").
631-
Count(&Repository{})
645+
func (env *accessibleReposEnv) Repos(page, pageSize int) ([]*Repository, error) {
646+
repoIDs, err := env.RepoIDs(page, pageSize)
632647
if err != nil {
633-
return nil, 0, fmt.Errorf("count user repositories in organization: %v", err)
648+
return nil, fmt.Errorf("GetUserRepositoryIDs: %v", err)
634649
}
635650

636-
return repos, repoCount, nil
651+
repos := make([]*Repository, 0, len(repoIDs))
652+
return repos, x.
653+
Select("`repository`.*").
654+
Where(builder.In("`repository`.id", repoIDs)).
655+
Find(&repos)
637656
}
638657

639-
// GetUserMirrorRepositories returns mirror repositories of the user
640-
// that the user with the given userID has access to.
641-
func (org *User) GetUserMirrorRepositories(userID int64) ([]*Repository, error) {
642-
teamIDs, err := org.GetUserTeamIDs(userID)
643-
if err != nil {
644-
return nil, fmt.Errorf("GetUserTeamIDs: %v", err)
645-
}
646-
if len(teamIDs) == 0 {
647-
teamIDs = []int64{-1}
648-
}
649-
658+
func (env *accessibleReposEnv) MirrorRepos() ([]*Repository, error) {
650659
repos := make([]*Repository, 0, 10)
651660
return repos, x.
652661
Select("`repository`.*").
653662
Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id AND `repository`.is_mirror=?", true).
654-
Where("(`repository`.owner_id=? AND `repository`.is_private=?)", org.ID, false).
655-
Or(builder.In("team_repo.team_id", teamIDs)).
663+
Where(env.cond()).
656664
GroupBy("`repository`.id").
657665
OrderBy("updated_unix DESC").
658666
Find(&repos)

‎routers/user/home.go

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,15 +114,20 @@ func Dashboard(ctx *context.Context) {
114114
var err error
115115
var repos, mirrors []*models.Repository
116116
if ctxUser.IsOrganization() {
117-
repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, setting.UI.User.RepoPagingNum)
117+
env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
118118
if err != nil {
119-
ctx.Handle(500, "GetUserRepositories", err)
119+
ctx.Handle(500, "AccessibleReposEnv", err)
120+
return
121+
}
122+
repos, err = env.Repos(1, setting.UI.User.RepoPagingNum)
123+
if err != nil {
124+
ctx.Handle(500, "env.Repos", err)
120125
return
121126
}
122127

123-
mirrors, err = ctxUser.GetUserMirrorRepositories(ctx.User.ID)
128+
mirrors, err = env.MirrorRepos()
124129
if err != nil {
125-
ctx.Handle(500, "GetUserMirrorRepositories", err)
130+
ctx.Handle(500, "env.MirrorRepos", err)
126131
return
127132
}
128133
} else {
@@ -205,7 +210,12 @@ func Issues(ctx *context.Context) {
205210
var err error
206211
var repos []*models.Repository
207212
if ctxUser.IsOrganization() {
208-
repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, ctxUser.NumRepos)
213+
env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
214+
if err != nil {
215+
ctx.Handle(500, "AccessibleReposEnv", err)
216+
return
217+
}
218+
repos, err = env.Repos(1, ctxUser.NumRepos)
209219
if err != nil {
210220
ctx.Handle(500, "GetRepositories", err)
211221
return
@@ -353,9 +363,19 @@ func showOrgProfile(ctx *context.Context) {
353363
err error
354364
)
355365
if ctx.IsSigned && !ctx.User.IsAdmin {
356-
repos, count, err = org.GetUserRepositories(ctx.User.ID, page, setting.UI.User.RepoPagingNum)
366+
env, err := org.AccessibleReposEnv(ctx.User.ID)
367+
if err != nil {
368+
ctx.Handle(500, "AccessibleReposEnv", err)
369+
return
370+
}
371+
repos, err = env.Repos(page, setting.UI.User.RepoPagingNum)
372+
if err != nil {
373+
ctx.Handle(500, "env.Repos", err)
374+
return
375+
}
376+
count, err = env.CountRepos()
357377
if err != nil {
358-
ctx.Handle(500, "GetUserRepositories", err)
378+
ctx.Handle(500, "env.CountRepos", err)
359379
return
360380
}
361381
ctx.Data["Repos"] = repos

0 commit comments

Comments
 (0)