From 02128453858afbb521fdbc917b67f1e6f4d7b608 Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Tue, 19 Nov 2019 11:38:49 -0500 Subject: [PATCH] MM-18356: Adds ability to paginate channel search. (#12830) * MM-18356: Adds ability to paginate channel search. * MM-18356: Minor refactor. * MM-18356: Adds doc. * MM-18356: Fixes doc. * MM-18356: Some commentary, adds the total count to non-paginated responses, and removes a stray fmt. * MM-18356: Fixes shadowed variable. * MM-18356: Removes paginate field and API parameter. * MM-18356: Adds method to check if channel search is a paginated request. * MM-18356: Vet fix. --- api4/channel.go | 15 ++++++- api4/channel_test.go | 18 ++++++++ app/channel.go | 5 ++- model/channel.go | 5 +++ model/channel_search.go | 2 + model/client4.go | 10 +++++ store/sqlstore/channel_store.go | 64 ++++++++++++++++++++++----- store/store.go | 11 ++++- store/storetest/channel_store.go | 36 ++++++++------- store/storetest/mocks/ChannelStore.go | 4 +- store/timer_layer.go | 8 ++-- 11 files changed, 141 insertions(+), 37 deletions(-) diff --git a/api4/channel.go b/api4/channel.go index 99c7d65134..e953d1b518 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -908,9 +908,11 @@ func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { NotAssociatedToGroup: props.NotAssociatedToGroup, ExcludeDefaultChannels: props.ExcludeDefaultChannels, IncludeDeleted: r.URL.Query().Get("include_deleted") == "true", + Page: props.Page, + PerPage: props.PerPage, } - channels, err := c.App.SearchAllChannels(props.Term, opts) + channels, totalCount, err := c.App.SearchAllChannels(props.Term, opts) if err != nil { c.Err = err return @@ -918,7 +920,16 @@ func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { // Don't fill in channels props, since unused by client and potentially expensive. - w.Write([]byte(channels.ToJson())) + var payload []byte + + if props.Page != nil && props.PerPage != nil { + data := model.ChannelsWithCount{Channels: channels, TotalCount: totalCount} + payload = data.ToJson() + } else { + payload = []byte(channels.ToJson()) + } + + w.Write(payload) } func deleteChannel(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/channel_test.go b/api4/channel_test.go index b7af775751..66c6cba07f 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -1075,6 +1075,24 @@ func TestSearchAllChannels(t *testing.T) { CheckForbiddenStatus(t, resp) } +func TestSearchAllChannelsPaged(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + Client := th.Client + + search := &model.ChannelSearch{Term: th.BasicChannel.Name} + search.Term = "" + search.Page = model.NewInt(0) + search.PerPage = model.NewInt(2) + channelsWithCount, resp := th.SystemAdminClient.SearchAllChannelsPaged(search) + CheckNoError(t, resp) + require.Len(t, *channelsWithCount.Channels, 2) + + search.Term = th.BasicChannel.Name + _, resp = Client.SearchAllChannels(search) + CheckForbiddenStatus(t, resp) +} + func TestSearchGroupChannels(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() diff --git a/app/channel.go b/app/channel.go index e678955a90..5bfee5ca45 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1870,7 +1870,8 @@ func (a *App) AutocompleteChannelsForSearch(teamId string, userId string, term s return a.Srv.Store.Channel().AutocompleteInTeamForSearch(teamId, userId, term, includeDeleted) } -func (a *App) SearchAllChannels(term string, opts model.ChannelSearchOpts) (*model.ChannelListWithTeamData, *model.AppError) { +// SearchAllChannels returns a list of channels, the total count of the results of the search (if the paginate search option is true), and an error. +func (a *App) SearchAllChannels(term string, opts model.ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) { opts.IncludeDeleted = *a.Config().TeamSettings.ExperimentalViewArchivedChannels && opts.IncludeDeleted if opts.ExcludeDefaultChannels { opts.ExcludeChannelNames = a.DefaultChannelNames() @@ -1879,6 +1880,8 @@ func (a *App) SearchAllChannels(term string, opts model.ChannelSearchOpts) (*mod ExcludeChannelNames: opts.ExcludeChannelNames, NotAssociatedToGroup: opts.NotAssociatedToGroup, IncludeDeleted: opts.IncludeDeleted, + Page: opts.Page, + PerPage: opts.PerPage, } term = strings.TrimSpace(term) diff --git a/model/channel.go b/model/channel.go index bb88bb34f3..920c85d37a 100644 --- a/model/channel.go +++ b/model/channel.go @@ -90,12 +90,17 @@ type DirectChannelForExport struct { // ExcludeDefaultChannels will exclude the configured default channels (ex 'town-square' and 'off-topic'). // IncludeDeleted will include channel records where DeleteAt != 0. // ExcludeChannelNames will exclude channels from the results by name. +// Paginate whether to paginate the results. +// Page page requested, if results are paginated. +// PerPage number of results per page, if paginated. // type ChannelSearchOpts struct { NotAssociatedToGroup string ExcludeDefaultChannels bool IncludeDeleted bool ExcludeChannelNames []string + Page *int + PerPage *int } func (o *Channel) DeepCopy() *Channel { diff --git a/model/channel_search.go b/model/channel_search.go index 7502372a6d..b50ff7e43c 100644 --- a/model/channel_search.go +++ b/model/channel_search.go @@ -14,6 +14,8 @@ type ChannelSearch struct { Term string `json:"term"` ExcludeDefaultChannels bool `json:"exclude_default_channels"` NotAssociatedToGroup string `json:"not_associated_to_group"` + Page *int `json:"page,omitempty"` + PerPage *int `json:"per_page,omitempty"` } // ToJson convert a Channel to a json string diff --git a/model/client4.go b/model/client4.go index 54457b6d3c..b09584cde4 100644 --- a/model/client4.go +++ b/model/client4.go @@ -2255,6 +2255,16 @@ func (c *Client4) SearchAllChannels(search *ChannelSearch) (*ChannelListWithTeam return ChannelListWithTeamDataFromJson(r.Body), BuildResponse(r) } +// SearchAllChannelsPaged searches all the channels and returns the results paged with the total count. +func (c *Client4) SearchAllChannelsPaged(search *ChannelSearch) (*ChannelsWithCount, *Response) { + r, err := c.DoApiPost(c.GetChannelsRoute()+"/search", search.ToJson()) + if err != nil { + return nil, BuildErrorResponse(r, err) + } + defer closeBody(r) + return ChannelsWithCountFromJson(r.Body), BuildResponse(r) +} + // SearchGroupChannels returns the group channels of the user whose members' usernames match the search term. func (c *Client4) SearchGroupChannels(search *ChannelSearch) ([]*Channel, *Response) { r, err := c.DoApiPost(c.GetChannelsRoute()+"/group/search", search.ToJson()) diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 4235b69e37..4627117694 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -2297,19 +2297,42 @@ func (s SqlChannelStore) SearchForUserInTeam(userId string, teamId string, term }) } -func (s SqlChannelStore) SearchAllChannels(term string, opts store.ChannelSearchOpts) (*model.ChannelListWithTeamData, *model.AppError) { +func (s SqlChannelStore) channelSearchQuery(term string, opts store.ChannelSearchOpts, countQuery bool) sq.SelectBuilder { + var limit int + if opts.PerPage != nil { + limit = *opts.PerPage + } else { + limit = 100 + } + + var selectStr string + if countQuery { + selectStr = "count(*)" + } else { + selectStr = "c.*, t.DisplayName AS TeamDisplayName, t.Name AS TeamName, t.UpdateAt as TeamUpdateAt" + } + query := s.getQueryBuilder(). - Select("c.*, t.DisplayName AS TeamDisplayName, t.Name AS TeamName, t.UpdateAt as TeamUpdateAt"). + Select(selectStr). From("Channels AS c"). Join("Teams AS t ON t.Id = c.TeamId"). - Where(sq.Eq{"c.Type": []string{model.CHANNEL_PRIVATE, model.CHANNEL_OPEN}}). - OrderBy("c.DisplayName, t.DisplayName"). - Limit(uint64(100)) + Where(sq.Eq{"c.Type": []string{model.CHANNEL_PRIVATE, model.CHANNEL_OPEN}}) + + // don't bother ordering or limiting if we're just getting the count + if !countQuery { + query = query. + OrderBy("c.DisplayName, t.DisplayName"). + Limit(uint64(limit)) + } if !opts.IncludeDeleted { query = query.Where(sq.Eq{"c.DeleteAt": int(0)}) } + if opts.IsPaginated() && !countQuery { + query = query.Offset(uint64(*opts.Page * *opts.PerPage)) + } + likeClause, likeTerm := s.buildLIKEClause(term, "c.Name, c.DisplayName, c.Purpose") if len(likeTerm) > 0 { likeClause = strings.ReplaceAll(likeClause, ":LikeTerm", "'"+likeTerm+"'") @@ -2326,18 +2349,35 @@ func (s SqlChannelStore) SearchAllChannels(term string, opts store.ChannelSearch query = query.Where("c.Id NOT IN (SELECT ChannelId FROM GroupChannels WHERE GroupChannels.GroupId = ? AND GroupChannels.DeleteAt = 0)", opts.NotAssociatedToGroup) } - queryString, args, err := query.ToSql() + return query +} + +func (s SqlChannelStore) SearchAllChannels(term string, opts store.ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) { + queryString, args, err := s.channelSearchQuery(term, opts, false).ToSql() if err != nil { - return nil, model.NewAppError("SqlChannelStore.SearchAllChannels", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, 0, model.NewAppError("SqlChannelStore.SearchAllChannels", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) } - var channels model.ChannelListWithTeamData - - if _, err := s.GetReplica().Select(&channels, queryString, args...); err != nil { - return nil, model.NewAppError("SqlChannelStore.Search", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) + if _, err = s.GetReplica().Select(&channels, queryString, args...); err != nil { + return nil, 0, model.NewAppError("SqlChannelStore.Search", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) } - return &channels, nil + var totalCount int64 + + // only query a 2nd time for the count if the results are being requested paginated. + if opts.IsPaginated() { + queryString, args, err = s.channelSearchQuery(term, opts, true).ToSql() + if err != nil { + return nil, 0, model.NewAppError("SqlChannelStore.SearchAllChannels", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + } + if totalCount, err = s.GetReplica().SelectInt(queryString, args...); err != nil { + return nil, 0, model.NewAppError("SqlChannelStore.Search", "store.sql_channel.search.app_error", nil, "term="+term+", "+", "+err.Error(), http.StatusInternalServerError) + } + } else { + totalCount = int64(len(channels)) + } + + return &channels, totalCount, nil } func (s SqlChannelStore) SearchMore(userId string, teamId string, term string) (*model.ChannelList, *model.AppError) { diff --git a/store/store.go b/store/store.go index deae1682f9..48e554ccf9 100644 --- a/store/store.go +++ b/store/store.go @@ -173,7 +173,7 @@ type ChannelStore interface { GetMembersForUserWithPagination(teamId, userId string, page, perPage int) (*model.ChannelMembers, *model.AppError) AutocompleteInTeam(teamId string, term string, includeDeleted bool) (*model.ChannelList, *model.AppError) AutocompleteInTeamForSearch(teamId string, userId string, term string, includeDeleted bool) (*model.ChannelList, *model.AppError) - SearchAllChannels(term string, opts ChannelSearchOpts) (*model.ChannelListWithTeamData, *model.AppError) + SearchAllChannels(term string, opts ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) SearchInTeam(teamId string, term string, includeDeleted bool) (*model.ChannelList, *model.AppError) SearchArchivedInTeam(teamId string, term string, userId string) (*model.ChannelList, *model.AppError) SearchForUserInTeam(userId string, teamId string, term string, includeDeleted bool) (*model.ChannelList, *model.AppError) @@ -628,11 +628,20 @@ type LinkMetadataStore interface { // NotAssociatedToGroup will exclude channels that have associated, active GroupChannels records. // IncludeDeleted will include channel records where DeleteAt != 0. // ExcludeChannelNames will exclude channels from the results by name. +// Paginate whether to paginate the results. +// Page page requested, if results are paginated. +// PerPage number of results per page, if paginated. // type ChannelSearchOpts struct { NotAssociatedToGroup string IncludeDeleted bool ExcludeChannelNames []string + Page *int + PerPage *int +} + +func (c *ChannelSearchOpts) IsPaginated() bool { + return c.Page != nil && c.PerPage != nil } type UserGetByIdsOpts struct { diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 0829335afc..19df48809d 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -2689,31 +2689,37 @@ func testChannelStoreSearchAllChannels(t *testing.T, ss store.Store) { Term string Opts store.ChannelSearchOpts ExpectedResults *model.ChannelList + TotalCount int }{ - {"ChannelA", "ChannelA", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o1, &o2, &o3}}, - {"ChannelA, include deleted", "ChannelA", store.ChannelSearchOpts{IncludeDeleted: true}, &model.ChannelList{&o1, &o2, &o3, &o13}}, - {"empty string", "", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o1, &o2, &o3, &o4, &o5, &o12, &o11, &o8, &o7, &o6, &o10, &o9}}, - {"no matches", "blargh", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{}}, - {"prefix", "off-", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o8, &o7, &o6}}, - {"full match with dash", "off-topic", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o6}}, - {"town square", "town square", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o9}}, - {"the in name", "the", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o10}}, - {"Mobile", "Mobile", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o11}}, - {"search purpose", "now searchable", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o12}}, - {"pipe ignored", "town square |", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o9}}, - {"exclude defaults search 'off'", "off-", store.ChannelSearchOpts{IncludeDeleted: false, ExcludeChannelNames: []string{"off-topic"}}, &model.ChannelList{&o8, &o7}}, - {"exclude defaults search 'town'", "town", store.ChannelSearchOpts{IncludeDeleted: false, ExcludeChannelNames: []string{"town-square"}}, &model.ChannelList{}}, - {"exclude by group association", "off", store.ChannelSearchOpts{IncludeDeleted: false, NotAssociatedToGroup: group.Id}, &model.ChannelList{&o8, &o6}}, + {"ChannelA", "ChannelA", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o1, &o2, &o3}, 0}, + {"ChannelA, include deleted", "ChannelA", store.ChannelSearchOpts{IncludeDeleted: true}, &model.ChannelList{&o1, &o2, &o3, &o13}, 0}, + {"empty string", "", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o1, &o2, &o3, &o4, &o5, &o12, &o11, &o8, &o7, &o6, &o10, &o9}, 0}, + {"no matches", "blargh", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{}, 0}, + {"prefix", "off-", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o8, &o7, &o6}, 0}, + {"full match with dash", "off-topic", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o6}, 0}, + {"town square", "town square", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o9}, 0}, + {"the in name", "the", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o10}, 0}, + {"Mobile", "Mobile", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o11}, 0}, + {"search purpose", "now searchable", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o12}, 0}, + {"pipe ignored", "town square |", store.ChannelSearchOpts{IncludeDeleted: false}, &model.ChannelList{&o9}, 0}, + {"exclude defaults search 'off'", "off-", store.ChannelSearchOpts{IncludeDeleted: false, ExcludeChannelNames: []string{"off-topic"}}, &model.ChannelList{&o8, &o7}, 0}, + {"exclude defaults search 'town'", "town", store.ChannelSearchOpts{IncludeDeleted: false, ExcludeChannelNames: []string{"town-square"}}, &model.ChannelList{}, 0}, + {"exclude by group association", "off", store.ChannelSearchOpts{IncludeDeleted: false, NotAssociatedToGroup: group.Id}, &model.ChannelList{&o8, &o6}, 0}, + {"paginate includes count", "off", store.ChannelSearchOpts{IncludeDeleted: false, PerPage: model.NewInt(100)}, &model.ChannelList{&o8, &o7, &o6}, 3}, + {"paginate, page 2 correct entries and count", "off", store.ChannelSearchOpts{IncludeDeleted: false, PerPage: model.NewInt(2), Page: model.NewInt(1)}, &model.ChannelList{&o6}, 3}, } for _, testCase := range testCases { t.Run(testCase.Description, func(t *testing.T) { - channels, err := ss.Channel().SearchAllChannels(testCase.Term, testCase.Opts) + channels, count, err := ss.Channel().SearchAllChannels(testCase.Term, testCase.Opts) require.Nil(t, err) require.Equal(t, len(*testCase.ExpectedResults), len(*channels)) for i, expected := range *testCase.ExpectedResults { require.Equal(t, expected.Id, (*channels)[i].Id) } + if testCase.Opts.Page != nil || testCase.Opts.PerPage != nil { + require.Equal(t, int64(testCase.TotalCount), count) + } }) } } diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 34939e1057..b68b7dab92 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -1440,7 +1440,7 @@ func (_m *ChannelStore) SaveMember(member *model.ChannelMember) (*model.ChannelM } // SearchAllChannels provides a mock function with given fields: term, opts -func (_m *ChannelStore) SearchAllChannels(term string, opts store.ChannelSearchOpts) (*model.ChannelListWithTeamData, *model.AppError) { +func (_m *ChannelStore) SearchAllChannels(term string, opts store.ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) { ret := _m.Called(term, opts) var r0 *model.ChannelListWithTeamData @@ -1461,7 +1461,7 @@ func (_m *ChannelStore) SearchAllChannels(term string, opts store.ChannelSearchO } } - return r0, r1 + return r0, 0, r1 } // SearchForUserInTeam provides a mock function with given fields: userId, teamId, term, includeDeleted diff --git a/store/timer_layer.go b/store/timer_layer.go index 9c8fa67f91..62a20641d3 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -1592,20 +1592,20 @@ func (s *TimerLayerChannelStore) SaveMember(member *model.ChannelMember) (*model return resultVar0, resultVar1 } -func (s *TimerLayerChannelStore) SearchAllChannels(term string, opts ChannelSearchOpts) (*model.ChannelListWithTeamData, *model.AppError) { +func (s *TimerLayerChannelStore) SearchAllChannels(term string, opts ChannelSearchOpts) (*model.ChannelListWithTeamData, int64, *model.AppError) { start := timemodule.Now() - resultVar0, resultVar1 := s.ChannelStore.SearchAllChannels(term, opts) + resultVar0, resultVar1, resultVar2 := s.ChannelStore.SearchAllChannels(term, opts) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil { success := "false" - if resultVar1 == nil { + if resultVar2 == nil { success = "true" } s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.SearchAllChannels", success, elapsed) } - return resultVar0, resultVar1 + return resultVar0, resultVar1, resultVar2 } func (s *TimerLayerChannelStore) SearchForUserInTeam(userId string, teamId string, term string, includeDeleted bool) (*model.ChannelList, *model.AppError) {