Feat: Use only one standardized method for pagination across the API

[mafredri]

What is your suggestion?

We should only use one standardized method of pagination across the API, preferably we'd keep the cursor based pagination and drop support for offset.

Another approach would be to return pagination metadata in the API responses, for instance, something like:

{
  "page": {
      "current_number": 2,
      "total_pages": 10,
      "current": "/api/path?limit=10&after_id=xxx",
      "next": "/api/path?limit=10&after_id=yyy",
      "previous": "/api/path?limit=10&after_id=zzz"
  }
}

Why do you want this feature?

This was discussed in #1308 and @spikecurtis had good motivations for only keeping one (#1308 (comment)), according to @presleyp it may suffice to only keep cursor based pagination around (#1308 (comment)).

Having two methods for pagination can be confusing and hard to enforce on the API side across all endpoints. It might also add complexity if we introduce more advanced features, like sorting on column.

I think it would be good if @Emyrk had a chance to give his two cents as developer of the original implementation.

The issue described in #1357 may also need to be considered here.

Are there any workarounds to get this functionality today?

No.

Are you interested in submitting a PR for this?

After we have consensus on what approach to take? Sure.

[Emyrk]

We actually originally were only going to support cursor pagination, but then it's quite challenging to make pages work. Offset pagination makes "Jump to page 10" really easy, which is great for the UI.

I see cursor pagination critical for streaming data, as the offset pagination would cause issues. If you consider our data, offset pagination for the UI is sufficient in almost every case. The risk of race conditions is not a big deal in the UI in that 0.1% case of viewing the users page. Github actually does this for their repos, orgs, and user pages. (https://github.com/orgs/coder/repositories?page=2&type=all) The only one that I can think of that would prefer cursor based is audit logs, since that is a data stream.

Now for a script, offset pagination is potentially risky, as in the edge case a new user is made while paging, you might skip someone. That is why I added a after_user field for pagination in the forward direction. For ui purposes we can have a page counter at the bottom, and for script purposes you can still pagination by staying on page 0. So @spikecurtis

I don't think both after_id and offset make sense together. And, I'll argue that we should drop offset entirely in favor of after_id.

The logic was that offset is sufficient for all ui purposes, but our customers do consume our api for automating various tasks. In that automation case, we need a way to paginate in a single direction that does not have race conditions. This was also taken from Github.

---

In summary, I do agree cursor based pagination is more correct, but for usability, offset pagination is much easier to work with in a UI. If we decide to do cursor based pagination, the UI would probably have to do something like call the next 2 pages, the previous 2 pages, and the last(?) page for the UI to show first ... 4 5 6 7 ... last on the page selector for the table.

For the audit log case though, I'm cool with making that cursor based.

(Sorry was on PTO during a lot of this talk)

[Emyrk]

Another comment on "total_pages": 10,. I really wanted to add this by adding count(*) OVER() to the postgres query, but sqlc does not handle embedded structs well yet. So doing that breaks the User struct returned by the query, and names it something like GetUsersRow. Then you have to write a convert function to convert that to a User type. It's garbage and annoying. I was waiting for sqlc to implement it.

See all the issues on their repo:

• (this could work for us) Struct Overrides (proposal) sqlc-dev/sqlc#1176
• (this could work for us) Support for custom models sqlc-dev/sqlc#755

And PRS:

• feat: Add embedded structs based on result match sqlc-dev/sqlc#984
• feat: add sqlc.embed to allow model re-use sqlc-dev/sqlc#1615

(probably more)

[spikecurtis]

The concern I have is setting up potential future developers for shooting themselves in the foot by writing scripts that use offset-paging. If cursor pagination is difficult for the UI, then perhaps we can mitigate the above concern in some other way.

Some ideas:

• Have different endpoints named appropriately to point people to the right one, e.g. call the offset one ...ui and the other ...safe
• Have a prominent SDK function that iterates over items, such that developers aren't tempted to write offset style code because it seems easier than after style code.

[Emyrk]

Do you think that is something that can be handled in our api documentation? The only way a user would know offset is a valid field is by inspecting the UI (which if you are reverse engineering our api, then idk), or reading our docs. So this could be a documentation problem.

We could also not even document the offset part of the api and only document the safe pagination method.

[mafredri]

I appreciate your input on this issue @Emyrk, you raise some very good points. Based on your comments I think we should keep both around like you originally designed it. We can either close this issue or re-purpose it for tracking improvements (total pages, split structs [see below]?).

I'd like to point out that currently the offset pagination is fairly discoverable in the SDK, but I don't think that should be a problem as long as we document it properly, maybe add a Warning: .... As for the API, I guess we don't have a way to document it yet? But either way I don't think we should hide any functionality and assume it'll only be used by the UI (gut feeling, "why can the UI do this but not I?").

What are your thoughts about splitting Paginate up into two structs (PaginateOffset and PaginateID, like suggested in #1308)? Ideally we'd accept one or the other, providing both would result in an error (even though supported by the implementation currently).

coder/codersdk/pagination.go

Lines 10 to 27 in 64a8b4a

	// Pagination sets pagination options for the endpoints that support it.
	type Pagination struct {
	// AfterID returns all or up to Limit results after the given
	// UUID. This option can be used with or as an alternative to
	// Offset for better performance. To use it as an alternative,
	// set AfterID to the last UUID returned by the previous
	// request.
	AfterID uuid.UUID `json:"after_id,omitempty"`
	// Limit sets the maximum number of users to be returned
	// in a single page. If the limit is <= 0, there is no limit
	// and all users are returned.
	Limit int `json:"limit,omitempty"`
	// Offset is used to indicate which page to return. An offset of 0
	// returns the first 'limit' number of users.
	// To get the next page, use offset=<limit>*<page_number>.
	// Offset is 0 indexed, so the first record sits at offset 0.
	Offset int `json:"offset,omitempty"`
	}

I also think @spikecurtis suggestion for having separate URLs/parameters for UI is pretty neat, but might be problematic to express in both SDK and generated typings.

---

It's too bad about sqlc, hope that functionality lands soon!

[Emyrk]

I'm ok with splitting it up into 2 structures, and throwing an error if you mix and match. I have no strong feelings on that.

The returned urls for the next/prev page are nifty. I was doing that originally when I had cursor pagination instead, and just also returned the appropriate data needed to make the url. If you just return the url, it is kinda annoying for an sdk to consume.

{
    // Data is the general "data" or "error" top level field
    "data": {
        // page is the list of paged data. Each object in the paged 
        // data set is an object.
        "page": [{}, {}],
        "pager": {
            // A page is a list of elements of length 'limit'.
            // The first element is index '0'.
            //    "ending_before" == page[0]
            // The last element is index `-1` (last element)
            //    "starting_after" == page[-1]
            "ending_before": "<before_uuid>",
            "starting_after": "<after_uuid>",
            // next_uri is the path to fetch the next page of users.
            // "" if there is no next page
            "next_uri": "/api/v2/users?after=<after_uuid>&limit=10"
            // previous_uri returns the orevious page
            // "" if there is no previous page
            "previous_uri": "/api/v2/users?before=<before_uuid>&limit=10",
            // Limit is the maximum number of elements that will be returned in
            // 1 page.
            "limit": 10,
        }
    }
}

[spikecurtis]

We should treat the SDK as a public API, just like the HTTP API. We're going to open source the repo, and so anyone writing go code to interact programmatically with a coder server can and should use the SDK.

[ammario]

Can we base this decision on prior art? I'm used to simpler APIs like this.

We will not generate client bindings for every programming language, and many will use curl for testing anyways. There is good reason to keep the API as simple as possible.

[Emyrk]

@ammario you can find examples of offset and cursor pagination in all enterprise products that I have seen. Github uses a mix for example.

It's a decision we should make based on our needs. I believe we can get a better UI experience with offset pagination. If the UI team doesn't think "Jump to page X" and showing the page numbers at the bottom is useful, then I have no issue with cursor based pagination for all things.

[tjcran]

@Emyrk @spikecurtis @mafredri @presleyp are we in agreement that we should keep the original implementation, with some modifications that @Emyrk suggested further in the thread?

If we are going to diverge from the original implementation, it should probably be decided sooner rather than later as it will have downstream implications. But if we are going to keep the original, perhaps with modifications, then it seems like this could safely come after the Community launch.

[spikecurtis]

Depends on our stance re: back compatibility of our APIs after Community Launch. If we're willing to break stuff, we can punt for now and revisit. If we're not willing to break people's code, then we kinda need to decide now. @tjcran

[ammario]

The major business risk is not delivering fast enough to make an amazing launch.

After we do that and begin accumulating a user base, we should look at stabilizing some APIs.

[tjcran]

Closing this issue because the decision was made that we would keep status quo for now in how Pagination is already implemented.