Skip to content

Fix repository search function #2689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 17, 2017
Merged

Fix repository search function #2689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bc7d95
Fix/rewrite searchRepositoryByName function
Morlinest Oct 11, 2017
e8a2aed
Fix and remove FIXME
Morlinest Oct 10, 2017
50d7154
Respect membership visibility
Morlinest Oct 11, 2017
b9cb558
Add unit tests
Morlinest Oct 11, 2017
db87c27
Add integration tests
Morlinest Oct 11, 2017
ff3dba9
Remove Searcher completely
Morlinest Oct 12, 2017
7686465
Remove trailing space
Morlinest Oct 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions integrations/api_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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},
Expand Down
26 changes: 25 additions & 1 deletion models/fixtures/access.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,28 @@
id: 7
user_id: 15
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
8 changes: 8 additions & 0 deletions models/fixtures/org_user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions models/fixtures/team.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]'
8 changes: 7 additions & 1 deletion models/fixtures/team_user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,10 @@
id: 6
org_id: 17
team_id: 5
uid: 15
uid: 15

-
id: 7
org_id: 17
team_id: 5
uid: 18
19 changes: 17 additions & 2 deletions models/fixtures/user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,5 +263,20 @@
avatar_email: [email protected]
num_repos: 2
is_active: true
num_members: 1
num_teams: 1
num_members: 2
num_teams: 1

-
id: 18
lower_name: user18
name: user18
full_name: User 18
email: [email protected]
passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
type: 0 # individual
salt: ZogKvWdyEx
is_admin: false
avatar: avatar18
avatar_email: [email protected]
num_repos: 0
is_active: true
89 changes: 38 additions & 51 deletions models/repo_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
var cond = builder.NewCond()

if err != nil {
return nil, 0, fmt.Errorf("Organization: %v", err)
}
if !opts.Private {
cond = cond.And(builder.Eq{"is_private": false})
}

for _, org := range opts.Searcher.Orgs {
ownerIds = append(ownerIds, org.ID)
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}

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))
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(searcherReposCond)
}

if !opts.Private {
cond = cond.And(builder.Eq{"is_private": false})
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 {
Expand All @@ -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")
}

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")
} 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)
repos := make(RepositoryList, 0, opts.PageSize)
if err = sess.
Where(cond).
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
Expand All @@ -230,5 +217,5 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
}
}

return
return repos, count, nil
}
29 changes: 19 additions & 10 deletions models/repo_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ func TestSearchRepositoryByName(t *testing.T) {
Page: 1,
PageSize: 10,
Private: true,
Searcher: &User{ID: 14},
})

assert.NoError(t, err)
Expand All @@ -56,7 +55,6 @@ func TestSearchRepositoryByName(t *testing.T) {
Page: 1,
PageSize: 10,
Private: true,
Searcher: &User{ID: 14},
})

assert.NoError(t, err)
Expand All @@ -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},
Expand All @@ -113,16 +123,16 @@ 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},
}

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)
Expand All @@ -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)
Expand Down
29 changes: 14 additions & 15 deletions routers/api/v1/repo/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,30 @@ 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{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be 404 if user doesn't exist?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig don't thinks so

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks IMO, it doesn't make sense to return a 500 if the client provides a bad user ID

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig yes but not 404. It should be some other error or just empty repo list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks @ethantkoenig Sounds like a usual 400 Bad Request for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change it as it is current behaviour. I'll change it to "bad request".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig @daviian I see problem, this response is defined in api (swagger):

	//     Responses:
	//       200: SearchResults
	//       500: SearchError

Copy link
Member Author

@Morlinest Morlinest Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep it as it is now and make changes in v2 api. I can fix only leaking internal information from response. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Morlinest @ethantkoenig as this is documented so let's not introduce breaking changes in API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, calling this a breaking change seems like a stretch, but I don't want to block this PR so I'll accept

OK: false,
Error: err.Error(),
})
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
}
}

Expand Down
1 change: 0 additions & 1 deletion routers/home.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand Down