@BrunoQuaresma just want to make sure you see this and get a chance to discuss it if needed

Does this mean we send eitherafter_id orlimit andoffset? I think typescript will complain if we leave anything out of this type, so can we send 0 or empty string for the ones we're not using?

@presleyp The way this is (currently) implemented means you can set any combination of them (including setting all or none). I don't know if we want to keep it that way though.

@Emyrk do you know if there's a way we can make the generator express the above? (E.g. would setting? all of these, likereadonly after_id?: string, work?)

Yeah I'm concerned that if the backend expects "just omit the ones you don't want to use" but the frontend enforces "these are all required" then we might have trouble sending well formed requests.

Yes I agree, that would be super inconvenient! I see pointers and a few other null types are marked as optional by the TypeScript generator, but not others. I could take a look at improving that. Perhaps using the jsonomitempty option (Go specific) could also be marked optional in TypeScript.

I don't think bothafter_id andoffset make sense together. And, I'll argue that we should dropoffset entirely in favor ofafter_id.

The problem withoffset is that it is not safe to race conditions that add or delete rows. Say we have {a0, a1, a2, b0, b1, b2} as our data. I paginate withlimit=3 offset=0 and get {a0, a1, a2}. Now someone else deletes a1. Then I call paginate withlimit=3 offset=3. I get {b1, b2}!

You might argue that in some use cases, it doesn't matter if we duplicate or omit data, so why not give both options? The problem is that it's really hard to be sure we've communicated the danger aboutoffset to potential users of this API. They might not read the docstrings. This API might end up being offered to end users outside of Coder. Thatoffset is so tempting because the code is easier to write thanafter_id, but we're setting users up to shoot themselves in the foot.

I agree with this. It's also why I suggested separating the options (offset and after_id) into two structs in my PR description. But since adding both was deliberate (#1057 (comment)) I don't think we should remove it in this PR (at least not for the users endpoint). Personally I don't see much of a usability difference between the two and would prefer only havingafter_id (or another solution entirely).

I don't understand the use case for using both AfterID and Offset. I would have thought they should be mutually exclusive.

I agree, I'd be in favor of droppingoffset if there's no use-case for it.

I found some context in an older PR:#1057 (comment)

@presleyp@BrunoQuaresma Can the frontend just use cursor based pagination? How much harder would it be to support over offset

I wouldn't argue for offset-based pagination over cursor-based pagination based on how hard it is to implement on the frontend, but...we don't need pagination on the frontend in CE so we are currently ignoring it, sending no limit or offset params, which I believe works because the offset-based pagination defaults are the same as if it wasn't paginated. Could cursor-based pagination be similarly ignored?

Cursor-based pagination is similarly ignored if not set in the implementation I'm reading.

I opened up an issue/feature for this:#1358

I don't think bothafter_id andoffset make sense together. And, I'll argue that we should dropoffset entirely in favor ofafter_id.

The problem withoffset is that it is not safe to race conditions that add or delete rows. Say we have {a0, a1, a2, b0, b1, b2} as our data. I paginate withlimit=3 offset=0 and get {a0, a1, a2}. Now someone else deletes a1. Then I call paginate withlimit=3 offset=3. I get {b1, b2}!

You might argue that in some use cases, it doesn't matter if we duplicate or omit data, so why not give both options? The problem is that it's really hard to be sure we've communicated the danger aboutoffset to potential users of this API. They might not read the docstrings. This API might end up being offered to end users outside of Coder. Thatoffset is so tempting because the code is easier to write thanafter_id, but we're setting users up to shoot themselves in the foot.

This locks us into having a UUID for every database table that we want to paginate. Have we fully considered the consequences?

I had the same thought, but at least all existing tables seem to be using UUID forid.

@coder/backend Does anyone else have input on this? Steven seems to be on PTO so can't check in with him.

This can easily be changed to a string or something if we ever want to support different ID types in the future so I think it's fine for now

What happens if the after_id doesn't exist? Either because user modified a URL, or because someone deleted the row?

No results, which is acceptable in the former scenario. As for the latter, good catch. Not sure if this has been thought of.

I opened up an issue for this#1357 (also relevant#1358).