Enable cursor pagination of value querysets. #4569
Conversation
To do `GROUP_BY` queries in django requires one to use `.values()`
eg this groups posts by user getting a count of posts per user.
```
Posts.objects.order_by('user').values('user').annotate(post_count=Count('post'))
```
This would produce a value queryset which serializes its result
objects as dictionaries while `CursorPagination` requires a queryset
with result objects that are model instances.
This commit enables cursor pagination for value querysets.
- had to mangle the tests a bit to test it out. They might need
some refactoring.
- tried the same for `.values_list()` but it turned out to be
trickier than I expected since you have to use tuple indexes.
| if isinstance(instance, dict): | ||
| attr = instance[field_name] | ||
| else: | ||
| attr = getattr(instance, field_name) |
There was a problem hiding this comment.
Fair enough, good to see that it's a small change. Main Q we need to ask is if we should pull this in for the project, or if we should leave it as a customization for users to make if it is needed.
There was a problem hiding this comment.
The real problem is with the specialization of the .values() queryset (it being an iterable of dicts instead of objects). I imagine that not a lot of people have dealt with this particular predicament unless they need to have an API on top of group bys and so on. Personally, I resorted to overriding this method in my project so maybe having it as a public method capable of being overridden (with docs and so on) could be a simpler fix.
| @@ -703,6 +627,145 @@ def test_cursor_pagination(self): | |||
| assert isinstance(self.pagination.to_html(), type('')) | |||
|
|
|||
|
|
|||
| class TestCursorPagination(CursorPaginationTestsMixin): | |||
There was a problem hiding this comment.
The large change footprint on these tests makes them v difficult to review. Is there any way around we could do this that ended up with a smaller review to judge this on?
There was a problem hiding this comment.
I tried several ways of testing this; first added another mock object that represented a value queryset object but that added a lot of ifs into the mock queryset and get_pages; then I ended up duplicating the entire test class but that would have duplicated the tests as well; finally ended up extracting out the tests into a mixin and mixing it into the unit tests and value queryset tests. At this point I'm open to any suggestions.
|
Good work. Thanks! |
|
Wohoo! You're welcome. |
To do
GROUP_BYqueries in django requires one to use.values()eg this groups posts by user getting a count of posts per user.
This would produce a value queryset which serializes its result
objects as dictionaries while
CursorPaginationrequires a querysetwith result objects that are model instances.
This commit enables cursor pagination for value querysets.
some refactoring.
.values_list()but it turned out to betrickier than I expected since you have to use tuple indexes.