From da813877fb8b6e2796d3305927ee4017351e998b Mon Sep 17 00:00:00 2001 From: Selene Date: Thu, 7 Oct 2021 16:06:16 +0200 Subject: [PATCH] Create search filters by interface (#39843) * Extract search users to a new service * Fix wire provider * Fix common_test and remove RouteRegister * Remove old endpoints * Fix test * Create search filters using interfaces * Move Enterprise filter, rename filter for filters and allow use filters with params * Each filter has unique key * Back activeLast30Days filter to OSS * Fix tests * Delete unusued param * Move filters to searchusers service and small refactor * Fix tests --- pkg/api/common_test.go | 4 +- pkg/api/org_users_test.go | 4 +- pkg/api/user_test.go | 10 +++-- pkg/models/search_user_filter.go | 30 +++++++++++++ pkg/models/user.go | 6 +-- pkg/server/wireexts_oss.go | 3 ++ pkg/services/searchusers/filters/filters.go | 37 ++++++++++++++++ .../filters/last_active_30_days_filter.go | 43 +++++++++++++++++++ pkg/services/searchusers/searchusers.go | 17 +++++--- pkg/services/sqlstore/user.go | 18 +++++--- 10 files changed, 150 insertions(+), 22 deletions(-) create mode 100644 pkg/models/search_user_filter.go create mode 100644 pkg/services/searchusers/filters/filters.go create mode 100644 pkg/services/searchusers/filters/last_active_30_days_filter.go diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 66aa758b9fc..33a96e5e4cb 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "testing" + "github.com/grafana/grafana/pkg/services/searchusers/filters" + "github.com/grafana/grafana/pkg/services/searchusers" "github.com/stretchr/testify/require" @@ -226,7 +228,7 @@ func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url strin QuotaService: "a.QuotaService{Cfg: cfg}, RouteRegister: routing.NewRouteRegister(), AccessControl: accesscontrolmock.New().WithPermissions(permissions), - searchUsersService: searchusers.ProvideUsersService(bus), + searchUsersService: searchusers.ProvideUsersService(bus, filters.ProvideOSSSearchUserFilter()), } sc := setupScenarioContext(t, url) diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 1228c614f29..4115e456e6c 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/services/searchusers/filters" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/services/searchusers" @@ -145,7 +147,7 @@ func setupOrgUsersAPIcontext(t *testing.T, role models.RoleType) (*scenarioConte RouteRegister: routing.NewRouteRegister(), AccessControl: accesscontrolmock.New().WithDisabled(), SQLStore: db, - searchUsersService: searchusers.ProvideUsersService(bus.New()), + searchUsersService: searchusers.ProvideUsersService(bus.New(), filters.ProvideOSSSearchUserFilter()), } sc := setupScenarioContext(t, "/api/org/users/lookup") diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 3589defdd98..b4abd3c40f5 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/services/searchusers/filters" + "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/api/dtos" @@ -136,7 +138,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + searchUsersService := searchusers.ProvideUsersService(bus.GetBus(), filters.ProvideOSSSearchUserFilter()) sc.handlerFunc = searchUsersService.SearchUsers sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -160,7 +162,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + searchUsersService := searchusers.ProvideUsersService(bus.GetBus(), filters.ProvideOSSSearchUserFilter()) sc.handlerFunc = searchUsersService.SearchUsers sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec() @@ -180,7 +182,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + searchUsersService := searchusers.ProvideUsersService(bus.GetBus(), filters.ProvideOSSSearchUserFilter()) sc.handlerFunc = searchUsersService.SearchUsersWithPaging sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -206,7 +208,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + searchUsersService := searchusers.ProvideUsersService(bus.GetBus(), filters.ProvideOSSSearchUserFilter()) sc.handlerFunc = searchUsersService.SearchUsersWithPaging sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec() diff --git a/pkg/models/search_user_filter.go b/pkg/models/search_user_filter.go new file mode 100644 index 00000000000..6811098ec17 --- /dev/null +++ b/pkg/models/search_user_filter.go @@ -0,0 +1,30 @@ +package models + +type SearchUserFilter interface { + GetFilter(filterName string, params []string) Filter + GetFilterList() map[string]FilterHandler +} + +type WhereCondition struct { + Condition string + Params interface{} +} + +type InCondition struct { + Condition string + Params interface{} +} + +type JoinCondition struct { + Operator string + Table string + Params string +} + +type FilterHandler func(params []string) (Filter, error) + +type Filter interface { + WhereCondition() *WhereCondition + InCondition() *InCondition + JoinCondition() *JoinCondition +} diff --git a/pkg/models/user.go b/pkg/models/user.go index dcde05933d6..f987a76f95b 100644 --- a/pkg/models/user.go +++ b/pkg/models/user.go @@ -138,17 +138,13 @@ type GetUserProfileQuery struct { Result UserProfileDTO } -type SearchUsersFilter string - -const ActiveLast30Days SearchUsersFilter = "activeLast30Days" - type SearchUsersQuery struct { OrgId int64 Query string Page int Limit int AuthModule string - Filter SearchUsersFilter + Filters []Filter IsDisabled *bool diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index 98a0ec229e6..c811418b73c 100644 --- a/pkg/server/wireexts_oss.go +++ b/pkg/server/wireexts_oss.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/services/login/authinfoservice" "github.com/grafana/grafana/pkg/services/provisioning" "github.com/grafana/grafana/pkg/services/searchusers" + "github.com/grafana/grafana/pkg/services/searchusers/filters" "github.com/grafana/grafana/pkg/services/sqlstore/migrations" "github.com/grafana/grafana/pkg/services/validations" "github.com/grafana/grafana/pkg/setting" @@ -49,6 +50,8 @@ var wireExtsBasicSet = wire.NewSet( wire.Bind(new(login.UserProtectionService), new(*authinfoservice.OSSUserProtectionImpl)), ossencryption.ProvideService, wire.Bind(new(encryption.Service), new(*ossencryption.Service)), + filters.ProvideOSSSearchUserFilter, + wire.Bind(new(models.SearchUserFilter), new(*filters.OSSSearchUserFilter)), searchusers.ProvideUsersService, wire.Bind(new(searchusers.Service), new(*searchusers.OSSService)), ) diff --git a/pkg/services/searchusers/filters/filters.go b/pkg/services/searchusers/filters/filters.go new file mode 100644 index 00000000000..9a573623521 --- /dev/null +++ b/pkg/services/searchusers/filters/filters.go @@ -0,0 +1,37 @@ +package filters + +import ( + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" +) + +const activeLast30Days = "activeLast30Days" + +type OSSSearchUserFilter struct { + filters map[string]models.FilterHandler +} + +func ProvideOSSSearchUserFilter() *OSSSearchUserFilter { + filters := make(map[string]models.FilterHandler) + filters[activeLast30Days] = NewActiveLast30DaysFilter + return &OSSSearchUserFilter{ + filters: filters, + } +} + +func (o *OSSSearchUserFilter) GetFilter(filterName string, params []string) models.Filter { + f, ok := o.filters[filterName] + if !ok || len(params) == 0 { + return nil + } + filter, err := f(params) + if err != nil { + log.Warnf("Cannot initialise the filter %s: %s", filterName, err) + return nil + } + return filter +} + +func (o *OSSSearchUserFilter) GetFilterList() map[string]models.FilterHandler { + return o.filters +} diff --git a/pkg/services/searchusers/filters/last_active_30_days_filter.go b/pkg/services/searchusers/filters/last_active_30_days_filter.go new file mode 100644 index 00000000000..d8cb3b7bc45 --- /dev/null +++ b/pkg/services/searchusers/filters/last_active_30_days_filter.go @@ -0,0 +1,43 @@ +package filters + +import ( + "strconv" + "time" + + "github.com/grafana/grafana/pkg/models" +) + +type ActiveLast30DaysFilter struct { + active bool +} + +func NewActiveLast30DaysFilter(params []string) (models.Filter, error) { + active, err := strconv.ParseBool(params[0]) + if err != nil { + return nil, err + } + return &ActiveLast30DaysFilter{active: active}, nil +} + +func (a *ActiveLast30DaysFilter) WhereCondition() *models.WhereCondition { + if !a.active { + return nil + } + return &models.WhereCondition{ + Condition: "last_seen_at > ?", + Params: a.whereParams(), + } +} + +func (a *ActiveLast30DaysFilter) JoinCondition() *models.JoinCondition { + return nil +} + +func (a *ActiveLast30DaysFilter) InCondition() *models.InCondition { + return nil +} + +func (a *ActiveLast30DaysFilter) whereParams() interface{} { + activeUserTimeLimit := time.Hour * 24 * 30 + return time.Now().Add(-activeUserTimeLimit) +} diff --git a/pkg/services/searchusers/searchusers.go b/pkg/services/searchusers/searchusers.go index 173dd96d189..1d72508e97b 100644 --- a/pkg/services/searchusers/searchusers.go +++ b/pkg/services/searchusers/searchusers.go @@ -13,11 +13,12 @@ type Service interface { } type OSSService struct { - bus bus.Bus + bus bus.Bus + searchUserFilter models.SearchUserFilter } -func ProvideUsersService(bus bus.Bus) *OSSService { - return &OSSService{bus: bus} +func ProvideUsersService(bus bus.Bus, searchUserFilter models.SearchUserFilter) *OSSService { + return &OSSService{bus: bus, searchUserFilter: searchUserFilter} } func (s *OSSService) SearchUsers(c *models.ReqContext) response.Response { @@ -50,9 +51,15 @@ func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery, } searchQuery := c.Query("query") - filter := c.Query("filter") + filters := make([]models.Filter, 0) + for filterName := range s.searchUserFilter.GetFilterList() { + filter := s.searchUserFilter.GetFilter(filterName, c.QueryStrings(filterName)) + if filter != nil { + filters = append(filters, filter) + } + } - query := &models.SearchUsersQuery{Query: searchQuery, Filter: models.SearchUsersFilter(filter), Page: page, Limit: perPage} + query := &models.SearchUsersQuery{Query: searchQuery, Filters: filters, Page: page, Limit: perPage} if err := s.bus.Dispatch(query); err != nil { return nil, err } diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 9000d7abb11..af0474867fc 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -614,16 +614,22 @@ func SearchUsers(ctx context.Context, query *models.SearchUsersQuery) error { whereParams = append(whereParams, query.AuthModule) } - if query.Filter == models.ActiveLast30Days { - activeUserDeadlineDate := time.Now().Add(-activeUserTimeLimit) - whereConditions = append(whereConditions, `last_seen_at > ?`) - whereParams = append(whereParams, activeUserDeadlineDate) - } - if len(whereConditions) > 0 { sess.Where(strings.Join(whereConditions, " AND "), whereParams...) } + for _, filter := range query.Filters { + if jc := filter.JoinCondition(); jc != nil { + sess.Join(jc.Operator, jc.Table, jc.Params) + } + if ic := filter.InCondition(); ic != nil { + sess.In(ic.Condition, ic.Params) + } + if wc := filter.WhereCondition(); wc != nil { + sess.Where(wc.Condition, wc.Params) + } + } + if query.Limit > 0 { offset := query.Limit * (query.Page - 1) sess.Limit(query.Limit, offset)