Skip to content

Fix repo search result #2600

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

Closed
wants to merge 2 commits into from
Closed

Fix repo search result #2600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fed4ee8
Add repository search tests
Morlinest Aug 30, 2017
866c5cf
fix search implementation
daviian Sep 22, 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
214 changes: 214 additions & 0 deletions integrations/api_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,220 @@ func TestAPISearchRepoNotLogin(t *testing.T) {
assert.Contains(t, repo.Name, keyword)
assert.False(t, repo.Private)
}

// Should return all (max 50) public repositories
req = NewRequest(t, "GET", "/api/v1/repos/search?limit=50")
Copy link
Member

@ethantkoenig ethantkoenig Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is a lot of duplicated code. Maybe we should add a helper function

func apiRepoSearchResult(t testing.T, params string, expectedLen int) *api.SearchResults
...
apiRepoSearchResult(t, "?limit=50", 12)

Even better, add a session parameter, so you can use it for the logged-in tests too.

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 If you want that you can review #2575 as it rewrites tests. I wanted to change as few as possible to make reviewing easier, but if it is desired the mentioned PR does this ;-)

resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 12)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Should return (max 10) public repositories
req = NewRequest(t, "GET", "/api/v1/repos/search?limit=10")
resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 10)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

const keyword2 = "big_test_"
// Should return all public repositories which (partial) match keyword
req = NewRequestf(t, "GET", "/api/v1/repos/search?q=%s", keyword2)
resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 4)
for _, repo := range body.Data {
assert.Contains(t, repo.Name, keyword2)
assert.False(t, repo.Private)
}

// Should return all public repositories accessible and related to user
const userID = int64(15)
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", userID)
resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 3)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Should return all public repositories accessible and related to user
const user2ID = int64(16)
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", user2ID)
resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 1)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Should return all public repositories owned by organization
const orgID = int64(17)
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", orgID)
resp = MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 1)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.Equal(t, repo.Owner.ID, orgID)
assert.False(t, repo.Private)
}
}

func TestAPISearchRepoLoggedUser(t *testing.T) {
prepareTestEnv(t)

user := models.AssertExistsAndLoadBean(t, &models.User{ID: 15}).(*models.User)
user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 16}).(*models.User)
session := loginUser(t, user.Name)
session2 := loginUser(t, user2.Name)

var body api.SearchResults

// Get public repositories accessible and not related to logged in user that match the keyword
// Should return all public repositories which (partial) match keyword
const keyword = "big_test_"
req := NewRequestf(t, "GET", "/api/v1/repos/search?q=%s", keyword)
resp := session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 4)
for _, repo := range body.Data {
assert.Contains(t, repo.Name, keyword)
assert.False(t, repo.Private)
}
// Test when user2 is logged in
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 4)
for _, repo := range body.Data {
assert.Contains(t, repo.Name, keyword)
assert.False(t, repo.Private)
}

// Get all public repositories accessible and not related to logged in user
// Should return all (max 50) public repositories
req = NewRequest(t, "GET", "/api/v1/repos/search?limit=50")
resp = session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 12)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}
// Test when user2 is logged in
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 12)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Get all public repositories accessible and not related to logged in user
// Should return all (max 10) public repositories
req = NewRequest(t, "GET", "/api/v1/repos/search?limit=10")
resp = session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 10)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}
// Test when user2 is logged in
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 10)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Get repositories of logged in user
// Should return all public and private repositories accessible and related to user
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", user.ID)
resp = session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 8)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
}
// Test when user2 is logged in
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", user2.ID)
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 2)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
}

// Get repositories of another user
// Should return all public repositories accessible and related to user
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", user2.ID)
resp = session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 1)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}
// Test when user2 is logged in
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", user.ID)
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 3)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.False(t, repo.Private)
}

// Get repositories of organization owned by logged in user
// Should return all public and private repositories owned by organization
const orgID = int64(17)
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", orgID)
resp = session.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 2)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.Equal(t, repo.Owner.ID, orgID)
}

// Get repositories of organization owned by another user
// Should return all public repositories owned by organization
req = NewRequestf(t, "GET", "/api/v1/repos/search?uid=%d", orgID)
resp = session2.MakeRequest(t, req, http.StatusOK)

DecodeJSON(t, resp, &body)
assert.Len(t, body.Data, 1)
for _, repo := range body.Data {
assert.NotEmpty(t, repo.Name)
assert.Equal(t, repo.Owner.ID, orgID)
assert.False(t, repo.Private)
}
}

func TestAPIViewRepo(t *testing.T) {
Expand Down
13 changes: 8 additions & 5 deletions models/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,11 @@ func GetOrgsByUserID(userID int64, showAll bool) ([]*User, error) {
return getOrgsByUserID(sess, userID, showAll)
}

func getOwnedOrgsByUserID(sess *xorm.Session, userID int64) ([]*User, error) {
func getOwnedOrgsByUserID(sess *xorm.Session, userID int64, showAll bool) ([]*User, error) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we call it includePrivate or something similar so it is clearer what is included and unincluded when the parameter is false?

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 called it showAll to be consistent with other methods used similarly. But I can change that at least for my PR.

orgs := make([]*User, 0, 10)
if !showAll {
sess.And("`org_user`.is_public=?", true)
}
return orgs, sess.
Where("`org_user`.uid=?", userID).
And("`org_user`.is_owner=?", true).
Expand All @@ -361,16 +364,16 @@ func getOwnedOrgsByUserID(sess *xorm.Session, userID int64) ([]*User, error) {
}

// GetOwnedOrgsByUserID returns a list of organizations are owned by given user ID.
func GetOwnedOrgsByUserID(userID int64) ([]*User, error) {
func GetOwnedOrgsByUserID(userID int64, showAll bool) ([]*User, error) {
sess := x.NewSession()
defer sess.Close()
return getOwnedOrgsByUserID(sess, userID)
return getOwnedOrgsByUserID(sess, userID, showAll)
}

// GetOwnedOrgsByUserIDDesc returns a list of organizations are owned by
// given user ID, ordered descending by the given condition.
func GetOwnedOrgsByUserIDDesc(userID int64, desc string) ([]*User, error) {
return getOwnedOrgsByUserID(x.Desc(desc), userID)
func GetOwnedOrgsByUserIDDesc(userID int64, desc string, showAll bool) ([]*User, error) {
return getOwnedOrgsByUserID(x.Desc(desc), userID, showAll)
}

// GetOrgUsersByUserID returns all organization-user relations by user ID.
Expand Down
8 changes: 4 additions & 4 deletions models/org_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,28 +322,28 @@ func TestGetOrgsByUserID(t *testing.T) {
func TestGetOwnedOrgsByUserID(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

orgs, err := GetOwnedOrgsByUserID(2)
orgs, err := GetOwnedOrgsByUserID(2, true)
assert.NoError(t, err)
if assert.Len(t, orgs, 1) {
assert.EqualValues(t, 3, orgs[0].ID)
}

orgs, err = GetOwnedOrgsByUserID(4)
orgs, err = GetOwnedOrgsByUserID(4, true)
assert.NoError(t, err)
assert.Len(t, orgs, 0)
}

func TestGetOwnedOrgsByUserIDDesc(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

orgs, err := GetOwnedOrgsByUserIDDesc(5, "id")
orgs, err := GetOwnedOrgsByUserIDDesc(5, "id", true)
assert.NoError(t, err)
if assert.Len(t, orgs, 2) {
assert.EqualValues(t, 7, orgs[0].ID)
assert.EqualValues(t, 6, orgs[1].ID)
}

orgs, err = GetOwnedOrgsByUserIDDesc(4, "id")
orgs, err = GetOwnedOrgsByUserIDDesc(4, "id", true)
assert.NoError(t, err)
assert.Len(t, orgs, 0)
}
Expand Down
38 changes: 30 additions & 8 deletions models/repo_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,26 +155,44 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
// Append conditions
if !opts.Starred && opts.OwnerID > 0 {
var searcherReposCond builder.Cond = builder.Eq{"owner_id": opts.OwnerID}
var ownerIds []int64
// Searcher == Owner
if opts.Searcher != nil {
var ownerIds []int64

ownerIds = append(ownerIds, opts.Searcher.ID)
err = opts.Searcher.GetOrganizations(true)
err = opts.Searcher.GetOwnedOrganizations(true)

if err != nil {
return nil, 0, fmt.Errorf("Organization: %v", err)
}

for _, org := range opts.Searcher.Orgs {
for _, org := range opts.Searcher.OwnedOrgs {
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 != ?)",
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))
}
} else if opts.OwnerID > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed. It's already inside opts.OwnerID > 0 block.

ownerIds = append(ownerIds, opts.OwnerID)

owner, err := GetUserByID(opts.OwnerID)
if err != nil {
return nil, 0, fmt.Errorf("User: %v", err)
}

err = owner.GetOwnedOrganizations(false)
Copy link
Member

Choose a reason for hiding this comment

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

Authorization decision making.


for _, org := range owner.OwnedOrgs {
ownerIds = append(ownerIds, org.ID)
}

if err != nil {
return nil, 0, fmt.Errorf("Organization: %v", err)
}

Copy link
Member

Choose a reason for hiding this comment

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

Missing collaborate repos.

}
searcherReposCond = searcherReposCond.Or(builder.In("owner_id", ownerIds))
Copy link
Member

Choose a reason for hiding this comment

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

Already in condition, line 157.

cond = cond.And(searcherReposCond)
}

Expand Down Expand Up @@ -268,17 +286,21 @@ func GetRecentUpdatedRepositories(opts *SearchRepoOptions) (repos RepositoryList
var ownerIds []int64

ownerIds = append(ownerIds, opts.Searcher.ID)
err := opts.Searcher.GetOrganizations(true)
err := opts.Searcher.GetOwnedOrganizations(true)

if err != nil {
return nil, 0, fmt.Errorf("Organization: %v", err)
}

for _, org := range opts.Searcher.Orgs {
for _, org := range opts.Searcher.OwnedOrgs {
ownerIds = append(ownerIds, org.ID)
}

cond = cond.Or(builder.In("owner_id", ownerIds))
if opts.Collaborate {
cond = cond.Or(builder.Expr("id IN (SELECT repo_id FROM `access` WHERE access.user_id = ?) AND owner_id != ?",
opts.Searcher.ID, opts.Searcher.ID))
}
}

count, err := x.Where(cond).Count(new(Repository))
Expand Down
Loading