Skip to content

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

Merged
merged 25 commits into from
Apr 22, 2022
Merged

feat: User pagination using offsets #1062

merged 25 commits into from
Apr 22, 2022

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 19, 2022

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 returned
  • offset : returns the [offset:] users from the list. This allows pagination, page n is offset = <page> * <limit>
  • search_name : a string that filters all users, only showing users that have this substring in their username, email, or name

Response

[ 
  {/*... user ...*/ },
  {/*... user ...*/ },
]

Comment on lines 59 to 76
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);
Copy link
Member Author

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 and offset to be on different lines. They kinda merge together
  • The Case WHEN/THEN is all on 1 line making it quite long.

@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

Merging #1062 (71f6f61) into main (2a95917) will increase coverage by 0.11%.
The diff coverage is 68.75%.

@@            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     
Flag Coverage Δ
unittest-go-macos-latest 53.36% <61.45%> (-0.05%) ⬇️
unittest-go-postgres- 65.96% <68.75%> (+0.07%) ⬆️
unittest-go-ubuntu-latest 55.96% <61.45%> (+0.12%) ⬆️
unittest-go-windows-2022 52.90% <61.45%> (+0.07%) ⬆️
unittest-js 66.83% <ø> (ø)
Impacted Files Coverage Δ
coderd/users.go 59.49% <57.89%> (-0.08%) ⬇️
codersdk/users.go 66.66% <72.72%> (+2.05%) ⬆️
coderd/coderd.go 97.35% <100.00%> (+0.04%) ⬆️
coderd/coderdtest/coderdtest.go 98.87% <100.00%> (+<0.01%) ⬆️
coderd/database/queries.sql.go 83.23% <100.00%> (+0.05%) ⬆️
coderd/httpmw/ratelimit.go 100.00% <100.00%> (ø)
codersdk/provisionerdaemons.go 59.70% <0.00%> (-5.98%) ⬇️
cli/cliui/agent.go 77.19% <0.00%> (-5.27%) ⬇️
provisioner/echo/serve.go 56.80% <0.00%> (-2.41%) ⬇️
cli/cliui/provisionerjob.go 76.42% <0.00%> (-2.15%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a95917...71f6f61. Read the comment docs.

coderd/coderd.go Outdated
Comment on lines 45 to 48
// 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
Copy link
Member Author

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.

Copy link
Member

@kylecarbs kylecarbs left a 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.

@Emyrk Emyrk marked this pull request as ready for review April 22, 2022 16:12
@Emyrk Emyrk requested a review from a team as a code owner April 22, 2022 16:12
Copy link
Member

@kylecarbs kylecarbs left a 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!

@@ -13,6 +14,26 @@ import (
// Me is used as a replacement for your own ID.
var Me = uuid.Nil

type PaginatedUsersRequest struct {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

type PaginatedUsers struct {
Pager PagerFields `json:"pager"`
Copy link
Member

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.

Copy link
Member Author

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

@Emyrk Emyrk merged commit 548de7d into main Apr 22, 2022
@Emyrk Emyrk deleted the stevenmasley/paged_users branch April 22, 2022 20:27
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants