From b6564fec6d62c4f9b3a742748cd2c2364f049646 Mon Sep 17 00:00:00 2001 From: Ashish Bhate Date: Tue, 31 Aug 2021 19:16:54 +0530 Subject: [PATCH] [MM-36792] limit number of threads returned from SQL store (#18260) Summary Limit the number of threads returned in a single SQL store call by using the per_page query param instead of pageSize. Our param handling code automatically limits the number of records that can be requested. To support older mobile clients we continue to support the pageSize param until version 6.0 of the server is the minimum supported server version on mobile. Related PRs: [MM-36792] Consistent query param names mattermost-webapp#8700 [MM-36792] Consistent query param names mattermost-mobile#5643 Ticket Link https://mattermost.atlassian.net/browse/MM-36792 --- api4/user.go | 11 +---------- api4/user_test.go | 3 ++- model/client4.go | 2 +- web/params.go | 33 +++++++++++++++++++++++++-------- web/params_test.go | 34 ++++++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 20 deletions(-) create mode 100644 web/params_test.go diff --git a/api4/user.go b/api4/user.go index 4b05ad3687..0166efc656 100644 --- a/api4/user.go +++ b/api4/user.go @@ -2939,7 +2939,7 @@ func getThreadsForUser(c *Context, w http.ResponseWriter, r *http.Request) { Since: 0, Before: "", After: "", - PageSize: 30, + PageSize: uint64(c.Params.PerPage), Unread: false, Extended: false, Deleted: false, @@ -2962,15 +2962,6 @@ func getThreadsForUser(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = model.NewAppError("api.getThreadsForUser", "api.getThreadsForUser.bad_params", nil, "", http.StatusBadRequest) return } - pageSizeString := r.URL.Query().Get("pageSize") - if pageSizeString != "" { - pageSize, parseError := strconv.ParseUint(pageSizeString, 10, 64) - if parseError != nil { - c.SetInvalidParam("pageSize") - return - } - options.PageSize = pageSize - } deletedStr := r.URL.Query().Get("deleted") unreadStr := r.URL.Query().Get("unread") diff --git a/api4/user_test.go b/api4/user_test.go index 785bb49373..fe6730c3be 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -5621,7 +5621,8 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) uss, _, err := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Deleted: false, + Deleted: false, + PageSize: 30, }) require.NoError(t, err) require.Len(t, uss.Threads, 30) diff --git a/model/client4.go b/model/client4.go index 4d6fe1f2c5..afe48000b8 100644 --- a/model/client4.go +++ b/model/client4.go @@ -6593,7 +6593,7 @@ func (c *Client4) GetUserThreads(userId, teamId string, options GetUserThreadsOp v.Set("after", options.After) } if options.PageSize != 0 { - v.Set("pageSize", fmt.Sprintf("%d", options.PageSize)) + v.Set("per_page", fmt.Sprintf("%d", options.PageSize)) } if options.Extended { v.Set("extended", "true") diff --git a/web/params.go b/web/params.go index bec2d26693..48fedfad2d 100644 --- a/web/params.go +++ b/web/params.go @@ -5,6 +5,7 @@ package web import ( "net/http" + "net/url" "strconv" "strings" @@ -255,14 +256,7 @@ func ParamsFromRequest(r *http.Request) *Params { params.Permanent = val } - if val, err := strconv.Atoi(query.Get("per_page")); err != nil || val < 0 { - params.PerPage = PerPageDefault - } else if val > PerPageMaximum { - params.PerPage = PerPageMaximum - } else { - params.PerPage = val - } - + params.PerPage = getPerPageFromQuery(query) if val, err := strconv.Atoi(query.Get("logs_per_page")); err != nil || val < 0 { params.LogsPerPage = LogsPerPageDefault } else if val > LogsPerPageMaximum { @@ -363,3 +357,26 @@ func ParamsFromRequest(r *http.Request) *Params { return params } + +// getPerPageFromQuery returns the PerPage value from the given query. +// This function should be removed and the support for `pageSize` +// should be dropped after v1.46 of the mobile app is no longer supported +// https://mattermost.atlassian.net/browse/MM-38131 +func getPerPageFromQuery(query url.Values) int { + val, err := strconv.Atoi(query.Get("per_page")) + if err != nil { + val, err = strconv.Atoi(query.Get("pageSize")) + // if err != nil || val < 0 { + // return PerPageDefault + // } else if val > PerPageMaximum { + // return PerPageMaximum + // } + // return val + } + if err != nil || val < 0 { + return PerPageDefault + } else if val > PerPageMaximum { + return PerPageMaximum + } + return val +} diff --git a/web/params_test.go b/web/params_test.go new file mode 100644 index 0000000000..54d8d82ddc --- /dev/null +++ b/web/params_test.go @@ -0,0 +1,34 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +package web + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestGetPerPageFromQuery(t *testing.T) { + t.Run("defaults should be set", func(t *testing.T) { + query := make(url.Values) + perPage := getPerPageFromQuery(query) + require.Equal(t, PerPageDefault, perPage) + }) + + t.Run("per_page should take priority", func(t *testing.T) { + query := make(url.Values) + query.Add("pageSize", "100") + query.Add("per_page", "50") + perPage := getPerPageFromQuery(query) + require.Equal(t, 50, perPage) + }) + + t.Run("pageSize should be used only if per_page is incorrectly set", func(t *testing.T) { + query := make(url.Values) + query.Add("pageSize", "100") + query.Add("per_page", "BAD VALUE") + perPage := getPerPageFromQuery(query) + require.Equal(t, 100, perPage) + }) +}