feat: add limit and offset parameters to routes that fetch lists from db #277
Conversation
- add limit and offset parameters with descriptions, to all routes that fetch lists from db (fix #243, #250) - use crud.fetch_all in all the routes above, to limit the number of items returned for non-admin users (fix #276) - forbid /installation/site-devices to access site outside of group, instead of filtering devices - improve test_installations.py (test_get_active_devices_on_site) - Dockerfile-dev: update FROM image name
|
Nice of you to open a PR 🙏 |
Codecov Report
@@ Coverage Diff @@
## main #277 +/- ##
==========================================
+ Coverage 95.12% 95.28% +0.16%
==========================================
Files 63 66 +3
Lines 1497 1549 +52
==========================================
+ Hits 1424 1476 +52
Misses 73 73
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/quack review |
There was a problem hiding this comment.
Thanks for the PR 🙏
Other sections
src/tests/routes/test_groups.py:L121For a better experience, it's recommended to check the review comments in the tab Files Changed.
Feedback is a gift! Quack will adjust your review style when you add reactions to a review comment
There was a problem hiding this comment.
Thanks for the PR! I added a few comments, but the overall one is: do we want to paginate query results or allow users to specify limits? (in the last case, if a large value is selected, that can blow up the RAM. even though we might not have a lot of malicious users around)
I saw something a few months ago to paginate easily in fastapi: https://uriyyo-fastapi-pagination.netlify.app/
What do you think?
| offset: Optional[int] = None, | ||
| query: Optional[Select] = None, | ||
| ) -> List[Mapping[str, Any]]: | ||
| query = table.select().order_by(table.c.id.desc()) | ||
| if query is None: | ||
| query = table.select() | ||
| if isinstance(query_filters, dict): | ||
| for key, value in query_filters.items(): | ||
| query = query.where(getattr(table.c, key) == value) | ||
|
|
||
| if isinstance(exclusions, dict): | ||
| for key, value in exclusions.items(): | ||
| query = query.where(getattr(table.c, key) != value) | ||
| return (await database.fetch_all(query=query.limit(limit)))[::-1] | ||
| query = query.order_by(table.c.id.desc()).limit(limit).offset(offset) | ||
| if isinstance(query, Query): | ||
| return [item.__dict__ for item in query[::-1]] | ||
| return (await database.fetch_all(query=query))[::-1] |
There was a problem hiding this comment.
Good idea. Two questions though:
- I think I understand this is an equivalent of the offset syntax in SQL. But here we seem to be making an arbitrary choice on order direction. Should we make that a boolean param?
- regarding the query arg, we expect it to already have a
.select()?
If we only want to paginate the queries, there are a few plugin options for this I think
There was a problem hiding this comment.
(paginate will prevent the case where we have a huge table and someone send limit=100000 which might blow up the RAM)
| async def fetch_accesses(_=Security(get_current_access, scopes=[AccessType.admin])): | ||
| async def fetch_accesses( | ||
| limit: Annotated[int, Query(description="maximum number of items", ge=1, le=1000)] = 50, | ||
| offset: Annotated[Optional[int], Query(description="number of items to skip", ge=0)] = None, | ||
| _=Security(get_current_access, scopes=[AccessType.admin]), | ||
| ): |
There was a problem hiding this comment.
Just a reminder for me: how do we change those values when sending a request?
This PR adds limit and offset parameters with descriptions, to all routes that fetch lists from db (fix #243, fix #250). Also: