Skip to content

Commit dca2cc8

Browse files
ethantkoenigethantkoenig
committed
Fix FIXME and remove superfluous queries in models/org
1 parent 8bc4319 commit dca2cc8

File tree

3 files changed

+101
-76
lines changed

3 files changed

+101
-76
lines changed

‎models/action.go

+5-8
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

+69-61
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

+27-7
Original file line numberDiff line numberDiff line change
@@ -113,15 +113,20 @@ func Dashboard(ctx *context.Context) {
113113
var err error
114114
var repos, mirrors []*models.Repository
115115
if ctxUser.IsOrganization() {
116-
repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, setting.UI.User.RepoPagingNum)
116+
env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
117117
if err != nil {
118-
ctx.Handle(500, "GetUserRepositories", err)
118+
ctx.Handle(500, "AccessibleReposEnv", err)
119+
return
120+
}
121+
repos, err = env.Repos(1, setting.UI.User.RepoPagingNum)
122+
if err != nil {
123+
ctx.Handle(500, "env.Repos", err)
119124
return
120125
}
121126

122-
mirrors, err = ctxUser.GetUserMirrorRepositories(ctx.User.ID)
127+
mirrors, err = env.MirrorRepos()
123128
if err != nil {
124-
ctx.Handle(500, "GetUserMirrorRepositories", err)
129+
ctx.Handle(500, "env.MirrorRepos", err)
125130
return
126131
}
127132
} else {
@@ -204,7 +209,12 @@ func Issues(ctx *context.Context) {
204209
var err error
205210
var repos []*models.Repository
206211
if ctxUser.IsOrganization() {
207-
repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, ctxUser.NumRepos)
212+
env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
213+
if err != nil {
214+
ctx.Handle(500, "AccessibleReposEnv", err)
215+
return
216+
}
217+
repos, err = env.Repos(1, ctxUser.NumRepos)
208218
if err != nil {
209219
ctx.Handle(500, "GetRepositories", err)
210220
return
@@ -352,9 +362,19 @@ func showOrgProfile(ctx *context.Context) {
352362
err error
353363
)
354364
if ctx.IsSigned && !ctx.User.IsAdmin {
355-
repos, count, err = org.GetUserRepositories(ctx.User.ID, page, setting.UI.User.RepoPagingNum)
365+
env, err := org.AccessibleReposEnv(ctx.User.ID)
366+
if err != nil {
367+
ctx.Handle(500, "AccessibleReposEnv", err)
368+
return
369+
}
370+
repos, err = env.Repos(page, setting.UI.User.RepoPagingNum)
371+
if err != nil {
372+
ctx.Handle(500, "env.Repos", err)
373+
return
374+
}
375+
count, err = env.CountRepos()
356376
if err != nil {
357-
ctx.Handle(500, "GetUserRepositories", err)
377+
ctx.Handle(500, "env.CountRepos", err)
358378
return
359379
}
360380
ctx.Data["Repos"] = repos

0 commit comments

Comments
 (0)