From b0936f4f4120169fab31f21610e9fcd915e36fb4 Mon Sep 17 00:00:00 2001 From: Philip Peterson <1326208+philip-peterson@users.noreply.github.com> Date: Tue, 27 May 2025 12:36:02 -0700 Subject: [PATCH] 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 --- models/user/search.go | 4 ++-- models/user/user_test.go | 36 ++++++++++++++++++------------------ routers/api/v1/admin/org.go | 2 +- routers/api/v1/admin/user.go | 2 +- routers/api/v1/org/org.go | 2 +- routers/api/v1/user/user.go | 2 +- routers/web/admin/orgs.go | 2 +- routers/web/admin/users.go | 2 +- routers/web/explore/org.go | 2 +- routers/web/explore/user.go | 4 ++-- routers/web/home.go | 2 +- routers/web/user/search.go | 2 +- 12 files changed, 31 insertions(+), 31 deletions(-) diff --git a/models/user/search.go b/models/user/search.go index f4436be09a..cfd0d011bc 100644 --- a/models/user/search.go +++ b/models/user/search.go @@ -137,7 +137,7 @@ func (opts *SearchUserOptions) toSearchQueryBase(ctx context.Context) *xorm.Sess // SearchUsers takes options i.e. keyword and part of user name to search, // it returns results in given range and number of total results. -func SearchUsers(ctx context.Context, opts *SearchUserOptions) (users []*User, _ int64, _ error) { +func SearchUsers(ctx context.Context, opts SearchUserOptions) (users []*User, _ int64, _ error) { sessCount := opts.toSearchQueryBase(ctx) defer sessCount.Close() count, err := sessCount.Count(new(User)) @@ -152,7 +152,7 @@ func SearchUsers(ctx context.Context, opts *SearchUserOptions) (users []*User, _ sessQuery := opts.toSearchQueryBase(ctx).OrderBy(opts.OrderBy.String()) defer sessQuery.Close() if opts.Page > 0 { - sessQuery = db.SetSessionPagination(sessQuery, opts) + sessQuery = db.SetSessionPagination(sessQuery, &opts) } // the sql may contain JOIN, so we must only select User related columns diff --git a/models/user/user_test.go b/models/user/user_test.go index dd232abe2e..26192f8d55 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -88,7 +88,7 @@ func TestCanCreateOrganization(t *testing.T) { func TestSearchUsers(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - testSuccess := func(opts *user_model.SearchUserOptions, expectedUserOrOrgIDs []int64) { + testSuccess := func(opts user_model.SearchUserOptions, expectedUserOrOrgIDs []int64) { users, _, err := user_model.SearchUsers(db.DefaultContext, opts) assert.NoError(t, err) cassText := fmt.Sprintf("ids: %v, opts: %v", expectedUserOrOrgIDs, opts) @@ -100,61 +100,61 @@ func TestSearchUsers(t *testing.T) { } // test orgs - testOrgSuccess := func(opts *user_model.SearchUserOptions, expectedOrgIDs []int64) { + testOrgSuccess := func(opts user_model.SearchUserOptions, expectedOrgIDs []int64) { opts.Type = user_model.UserTypeOrganization testSuccess(opts, expectedOrgIDs) } - testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1, PageSize: 2}}, []int64{3, 6}) - testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 2, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 2, PageSize: 2}}, []int64{7, 17}) - testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 3, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 3, PageSize: 2}}, []int64{19, 25}) - testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 4, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 4, PageSize: 2}}, []int64{26, 41}) - testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 5, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 5, PageSize: 2}}, []int64{42}) - testOrgSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 6, PageSize: 2}}, + testOrgSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 6, PageSize: 2}}, []int64{}) // test users - testUserSuccess := func(opts *user_model.SearchUserOptions, expectedUserIDs []int64) { + testUserSuccess := func(opts user_model.SearchUserOptions, expectedUserIDs []int64) { opts.Type = user_model.UserTypeIndividual testSuccess(opts, expectedUserIDs) } - testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}}, + testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}}, []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}) - testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(false)}, + testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(false)}, []int64{9}) - testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, []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}) - testUserSuccess(&user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) // order by name asc default - testUserSuccess(&user_model.SearchUserOptions{Keyword: "user1", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{Keyword: "user1", ListOptions: db.ListOptions{Page: 1}, IsActive: optional.Some(true)}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) - testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: optional.Some(true)}, []int64{1}) - testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: optional.Some(true)}, []int64{29}) - testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsProhibitLogin: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsProhibitLogin: optional.Some(true)}, []int64{37}) - testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsTwoFactorEnabled: optional.Some(true)}, + testUserSuccess(user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsTwoFactorEnabled: optional.Some(true)}, []int64{24}) } diff --git a/routers/api/v1/admin/org.go b/routers/api/v1/admin/org.go index 8808a1587d..c7a4ae8419 100644 --- a/routers/api/v1/admin/org.go +++ b/routers/api/v1/admin/org.go @@ -101,7 +101,7 @@ func GetAllOrgs(ctx *context.APIContext) { listOptions := utils.GetListOptions(ctx) - users, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeOrganization, OrderBy: db.SearchOrderByAlphabetically, diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 3ba77604ec..85dd547ac1 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -423,7 +423,7 @@ func SearchUsers(ctx *context.APIContext) { listOptions := utils.GetListOptions(ctx) - users, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeIndividual, LoginName: ctx.FormTrim("login_name"), diff --git a/routers/api/v1/org/org.go b/routers/api/v1/org/org.go index c9208f4757..27c646896a 100644 --- a/routers/api/v1/org/org.go +++ b/routers/api/v1/org/org.go @@ -201,7 +201,7 @@ func GetAll(ctx *context.APIContext) { listOptions := utils.GetListOptions(ctx) - publicOrgs, maxResults, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + publicOrgs, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, ListOptions: listOptions, Type: user_model.UserTypeOrganization, diff --git a/routers/api/v1/user/user.go b/routers/api/v1/user/user.go index 757a548518..2b98fb5ac7 100644 --- a/routers/api/v1/user/user.go +++ b/routers/api/v1/user/user.go @@ -73,7 +73,7 @@ func Search(ctx *context.APIContext) { if ctx.PublicOnly { visible = []structs.VisibleType{structs.VisibleTypePublic} } - users, maxResults, err = user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + users, maxResults, err = user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Keyword: ctx.FormTrim("q"), UID: uid, diff --git a/routers/web/admin/orgs.go b/routers/web/admin/orgs.go index 35e61efa17..e34f203aaf 100644 --- a/routers/web/admin/orgs.go +++ b/routers/web/admin/orgs.go @@ -27,7 +27,7 @@ func Organizations(ctx *context.Context) { ctx.SetFormString("sort", UserSearchDefaultAdminSort) } - explore.RenderUserSearch(ctx, &user_model.SearchUserOptions{ + explore.RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeOrganization, IncludeReserved: true, // administrator needs to list all accounts include reserved diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index e42cbb316c..c6e8f4eab2 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -64,7 +64,7 @@ func Users(ctx *context.Context) { "SortType": sortType, } - explore.RenderUserSearch(ctx, &user_model.SearchUserOptions{ + explore.RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeIndividual, ListOptions: db.ListOptions{ diff --git a/routers/web/explore/org.go b/routers/web/explore/org.go index 7bb71acfd7..f8f7f5c18c 100644 --- a/routers/web/explore/org.go +++ b/routers/web/explore/org.go @@ -44,7 +44,7 @@ func Organizations(ctx *context.Context) { ctx.SetFormString("sort", sortOrder) } - RenderUserSearch(ctx, &user_model.SearchUserOptions{ + RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeOrganization, ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum}, diff --git a/routers/web/explore/user.go b/routers/web/explore/user.go index e1e1ec1cfd..af48e6fb79 100644 --- a/routers/web/explore/user.go +++ b/routers/web/explore/user.go @@ -32,7 +32,7 @@ func isKeywordValid(keyword string) bool { } // RenderUserSearch render user search page -func RenderUserSearch(ctx *context.Context, opts *user_model.SearchUserOptions, tplName templates.TplName) { +func RenderUserSearch(ctx *context.Context, opts user_model.SearchUserOptions, tplName templates.TplName) { // Sitemap index for sitemap paths opts.Page = int(ctx.PathParamInt64("idx")) isSitemap := ctx.PathParam("idx") != "" @@ -151,7 +151,7 @@ func Users(ctx *context.Context) { ctx.SetFormString("sort", sortOrder) } - RenderUserSearch(ctx, &user_model.SearchUserOptions{ + RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Type: user_model.UserTypeIndividual, ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum}, diff --git a/routers/web/home.go b/routers/web/home.go index 208cc36dfb..c2d3184d39 100644 --- a/routers/web/home.go +++ b/routers/web/home.go @@ -68,7 +68,7 @@ func Home(ctx *context.Context) { func HomeSitemap(ctx *context.Context) { m := sitemap.NewSitemapIndex() if !setting.Service.Explore.DisableUsersPage { - _, cnt, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + _, cnt, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Type: user_model.UserTypeIndividual, ListOptions: db.ListOptions{PageSize: 1}, IsActive: optional.Some(true), diff --git a/routers/web/user/search.go b/routers/web/user/search.go index be5eee90a9..9acb9694d7 100644 --- a/routers/web/user/search.go +++ b/routers/web/user/search.go @@ -16,7 +16,7 @@ import ( // SearchCandidates searches candidate users for dropdown list func SearchCandidates(ctx *context.Context) { - users, _, err := user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ + users, _, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Keyword: ctx.FormTrim("q"), Type: user_model.UserTypeIndividual,