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

feat: add count to get users endpoint #5016

Merged
merged 17 commits into from
Nov 14, 2022
Merged

feat: add count to get users endpoint #5016

merged 17 commits into from
Nov 14, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Nov 10, 2022

What this changes:

  • Removes the GET /api/v2/users/count endpoint and removes usage from the dashboard
  • Adds count field to the GET /api/v2/users response
  • Uses a window function to get count in single query from postgres

This will ideally be the template for paginated queries moving forward.

@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 10, 2022

@coadler would this work? I will go further with this if there aren't any immediate concern.

@f0ssel f0ssel requested a review from a team as a code owner November 14, 2022 17:00
@f0ssel f0ssel requested review from BrunoQuaresma and removed request for a team November 14, 2022 17:00
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Nice changes!

@f0ssel f0ssel changed the title feat: add count to queries in window function feat: add count to get users endpoint Nov 14, 2022
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.

@coadler pls review. I'm pretty packed today.

@f0ssel f0ssel requested a review from presleyp November 14, 2022 20:07
Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

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

Left some comments but it looks good and it's working for me!

Comment on lines +216 to +224
// GetUsers does not return ErrNoRows because it uses a window function to get the count.
// So we need to check if the userRows is empty and return an empty array if so.
if len(userRows) == 0 {
httpapi.Write(ctx, rw, http.StatusOK, codersdk.GetUsersResponse{
Users: []codersdk.User{},
Count: 0,
})
return
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it ever could return that error anyways since it returns an array, and only :one queries will error. I think this simplifies the code below though, which is nice.

@f0ssel f0ssel merged commit 88f3691 into main Nov 14, 2022
@f0ssel f0ssel deleted the f0ssel/try-window branch November 14, 2022 22:22
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants