-
Notifications
You must be signed in to change notification settings - Fork 132
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
Ensure Pagination and sorting are implemented on the data endpoint #2113
Conversation
b6ae263
to
4442ae1
Compare
a7fcdd9
to
20303c8
Compare
20303c8
to
ad1b77e
Compare
62964f3
to
e2ca6f7
Compare
should_paginate: | ||
# Unpack generator object to list | ||
self.object_list = list(self.object_list) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the pagination headers are skipped if the object_list is a generator object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I assume, from the previously changes I observed a scenerio whereby data returned in JSON format was to be streamed, with no pagination.
I left the else condition to continue to handle such kinds of requests, i.e that the data returned is a generator objects and we do not want to paginate the returned response data.
A good example can be found on this test, we first return a generator object, and we dont want to paginate it, but use the limit flag instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should always paginate requests if requested no matter if we're streaming the data... It's still going to be a pain if a user requests a lot of records while streaming data is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have a few tests that use pagination and stream data
onadata/onadata/apps/api/tests/viewsets/test_data_viewset.py
Lines 653 to 654 in e2ca6f7
@override_settings(STREAM_DATA=True) | |
def test_paginate_and_sort_streaming_data(self): |
6e40760
to
9421052
Compare
if isinstance(self.object_list, types.GeneratorType): | ||
# Unpack generator object to list | ||
self.object_list = list(self.object_list) | ||
elif should_paginate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if it's a generator and the user asked for pagination ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will mostly result to a generator object if sort query param is passed for the JSON result.
If page
and page_size
headers are passed, the result will be unpacked and paginated appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still seems like if it's a generator object the pagination headers won't be included.... Seems like it's unpacked but the pagination headers are not included
01f4fd2
to
5f5ec80
Compare
should_paginate: | ||
if isinstance(self.object_list, types.GeneratorType): | ||
# Unpack generator object to list | ||
self.object_list = list(self.object_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This converts the stream to a list and breaks the stream data functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we paginate when fetching from the db?
29c157c
to
ef3c04b
Compare
ef3c04b
to
9b85544
Compare
Changes / Features implemented
Steps taken to verify this change does what is intended
Side effects of implementing this change
Before submitting this PR for review, please make sure you have:
Closes #2108