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
This commit is contained in:
Morlinest 2017-10-17 17:20:22 +02:00 committed by Lunny Xiao
parent af4a094e5d
commit ccd3577970
10 changed files with 145 additions and 93 deletions

View file

@ -50,6 +50,7 @@ func TestAPISearchRepo(t *testing.T) {
user := models.AssertExistsAndLoadBean(t, &models.User{ID: 15}).(*models.User) user := models.AssertExistsAndLoadBean(t, &models.User{ID: 15}).(*models.User)
user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 16}).(*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) orgUser := models.AssertExistsAndLoadBean(t, &models.User{ID: 17}).(*models.User)
// Map of expected results, where key is user for login // 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_"}}, user2: {count: 4, repoName: "big_test_"}},
}, },
{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{ {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: 4},
nil: {count: 2}, user: {count: 8, includesPrivate: true},
user: {count: 8, includesPrivate: true}, user2: {count: 4}},
// 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}},
}, },
{name: "RepositoriesAccessibleAndRelatedToUser2", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user2.ID), expectedResults: expectedResults{ {name: "RepositoriesAccessibleAndRelatedToUser2", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user2.ID), expectedResults: expectedResults{
nil: {count: 1}, nil: {count: 1},
user: {count: 1}, user: {count: 1},
user2: {count: 2, includesPrivate: true}}, 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{ {name: "RepositoriesOwnedByOrganization", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", orgUser.ID), expectedResults: expectedResults{
nil: {count: 1, repoOwnerID: orgUser.ID}, nil: {count: 1, repoOwnerID: orgUser.ID},
user: {count: 2, repoOwnerID: orgUser.ID, includesPrivate: true}, user: {count: 2, repoOwnerID: orgUser.ID, includesPrivate: true},

View file

@ -39,3 +39,27 @@
user_id: 15 user_id: 15
repo_id: 24 repo_id: 24
mode: 4 # owner 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

View file

@ -37,3 +37,11 @@
is_public: true is_public: true
is_owner: true is_owner: true
num_teams: 1 num_teams: 1
-
id: 6
uid: 18
org_id: 17
is_public: false
is_owner: true
num_teams: 1

View file

@ -44,5 +44,5 @@
name: Owners name: Owners
authorize: 4 # owner authorize: 4 # owner
num_repos: 2 num_repos: 2
num_members: 1 num_members: 2
unit_types: '[1,2,3,4,5,6,7]' unit_types: '[1,2,3,4,5,6,7]'

View file

@ -33,3 +33,9 @@
org_id: 17 org_id: 17
team_id: 5 team_id: 5
uid: 15 uid: 15
-
id: 7
org_id: 17
team_id: 5
uid: 18

View file

@ -263,5 +263,20 @@
avatar_email: user17@example.com avatar_email: user17@example.com
num_repos: 2 num_repos: 2
is_active: true is_active: true
num_members: 1 num_members: 2
num_teams: 1 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

View file

@ -98,7 +98,6 @@ type SearchRepoOptions struct {
// //
// in: query // in: query
OwnerID int64 `json:"uid"` OwnerID int64 `json:"uid"`
Searcher *User `json:"-"` //ID of the person who's seeking
OrderBy SearchOrderBy `json:"-"` OrderBy SearchOrderBy `json:"-"`
Private bool `json:"-"` // Include private repositories in results Private bool `json:"-"` // Include private repositories in results
Collaborate bool `json:"-"` // Include collaborative repositories Collaborate bool `json:"-"` // Include collaborative repositories
@ -136,57 +135,48 @@ const (
// SearchRepositoryByName takes keyword and part of repository name to search, // SearchRepositoryByName takes keyword and part of repository name to search,
// it returns results in given range and number of total results. // it returns results in given range and number of total results.
func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, count int64, err error) { func SearchRepositoryByName(opts *SearchRepoOptions) (RepositoryList, int64, error) {
var cond = builder.NewCond()
if opts.Page <= 0 { if opts.Page <= 0 {
opts.Page = 1 opts.Page = 1
} }
var starJoin bool var cond = builder.NewCond()
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)
}
if !opts.Private { if !opts.Private {
cond = cond.And(builder.Eq{"is_private": false}) 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 { if opts.OwnerID > 0 && opts.AllPublic {
cond = cond.Or(builder.Eq{"is_private": false}) cond = cond.Or(builder.Eq{"is_private": false})
} }
opts.Keyword = strings.ToLower(opts.Keyword)
if 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 { if len(opts.OrderBy) == 0 {
@ -196,26 +186,23 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
sess := x.NewSession() sess := x.NewSession()
defer sess.Close() defer sess.Close()
if starJoin { if starred {
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)
}
sess.Join("INNER", "star", "star.repo_id = repository.id") 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. if err = sess.
Where(cond). Where(cond).
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize). Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
@ -230,5 +217,5 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
} }
} }
return return repos, count, nil
} }

View file

@ -42,7 +42,6 @@ func TestSearchRepositoryByName(t *testing.T) {
Page: 1, Page: 1,
PageSize: 10, PageSize: 10,
Private: true, Private: true,
Searcher: &User{ID: 14},
}) })
assert.NoError(t, err) assert.NoError(t, err)
@ -56,7 +55,6 @@ func TestSearchRepositoryByName(t *testing.T) {
Page: 1, Page: 1,
PageSize: 10, PageSize: 10,
Private: true, Private: true,
Searcher: &User{ID: 14},
}) })
assert.NoError(t, err) assert.NoError(t, err)
@ -82,16 +80,28 @@ func TestSearchRepositoryByName(t *testing.T) {
count: 8}, count: 8},
{name: "PublicRepositoriesOfUser", {name: "PublicRepositoriesOfUser",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15}, 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", {name: "PublicAndPrivateRepositoriesOfUser",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true}, 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", {name: "PublicRepositoriesOfUserIncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Collaborate: true}, opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Collaborate: true},
count: 4}, count: 4},
{name: "PublicRepositoriesOfUser2IncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Collaborate: true},
count: 1},
{name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative", {name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true}, opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true},
count: 8}, count: 8},
{name: "PublicAndPrivateRepositoriesOfUser2IncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true, Collaborate: true},
count: 4},
{name: "PublicRepositoriesOfOrganization", {name: "PublicRepositoriesOfOrganization",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17}, opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17},
count: 1}, count: 1},
@ -113,6 +123,9 @@ func TestSearchRepositoryByName(t *testing.T) {
{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true, AllPublic: true}, opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true, AllPublic: true},
count: 10}, 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", {name: "AllPublic/PublicRepositoriesOfOrganization",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, AllPublic: true}, opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, AllPublic: true},
count: 12}, count: 12},
@ -120,9 +133,6 @@ func TestSearchRepositoryByName(t *testing.T) {
for _, testCase := range testCases { for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) { 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) repos, count, err := SearchRepositoryByName(testCase.opts)
assert.NoError(t, err) assert.NoError(t, err)
@ -143,10 +153,9 @@ func TestSearchRepositoryByName(t *testing.T) {
assert.Contains(t, repo.Name, testCase.opts.Keyword) 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 && !testCase.opts.AllPublic {
/*if testCase.opts.OwnerID > 0 && !testCase.opts.Collaborate && !AllPublic {
assert.Equal(t, testCase.opts.OwnerID, repo.Owner.ID) assert.Equal(t, testCase.opts.OwnerID, repo.Owner.ID)
}*/ }
if !testCase.opts.Private { if !testCase.opts.Private {
assert.False(t, repo.IsPrivate) assert.False(t, repo.IsPrivate)

View file

@ -34,17 +34,14 @@ func Search(ctx *context.APIContext) {
OwnerID: ctx.QueryInt64("uid"), OwnerID: ctx.QueryInt64("uid"),
PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")), PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")),
} }
if ctx.User != nil && ctx.User.ID == opts.OwnerID {
opts.Searcher = ctx.User
}
// Check visibility. if opts.OwnerID > 0 {
if ctx.IsSigned && opts.OwnerID > 0 { var repoOwner *models.User
if ctx.User.ID == opts.OwnerID { if ctx.User != nil && ctx.User.ID == opts.OwnerID {
opts.Private = true repoOwner = ctx.User
opts.Collaborate = true
} else { } else {
u, err := models.GetUserByID(opts.OwnerID) var err error
repoOwner, err = models.GetUserByID(opts.OwnerID)
if err != nil { if err != nil {
ctx.JSON(500, api.SearchError{ ctx.JSON(500, api.SearchError{
OK: false, OK: false,
@ -52,13 +49,15 @@ func Search(ctx *context.APIContext) {
}) })
return return
} }
if u.IsOrganization() && u.IsOwnedBy(ctx.User.ID) { }
opts.Private = true
}
if !u.IsOrganization() { if !repoOwner.IsOrganization() {
opts.Collaborate = true opts.Collaborate = true
} }
// Check visibility.
if ctx.IsSigned && (ctx.User.ID == repoOwner.ID || (repoOwner.IsOrganization() && repoOwner.IsOwnedBy(ctx.User.ID))) {
opts.Private = true
} }
} }

View file

@ -120,7 +120,6 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) {
Private: opts.Private, Private: opts.Private,
Keyword: keyword, Keyword: keyword,
OwnerID: opts.OwnerID, OwnerID: opts.OwnerID,
Searcher: ctx.User,
Collaborate: true, Collaborate: true,
AllPublic: true, AllPublic: true,
}) })