Skip to content
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

Jump to bottom

SearchRepositoryByName improvements and unification #6897

Merged
merged 12 commits into from
May 15, 2019
Merged

SearchRepositoryByName improvements and unification #6897

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d8245f8
SearchRepositoryByName improvements and unification
zeripath May 9, 2019
2d2af01
Return repositories that ownerID is a member of team for
zeripath May 9, 2019
351b0e7
More fixes for \#6670
zeripath May 9, 2019
61c01d5
update tests
zeripath May 9, 2019
80972af
Update integrations/sqlite.ini
zeripath May 10, 2019
d6c73a4
Update routers/user/profile.go
zeripath May 10, 2019
96128e4
Update routers/user/profile.go
zeripath May 10, 2019
100c472
Merge branch 'master' into fix-#5454-improve-api-repos-search
zeripath May 10, 2019
5204ea1
Merge branch 'master' into fix-#5454-improve-api-repos-search
zeripath May 11, 2019
b5bf3e2
Merge branch 'master' into fix-#5454-improve-api-repos-search
zeripath May 12, 2019
6e4aea5
Merge branch 'master' into fix-#5454-improve-api-repos-search
zeripath May 13, 2019
9ac2f63
Merge branch 'master' into fix-#5454-improve-api-repos-search
lafriks May 15, 2019
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
48 changes: 27 additions & 21 deletions integrations/api_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,40 +69,41 @@ func TestAPISearchRepo(t *testing.T) {
name, requestURL string
expectedResults
}{
{name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50", expectedResults: expectedResults{
{name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{
nil: {count: 21},
user: {count: 21},
user2: {count: 21}},
},
{name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10", expectedResults: expectedResults{
{name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10&private=false", expectedResults: expectedResults{
nil: {count: 10},
user: {count: 10},
user2: {count: 10}},
},
{name: "RepositoriesDefaultMax10", requestURL: "/api/v1/repos/search?default", expectedResults: expectedResults{
{name: "RepositoriesDefaultMax10", requestURL: "/api/v1/repos/search?default&private=false", expectedResults: expectedResults{
nil: {count: 10},
user: {count: 10},
user2: {count: 10}},
},
{name: "RepositoriesByName", requestURL: fmt.Sprintf("/api/v1/repos/search?q=%s", "big_test_"), expectedResults: expectedResults{
{name: "RepositoriesByName", requestURL: fmt.Sprintf("/api/v1/repos/search?q=%s&private=false", "big_test_"), expectedResults: expectedResults{
nil: {count: 7, repoName: "big_test_"},
user: {count: 7, repoName: "big_test_"},
user2: {count: 7, repoName: "big_test_"}},
},
{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{
nil: {count: 4},
user: {count: 8, includesPrivate: true},
user2: {count: 4}},
nil: {count: 5},
user: {count: 9, includesPrivate: true},
user2: {count: 5, includesPrivate: true}},
},
{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}},
user: {count: 2, includesPrivate: true},
user2: {count: 2, includesPrivate: true},
user4: {count: 1}},
},
{name: "RepositoriesAccessibleAndRelatedToUser3", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user3.ID), expectedResults: expectedResults{
nil: {count: 1},
user: {count: 1},
user2: {count: 1},
user: {count: 4, includesPrivate: true},
user2: {count: 2, includesPrivate: true},
user3: {count: 4, includesPrivate: true}},
},
{name: "RepositoriesOwnedByOrganization", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", orgUser.ID), expectedResults: expectedResults{
Expand All @@ -112,12 +113,12 @@ func TestAPISearchRepo(t *testing.T) {
},
{name: "RepositoriesAccessibleAndRelatedToUser4", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user4.ID), expectedResults: expectedResults{
nil: {count: 3},
user: {count: 3},
user4: {count: 6, includesPrivate: true}}},
user: {count: 4, includesPrivate: true},
user4: {count: 7, includesPrivate: true}}},
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeSource", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "source"), expectedResults: expectedResults{
nil: {count: 0},
user: {count: 0},
user4: {count: 0, includesPrivate: true}}},
user: {count: 1, includesPrivate: true},
user4: {count: 1, includesPrivate: true}}},
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeFork", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "fork"), expectedResults: expectedResults{
nil: {count: 1},
user: {count: 1},
Expand All @@ -136,8 +137,8 @@ func TestAPISearchRepo(t *testing.T) {
user4: {count: 2, includesPrivate: true}}},
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeCollaborative", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "collaborative"), expectedResults: expectedResults{
nil: {count: 0},
user: {count: 0},
user4: {count: 0, includesPrivate: true}}},
user: {count: 1, includesPrivate: true},
user4: {count: 1, includesPrivate: true}}},
}

for _, testCase := range testCases {
Expand All @@ -164,14 +165,19 @@ func TestAPISearchRepo(t *testing.T) {
var body api.SearchResults
DecodeJSON(t, response, &body)

assert.Len(t, body.Data, expected.count)
repoNames := make([]string, 0, len(body.Data))
for _, repo := range body.Data {
repoNames = append(repoNames, fmt.Sprintf("%d:%s:%t", repo.ID, repo.FullName, repo.Private))
}
assert.Len(t, repoNames, expected.count)
for _, repo := range body.Data {
r := getRepo(t, repo.ID)
hasAccess, err := models.HasAccess(userID, r)
assert.NoError(t, err)
assert.True(t, hasAccess)
assert.NoError(t, err, "Error when checking if User: %d has access to %s: %v", userID, repo.FullName, err)
assert.True(t, hasAccess, "User: %d does not have access to %s", userID, repo.FullName)

assert.NotEmpty(t, repo.Name)
assert.Equal(t, repo.Name, r.Name)

if len(expected.repoName) > 0 {
assert.Contains(t, repo.Name, expected.repoName)
Expand All @@ -182,7 +188,7 @@ func TestAPISearchRepo(t *testing.T) {
}

if !expected.includesPrivate {
assert.False(t, repo.Private)
assert.False(t, repo.Private, "User: %d not expecting private repository: %s", userID, repo.FullName)
}
}
})
Expand Down
1 change: 1 addition & 0 deletions integrations/sqlite.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ PROVIDER = file
MODE = test,file
ROOT_PATH = sqlite-log
REDIRECT_MACARON_LOG = true
DISABLE_ROUTER_LOG = true
zeripath marked this conversation as resolved.
Show resolved Hide resolved
ROUTER = ,
MACARON = ,
XORM = file
Expand Down
132 changes: 70 additions & 62 deletions models/repo_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"code.gitea.io/gitea/modules/util"

"github.com/go-xorm/builder"
"github.com/go-xorm/core"
)

// RepositoryListDefaultPageSize is the default number of repositories
Expand Down Expand Up @@ -112,15 +111,17 @@ func (repos MirrorRepositoryList) LoadAttributes() error {

// SearchRepoOptions holds the search options
type SearchRepoOptions struct {
Keyword string
OwnerID int64
OrderBy SearchOrderBy
Private bool // Include private repositories in results
Starred bool
Page int
IsProfile bool
AllPublic bool // Include also all public repositories
PageSize int // Can be smaller than or equal to setting.ExplorePagingNum
UserID int64
UserIsAdmin bool
Keyword string
OwnerID int64
OrderBy SearchOrderBy
Private bool // Include private repositories in results
StarredByID int64
Page int
IsProfile bool
AllPublic bool // Include also all public repositories
PageSize int // Can be smaller than or equal to setting.ExplorePagingNum
// None -> include collaborative AND non-collaborative
// True -> include just collaborative
// False -> incude just non-collaborative
Expand Down Expand Up @@ -168,72 +169,79 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (RepositoryList, int64, err
if opts.Page <= 0 {
opts.Page = 1
}

var cond = builder.NewCond()

if !opts.Private {
if opts.Private {
if !opts.UserIsAdmin && opts.UserID != 0 && opts.UserID != opts.OwnerID {
// OK we're in the context of a User
// We should be Either
cond = cond.And(builder.Or(
// 1. Be able to see all non-private repositories that either:
cond.And(
builder.Eq{"is_private": false},
builder.Or(
// A. Aren't in organisations __OR__
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
// B. Isn't a private organisation. (Limited is OK because we're logged in)
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"visibility": structs.VisibleTypePrivate}))),
),
// 2. Be able to see all repositories that we have access to
builder.In("id", builder.Select("repo_id").
From("`access`").
Where(builder.And(
builder.Eq{"user_id": opts.UserID},
builder.Gt{"mode": int(AccessModeNone)}))),
// 3. Be able to see all repositories that we are in a team
builder.In("id", builder.Select("`team_repo`.repo_id").
From("team_repo").
Where(builder.Eq{"`team_user`.uid": opts.UserID}).
Join("INNER", "team_user", "`team_user`.team_id = `team_repo`.team_id"))))
}
} else {
// Not looking at private organisations
// We should be able to see all non-private repositories that either:
cond = cond.And(builder.Eq{"is_private": false})
accessCond := builder.Or(
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Or(builder.Eq{"visibility": structs.VisibleTypeLimited}, builder.Eq{"visibility": structs.VisibleTypePrivate}))),
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})))
// A. Aren't in organisations __OR__
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
// B. Isn't a private or limited organisation.
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Or(builder.Eq{"visibility": structs.VisibleTypeLimited}, builder.Eq{"visibility": structs.VisibleTypePrivate}))))
cond = cond.And(accessCond)
}

// Restrict to starred repositories
if opts.StarredByID > 0 {
cond = cond.And(builder.In("id", builder.Select("repo_id").From("star").Where(builder.Eq{"uid": opts.StarredByID})))
}

// Restrict repositories to those the OwnerID owns or contributes to as per opts.Collaborate
if opts.OwnerID > 0 {
if opts.Starred {
cond = cond.And(builder.In("id", builder.Select("repo_id").From("star").Where(builder.Eq{"uid": opts.OwnerID})))
} else {
var accessCond = builder.NewCond()
if opts.Collaborate != util.OptionalBoolTrue {
accessCond = builder.Eq{"owner_id": opts.OwnerID}
}
var accessCond = builder.NewCond()
if opts.Collaborate != util.OptionalBoolTrue {
accessCond = builder.Eq{"owner_id": opts.OwnerID}
}

if opts.Collaborate != util.OptionalBoolFalse {
collaborateCond := builder.And(
if opts.Collaborate != util.OptionalBoolFalse {
collaborateCond := builder.And(
builder.Or(
builder.Expr("repository.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)
builder.In("id", builder.Select("`team_repo`.repo_id").
From("team_repo").
Where(builder.Eq{"`team_user`.uid": opts.OwnerID}).
Join("INNER", "team_user", "`team_user`.team_id = `team_repo`.team_id"))),
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))
}

var exprCond builder.Cond
if DbCfg.Type == core.POSTGRES {
exprCond = builder.Expr("org_user.org_id = \"user\".id")
} else if DbCfg.Type == core.MSSQL {
exprCond = builder.Expr("org_user.org_id = [user].id")
} else {
exprCond = builder.Eq{"org_user.org_id": "user.id"}
}

visibilityCond := builder.Or(
builder.In("owner_id",
builder.Select("org_id").From("org_user").
LeftJoin("`user`", exprCond).
Where(
builder.And(
builder.Eq{"uid": opts.OwnerID},
builder.Eq{"visibility": structs.VisibleTypePrivate})),
),
builder.In("owner_id",
builder.Select("id").From("`user`").
Where(
builder.Or(
builder.Eq{"visibility": structs.VisibleTypePublic},
builder.Eq{"visibility": structs.VisibleTypeLimited})),
),
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
)
cond = cond.And(visibilityCond)

if opts.AllPublic {
accessCond = accessCond.Or(builder.Eq{"is_private": false})
}
accessCond = accessCond.Or(collaborateCond)
}

cond = cond.And(accessCond)
if opts.AllPublic {
accessCond = accessCond.Or(builder.Eq{"is_private": false})
}

cond = cond.And(accessCond)
}

if opts.Keyword != "" {
Expand Down
8 changes: 4 additions & 4 deletions models/repo_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func TestSearchRepositoryByName(t *testing.T) {
count: 4},
{name: "PublicRepositoriesOfUserIncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15},
count: 4},
count: 5},
{name: "PublicRepositoriesOfUser2IncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18},
count: 1},
Expand All @@ -126,13 +126,13 @@ func TestSearchRepositoryByName(t *testing.T) {
count: 3},
{name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true},
count: 8},
count: 9},
{name: "PublicAndPrivateRepositoriesOfUser2IncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true},
count: 4},
{name: "PublicAndPrivateRepositoriesOfUser3IncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 20, Private: true},
count: 6},
count: 7},
{name: "PublicRepositoriesOfOrganization",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, Collaborate: util.OptionalBoolFalse},
count: 1},
Expand All @@ -150,7 +150,7 @@ func TestSearchRepositoryByName(t *testing.T) {
count: 21},
{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, AllPublic: true},
count: 26},
count: 27},
{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, AllPublic: true},
count: 15},
Expand Down
49 changes: 13 additions & 36 deletions routers/api/v1/repo/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ func Search(ctx *context.APIContext) {
// description: search only for repos that the user with the given id owns or contributes to
// type: integer
// format: int64
// - name: starredBy
// in: query
// description: search only for repos that the user with the given id has starred
// type: integer
// format: int64
// - name: private
// in: query
// description: include private repositories this user has access to (defaults to true)
// type: boolean
// - name: page
// in: query
// description: page number of results to return (1-based)
Expand Down Expand Up @@ -96,6 +105,10 @@ func Search(ctx *context.APIContext) {
PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")),
TopicOnly: ctx.QueryBool("topic"),
Collaborate: util.OptionalBoolNone,
Private: ctx.IsSigned && (ctx.Query("private") == "" || ctx.QueryBool("private")),
UserIsAdmin: ctx.IsUserSiteAdmin(),
UserID: ctx.Data["SignedUserID"].(int64),
StarredByID: ctx.QueryInt64("starredBy"),
}

if ctx.QueryBool("exclusive") {
Expand Down Expand Up @@ -140,42 +153,6 @@ func Search(ctx *context.APIContext) {
}

var err error
if opts.OwnerID > 0 {
var repoOwner *models.User
if ctx.User != nil && ctx.User.ID == opts.OwnerID {
repoOwner = ctx.User
} else {
repoOwner, err = models.GetUserByID(opts.OwnerID)
if err != nil {
ctx.JSON(500, api.SearchError{
OK: false,
Error: err.Error(),
})
return
}
}

if repoOwner.IsOrganization() {
opts.Collaborate = util.OptionalBoolFalse
}

// Check visibility.
if ctx.IsSigned {
if ctx.User.ID == repoOwner.ID {
opts.Private = true
} else if repoOwner.IsOrganization() {
opts.Private, err = repoOwner.IsOwnedBy(ctx.User.ID)
if err != nil {
ctx.JSON(500, api.SearchError{
OK: false,
Error: err.Error(),
})
return
}
}
}
}

repos, count, err := models.SearchRepositoryByName(opts)
if err != nil {
ctx.JSON(500, api.SearchError{
Expand Down
Loading