Skip to content

Commit 608f46e

Browse files
lafrikslafriks
authored
SearchRepositoryByName improvements and unification (#6897) (#7002)
* SearchRepositoryByName improvements and unification (#6897) * Update tests Co-Authored-By: zeripath <[email protected]> * Update tests Co-Authored-By: zeripath <[email protected]> * Update tests Co-Authored-By: zeripath <[email protected]> * Fix tests
1 parent 895764e commit 608f46e

File tree

7 files changed

+175
-237
lines changed

7 files changed

+175
-237
lines changed

‎integrations/api_repo_test.go‎

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -69,40 +69,41 @@ func TestAPISearchRepo(t *testing.T) {
6969
name, requestURL string
7070
expectedResults
7171
}{
72-
{name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50", expectedResults: expectedResults{
72+
{name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{
7373
nil: {count: 19},
7474
user: {count: 19},
7575
user2: {count: 19}},
7676
},
77-
{name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10", expectedResults: expectedResults{
77+
{name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10&private=false", expectedResults: expectedResults{
7878
nil: {count: 10},
7979
user: {count: 10},
8080
user2: {count: 10}},
8181
},
82-
{name: "RepositoriesDefaultMax10", requestURL: "/api/v1/repos/search?default", expectedResults: expectedResults{
82+
{name: "RepositoriesDefaultMax10", requestURL: "/api/v1/repos/search?default&private=false", expectedResults: expectedResults{
8383
nil: {count: 10},
8484
user: {count: 10},
8585
user2: {count: 10}},
8686
},
87-
{name: "RepositoriesByName", requestURL: fmt.Sprintf("/api/v1/repos/search?q=%s", "big_test_"), expectedResults: expectedResults{
87+
{name: "RepositoriesByName", requestURL: fmt.Sprintf("/api/v1/repos/search?q=%s&private=false", "big_test_"), expectedResults: expectedResults{
8888
nil: {count: 7, repoName: "big_test_"},
8989
user: {count: 7, repoName: "big_test_"},
9090
user2: {count: 7, repoName: "big_test_"}},
9191
},
9292
{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{
93-
nil: {count: 4},
94-
user: {count: 8, includesPrivate: true},
95-
user2: {count: 4}},
93+
nil: {count: 5},
94+
user: {count: 9, includesPrivate: true},
95+
user2: {count: 5, includesPrivate: true}},
9696
},
9797
{name: "RepositoriesAccessibleAndRelatedToUser2", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user2.ID), expectedResults: expectedResults{
9898
nil: {count: 1},
99-
user: {count: 1},
100-
user2: {count: 2, includesPrivate: true}},
99+
user: {count: 2, includesPrivate: true},
100+
user2: {count: 2, includesPrivate: true},
101+
user4: {count: 1}},
101102
},
102103
{name: "RepositoriesAccessibleAndRelatedToUser3", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user3.ID), expectedResults: expectedResults{
103104
nil: {count: 1},
104-
user: {count: 1},
105-
user2: {count: 1},
105+
user: {count: 4, includesPrivate: true},
106+
user2: {count: 2, includesPrivate: true},
106107
user3: {count: 4, includesPrivate: true}},
107108
},
108109
{name: "RepositoriesOwnedByOrganization", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", orgUser.ID), expectedResults: expectedResults{
@@ -112,12 +113,12 @@ func TestAPISearchRepo(t *testing.T) {
112113
},
113114
{name: "RepositoriesAccessibleAndRelatedToUser4", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user4.ID), expectedResults: expectedResults{
114115
nil: {count: 3},
115-
user: {count: 3},
116-
user4: {count: 6, includesPrivate: true}}},
116+
user: {count: 4, includesPrivate: true},
117+
user4: {count: 7, includesPrivate: true}}},
117118
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeSource", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "source"), expectedResults: expectedResults{
118119
nil: {count: 0},
119-
user: {count: 0},
120-
user4: {count: 0, includesPrivate: true}}},
120+
user: {count: 1, includesPrivate: true},
121+
user4: {count: 1, includesPrivate: true}}},
121122
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeFork", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "fork"), expectedResults: expectedResults{
122123
nil: {count: 1},
123124
user: {count: 1},
@@ -136,8 +137,8 @@ func TestAPISearchRepo(t *testing.T) {
136137
user4: {count: 2, includesPrivate: true}}},
137138
{name: "RepositoriesAccessibleAndRelatedToUser4/SearchModeCollaborative", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d&mode=%s", user4.ID, "collaborative"), expectedResults: expectedResults{
138139
nil: {count: 0},
139-
user: {count: 0},
140-
user4: {count: 0, includesPrivate: true}}},
140+
user: {count: 1, includesPrivate: true},
141+
user4: {count: 1, includesPrivate: true}}},
141142
}
142143

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

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

174179
assert.NotEmpty(t, repo.Name)
180+
assert.Equal(t, repo.Name, r.Name)
175181

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

184190
if !expected.includesPrivate {
185-
assert.False(t, repo.Private)
191+
assert.False(t, repo.Private, "User: %d not expecting private repository: %s", userID, repo.FullName)
186192
}
187193
}
188194
})

‎models/repo_list.go‎

Lines changed: 70 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"code.gitea.io/gitea/modules/util"
1313

1414
"github.com/go-xorm/builder"
15-
"github.com/go-xorm/core"
1615
)
1716

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

113112
// SearchRepoOptions holds the search options
114113
type SearchRepoOptions struct {
115-
Keyword string
116-
OwnerID int64
117-
OrderBy SearchOrderBy
118-
Private bool // Include private repositories in results
119-
Starred bool
120-
Page int
121-
IsProfile bool
122-
AllPublic bool // Include also all public repositories
123-
PageSize int // Can be smaller than or equal to setting.ExplorePagingNum
114+
UserID int64
115+
UserIsAdmin bool
116+
Keyword string
117+
OwnerID int64
118+
OrderBy SearchOrderBy
119+
Private bool // Include private repositories in results
120+
StarredByID int64
121+
Page int
122+
IsProfile bool
123+
AllPublic bool // Include also all public repositories
124+
PageSize int // Can be smaller than or equal to setting.ExplorePagingNum
124125
// None -> include collaborative AND non-collaborative
125126
// True -> include just collaborative
126127
// False -> incude just non-collaborative
@@ -168,72 +169,79 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (RepositoryList, int64, err
168169
if opts.Page <= 0 {
169170
opts.Page = 1
170171
}
171-
172172
var cond = builder.NewCond()
173173

174-
if !opts.Private {
174+
if opts.Private {
175+
if !opts.UserIsAdmin && opts.UserID != 0 && opts.UserID != opts.OwnerID {
176+
// OK we're in the context of a User
177+
// We should be Either
178+
cond = cond.And(builder.Or(
179+
// 1. Be able to see all non-private repositories that either:
180+
cond.And(
181+
builder.Eq{"is_private": false},
182+
builder.Or(
183+
// A. Aren't in organisations __OR__
184+
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
185+
// B. Isn't a private organisation. (Limited is OK because we're logged in)
186+
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"visibility": structs.VisibleTypePrivate}))),
187+
),
188+
// 2. Be able to see all repositories that we have access to
189+
builder.In("id", builder.Select("repo_id").
190+
From("`access`").
191+
Where(builder.And(
192+
builder.Eq{"user_id": opts.UserID},
193+
builder.Gt{"mode": int(AccessModeNone)}))),
194+
// 3. Be able to see all repositories that we are in a team
195+
builder.In("id", builder.Select("`team_repo`.repo_id").
196+
From("team_repo").
197+
Where(builder.Eq{"`team_user`.uid": opts.UserID}).
198+
Join("INNER", "team_user", "`team_user`.team_id = `team_repo`.team_id"))))
199+
}
200+
} else {
201+
// Not looking at private organisations
202+
// We should be able to see all non-private repositories that either:
175203
cond = cond.And(builder.Eq{"is_private": false})
176204
accessCond := builder.Or(
177-
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Or(builder.Eq{"visibility": structs.VisibleTypeLimited}, builder.Eq{"visibility": structs.VisibleTypePrivate}))),
178-
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})))
205+
// A. Aren't in organisations __OR__
206+
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
207+
// B. Isn't a private or limited organisation.
208+
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Or(builder.Eq{"visibility": structs.VisibleTypeLimited}, builder.Eq{"visibility": structs.VisibleTypePrivate}))))
179209
cond = cond.And(accessCond)
180210
}
181211

212+
// Restrict to starred repositories
213+
if opts.StarredByID > 0 {
214+
cond = cond.And(builder.In("id", builder.Select("repo_id").From("star").Where(builder.Eq{"uid": opts.StarredByID})))
215+
}
216+
217+
// Restrict repositories to those the OwnerID owns or contributes to as per opts.Collaborate
182218
if opts.OwnerID > 0 {
183-
if opts.Starred {
184-
cond = cond.And(builder.In("id", builder.Select("repo_id").From("star").Where(builder.Eq{"uid": opts.OwnerID})))
185-
} else {
186-
var accessCond = builder.NewCond()
187-
if opts.Collaborate != util.OptionalBoolTrue {
188-
accessCond = builder.Eq{"owner_id": opts.OwnerID}
189-
}
219+
var accessCond = builder.NewCond()
220+
if opts.Collaborate != util.OptionalBoolTrue {
221+
accessCond = builder.Eq{"owner_id": opts.OwnerID}
222+
}
190223

191-
if opts.Collaborate != util.OptionalBoolFalse {
192-
collaborateCond := builder.And(
224+
if opts.Collaborate != util.OptionalBoolFalse {
225+
collaborateCond := builder.And(
226+
builder.Or(
193227
builder.Expr("repository.id IN (SELECT repo_id FROM `access` WHERE access.user_id = ?)", opts.OwnerID),
194-
builder.Neq{"owner_id": opts.OwnerID})
195-
if !opts.Private {
196-
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))
197-
}
198-
199-
accessCond = accessCond.Or(collaborateCond)
228+
builder.In("id", builder.Select("`team_repo`.repo_id").
229+
From("team_repo").
230+
Where(builder.Eq{"`team_user`.uid": opts.OwnerID}).
231+
Join("INNER", "team_user", "`team_user`.team_id = `team_repo`.team_id"))),
232+
builder.Neq{"owner_id": opts.OwnerID})
233+
if !opts.Private {
234+
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))
200235
}
201236

202-
var exprCond builder.Cond
203-
if DbCfg.Type == core.POSTGRES {
204-
exprCond = builder.Expr("org_user.org_id = \"user\".id")
205-
} else if DbCfg.Type == core.MSSQL {
206-
exprCond = builder.Expr("org_user.org_id = [user].id")
207-
} else {
208-
exprCond = builder.Eq{"org_user.org_id": "user.id"}
209-
}
210-
211-
visibilityCond := builder.Or(
212-
builder.In("owner_id",
213-
builder.Select("org_id").From("org_user").
214-
LeftJoin("`user`", exprCond).
215-
Where(
216-
builder.And(
217-
builder.Eq{"uid": opts.OwnerID},
218-
builder.Eq{"visibility": structs.VisibleTypePrivate})),
219-
),
220-
builder.In("owner_id",
221-
builder.Select("id").From("`user`").
222-
Where(
223-
builder.Or(
224-
builder.Eq{"visibility": structs.VisibleTypePublic},
225-
builder.Eq{"visibility": structs.VisibleTypeLimited})),
226-
),
227-
builder.NotIn("owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
228-
)
229-
cond = cond.And(visibilityCond)
230-
231-
if opts.AllPublic {
232-
accessCond = accessCond.Or(builder.Eq{"is_private": false})
233-
}
237+
accessCond = accessCond.Or(collaborateCond)
238+
}
234239

235-
cond = cond.And(accessCond)
240+
if opts.AllPublic {
241+
accessCond = accessCond.Or(builder.Eq{"is_private": false})
236242
}
243+
244+
cond = cond.And(accessCond)
237245
}
238246

239247
if opts.Keyword != "" {

‎models/repo_list_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func TestSearchRepositoryByName(t *testing.T) {
117117
count: 4},
118118
{name: "PublicRepositoriesOfUserIncludingCollaborative",
119119
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15},
120-
count: 4},
120+
count: 5},
121121
{name: "PublicRepositoriesOfUser2IncludingCollaborative",
122122
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18},
123123
count: 1},
@@ -126,13 +126,13 @@ func TestSearchRepositoryByName(t *testing.T) {
126126
count: 3},
127127
{name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
128128
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true},
129-
count: 8},
129+
count: 9},
130130
{name: "PublicAndPrivateRepositoriesOfUser2IncludingCollaborative",
131131
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true},
132132
count: 4},
133133
{name: "PublicAndPrivateRepositoriesOfUser3IncludingCollaborative",
134134
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 20, Private: true},
135-
count: 6},
135+
count: 7},
136136
{name: "PublicRepositoriesOfOrganization",
137137
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, Collaborate: util.OptionalBoolFalse},
138138
count: 1},
@@ -150,7 +150,7 @@ func TestSearchRepositoryByName(t *testing.T) {
150150
count: 19},
151151
{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
152152
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, AllPublic: true},
153-
count: 24},
153+
count: 25},
154154
{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
155155
opts: &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, AllPublic: true},
156156
count: 13},

‎routers/api/v1/repo/repo.go‎

Lines changed: 13 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,15 @@ func Search(ctx *context.APIContext) {
5555
// description: search only for repos that the user with the given id owns or contributes to
5656
// type: integer
5757
// format: int64
58+
// - name: starredBy
59+
// in: query
60+
// description: search only for repos that the user with the given id has starred
61+
// type: integer
62+
// format: int64
63+
// - name: private
64+
// in: query
65+
// description: include private repositories this user has access to (defaults to true)
66+
// type: boolean
5867
// - name: page
5968
// in: query
6069
// description: page number of results to return (1-based)
@@ -95,6 +104,10 @@ func Search(ctx *context.APIContext) {
95104
PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")),
96105
TopicOnly: ctx.QueryBool("topic"),
97106
Collaborate: util.OptionalBoolNone,
107+
Private: ctx.IsSigned && (ctx.Query("private") == "" || ctx.QueryBool("private")),
108+
UserIsAdmin: ctx.IsSigned && ctx.User.IsAdmin,
109+
UserID: ctx.Data["SignedUserID"].(int64),
110+
StarredByID: ctx.QueryInt64("starredBy"),
98111
}
99112

100113
if ctx.QueryBool("exclusive") {
@@ -139,42 +152,6 @@ func Search(ctx *context.APIContext) {
139152
}
140153

141154
var err error
142-
if opts.OwnerID > 0 {
143-
var repoOwner *models.User
144-
if ctx.User != nil && ctx.User.ID == opts.OwnerID {
145-
repoOwner = ctx.User
146-
} else {
147-
repoOwner, err = models.GetUserByID(opts.OwnerID)
148-
if err != nil {
149-
ctx.JSON(500, api.SearchError{
150-
OK: false,
151-
Error: err.Error(),
152-
})
153-
return
154-
}
155-
}
156-
157-
if repoOwner.IsOrganization() {
158-
opts.Collaborate = util.OptionalBoolFalse
159-
}
160-
161-
// Check visibility.
162-
if ctx.IsSigned {
163-
if ctx.User.ID == repoOwner.ID {
164-
opts.Private = true
165-
} else if repoOwner.IsOrganization() {
166-
opts.Private, err = repoOwner.IsOwnedBy(ctx.User.ID)
167-
if err != nil {
168-
ctx.JSON(500, api.SearchError{
169-
OK: false,
170-
Error: err.Error(),
171-
})
172-
return
173-
}
174-
}
175-
}
176-
}
177-
178155
repos, count, err := models.SearchRepositoryByName(opts)
179156
if err != nil {
180157
ctx.JSON(500, api.SearchError{

0 commit comments

Comments
 (0)