From 5414c1bbc6a23317f44e62d7f70f4e792a3ab5ae Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 8 May 2020 16:26:20 +0100 Subject: [PATCH 1/7] Correctly set the organization num repos Correctly set the organization num repos to the number of accessible repos for the user Fix #11194 Signed-off-by: Andrew Thornton --- models/user.go | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/models/user.go b/models/user.go index a89eb144cedc..337e87f2f15c 100644 --- a/models/user.go +++ b/models/user.go @@ -712,19 +712,42 @@ func (u *User) GetOwnedOrganizations() (err error) { // GetOrganizations returns paginated organizations that user belongs to. func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { - ous, err := GetOrgUsersByUserID(u.ID, opts) - if err != nil { + sess := x.NewSession() + if err := sess.Begin(); err != nil { return err } - u.Orgs = make([]*User, len(ous)) - for i, ou := range ous { - u.Orgs[i], err = GetUserByID(ou.OrgID) - if err != nil { - return err - } + sess.Select("`user`.*, count(`repository`.id) as org_count"). + Table("user"). + Join("INNER", "org_user", "`org_user`.org_id=`user`.id"). + Join("INNER", "repository", "`repository`.owner_id = `org_user`.org_id"). + Where(accessibleRepositoryCondition(u)). + And("`org_user`.uid=?", u.ID). + GroupBy("`repository`.owner_id") + if opts.PageSize != 0 { + sess = opts.setSessionPagination(sess) } - return nil + type OrgCount struct { + User `xorm:"extends"` + OrgCount int + } + orgCounts := make([]*OrgCount, 0, 10) + + if err := sess. + Asc("`user`.name"). + Find(&orgCounts); err != nil { + return err + } + + orgs := make([]*User, len(orgCounts)) + for i, orgCount := range orgCounts { + orgCount.User.NumRepos = orgCount.OrgCount + orgs[i] = &orgCount.User + } + + u.Orgs = orgs + + return sess.Commit() } // DisplayName returns full name if it's not empty, From 70ad0be72689108252ad2471352126bce35128cf Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 8 May 2020 17:04:53 +0100 Subject: [PATCH 2/7] as per @lunny Signed-off-by: Andrew Thornton --- models/user.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/models/user.go b/models/user.go index 337e87f2f15c..b3f82f7e4219 100644 --- a/models/user.go +++ b/models/user.go @@ -713,9 +713,6 @@ func (u *User) GetOwnedOrganizations() (err error) { // GetOrganizations returns paginated organizations that user belongs to. func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { sess := x.NewSession() - if err := sess.Begin(); err != nil { - return err - } sess.Select("`user`.*, count(`repository`.id) as org_count"). Table("user"). @@ -747,7 +744,7 @@ func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { u.Orgs = orgs - return sess.Commit() + return nil } // DisplayName returns full name if it's not empty, From 1a0f66d5643cd081285ec72048da53761691c088 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 8 May 2020 17:07:08 +0100 Subject: [PATCH 3/7] attempt to fix mssql Signed-off-by: Andrew Thornton --- models/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user.go b/models/user.go index b3f82f7e4219..c0efdd090e94 100644 --- a/models/user.go +++ b/models/user.go @@ -720,7 +720,7 @@ func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { Join("INNER", "repository", "`repository`.owner_id = `org_user`.org_id"). Where(accessibleRepositoryCondition(u)). And("`org_user`.uid=?", u.ID). - GroupBy("`repository`.owner_id") + GroupBy("`user`.id") if opts.PageSize != 0 { sess = opts.setSessionPagination(sess) } From 2ffb17f79ab62d74d7bbf7372ef9e4663cebfbab Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 8 May 2020 17:52:34 +0100 Subject: [PATCH 4/7] Update models/user.go --- models/user.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/user.go b/models/user.go index c0efdd090e94..e99ffcc96b49 100644 --- a/models/user.go +++ b/models/user.go @@ -713,6 +713,7 @@ func (u *User) GetOwnedOrganizations() (err error) { // GetOrganizations returns paginated organizations that user belongs to. func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { sess := x.NewSession() + defer sess.Close() sess.Select("`user`.*, count(`repository`.id) as org_count"). Table("user"). From 3293c3961d800e34ded3d6e47134311c219421a3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 8 May 2020 20:03:49 +0100 Subject: [PATCH 5/7] Explicit columns Signed-off-by: Andrew Thornton --- models/user.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/models/user.go b/models/user.go index e99ffcc96b49..a4afe677d0f2 100644 --- a/models/user.go +++ b/models/user.go @@ -715,13 +715,24 @@ func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { sess := x.NewSession() defer sess.Close() + schema, err := x.TableInfo(new(User)) + if err != nil { + return err + } + groupByCols := &strings.Builder{} + for _, col := range schema.Columns() { + fmt.Fprintf(groupByCols, "`%s`.%s,", schema.Name, col.Name) + } + groupByStr := groupByCols.String() + groupByStr = groupByStr[0 : len(groupByStr)-1] + sess.Select("`user`.*, count(`repository`.id) as org_count"). Table("user"). Join("INNER", "org_user", "`org_user`.org_id=`user`.id"). Join("INNER", "repository", "`repository`.owner_id = `org_user`.org_id"). Where(accessibleRepositoryCondition(u)). And("`org_user`.uid=?", u.ID). - GroupBy("`user`.id") + GroupBy(groupByStr) if opts.PageSize != 0 { sess = opts.setSessionPagination(sess) } From db46f1756dd17fb60a20d10ee07ffae5552d621b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 11 May 2020 19:56:49 +0100 Subject: [PATCH 6/7] Add test and fix 0 counted orgs Signed-off-by: Andrew Thornton --- .../api_helper_for_declarative_test.go | 83 +++++++++++ integrations/org_count_test.go | 140 ++++++++++++++++++ models/user.go | 8 +- routers/api/v1/repo/repo.go | 2 +- 4 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 integrations/org_count_test.go diff --git a/integrations/api_helper_for_declarative_test.go b/integrations/api_helper_for_declarative_test.go index cae7691c4b96..ec7f1d749690 100644 --- a/integrations/api_helper_for_declarative_test.go +++ b/integrations/api_helper_for_declarative_test.go @@ -266,3 +266,86 @@ func doAPICreateFile(ctx APITestContext, treepath string, options *api.CreateFil } } } + +func doAPICreateOrganization(ctx APITestContext, options *api.CreateOrgOption, callback ...func(*testing.T, api.Organization)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs?token=%s", ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Organization + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPICreateOrganizationRepository(ctx APITestContext, orgName string, options *api.CreateRepoOption, callback ...func(*testing.T, api.Repository)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs/%s/repos?token=%s", orgName, ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Repository + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPICreateOrganizationTeam(ctx APITestContext, orgName string, options *api.CreateTeamOption, callback ...func(*testing.T, api.Team)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs/%s/teams?token=%s", orgName, ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Team + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPIAddUserToOrganizationTeam(ctx APITestContext, teamID int64, username string) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/teams/%d/members/%s?token=%s", teamID, username, ctx.Token) + + req := NewRequest(t, "PUT", url) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + ctx.Session.MakeRequest(t, req, http.StatusNoContent) + } +} + +func doAPIAddRepoToOrganizationTeam(ctx APITestContext, teamID int64, orgName, repoName string) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/teams/%d/repos/%s/%s?token=%s", teamID, orgName, repoName, ctx.Token) + + req := NewRequest(t, "PUT", url) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + ctx.Session.MakeRequest(t, req, http.StatusNoContent) + } +} diff --git a/integrations/org_count_test.go b/integrations/org_count_test.go new file mode 100644 index 000000000000..755ee3cee59f --- /dev/null +++ b/integrations/org_count_test.go @@ -0,0 +1,140 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/url" + "strings" + "testing" + + "code.gitea.io/gitea/models" + api "code.gitea.io/gitea/modules/structs" + "github.com/stretchr/testify/assert" +) + +func TestOrgCounts(t *testing.T) { + onGiteaRun(t, testOrgCounts) +} + +func testOrgCounts(t *testing.T, u *url.URL) { + orgOwner := "user2" + orgName := "testOrg" + orgCollaborator := "user4" + ctx := NewAPITestContext(t, orgOwner, "repo1") + + var ownerCountRepos map[string]int + var collabCountRepos map[string]int + + t.Run("GetTheOwnersNumRepos", doCheckOrgCounts(orgOwner, map[string]int{}, + false, + func(_ *testing.T, calcOrgCounts map[string]int) { + ownerCountRepos = calcOrgCounts + }, + )) + t.Run("GetTheCollaboratorsNumRepos", doCheckOrgCounts(orgCollaborator, map[string]int{}, + false, + func(_ *testing.T, calcOrgCounts map[string]int) { + collabCountRepos = calcOrgCounts + }, + )) + + t.Run("CreatePublicTestOrganization", doAPICreateOrganization(ctx, &api.CreateOrgOption{ + UserName: orgName, + Visibility: "public", + })) + + // Following the creation of the organization, the orgName must appear in the counts with 0 repos + ownerCountRepos[orgName] = 0 + + t.Run("AssertNumRepos0ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // the collaborator is not a collaborator yet + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + t.Run("CreateOrganizationPrivateRepo", doAPICreateOrganizationRepository(ctx, orgName, &api.CreateRepoOption{ + Name: "privateTestRepo", + AutoInit: true, + Private: true, + })) + + ownerCountRepos[orgName] = 1 + t.Run("AssertNumRepos1ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + var testTeam api.Team + + t.Run("CreateTeamForPublicTestOrganization", doAPICreateOrganizationTeam(ctx, orgName, &api.CreateTeamOption{ + Name: "test", + Permission: "read", + Units: []string{"repo.code", "repo.issues", "repo.wiki", "repo.pulls", "repo.releases"}, + CanCreateOrgRepo: true, + }, func(_ *testing.T, team api.Team) { + testTeam = team + })) + + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + t.Run("AddCollboratorToTeam", doAPIAddUserToOrganizationTeam(ctx, testTeam.ID, orgCollaborator)) + + collabCountRepos[orgName] = 0 + t.Run("AssertNumRepos0ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // Now create a Public Repo + t.Run("CreateOrganizationPublicRepo", doAPICreateOrganizationRepository(ctx, orgName, &api.CreateRepoOption{ + Name: "publicTestRepo", + AutoInit: true, + })) + + ownerCountRepos[orgName] = 2 + t.Run("AssertNumRepos2ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + collabCountRepos[orgName] = 1 + t.Run("AssertNumRepos1ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // Now add the testTeam to the privateRepo + t.Run("AddTestTeamToPrivateRepo", doAPIAddRepoToOrganizationTeam(ctx, testTeam.ID, orgName, "privateTestRepo")) + + t.Run("AssertNumRepos2ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + collabCountRepos[orgName] = 2 + t.Run("AssertNumRepos2ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) +} + +func doCheckOrgCounts(username string, orgCounts map[string]int, strict bool, callback ...func(*testing.T, map[string]int)) func(t *testing.T) { + canonicalCounts := make(map[string]int, len(orgCounts)) + + for key, value := range orgCounts { + newKey := strings.TrimSpace(strings.ToLower(key)) + canonicalCounts[newKey] = value + } + + return func(t *testing.T) { + user := models.AssertExistsAndLoadBean(t, &models.User{ + Name: username, + }).(*models.User) + + user.GetOrganizations(&models.SearchOrganizationsOptions{All: true}) + + calcOrgCounts := map[string]int{} + + for _, org := range user.Orgs { + calcOrgCounts[org.LowerName] = org.NumRepos + count, ok := canonicalCounts[org.LowerName] + if ok { + assert.True(t, count == org.NumRepos, "Number of Repos in %s is %d when we expected %d", org.Name, org.NumRepos, count) + } else { + assert.False(t, strict, "Did not expect to see %s with count %d", org.Name, org.NumRepos) + } + } + + for key, value := range orgCounts { + _, seen := calcOrgCounts[strings.TrimSpace(strings.ToLower(key))] + assert.True(t, seen, "Expected to see %s with %d but did not", key, value) + } + + if len(callback) > 0 { + callback[0](t, calcOrgCounts) + } + } +} diff --git a/models/user.go b/models/user.go index a4afe677d0f2..01cce2506d23 100644 --- a/models/user.go +++ b/models/user.go @@ -726,11 +726,13 @@ func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { groupByStr := groupByCols.String() groupByStr = groupByStr[0 : len(groupByStr)-1] - sess.Select("`user`.*, count(`repository`.id) as org_count"). + sess.Select("`user`.*, count(repo_id) as org_count"). Table("user"). Join("INNER", "org_user", "`org_user`.org_id=`user`.id"). - Join("INNER", "repository", "`repository`.owner_id = `org_user`.org_id"). - Where(accessibleRepositoryCondition(u)). + Join("LEFT", builder. + Select("id as repo_id, owner_id as repo_owner_id"). + From("repository"). + Where(accessibleRepositoryCondition(u)), "`repository`.repo_owner_id = `org_user`.org_id"). And("`org_user`.uid=?", u.ID). GroupBy(groupByStr) if opts.PageSize != 0 { diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 5eeef9fb9d88..498e014dee5e 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -331,7 +331,7 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) { // "403": // "$ref": "#/responses/forbidden" - org, err := models.GetOrgByName(ctx.Params(":org")) + org, err := models.GetOrgByName(ctx.Params(":orgname")) if err != nil { if models.IsErrOrgNotExist(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) From 0af8b3592621688bcc363a2b07aacaaf58774c48 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 11 May 2020 20:24:38 +0100 Subject: [PATCH 7/7] remove orgname from api Signed-off-by: Andrew Thornton --- routers/api/v1/api.go | 6 +++--- routers/api/v1/repo/repo.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 6973e1df5318..0d62b751cc89 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -383,7 +383,7 @@ func orgAssignment(args ...bool) macaron.Handler { var err error if assignOrg { - ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":orgname")) + ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":org")) if err != nil { if models.IsErrOrgNotExist(err) { ctx.NotFound() @@ -857,7 +857,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/users/:username/orgs", org.ListUserOrgs) m.Post("/orgs", reqToken(), bind(api.CreateOrgOption{}), org.Create) m.Get("/orgs", org.GetAll) - m.Group("/orgs/:orgname", func() { + m.Group("/orgs/:org", func() { m.Combo("").Get(org.Get). Patch(reqToken(), reqOrgOwnership(), bind(api.EditOrgOption{}), org.Edit). Delete(reqToken(), reqOrgOwnership(), org.Delete) @@ -907,7 +907,7 @@ func RegisterRoutes(m *macaron.Macaron) { }) m.Group("/repos", func() { m.Get("", org.GetTeamRepos) - m.Combo("/:orgname/:reponame"). + m.Combo("/:org/:reponame"). Put(org.AddTeamRepository). Delete(org.RemoveTeamRepository) }) diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 498e014dee5e..5eeef9fb9d88 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -331,7 +331,7 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) { // "403": // "$ref": "#/responses/forbidden" - org, err := models.GetOrgByName(ctx.Params(":orgname")) + org, err := models.GetOrgByName(ctx.Params(":org")) if err != nil { if models.IsErrOrgNotExist(err) { ctx.Error(http.StatusUnprocessableEntity, "", err)