Skip to content

Commit b0936f4

Browse files
philip-petersonphilip-peterson
authored
Do not mutate incoming options to RenderUserSearch and SearchUsers (#34544)
This PR changes the `opts` argument in `SearchUsers()` to be passed by value instead of by pointer, as its mutations do not escape the function scope and are not used elsewhere. This simplifies reasoning about the function and avoids unnecessary pointer usage. This insight emerged during an initial attempt to refactor `RenderUserSearch()`, which currently intermixes multiple concerns. Co-authored-by: Philip Peterson <[email protected]>
1 parent 498088c commit b0936f4

File tree

12 files changed

+31
-31
lines changed

12 files changed

+31
-31
lines changed

‎models/user/search.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ func (opts *SearchUserOptions) toSearchQueryBase(ctx context.Context) *xorm.Sess
137137

138138
// SearchUsers takes options i.e. keyword and part of user name to search,
139139
// it returns results in given range and number of total results.
140-
func SearchUsers(ctx context.Context, opts *SearchUserOptions) (users []*User, _ int64, _ error) {
140+
func SearchUsers(ctx context.Context, opts SearchUserOptions) (users []*User, _ int64, _ error) {
141141
sessCount := opts.toSearchQueryBase(ctx)
142142
defer sessCount.Close()
143143
count, err := sessCount.Count(new(User))
@@ -152,7 +152,7 @@ func SearchUsers(ctx context.Context, opts *SearchUserOptions) (users []*User, _
152152
sessQuery := opts.toSearchQueryBase(ctx).OrderBy(opts.OrderBy.String())
153153
defer sessQuery.Close()
154154
if opts.Page > 0 {
155-
sessQuery = db.SetSessionPagination(sessQuery, opts)
155+
sessQuery = db.SetSessionPagination(sessQuery, &opts)
156156
}
157157

158158
// the sql may contain JOIN, so we must only select User related columns

‎models/user/user_test.go

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func TestCanCreateOrganization(t *testing.T) {
8888

8989
func TestSearchUsers(t *testing.T) {
9090
assert.NoError(t, unittest.PrepareTestDatabase())
91-
testSuccess := func(opts *user_model.SearchUserOptions, expectedUserOrOrgIDs []int64) {
91+
testSuccess := func(opts user_model.SearchUserOptions, expectedUserOrOrgIDs []int64) {
9292
users, _, err := user_model.SearchUsers(db.DefaultContext, opts)
9393
assert.NoError(t, err)
9494
cassText := fmt.Sprintf("ids: %v, opts: %v", expectedUserOrOrgIDs, opts)
@@ -100,61 +100,61 @@ func TestSearchUsers(t *testing.T) {
100100
}
101101

102102
// test orgs
103-
testOrgSuccess := func(opts *user_model.SearchUserOptions, expectedOrgIDs []int64) {
103+
testOrgSuccess := func(opts user_model.SearchUserOptions, expectedOrgIDs []int64) {
104104
opts.Type = user_model.UserTypeOrganization
105105
testSuccess(opts, expectedOrgIDs)
106106
}
107107

108-
testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1, PageSize: 2}},
108+
testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1, PageSize: 2}},
109109
[]int64{3, 6})
110110

111-
testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 2, PageSize: 2}},
111+
testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 2, PageSize: 2}},
112112
[]int64{7, 17})
113113

114-
testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 3, PageSize: 2}},
114+
testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 3, PageSize: 2}},
115115
[]int64{19, 25})
116116

117-
testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 4, PageSize: 2}},
117+
testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 4, PageSize: 2}},
118118
[]int64{26, 41})
119119

120-
testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 5, PageSize: 2}},
120+
testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 5, PageSize: 2}},
121121
[]int64{42})
122122

123-
testOrgSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 6, PageSize: 2}},
123+
testOrgSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 6, PageSize: 2}},
124124
[]int64{})
125125

126126
// test users
127-
testUserSuccess := func(opts *user_model.SearchUserOptions, expectedUserIDs []int64) {
127+
testUserSuccess := func(opts user_model.SearchUserOptions, expectedUserIDs []int64) {
128128
opts.Type = user_model.UserTypeIndividual
129129
testSuccess(opts, expectedUserIDs)
130130
}
131131

132-
testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}},
132+
testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}},
133133
[]int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40})
134134

135-
testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(false)},
135+
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(false)},
136136
[]int64{9})
137137

138-
testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
138+
testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
139139
[]int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40})
140140

141-
testUserSuccess(&user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
141+
testUserSuccess(user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
142142
[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
143143

144144
// order by name asc default
145-
testUserSuccess(&user_model.SearchUserOptions{Keyword: "user1", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
145+
testUserSuccess(user_model.SearchUserOptions{Keyword: "user1", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)},
146146
[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
147147

148-
testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: optional.Some(true)},
148+
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: optional.Some(true)},
149149
[]int64{1})
150150

151-
testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: optional.Some(true)},
151+
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: optional.Some(true)},
152152
[]int64{29})
153153

154-
testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsProhibitLogin: optional.Some(true)},
154+
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsProhibitLogin: optional.Some(true)},
155155
[]int64{37})
156156

157-
testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsTwoFactorEnabled: optional.Some(true)},
157+
testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsTwoFactorEnabled: optional.Some(true)},
158158
[]int64{24})
159159
}
160160

‎routers/api/v1/admin/org.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func GetAllOrgs(ctx *context.APIContext) {
101101

102102
listOptions := utils.GetListOptions(ctx)
103103

104-
users, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
104+
users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
105105
Actor: ctx.Doer,
106106
Type: user_model.UserTypeOrganization,
107107
OrderBy: db.SearchOrderByAlphabetically,

‎routers/api/v1/admin/user.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ func SearchUsers(ctx *context.APIContext) {
423423

424424
listOptions := utils.GetListOptions(ctx)
425425

426-
users, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
426+
users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
427427
Actor: ctx.Doer,
428428
Type: user_model.UserTypeIndividual,
429429
LoginName: ctx.FormTrim("login_name"),

‎routers/api/v1/org/org.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ func GetAll(ctx *context.APIContext) {
201201

202202
listOptions := utils.GetListOptions(ctx)
203203

204-
publicOrgs, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
204+
publicOrgs, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
205205
Actor: ctx.Doer,
206206
ListOptions: listOptions,
207207
Type: user_model.UserTypeOrganization,

‎routers/api/v1/user/user.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func Search(ctx *context.APIContext) {
7373
if ctx.PublicOnly {
7474
visible = []structs.VisibleType{structs.VisibleTypePublic}
7575
}
76-
users, maxResults, err = user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
76+
users, maxResults, err = user_model.SearchUsers(ctx, user_model.SearchUserOptions{
7777
Actor: ctx.Doer,
7878
Keyword: ctx.FormTrim("q"),
7979
UID: uid,

‎routers/web/admin/orgs.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func Organizations(ctx *context.Context) {
2727
ctx.SetFormString("sort", UserSearchDefaultAdminSort)
2828
}
2929

30-
explore.RenderUserSearch(ctx, &user_model.SearchUserOptions{
30+
explore.RenderUserSearch(ctx, user_model.SearchUserOptions{
3131
Actor: ctx.Doer,
3232
Type: user_model.UserTypeOrganization,
3333
IncludeReserved: true, // administrator needs to list all accounts include reserved

‎routers/web/admin/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func Users(ctx *context.Context) {
6464
"SortType": sortType,
6565
}
6666

67-
explore.RenderUserSearch(ctx, &user_model.SearchUserOptions{
67+
explore.RenderUserSearch(ctx, user_model.SearchUserOptions{
6868
Actor: ctx.Doer,
6969
Type: user_model.UserTypeIndividual,
7070
ListOptions: db.ListOptions{

‎routers/web/explore/org.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func Organizations(ctx *context.Context) {
4444
ctx.SetFormString("sort", sortOrder)
4545
}
4646

47-
RenderUserSearch(ctx, &user_model.SearchUserOptions{
47+
RenderUserSearch(ctx, user_model.SearchUserOptions{
4848
Actor: ctx.Doer,
4949
Type: user_model.UserTypeOrganization,
5050
ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum},

‎routers/web/explore/user.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func isKeywordValid(keyword string) bool {
3232
}
3333

3434
// RenderUserSearch render user search page
35-
func RenderUserSearch(ctx *context.Context, opts *user_model.SearchUserOptions, tplName templates.TplName) {
35+
func RenderUserSearch(ctx *context.Context, opts user_model.SearchUserOptions, tplName templates.TplName) {
3636
// Sitemap index for sitemap paths
3737
opts.Page = int(ctx.PathParamInt64("idx"))
3838
isSitemap := ctx.PathParam("idx") != ""
@@ -151,7 +151,7 @@ func Users(ctx *context.Context) {
151151
ctx.SetFormString("sort", sortOrder)
152152
}
153153

154-
RenderUserSearch(ctx, &user_model.SearchUserOptions{
154+
RenderUserSearch(ctx, user_model.SearchUserOptions{
155155
Actor: ctx.Doer,
156156
Type: user_model.UserTypeIndividual,
157157
ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum},

‎routers/web/home.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func Home(ctx *context.Context) {
6868
func HomeSitemap(ctx *context.Context) {
6969
m := sitemap.NewSitemapIndex()
7070
if !setting.Service.Explore.DisableUsersPage {
71-
_, cnt, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
71+
_, cnt, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
7272
Type: user_model.UserTypeIndividual,
7373
ListOptions: db.ListOptions{PageSize: 1},
7474
IsActive: optional.Some(true),

‎routers/web/user/search.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616

1717
// SearchCandidates searches candidate users for dropdown list
1818
func SearchCandidates(ctx *context.Context) {
19-
users, _, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{
19+
users, _, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
2020
Actor: ctx.Doer,
2121
Keyword: ctx.FormTrim("q"),
2222
Type: user_model.UserTypeIndividual,

0 commit comments

Comments
 (0)