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(api): Add 'count' parameter to return total item count in API v4 #4715

Merged
merged 18 commits into from
Nov 27, 2024

Conversation

elisa-a-v
Copy link
Contributor

Resolves #4506

This PR introduces a new count=on query parameter to our paginated API endpoints. When this parameter is included in a request, the API returns only the total count of items matching the query filters, without including any result data.

If no count=on is added to the request, a count key is now included in the response just like in v3, but instead of providing the count immediately, it provides a URL that points to the count endpoint for the current query parameters to guide users on how to obtain the total count if needed.

V4 removed the count due to the overhead of counting huge querysets in each request, which is why we're adding this tweaked version of the count in the response so the users have access to the information if needed without reintroducing the overhead.

Also updated docs to reflect these changes:
image

- When `count=on` is specified in a paginated list endpoint query params, the API returns only the total count of items matching the query
- In standard v4 paginated responses, include a `count` key with a URL to get the total count
- This feature helps users verify if their filters are working by easily accessing the total count
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

Looks nice!

One thought that might be helpful is to confirm the number and type of queries via a test. Could you add that?

The rest looks great to me, if it looks good to other folks.

@mlissner mlissner removed the request for review from ERosendo November 21, 2024 06:50
@mlissner
Copy link
Member

Process notes:

  • I assigned this to Alberto since he worked in this area most recently. It's probably best if I continue doing this for PRs.
  • I set him as reviewer and removed Eduardo. It's probably best if I continue doing this for PRs.
  • I added it to the current sprint and put it in the To Do column. It's probably best if you do this in the future. :)

Thank you!

Copy link
Contributor

@albertisfu albertisfu left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me.

I just left a couple of minor comments.

I also agree with Mike's suggestion to add or tweak the current tests to verify the number and type of queries performed during a regular request and a count request. To ensure that no count query is executed during regular requests, and only the count query is executed during count requests. I think you can base on ApiQueryCountTests

Thank you!

cl/api/pagination.py Outdated Show resolved Hide resolved
cl/api/tests.py Outdated Show resolved Hide resolved
cl/api/tests.py Outdated Show resolved Hide resolved
cl/api/tests.py Outdated Show resolved Hide resolved
cl/api/templates/rest-docs-vlatest.html Outdated Show resolved Hide resolved
@albertisfu albertisfu assigned elisa-a-v and unassigned albertisfu Nov 21, 2024
@elisa-a-v
Copy link
Contributor Author

Process notes:

  • I assigned this to Alberto since he worked in this area most recently. It's probably best if I continue doing this for PRs.
  • I set him as reviewer and removed Eduardo. It's probably best if I continue doing this for PRs.
  • I added it to the current sprint and put it in the To Do column. It's probably best if you do this in the future. :)

That makes sense, so from now on I'll just add you as a reviewer once a PR is ready for review, and I'll add it to the current sprint 🫡

@elisa-a-v
Copy link
Contributor Author

Thank you @albertisfu and @mlissner for the comments! I'm working on the tests and I think the class you mentioned (ApiQueryCountTests) seems like the right place to add the new tests, do you agree?

@albertisfu
Copy link
Contributor

I'm working on the tests and I think the class you mentioned (ApiQueryCountTests) seems like the right place to add the new tests, do you agree?

Yeah, I agree. that's a good place to add the new tests!

@elisa-a-v elisa-a-v requested a review from albertisfu November 25, 2024 17:14
@elisa-a-v elisa-a-v assigned albertisfu and unassigned elisa-a-v Nov 25, 2024
Copy link
Contributor

@albertisfu albertisfu left a comment

Choose a reason for hiding this comment

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

Thanks @elisa-a-v the changes and the API count tests added look great.

We can merge this one @mlissner

@mlissner mlissner enabled auto-merge November 27, 2024 19:00
@mlissner mlissner merged commit f004aa8 into main Nov 27, 2024
15 checks passed
@mlissner mlissner deleted the 4506-restore-count-v4 branch November 27, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add count API or parameter
3 participants