[MM-46912] Add pagination #21026

vish9812 · 2022-09-14T10:04:31Z

Summary

Accept a pagination limit from frontend.

Ticket Link

JIRA - https://mattermost.atlassian.net/browse/MM-46912

Related Pull Requests

Has web-app changes ([MM-46912] Add pagination mattermost-webapp#11132)

Release Note

Used pagination params(page & per_page) available in api /channels/:channel_id/posts for the "since" route.

mattermod · 2022-09-14T10:04:33Z

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

vish9812 · 2022-09-14T10:06:59Z

@lieut-data Adding you in-case you want to have a look in "plugin" changes.

zefhemel · 2022-09-14T10:47:45Z

cc @agnivade @agarciamontoro to make sure we're not introducing surprising performance issues here

lieut-data · 2022-09-14T13:01:03Z

app/plugin_api.go

@@ -682,8 +682,11 @@ func (api *PluginAPI) GetPost(postID string) (*model.Post, *model.AppError) {
 	return post, appErr
 }

-func (api *PluginAPI) GetPostsSince(channelID string, time int64) (*model.PostList, *model.AppError) {
-	list, appErr := api.app.GetPostsSince(model.GetPostsSinceOptions{ChannelId: channelID, Time: time})
+func (api *PluginAPI) GetPostsSince(channelID string, time int64, page, perPage int) (*model.PostList, *model.AppError) {


At the RPC level, this should be fine, but semantically, I think it's still a breaking change if old plugins are expecting to get all the posts from a call to GetPostsSince.

Might we consider handling that by treating page == perPage == 0 specially?

At the same time, I'm also not 5/5 for this. It /feels/ unlikely that a plugin was depending on this behaviour, so we might be able to at least scan the known plugins for usage and make a judgment call here instead.

old plugins are expecting to get all the posts

Even before it wasn't returning all the posts technically, since it always had a limit. It's just that, the limit was quite big.

scan the known plugins for usage

Any guidance on this, how or where can I verify it?

handling that by treating page == perPage == 0

You mean, don't limit/paginate in this case?

technically, since it always had a limit. It's just that, the limit was quite big.

@vish9812, for reference, what was that big limit?

Any guidance on this, how or where can I verify it?

@mickmister / @levb, I know we've done analysis of this kind in the past. (I sometimes do it locally just be grepping over my plugins src directories to see if anything matches.) Also interested in your feedback on maybe just ignoring it instead (allowing the "breaking change").

You mean, don't limit/paginate in this case?

Yeah, the idea would be to essentially revert to the big limit as previously. Not sure it's worth this, as noted above.

Sorry for the late response, grepping the local plugin repos is all I know of as far as analysis goes.

As to the breaking change, this is an interface change and will break compatibility with the old plugin's signatures, no? I am not sure if the extra parameter is a breaking change for the RPC or not. Was introducing a new plugin API, GetPostsSincePaginated considered while keeping the old one intact, perhaps as deprecated?

As to the breaking change, this is an interface change and will break compatibility with the old plugin's signatures, no? I am not sure if the extra parameter is a breaking change for the RPC or not.

@levb @lieut-data IIRC this is a breaking change when the argument types of the function change. The "safe" way to introduce an API change is to add fields to an existing struct being used, but that's not an option here.

Was introducing a new plugin API, GetPostsSincePaginated considered while keeping the old one intact, perhaps as deprecated?

+1 for introducing a new method. 3/5 An "ugly" method name is worth avoiding backwards compatibility issues

It looks like the change was reverted for plugins though. Makes sense as it seems this PR is specifically meant for the webapp

agarciamontoro · 2022-09-15T11:28:51Z

Thanks for the ping, @zefhemel.

At a first glance, this does not seem harmful performance-wise, since we're only adding the OFFSET, and I assume the LIMIT (i.e. the perPage param) will be probably less than it was before, right?

@vish9812, I see that the webapp PR just adds the params. Where are we planning to use this new pagination?

vish9812 · 2022-09-15T12:48:46Z

I assume the LIMIT (i.e. the perPage param) will be probably less than it was before, right?

Yes, it's quite smaller than the previous one.

the webapp PR just adds the params

Yeah, but with default values. So there is no "new" use-case for it. It's just that it will be fetching the data in smaller batches now.

agarciamontoro · 2022-09-15T15:36:39Z

So there is no "new" use-case for it. It's just that it will be fetching the data in smaller batches now.

But if pagination wasn't there, I guess we'll need to modify the clients now to use it, right? Or are all usages of this ok with always fetching 60 instead of 1000 posts in total?

agnivade · 2022-09-16T03:23:46Z

Bear in mind that LIMIT/OFFSET doesn't really control the memory usage in any way of the DB. If you have 10,000 elements and want to load the last 100 elements, a LIMIT 100 OFFSET 9900 will still load 10,000 elements in the DB. That's why this approach is not recommended and it is suggested to use cursor based pagination.

See https://github.com/mattermost/mattermost-server/blob/1a5ecfa9572ef18f93cd13c99ad94fc8d548a820/store/sqlstore/post_store.go#L598 for what I am talking about.

vish9812 · 2022-09-16T11:51:27Z

@agarciamontoro So far it seems to work with the smaller limit. I tried a few scenarios.

@agnivade As per the "since" api usage, it seems unlikely that it's gonna have a high Offset. There are "after" & "before" apis which are used more often to load the data. Its main usage seems to "sync" the posts, and during this "disconnected-client" time, 1000s of posts shouldn't be a common scenario. Also, at the moment it's similar to what "after" & "before" apis follow.

agnivade · 2022-09-16T18:34:17Z

I think it's hard to say what is a common scenario here. If someone is gone for a week in a busy installation, it's not that far fetched for a busy channel to have several thousand posts.

In any case, this is more about API security than about API usage. Even if it's not common, someone can make such an API request and possibly crash the DB. Nothing can stop them.

All that being said, I understand that it's been always like this, so there's not a big urgency to change the model. And moreover, if we start using GraphQL for posts, we will automatically use a cursor based model.

I'll leave you to take a call.

enahum · 2022-09-17T13:13:08Z

I'm afraid I must block this PR until the changes with backwards compatibility are added to v1 and v2 of the mobile apps!!

vish9812 · 2022-09-26T17:13:27Z

@enahum I've added backwards compatibility. LMK, if it works or something else needs to be done.

vish9812 · 2022-09-26T17:26:58Z

this is more about API security than about API usage. Even if it's not common, someone can make such an API request and possibly crash the DB

@agnivade Yeah, makes sense. I'll discuss it with the team, as it would require more client-side updates, also it can become a little messy to use two types of pagination within a single function. So ideally, we should update the "after" and "before" routes as well to use the cursor based model.

agnivade · 2022-09-29T06:00:57Z

Agree with your thinking @vish9812

vish9812 · 2022-10-12T13:20:15Z

@agnivade Created another jira-ticket to change it to the Cursor-based pagination.

@enahum Can you please confirm, if the backwards compatibility looks okay?

enahum

couple of notes added.

Really important that the QA tester covers this on the v1 and v2 app to make sure nothing broke.

enahum · 2022-10-18T12:00:50Z

api4/post.go

@@ -179,13 +182,18 @@ func getPostsForChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 			c.SetInvalidParam("since")
 			return
 		}
+
+		// In case of "since" route, return all items by default for now, to maintain
+		// backwards compatibility with mobile. But when the next ESR passes, just


I'd say when the version of the server that ships this becomes the ESR this can be removed, not before.

Should I just remove the ESR-part from the comment. That should automatically means, remove this "if" block whenever the min. mobile version starts supporting the new pagination, right?

enahum · 2022-10-18T12:04:06Z

store/sqlstore/post_store.go

@@ -1231,7 +1231,8 @@ func (s *SqlPostStore) getPostsSinceCollapsedThreads(options model.GetPostsSince
 		Where(sq.Gt{"Posts.UpdateAt": options.Time}).
 		Where(sq.Eq{"Posts.RootId": ""}).
 		OrderBy("Posts.CreateAt DESC").
-		Limit(1000).
+		Offset(uint64(options.Page * options.PerPage)).
+		Limit(uint64(options.PerPage)).


if perPage is 0, should we keep the limit of 1000 ?

It's already added here - https://github.com/mattermost/mattermost-server/pull/21026/files/1d1d6ee72eb7ee052acd94ae72f22659ad3332e4#diff-17a4db18ae781c89b1357c12bb74c8f78396103865f014b1e2c413e4739e7dc1R1257

vish9812 · 2022-11-01T07:48:16Z

@ashishbhate @koox00 It's up for review.

vish9812 · 2022-11-01T07:50:47Z

/update-branch

isacikgoz · 2023-04-25T12:51:12Z

@vish9812 is this PR active? If not can you please close it? Thanks!

kostaspt · 2023-05-05T16:18:58Z

webapp/platform/client/src/client4.ts

@@ -2033,9 +2033,9 @@ export default class Client4 {
        );
    };

-    getPostsSince = (channelId: string, since: number, fetchThreads = true, collapsedThreads = false, collapsedThreadsExtended = false) => {
+    getPostsSince = (channelId: string, since: number, page = 0, perPage = PER_PAGE_DEFAULT, fetchThreads = true, collapsedThreads = false, collapsedThreadsExtended = false) => {


Why not put the new params at the end of the method? It's not optimal context-wise, but it will improve backward incompatibility since the order of the params is important.

It is to keep the signature similar to all other functions.

agarciamontoro

First of all, sorry for the delay on my review!

Changes look good, but I'm a bit concerned about the unforeseen effects, since we're effectively changing the default value of perPage from 1000 to 60, which is quite a large change. Now, I'm not against this change, but I feel like we need extensive testing before merging (probably including load-testing), since this change means that where we made one query before, we now do 17 (of course much smaller). Now, I don't know how frequent that case is: do we usually get thousands of posts from this API endpoint? Or is the code usually getting much less? I see that the case for third-party callers of this endpoint (plugin) is taken care of, and we're still defaulting to 1000, but the webapp is what concerns me the most.

So from a code point of view, this PR is perfect, but for it to be merged I'd ask for:

Extensive QA
Load-testing focused on the number and duration of the requests to this endpoint and its general effect in the overall health of the server.

So I guess the thing is: do we have enough resources for doing this? Or should we focus our efforts on something with more priority? I'll let you guys answer those.

agarciamontoro · 2023-07-17T15:56:56Z

server/channels/store/sqlstore/post_store.go

+	// Add original limit of 1000 to maintain backwards compatibility with mobile and plugins.
+	if options.PerPage == 0 {
+		options.PerPage = 1000
+	}


Should we set option.Page = 0 as well to make sure nothing weird happens in the default case?

vish9812 added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 14, 2022

vish9812 requested review from lieut-data, ashishbhate and koox00 September 14, 2022 10:04

mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 14, 2022

vish9812 mentioned this pull request Sep 14, 2022

[MM-46912] Add pagination mattermost/mattermost-webapp#11132

Closed

lieut-data reviewed Sep 14, 2022

View reviewed changes

enahum added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Sep 17, 2022

enahum self-requested a review October 12, 2022 13:39

enahum previously approved these changes Oct 18, 2022

View reviewed changes

vish9812 removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Nov 1, 2022

ashishbhate approved these changes Nov 8, 2022

View reviewed changes

koox00 approved these changes Feb 6, 2023

View reviewed changes

calebroseland dismissed enahum’s stale review via 13e93db April 7, 2023 15:43

vish9812 added 3 commits May 3, 2023 20:18

Add pagination

21b2082

Add plugin support

ba2a506

Add backwards compatibility for mobile and plugins

a40f19f

vish9812 force-pushed the MM-46912_page-limit branch from 13e93db to a40f19f Compare May 3, 2023 16:06

pass params to api

d1a19d4

vish9812 requested a review from kostaspt May 4, 2023 07:32

kostaspt reviewed May 5, 2023

View reviewed changes

hanzei requested review from agnivade and agarciamontoro June 1, 2023 21:05

agnivade removed their request for review June 9, 2023 03:32

Merge remote-tracking branch 'origin/master' into MM-46912_page-limit

7358757

vish9812 self-assigned this Jul 17, 2023

vish9812 requested a review from harshilsharma63 July 17, 2023 08:41

agarciamontoro reviewed Jul 17, 2023

View reviewed changes

harshilsharma63 removed their request for review August 28, 2023 05:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-46912] Add pagination #21026

[MM-46912] Add pagination #21026

vish9812 commented Sep 14, 2022

mattermod commented Sep 14, 2022

vish9812 commented Sep 14, 2022

zefhemel commented Sep 14, 2022

lieut-data Sep 14, 2022 •

edited

vish9812 Sep 15, 2022

lieut-data Sep 15, 2022

levb Sep 29, 2022

mickmister Oct 13, 2022

mickmister Oct 13, 2022

agarciamontoro commented Sep 15, 2022

vish9812 commented Sep 15, 2022

agarciamontoro commented Sep 15, 2022

agnivade commented Sep 16, 2022

vish9812 commented Sep 16, 2022 •

edited

agnivade commented Sep 16, 2022

enahum commented Sep 17, 2022

vish9812 commented Sep 26, 2022

vish9812 commented Sep 26, 2022

agnivade commented Sep 29, 2022

vish9812 commented Oct 12, 2022

enahum left a comment

enahum Oct 18, 2022

vish9812 Oct 20, 2022

enahum Oct 18, 2022

vish9812 Oct 20, 2022

vish9812 commented Nov 1, 2022

vish9812 commented Nov 1, 2022

isacikgoz commented Apr 25, 2023

kostaspt May 5, 2023

vish9812 May 16, 2023

agarciamontoro left a comment •

edited

agarciamontoro Jul 17, 2023

[MM-46912] Add pagination #21026

Are you sure you want to change the base?

[MM-46912] Add pagination #21026

Conversation

vish9812 commented Sep 14, 2022

Summary

Ticket Link

Related Pull Requests

Release Note

mattermod commented Sep 14, 2022

vish9812 commented Sep 14, 2022

zefhemel commented Sep 14, 2022

lieut-data Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro commented Sep 15, 2022

vish9812 commented Sep 15, 2022

agarciamontoro commented Sep 15, 2022

agnivade commented Sep 16, 2022

vish9812 commented Sep 16, 2022 • edited

agnivade commented Sep 16, 2022

enahum commented Sep 17, 2022

vish9812 commented Sep 26, 2022

vish9812 commented Sep 26, 2022

agnivade commented Sep 29, 2022

vish9812 commented Oct 12, 2022

enahum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vish9812 commented Nov 1, 2022

vish9812 commented Nov 1, 2022

isacikgoz commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lieut-data Sep 14, 2022 •

edited

vish9812 commented Sep 16, 2022 •

edited

agarciamontoro left a comment •

edited