DAO Pagination (GSI-2412)#231
Conversation
Coverage Report for CI Build 27284487376Coverage increased (+0.009%) to 93.308%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Cito
left a comment
Cito
left a comment
There was a problem hiding this comment.
Thank you, that looks useful.
I have pointed out two edge cases that need some refinement, and made one suggestions regarding the sort spec.
| A FindResult that is async-iterable and also provides total_count(). | ||
| """ | ||
| skip = skip or 0 | ||
| limit = limit or 0 |
There was a problem hiding this comment.
maybe add a comment that limit=0 means unbounded in mongo (not all db engines do this)
| if sort: | ||
| cursor = cursor.sort(sort) | ||
| async for document in cursor.skip(skip).limit(limit): | ||
| if document.get("__metadata__", {}).get("deleted", False): |
There was a problem hiding this comment.
here you filter after skipping and limiting - it should be the other way round
i guess the most performant way is to add a filter for (metadata missing or metadata.deleted false) upfront.
There was a problem hiding this comment.
Ugh, yes, I don't know how I didn't catch that. Good spot
| Number of matching resources to skip before yielding results. | ||
| Defaults to None (no skipping). | ||
| limit: | ||
| Maximum number of resources to yield. Defaults to None (no limit). |
There was a problem hiding this comment.
The docstring should also define what limit=0 means - unbounded (as in mongo) or literally 0 (which I prefer since it#s much more intuitive). The implementation is currently for the former, the latter would need slight adjustment for mongo.
There was a problem hiding this comment.
What is the use case for having a limit of literally 0?
There was a problem hiding this comment.
Passing a value of 0 is not really useful (if you want unbound you can already just pass nothing/None). My main point is that the behavior should be well specified in the docstring. Options are 1) raise error, 2) do exactly what was requested (return nothing) or 3) re-interpret as "infinity". I think 2) is the most obvious one - Django ORM, Python slicing, Postgres and MySQL all work that way.
| results = [x.title async for x in dao.find_all(mapping={}, skip=10, sort=asc_sort)] | ||
| assert results == [] | ||
|
|
||
| # skip=0, limit=0 behaves like no pagination (limit=0 means no limit) |
There was a problem hiding this comment.
we should test both, limit=0 and limit=None and check that they do what we say in the docstring (for all providers)
|
|
||
| with pytest.raises(UniqueConstraintViolationError): | ||
| await dao.upsert(dto5) | ||
|
|
There was a problem hiding this comment.
Here we should also add some pagination tests after adding raw documents with no metadata at all, and metadata.deleted = True
| ] | ||
|
|
||
| SortSpec = list[tuple[str, Literal[1, -1]]] | ||
|
|
There was a problem hiding this comment.
We could use simple simple strings with a "-" prefix for descending order - that's how the Django ORM does it, so it's a well-established pattern in Python.
3 participants
The DAO protocol and providers now expose pagination parameters in
find_all():sort,skip, andlimit. Additionally, the result offind_all()exposes a.total_count()method that returns the total number of items matching the filter regardless of skip/limit. Together, these should enable both offset-based and cursor-based pagination.Bumps the version to
8.3.0