Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination section refers to non-existent prev_batch token when back-paginating #1898

Closed
richvdh opened this issue Feb 21, 2019 · 9 comments · Fixed by #3366
Closed

Pagination section refers to non-existent prev_batch token when back-paginating #1898

richvdh opened this issue Feb 21, 2019 · 9 comments · Fixed by #3366
Labels
client-server Client-Server API spec-bug Something which is in the spec, but is wrong wart A point where the protocol is inconsistent or inelegant

Comments

@richvdh
Copy link
Member

richvdh commented Feb 21, 2019

https://spec.matrix.org/unstable/client-server-api/#pagination says 'In this scenario, the prev_batch token would be excluded from the response', but /messages doesn't have a prev_batch token. Presumably it means end.

@richvdh
Copy link
Member Author

richvdh commented Feb 21, 2019

The /messages spec could do with making it clear that end will be omitted when you hit the end of the list, too.

@turt2live
Copy link
Member

I thought we had an issue for it, but apparently not: the pagination API is inconsistently used on top of this. It's applied to the letter on some endpoints, and not at all on others. We may have to think critically about whether or not we want it, and how we want to fix it.

@turt2live turt2live added client-server Client-Server API spec-bug Something which is in the spec, but is wrong wart A point where the protocol is inconsistent or inelegant labels May 28, 2019
@richvdh
Copy link
Member Author

richvdh commented May 28, 2019

I thought we had an issue for it, but apparently not: the pagination API is inconsistently used on top of this.

You might be thinking of #1523?

@turt2live
Copy link
Member

Yup, that would explain why I can't find it too. I don't remember fixing it, but it sounds like I did.

@richvdh
Copy link
Member Author

richvdh commented Aug 10, 2021

A list of endpoints which implement pagination:

Endpoint Inputs Pagination token output params Array output param Notes
/initialSync, /rooms/{room_id}/initialSync limit start, end chunk deprecated
/events from start, end chunk deprecated
/messages from, to, dir, limit start, end chunk
/sync since next_batch, prev_batch rooms/events
/publicRooms, /federation/v1/publicRooms since, limit next_batch, prev_batch chunk
/keys/changes from, to n/a changed, left Uses /sync tokens
/members at n/a chunk Takes a sync token. Presumably there was an intention to add pagination later.
/notifications from, limit next_token notifications Paginates backwards from "now"
/search next_batch next_batch results Multiple levels of pagination, documented at https://spec.matrix.org/unstable/client-server-api/#pagination-1

Note that /sync and /messages have their own documentation at https://spec.matrix.org/unstable/client-server-api/#syncing.

@richvdh
Copy link
Member Author

richvdh commented Aug 11, 2021

I'd like to try and come up with a convention for pagination parameters, which at least new APIs (such as the one being discussed in MSC2946, and sync v3) can follow. My thoughts so far:

  • there seem to be broadly two categories at the moment: from/to/start/end/chunk, and since/next_batch/prev_batch/no chunk.
  • start and end are rather generic names when used without chunk.
  • On the other hand chunk seems to add overhead, and I'm not convinced it fits all use cases. It would seem a bit silly trying to use it with /sync (which has multiple pagination tokens).
  • since is a funny name for APIs that paginate rather than stream.
  • Other conventions in the world include before/after (Github), pagination_token->previous_token/next_token (Twitter). I like before/after but am not enthusiastic about using completely new names (and introducing an xkcd #927 situation).

Broadly I think I favour:

  • input parameters called from (and to, where appropriate) for true paginated data
  • an input parameter called since for streaming data
  • outputs called next_batch (and prev_batch where appropriate)

As a final thought: we could do something completely different like follow Github's pattern of using Link headers to return pagination tokens rather than returning the tokens in the JSON response body. That is probably a bit too radical though.

@kegsay
Copy link
Member

kegsay commented Aug 11, 2021

So if I understand correctly:

  • Streaming /sync: accept a since input and outputs a next_batch token? "Batch" feels spurious here as there isn't really any "batches" (terminology wise) anywhere in Matrix APIs.
  • Paginated /messages accept a from and to, output a next_batch (and possible prev_batch)?

This makes next_batch context-dependent, and makes it so you cannot compose streaming/pagination together without introducing a level of nesting. Sync v3 uses a level of nesting for this purpose: the next token is contained under an object called p.

These aren't problems per se, just observations based on the proposed naming. I care more about consensus more than anything else, so things like this seem reasonable.

@richvdh
Copy link
Member Author

richvdh commented Aug 11, 2021

Yes, sounds like you understand correctly.

"Batch" feels spurious here as there isn't really any "batches" (terminology wise) anywhere in Matrix APIs.

Surely any API that paginates returns batches? Unless the word "batch" means something special to you that it doesn't to me? Anyway, yes I agree it's a bit redundant, but since it's reasonably widely used currently I feel like I'd rather live with that than change everything. Similarly @ara4n proposed next_token but again I'd prefer to follow the existing precedent.

This makes next_batch context-dependent and makes it so you cannot compose streaming/pagination together without introducing a level of nesting. Sync v3 uses a level of nesting for this purpose: the next token is contained under an object called p

@kegsay explained in chat that sync v3 does this to support two different types of token, where one token is used to paginate results, and the other is used to skip straight to the next point in the stream.

If this is a requirement, I think it's fine to diverge from the convention for complicated APIs like /sync which need to deal with both pagination and streaming. You could call the two different tokens next_page and next_batch, or something.

@kegsay
Copy link
Member

kegsay commented Aug 16, 2021

FYI sync v3 now uses next_page and next_batch.

richvdh added a commit that referenced this issue Aug 27, 2021
The Pagination section in the C-S API was, basically, full of rubbish. I think that anything of any value it contained was repeated either directly on the API definitions or in the text specific to syncing at https://spec.matrix.org/unstable/client-server-api/#syncing.

The conventions I've added to the Appendices are based on the discussions in #1898. They are there because I don't want to have to go through it all again next time we add a paginated API.

Fixes: #1898
Fixes: #2268
richvdh added a commit that referenced this issue Aug 27, 2021
The Pagination section in the C-S API was, basically, full of rubbish. I think that anything of any value it contained was repeated either directly on the API definitions or in the text specific to syncing at https://spec.matrix.org/unstable/client-server-api/#syncing.

The conventions I've added to the Appendices are based on the discussions in #1898. They are there because I don't want to have to go through it all again next time we add a paginated API.

Fixes: #1898
Fixes: #2268
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API spec-bug Something which is in the spec, but is wrong wart A point where the protocol is inconsistent or inelegant
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants