From ccd3577970b64078eb156a736b1583833f80b4a3 Mon Sep 17 00:00:00 2001 From: Morlinest Date: Tue, 17 Oct 2017 17:20:22 +0200 Subject: [PATCH] Fix repository search function (#2689) * Fix and remove FIXME * Respect membership visibility * Fix/rewrite searchRepositoryByName function * Add unit tests * Add integration tests * Remove Searcher completely * Remove trailing space --- integrations/api_repo_test.go | 15 ++++-- models/fixtures/access.yml | 26 ++++++++- models/fixtures/org_user.yml | 8 +++ models/fixtures/team.yml | 4 +- models/fixtures/team_user.yml | 8 ++- models/fixtures/user.yml | 19 ++++++- models/repo_list.go | 99 +++++++++++++++-------------------- models/repo_list_test.go | 29 ++++++---- routers/api/v1/repo/repo.go | 29 +++++----- routers/home.go | 1 - 10 files changed, 145 insertions(+), 93 deletions(-) diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go index c536b8d32..f517ee42c 100644 --- a/integrations/api_repo_test.go +++ b/integrations/api_repo_test.go @@ -50,6 +50,7 @@ func TestAPISearchRepo(t *testing.T) { user := models.AssertExistsAndLoadBean(t, &models.User{ID: 15}).(*models.User) user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 16}).(*models.User) + user3 := models.AssertExistsAndLoadBean(t, &models.User{ID: 18}).(*models.User) orgUser := models.AssertExistsAndLoadBean(t, &models.User{ID: 17}).(*models.User) // Map of expected results, where key is user for login @@ -85,17 +86,21 @@ func TestAPISearchRepo(t *testing.T) { user2: {count: 4, repoName: "big_test_"}}, }, {name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{ - // FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user - nil: {count: 2}, - user: {count: 8, includesPrivate: true}, - // FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user - user2: {count: 2}}, + nil: {count: 4}, + user: {count: 8, includesPrivate: true}, + user2: {count: 4}}, }, {name: "RepositoriesAccessibleAndRelatedToUser2", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user2.ID), expectedResults: expectedResults{ nil: {count: 1}, user: {count: 1}, user2: {count: 2, includesPrivate: true}}, }, + {name: "RepositoriesAccessibleAndRelatedToUser3", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user3.ID), expectedResults: expectedResults{ + nil: {count: 1}, + user: {count: 1}, + user2: {count: 1}, + user3: {count: 4, includesPrivate: true}}, + }, {name: "RepositoriesOwnedByOrganization", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", orgUser.ID), expectedResults: expectedResults{ nil: {count: 1, repoOwnerID: orgUser.ID}, user: {count: 2, repoOwnerID: orgUser.ID, includesPrivate: true}, diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 34d5c5c59..9c149b78d 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -38,4 +38,28 @@ id: 7 user_id: 15 repo_id: 24 - mode: 4 # owner \ No newline at end of file + mode: 4 # owner + +- + id: 8 + user_id: 18 + repo_id: 23 + mode: 4 # owner + +- + id: 9 + user_id: 18 + repo_id: 24 + mode: 4 # owner + +- + id: 10 + user_id: 18 + repo_id: 22 + mode: 2 # write + +- + id: 11 + user_id: 18 + repo_id: 21 + mode: 2 # write \ No newline at end of file diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index c7fe2cf36..50d8ef5e6 100644 --- a/models/fixtures/org_user.yml +++ b/models/fixtures/org_user.yml @@ -37,3 +37,11 @@ is_public: true is_owner: true num_teams: 1 + +- + id: 6 + uid: 18 + org_id: 17 + is_public: false + is_owner: true + num_teams: 1 \ No newline at end of file diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index 298de648d..2b2186dea 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -44,5 +44,5 @@ name: Owners authorize: 4 # owner num_repos: 2 - num_members: 1 - unit_types: '[1,2,3,4,5,6,7]' + num_members: 2 + unit_types: '[1,2,3,4,5,6,7]' \ No newline at end of file diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index 8d9d92037..56025bb0b 100644 --- a/models/fixtures/team_user.yml +++ b/models/fixtures/team_user.yml @@ -32,4 +32,10 @@ id: 6 org_id: 17 team_id: 5 - uid: 15 \ No newline at end of file + uid: 15 + +- + id: 7 + org_id: 17 + team_id: 5 + uid: 18 \ No newline at end of file diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index e1fbc9e57..73ec1c859 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -263,5 +263,20 @@ avatar_email: user17@example.com num_repos: 2 is_active: true - num_members: 1 - num_teams: 1 \ No newline at end of file + num_members: 2 + num_teams: 1 + +- + id: 18 + lower_name: user18 + name: user18 + full_name: User 18 + email: user18@example.com + passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password + type: 0 # individual + salt: ZogKvWdyEx + is_admin: false + avatar: avatar18 + avatar_email: user18@example.com + num_repos: 0 + is_active: true \ No newline at end of file diff --git a/models/repo_list.go b/models/repo_list.go index 4aeda9067..2c4c66ac3 100644 --- a/models/repo_list.go +++ b/models/repo_list.go @@ -98,7 +98,6 @@ type SearchRepoOptions struct { // // in: query OwnerID int64 `json:"uid"` - Searcher *User `json:"-"` //ID of the person who's seeking OrderBy SearchOrderBy `json:"-"` Private bool `json:"-"` // Include private repositories in results Collaborate bool `json:"-"` // Include collaborative repositories @@ -136,57 +135,48 @@ const ( // SearchRepositoryByName takes keyword and part of repository name to search, // it returns results in given range and number of total results. -func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, count int64, err error) { - var cond = builder.NewCond() +func SearchRepositoryByName(opts *SearchRepoOptions) (RepositoryList, int64, error) { if opts.Page <= 0 { opts.Page = 1 } - var starJoin bool - if opts.Starred && opts.OwnerID > 0 { - cond = builder.Eq{ - "star.uid": opts.OwnerID, - } - starJoin = true - } - - // Append conditions - if !opts.Starred && opts.OwnerID > 0 { - var searcherReposCond builder.Cond = builder.Eq{"owner_id": opts.OwnerID} - if opts.Searcher != nil { - var ownerIds []int64 - - ownerIds = append(ownerIds, opts.Searcher.ID) - err = opts.Searcher.GetOrganizations(true) - - if err != nil { - return nil, 0, fmt.Errorf("Organization: %v", err) - } - - for _, org := range opts.Searcher.Orgs { - ownerIds = append(ownerIds, org.ID) - } - - searcherReposCond = searcherReposCond.Or(builder.In("owner_id", ownerIds)) - if opts.Collaborate { - searcherReposCond = searcherReposCond.Or(builder.Expr("id IN (SELECT repo_id FROM `access` WHERE access.user_id = ? AND owner_id != ?)", - opts.Searcher.ID, opts.Searcher.ID)) - } - } - cond = cond.And(searcherReposCond) - } + var cond = builder.NewCond() if !opts.Private { cond = cond.And(builder.Eq{"is_private": false}) } + starred := false + if opts.OwnerID > 0 { + if opts.Starred { + starred = true + cond = builder.Eq{ + "star.uid": opts.OwnerID, + } + } else { + var accessCond builder.Cond = builder.Eq{"owner_id": opts.OwnerID} + + if opts.Collaborate { + collaborateCond := builder.And( + builder.Expr("id IN (SELECT repo_id FROM `access` WHERE access.user_id = ?)", opts.OwnerID), + builder.Neq{"owner_id": opts.OwnerID}) + if !opts.Private { + collaborateCond = collaborateCond.And(builder.Expr("owner_id NOT IN (SELECT org_id FROM org_user WHERE org_user.uid = ? AND org_user.is_public = ?)", opts.OwnerID, false)) + } + + accessCond = accessCond.Or(collaborateCond) + } + + cond = cond.And(accessCond) + } + } + if opts.OwnerID > 0 && opts.AllPublic { cond = cond.Or(builder.Eq{"is_private": false}) } - opts.Keyword = strings.ToLower(opts.Keyword) if opts.Keyword != "" { - cond = cond.And(builder.Like{"lower_name", opts.Keyword}) + cond = cond.And(builder.Like{"lower_name", strings.ToLower(opts.Keyword)}) } if len(opts.OrderBy) == 0 { @@ -196,26 +186,23 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun sess := x.NewSession() defer sess.Close() - if starJoin { - count, err = sess. - Join("INNER", "star", "star.repo_id = repository.id"). - Where(cond). - Count(new(Repository)) - if err != nil { - return nil, 0, fmt.Errorf("Count: %v", err) - } - + if starred { sess.Join("INNER", "star", "star.repo_id = repository.id") - } else { - count, err = sess. - Where(cond). - Count(new(Repository)) - if err != nil { - return nil, 0, fmt.Errorf("Count: %v", err) - } } - repos = make([]*Repository, 0, opts.PageSize) + count, err := sess. + Where(cond). + Count(new(Repository)) + if err != nil { + return nil, 0, fmt.Errorf("Count: %v", err) + } + + // Set again after reset by Count() + if starred { + sess.Join("INNER", "star", "star.repo_id = repository.id") + } + + repos := make(RepositoryList, 0, opts.PageSize) if err = sess. Where(cond). Limit(opts.PageSize, (opts.Page-1)*opts.PageSize). @@ -230,5 +217,5 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun } } - return + return repos, count, nil } diff --git a/models/repo_list_test.go b/models/repo_list_test.go index 160ba57d3..4d125633a 100644 --- a/models/repo_list_test.go +++ b/models/repo_list_test.go @@ -42,7 +42,6 @@ func TestSearchRepositoryByName(t *testing.T) { Page: 1, PageSize: 10, Private: true, - Searcher: &User{ID: 14}, }) assert.NoError(t, err) @@ -56,7 +55,6 @@ func TestSearchRepositoryByName(t *testing.T) { Page: 1, PageSize: 10, Private: true, - Searcher: &User{ID: 14}, }) assert.NoError(t, err) @@ -82,16 +80,28 @@ func TestSearchRepositoryByName(t *testing.T) { count: 8}, {name: "PublicRepositoriesOfUser", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15}, - count: 3}, // FIXME: Should return 2 (only directly owned repositories), now includes 1 public repository from owned organization + count: 2}, + {name: "PublicRepositoriesOfUser2", + opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18}, + count: 0}, {name: "PublicAndPrivateRepositoriesOfUser", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true}, - count: 6}, // FIXME: Should return 4 (only directly owned repositories), now includes 2 repositories from owned organization + count: 4}, + {name: "PublicAndPrivateRepositoriesOfUser2", + opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true}, + count: 0}, {name: "PublicRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Collaborate: true}, count: 4}, + {name: "PublicRepositoriesOfUser2IncludingCollaborative", + opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Collaborate: true}, + count: 1}, {name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true}, count: 8}, + {name: "PublicAndPrivateRepositoriesOfUser2IncludingCollaborative", + opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true, Collaborate: true}, + count: 4}, {name: "PublicRepositoriesOfOrganization", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17}, count: 1}, @@ -113,6 +123,9 @@ func TestSearchRepositoryByName(t *testing.T) { {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true, AllPublic: true}, count: 10}, + {name: "AllPublic/PublicAndPrivateRepositoriesOfUser2IncludingCollaborativeByName", + opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 18, Private: true, Collaborate: true, AllPublic: true}, + count: 8}, {name: "AllPublic/PublicRepositoriesOfOrganization", opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, AllPublic: true}, count: 12}, @@ -120,9 +133,6 @@ func TestSearchRepositoryByName(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - if testCase.opts.OwnerID > 0 { - testCase.opts.Searcher = &User{ID: testCase.opts.OwnerID} - } repos, count, err := SearchRepositoryByName(testCase.opts) assert.NoError(t, err) @@ -143,10 +153,9 @@ func TestSearchRepositoryByName(t *testing.T) { assert.Contains(t, repo.Name, testCase.opts.Keyword) } - // FIXME: Can't check, need to fix current behaviour (see previous FIXME comments in test cases) - /*if testCase.opts.OwnerID > 0 && !testCase.opts.Collaborate && !AllPublic { + if testCase.opts.OwnerID > 0 && !testCase.opts.Collaborate && !testCase.opts.AllPublic { assert.Equal(t, testCase.opts.OwnerID, repo.Owner.ID) - }*/ + } if !testCase.opts.Private { assert.False(t, repo.IsPrivate) diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 20393102f..30e1186c0 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -34,17 +34,14 @@ func Search(ctx *context.APIContext) { OwnerID: ctx.QueryInt64("uid"), PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")), } - if ctx.User != nil && ctx.User.ID == opts.OwnerID { - opts.Searcher = ctx.User - } - // Check visibility. - if ctx.IsSigned && opts.OwnerID > 0 { - if ctx.User.ID == opts.OwnerID { - opts.Private = true - opts.Collaborate = true + if opts.OwnerID > 0 { + var repoOwner *models.User + if ctx.User != nil && ctx.User.ID == opts.OwnerID { + repoOwner = ctx.User } else { - u, err := models.GetUserByID(opts.OwnerID) + var err error + repoOwner, err = models.GetUserByID(opts.OwnerID) if err != nil { ctx.JSON(500, api.SearchError{ OK: false, @@ -52,13 +49,15 @@ func Search(ctx *context.APIContext) { }) return } - if u.IsOrganization() && u.IsOwnedBy(ctx.User.ID) { - opts.Private = true - } + } - if !u.IsOrganization() { - opts.Collaborate = true - } + if !repoOwner.IsOrganization() { + opts.Collaborate = true + } + + // Check visibility. + if ctx.IsSigned && (ctx.User.ID == repoOwner.ID || (repoOwner.IsOrganization() && repoOwner.IsOwnedBy(ctx.User.ID))) { + opts.Private = true } } diff --git a/routers/home.go b/routers/home.go index afcb3d35e..cf957c121 100644 --- a/routers/home.go +++ b/routers/home.go @@ -120,7 +120,6 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) { Private: opts.Private, Keyword: keyword, OwnerID: opts.OwnerID, - Searcher: ctx.User, Collaborate: true, AllPublic: true, })