-
Notifications
You must be signed in to change notification settings - Fork 937
feat: User pagination using offsets #1062
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
Conversation
- Currently "before" does not work - Filters not added yet
coderd/database/queries/users.sql
Outdated
users | ||
WHERE | ||
CASE | ||
-- This allows using the last element on a page as effectively a cursor. | ||
-- This is an important option for scripts that need to paginate without | ||
-- duplicating or missing data. | ||
WHEN @created_after :: timestamp with time zone != '0001-01-01 00:00:00+00' THEN created_at > @created_after | ||
ELSE true | ||
END | ||
AND CASE | ||
WHEN @search_email :: text != '' THEN email LIKE concat('%', @search_email, '%') | ||
ELSE true | ||
END | ||
ORDER BY | ||
created_at ASC OFFSET @offset_opt | ||
LIMIT | ||
-- A null limit means "no limit", so -1 means return all | ||
NULLIF(@limit_opt :: int, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sql formatter was pretty set on how this looks. I'm not super happy with it, but I couldn't get the formatter to look different even with comments. Some things I don't like:
- Would prefer
order by
andoffset
to be on different lines. They kinda merge together - The Case
WHEN/THEN
is all on 1 line making it quite long.
Codecov Report
@@ Coverage Diff @@
## main #1062 +/- ##
==========================================
+ Coverage 66.53% 66.65% +0.11%
==========================================
Files 255 255
Lines 15675 15743 +68
Branches 152 152
==========================================
+ Hits 10430 10494 +64
- Misses 4178 4183 +5
+ Partials 1067 1066 -1
Continue to review full report at Codecov.
|
coderd/coderd.go
Outdated
// APIRateLimit is the minutely throughput rate limit per user or ip. | ||
// Setting a rate limit <0 will disable the rate limiter across the entire | ||
// app. Specific routes may have their own limiters. | ||
APIRateLimit int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful to disable during a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm impressed how simple this pagination ended up being. 'tis very simple to understand, and much simpler than we were able to achieve in v1. Just a few questions re offset, but LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor naming nits, but LGTM. Happy with how simple this came out!
codersdk/users.go
Outdated
@@ -13,6 +14,26 @@ import ( | |||
// Me is used as a replacement for your own ID. | |||
var Me = uuid.Nil | |||
|
|||
type PaginatedUsersRequest struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming all the PaginatedUsers
stuff to Users
? Since the endpoint is paginated anyways, seems like it could be implicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
codersdk/users.go
Outdated
} | ||
|
||
type PaginatedUsers struct { | ||
Pager PagerFields `json:"pager"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to return this at all? Or could we keep it similar to before as a top-level array? I believe both parameters are specified by the user anyways. Once we add the totals we may have to increase it, but it's possible things like TotalUsers
will be added to Organization
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the counts, unfortunately a count like that is not sufficient, as you need to match the search
and after_user
params. Those affect the resulting count. So if it's a separate query, you have to accept the same arguments and make two calls 🤷. Not terrible, just not ideal.
Bruno is flying right now, but what I can do is drop this and return []users
. When the UI comes, if there is a need to display pages, we'll revisit. It's not hard to come back to.
tl;dr: I'll drop it and go []users
as the return for now
What does this do
Adds offset pagination to the user query. Also has
search_name
search field (v1 parity) that searches usernames and emails.Supports cursor pagination using
after_user
query param.This also drops sql linting. Sql linting was not working with sqlc type arguments and kept destroying my queries making them invalid sqlc queries.
Request GET
/api/v2/users
Query params
after_user
: a uuid that returns all users after the argument. All users are sorted in a global order by their creation time.limit
: maximum number of users returnedoffset
: returns the[offset:]
users from the list. This allows pagination, pagen
isoffset = <page> * <limit>
search_name
: a string that filters all users, only showing users that have this substring in theirusername
,email
, orname
Response