-
-
Notifications
You must be signed in to change notification settings - Fork 227
Enterprise Form Submissions Iterators #35295
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
Changes from 13 commits
b9d3b6d
463d97b
3eccde7
5154360
4112456
399b013
185a143
05eaa9a
2504668
dd334de
080d837
09c104b
409f725
bff5fac
c65f0b6
593882c
bee7055
7082597
b85a5f5
2d9d74b
dedb429
d489a36
c5d96fb
e65970d
7d0e897
18faad3
e55664e
4cce055
eb67336
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
from itertools import islice | ||
from django.http.request import QueryDict | ||
from urllib.parse import urlencode | ||
from tastypie.paginator import Paginator | ||
|
||
|
||
class KeysetPaginator(Paginator): | ||
''' | ||
An alternate paginator meant to support paginating by keyset rather than by index/offset. | ||
`objects` is expected to represent a query object that exposes an `.execute(limit)` | ||
method that returns an iterable, and a `get_query_params(object)` method to retrieve the parameters | ||
for the next query | ||
Because keyset pagination does not efficiently handle slicing or offset operations, | ||
these methods have been intentionally disabled | ||
''' | ||
def __init__(self, request_data, objects, | ||
resource_uri=None, limit=None, max_limit=1000, collection_name='objects'): | ||
super().__init__( | ||
request_data, | ||
objects, | ||
resource_uri=resource_uri, | ||
limit=limit, | ||
max_limit=max_limit, | ||
collection_name=collection_name | ||
) | ||
|
||
def get_offset(self): | ||
raise NotImplementedError() | ||
|
||
def get_slice(self, limit, offset): | ||
raise NotImplementedError() | ||
|
||
def get_count(self): | ||
raise NotImplementedError() | ||
|
||
def get_previous(self, limit, offset): | ||
raise NotImplementedError() | ||
|
||
def get_next(self, limit, **next_params): | ||
return self._generate_uri(limit, **next_params) | ||
|
||
def _generate_uri(self, limit, **next_params): | ||
if self.resource_uri is None: | ||
return None | ||
|
||
if isinstance(self.request_data, QueryDict): | ||
# Because QueryDict allows multiple values for the same key, we need to remove existing values | ||
# prior to updating | ||
request_params = self.request_data.copy() | ||
if 'limit' in request_params: | ||
del request_params['limit'] | ||
for key in next_params: | ||
if key in request_params: | ||
del request_params[key] | ||
|
||
request_params.update({'limit': str(limit), **next_params}) | ||
encoded_params = request_params.urlencode() | ||
else: | ||
request_params = {} | ||
for k, v in self.request_data.items(): | ||
if isinstance(v, str): | ||
request_params[k] = v.encode('utf-8') | ||
else: | ||
request_params[k] = v | ||
|
||
request_params.update({'limit': limit, **next_params}) | ||
encoded_params = urlencode(request_params) | ||
|
||
return '%s?%s' % ( | ||
self.resource_uri, | ||
encoded_params | ||
) | ||
|
||
def page(self): | ||
""" | ||
Generates all pertinent data about the requested page. | ||
""" | ||
limit = self.get_limit() | ||
padded_limit = limit + 1 if limit else limit | ||
# Fetch 1 more record than requested to allow us to determine if further queries will be needed | ||
it = iter(self.objects.execute(limit=padded_limit)) | ||
objects = list(islice(it, limit)) | ||
|
||
try: | ||
next(it) | ||
has_more = True | ||
except StopIteration: | ||
has_more = False | ||
|
||
meta = { | ||
'limit': limit, | ||
} | ||
|
||
if limit and has_more: | ||
last_fetched = objects[-1] | ||
next_page_params = self.objects.get_query_params(last_fetched) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check if there is a missing test for this? didn't fail for misnamed method name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned in another comment, the test is generic |
||
meta['next'] = self.get_next(limit, **next_page_params) | ||
|
||
return { | ||
self.collection_name: objects, | ||
'meta': meta, | ||
} | ||
|
||
|
||
class PageableQueryInterface: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this still being used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was unused. Removed |
||
def execute(limit=None): | ||
''' | ||
Should return an iterable that exposes a `.get_query_params()` method | ||
''' | ||
raise NotImplementedError() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
from django.test import SimpleTestCase | ||
from django.http import QueryDict | ||
from corehq.apps.api.keyset_paginator import KeysetPaginator | ||
|
||
|
||
class SequenceQuery: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what model is this supposed to emulate in the real code? can you add a comment there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking on this further, would it help to make My impression is that Python prefers to work with duck-typing rather than explicit class inheritance, but I can switch if that would make it easier to understand this code |
||
def __init__(self, seq): | ||
self.seq = seq | ||
|
||
def execute(self, limit=None): | ||
return self.seq | ||
|
||
@classmethod | ||
def get_query_params(cls, form): | ||
return {'next': form} | ||
|
||
|
||
class KeysetPaginatorTests(SimpleTestCase): | ||
def test_page_fetches_all_results_below_limit(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects, limit=10) | ||
page = paginator.page() | ||
self.assertEqual(page['objects'], [0, 1, 2, 3, 4]) | ||
self.assertEqual(page['meta'], {'limit': 10}) | ||
|
||
def test_page_includes_next_information_when_more_results_are_available(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects, resource_uri='http://test.com/', limit=3) | ||
page = paginator.page() | ||
self.assertEqual(page['objects'], [0, 1, 2]) | ||
self.assertEqual(page['meta'], {'limit': 3, 'next': 'http://test.com/?limit=3&next=2'}) | ||
|
||
def test_does_not_include_duplicate_limits(self): | ||
request_data = QueryDict(mutable=True) | ||
request_data['limit'] = 3 | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(request_data, objects, resource_uri='http://test.com/') | ||
page = paginator.page() | ||
self.assertEqual(page['meta']['next'], 'http://test.com/?limit=3&next=2') | ||
|
||
def test_supports_dict_request_data(self): | ||
request_data = { | ||
'limit': 3, | ||
'some_param': 'yes' | ||
} | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(request_data, objects, resource_uri='http://test.com/') | ||
page = paginator.page() | ||
self.assertEqual(page['meta']['next'], 'http://test.com/?limit=3&some_param=yes&next=2') | ||
|
||
def test_get_offset_not_implemented(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects) | ||
|
||
with self.assertRaises(NotImplementedError): | ||
paginator.get_offset() | ||
|
||
def test_get_slice_not_implemented(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects) | ||
|
||
with self.assertRaises(NotImplementedError): | ||
paginator.get_slice(limit=10, offset=20) | ||
|
||
def test_get_count_not_implemented(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects) | ||
|
||
with self.assertRaises(NotImplementedError): | ||
paginator.get_count() | ||
|
||
def test_get_previous_not_implemented(self): | ||
objects = SequenceQuery(range(5)) | ||
paginator = KeysetPaginator(QueryDict(), objects) | ||
|
||
with self.assertRaises(NotImplementedError): | ||
paginator.get_previous(limit=10, offset=20) |
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.
potentially rename limit to indicate page size as it is used?
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.
Addressed in eb67336