diff --git a/ibmcloudant/features/pagination.py b/ibmcloudant/features/pagination.py index ce97cde9..3ef485c5 100644 --- a/ibmcloudant/features/pagination.py +++ b/ibmcloudant/features/pagination.py @@ -41,6 +41,8 @@ _MAX_LIMIT = 200 _MIN_LIMIT = 1 +_DOCS_KEY_ERROR = "No need to paginate as 'key' returns a single result for an ID." +_VIEW_KEY_ERROR = "Use 'start_key' and 'end_key' instead." class PagerType(Enum): """ @@ -148,12 +150,17 @@ def _validate_limit(cls, opts: dict): if limit < _MIN_LIMIT: raise ValueError(f'The provided limit {limit} is lower than the minimum page size value of {_MIN_LIMIT}.') + @classmethod + def _validate_option_absent(cls, invalid_opt: str, opts: dict, message_suffix: Optional[str]=None): + # check if the invalid_opt is present in opts dict + if invalid_opt in opts: + raise ValueError(f"The option '{invalid_opt}' is invalid when using pagination.{' ' + message_suffix if message_suffix else ''}") + @classmethod def _validate_options_absent(cls, invalid_opts: Sequence[str], opts: dict): # for each invalid_opts entry check if it is present in opts dict for invalid_opt in invalid_opts: - if invalid_opt in opts: - raise ValueError(f"The option '{invalid_opt}' is invalid when using pagination.") + cls._validate_option_absent(invalid_opt, opts) @classmethod def new_pagination(cls, client:CloudantV1, type: PagerType, **kwargs): @@ -167,14 +174,17 @@ def new_pagination(cls, client:CloudantV1, type: PagerType, **kwargs): # Validate the limit cls._validate_limit(kwargs) if type == PagerType.POST_ALL_DOCS: + cls._validate_option_absent('key', kwargs, _DOCS_KEY_ERROR) cls._validate_options_absent(('keys',), kwargs) return Pagination(client, _AllDocsPageIterator, kwargs) if type == PagerType.POST_DESIGN_DOCS: + cls._validate_option_absent('key', kwargs, _DOCS_KEY_ERROR) cls._validate_options_absent(('keys',), kwargs) return Pagination(client, _DesignDocsPageIterator, kwargs) if type == PagerType.POST_FIND: return Pagination(client, _FindPageIterator, kwargs) if type == PagerType.POST_PARTITION_ALL_DOCS: + cls._validate_option_absent('key', kwargs, _DOCS_KEY_ERROR) cls._validate_options_absent(('keys',), kwargs) return Pagination(client, _AllDocsPartitionPageIterator, kwargs) if type == PagerType.POST_PARTITION_FIND: @@ -182,12 +192,14 @@ def new_pagination(cls, client:CloudantV1, type: PagerType, **kwargs): if type == PagerType.POST_PARTITION_SEARCH: return Pagination(client, _SearchPartitionPageIterator, kwargs) if type == PagerType.POST_PARTITION_VIEW: + cls._validate_option_absent('key', kwargs, _VIEW_KEY_ERROR) cls._validate_options_absent(('keys',), kwargs) return Pagination(client, _ViewPartitionPageIterator, kwargs) if type == PagerType.POST_SEARCH: cls._validate_options_absent(('counts', 'group_field', 'group_limit', 'group_sort', 'ranges',), kwargs) return Pagination(client, _SearchPageIterator, kwargs) if type == PagerType.POST_VIEW: + cls._validate_option_absent('key', kwargs, _VIEW_KEY_ERROR) cls._validate_options_absent(('keys',), kwargs) return Pagination(client, _ViewPageIterator, kwargs) @@ -199,8 +211,8 @@ class _IteratorPagerState(Enum): class _IteratorPager(Pager[I]): - _state_mixed_msg = 'This pager has been consumed, use a new Pager.' - _state_consumed_msg = 'Cannot mix get_all() and get_next() use only one method or make a new Pager.' + _state_consumed_msg = 'This pager has been consumed, use a new Pager.' + _state_mixed_msg = 'Cannot mix get_all() and get_next() use only one method or make a new Pager.' def __init__(self, iterable_func: Callable[[], Iterator[Sequence[I]]]): self._iterable_func: Callable[[], Iterator[Sequence[I]]] = iterable_func @@ -251,7 +263,7 @@ class _BasePageIterator(Iterator[Sequence[I]]): def __init__(self, client: CloudantV1, operation: Callable[..., DetailedResponse], - page_opts: list[str], + page_opts: Sequence[str], opts: dict): self._client: CloudantV1 = client self._has_next: bool = True @@ -306,7 +318,7 @@ def _get_next_page_options(self, result: R) -> dict: class _KeyPageIterator(_BasePageIterator, Generic[K]): def __init__(self, client: CloudantV1, operation: Callable[..., DetailedResponse], opts: dict): - super().__init__(client, operation, ['start_key', 'start_key_doc_id'], opts) + super().__init__(client, operation, ('skip', 'start_key', 'start_key_doc_id',), opts) self._boundary_failure: Optional[str] = None def _next_request(self) -> list[I]: @@ -341,8 +353,8 @@ def check_boundary(self, penultimate_item: I, last_item: I) -> Optional[str]: class _BookmarkPageIterator(_BasePageIterator): - def __init__(self, client: CloudantV1, operation: Callable[..., DetailedResponse], opts: dict): - super().__init__(client, operation, ['bookmark'], opts) + def __init__(self, client: CloudantV1, operation: Callable[..., DetailedResponse], opts: dict, extra_page_opts:Sequence[str]=()): + super().__init__(client, operation, ('bookmark',) + extra_page_opts, opts) def _get_next_page_options(self, result: R) -> dict: return {'bookmark': result.bookmark} @@ -379,6 +391,12 @@ def __init__(self, client: CloudantV1, opts: dict): class _FindBasePageIterator(_BookmarkPageIterator): + def __init__(self, client: CloudantV1, operation: Callable[..., DetailedResponse], opts: dict): + # Find requests allow skip, but it should only be used on the first request. + # Since we don't want it on subsequent page requests we need to exclude it from + # fixed opts used for the partial function. + super().__init__(client, operation, opts, extra_page_opts=('skip',)) + def _items(self, result: FindResult): return result.docs diff --git a/test/unit/features/test_pagination_base.py b/test/unit/features/test_pagination_base.py index 9a3caa29..77d2a16f 100644 --- a/test/unit/features/test_pagination_base.py +++ b/test/unit/features/test_pagination_base.py @@ -15,6 +15,7 @@ # limitations under the License. from collections.abc import Callable +from re import escape from typing import Iterable from unittest.mock import Mock, patch from ibmcloudant.cloudant_v1 import ViewResult, ViewResultRow @@ -310,7 +311,7 @@ def test_as_pager_get_all(self): actual_items = pager.get_all() self.assertSequenceEqual(actual_items, mock.all_expected_items(), "The results should match all the pages.") # Assert consumed state prevents calling again - with self.assertRaises(Exception, msg=_IteratorPager._state_consumed_msg): + with self.assertRaisesRegex(Exception, escape(_IteratorPager._state_consumed_msg)): pager.get_all() def test_as_pager_get_all_restarts_after_error(self): @@ -332,7 +333,7 @@ def test_as_pager_get_all_restarts_after_error(self): pager.get_all() self.assertSequenceEqual(pager.get_all(), mock.all_expected_items(), "The results should match all the pages.") - def test_as_pager_get_next_get_all_throws(self): + def test_as_pager_get_next_get_all_raises(self): page_size = 11 # Mock that returns 6 pages of 11 items, then 1 more page with 5 items mock = BasePageMockResponses(71, page_size) @@ -341,13 +342,13 @@ def test_as_pager_get_next_get_all_throws(self): pager: Pager[ViewResultRow] = pagination.pager() first_page = pager.get_next() self.assertSequenceEqual(first_page, mock.get_expected_page(1), "The actual page should match the expected page") - # Assert throws - with self.assertRaises(Exception, msg=_IteratorPager._state_mixed_msg): + # Assert raises + with self.assertRaisesRegex(Exception, escape(_IteratorPager._state_mixed_msg)): pager.get_all() # Assert second page ok self.assertSequenceEqual(pager.get_next(), mock.get_expected_page(2), "The actual page should match the expected page") - def test_as_pager_get_all_get_next_throws(self): + def test_as_pager_get_all_get_next_raises(self): page_size = 1 mock = BasePageMockResponses(2*page_size, page_size) first_page = mock.get_next_page() @@ -363,8 +364,8 @@ def test_as_pager_get_all_get_next_throws(self): # Stop get all part way through so it isn't consumed when we call get Next with self.assertRaises(Exception): pager.get_all() - # Assert calling get_next() throws - with self.assertRaises(Exception, msg=_IteratorPager._state_mixed_msg): + # Assert calling get_next() raises + with self.assertRaisesRegex(Exception, escape(_IteratorPager._state_mixed_msg)): pager.get_next() def test_as_pager_get_next_resumes_after_error(self): @@ -401,5 +402,5 @@ def test_as_pager_get_next_until_consumed(self): # Note 3 because third page is empty self.assertEqual(page_count, 3, 'There should be the expected number of pages.') # Assert consumed state prevents calling again - with self.assertRaises(Exception, msg=_IteratorPager._state_consumed_msg): + with self.assertRaisesRegex(Exception, escape(_IteratorPager._state_consumed_msg)): pager.get_next() diff --git a/test/unit/features/test_pagination_bookmark.py b/test/unit/features/test_pagination_bookmark.py index 7ad4c952..e9ede072 100644 --- a/test/unit/features/test_pagination_bookmark.py +++ b/test/unit/features/test_pagination_bookmark.py @@ -28,7 +28,7 @@ class BookmarkTestPageIterator(_BookmarkPageIterator): boundary_func: Callable = lambda p,l: None def __init__(self, client, opts): - super().__init__(client, BookmarkTestPageIterator.operation or client.post_view, opts) + super().__init__(client, BookmarkTestPageIterator.operation or client.post_search, opts) def _result_converter(self) -> Callable[[dict], SearchResult]: return lambda d: SearchResult.from_dict(d) diff --git a/test/unit/features/test_pagination_operations.py b/test/unit/features/test_pagination_operations.py index 16383431..e1b90c81 100644 --- a/test/unit/features/test_pagination_operations.py +++ b/test/unit/features/test_pagination_operations.py @@ -191,3 +191,17 @@ def test_rows_errors(self): for row in Pagination.new_pagination(self.client, pager_type, limit=page_size).rows(): actual_item_count += 1 self.assertEqual(actual_item_count, expected_item_count, 'Should have got the correct number of items before error.') + + # Test skip omitted from subsequent page requests + # Applies to key pagers and find pagers + def test_skip_removed_for_subsquent_page(self): + page_size = 14 + for pager_type in (PaginationMockSupport.key_pagers + PaginationMockSupport.find_pagers): + with self.subTest(pager_type): + with patch(PaginationMockSupport.operation_map[pager_type], PaginationMockResponse(2*page_size, page_size, pager_type).get_next_page): + pager = Pagination.new_pagination(self.client, pager_type, limit=page_size, skip=1).pager() + # Assert first page has skip option + self.assertEqual(pager._iterator._next_page_opts['skip'], 1, 'The skip option should be 1 for the first page') + pager.get_next() + # Assert second page has no skip option + self.assertIsNone(pager._iterator._next_page_opts.get('skip'), 'The skip option should be absent for the next page') diff --git a/test/unit/features/test_pagination_option_validation.py b/test/unit/features/test_pagination_option_validation.py index c694262d..c2716425 100644 --- a/test/unit/features/test_pagination_option_validation.py +++ b/test/unit/features/test_pagination_option_validation.py @@ -15,7 +15,7 @@ # limitations under the License. from unittest import TestCase -from ibmcloudant.features.pagination import _MIN_LIMIT, _MAX_LIMIT, PagerType, Pagination +from ibmcloudant.features.pagination import _DOCS_KEY_ERROR, _MIN_LIMIT, _MAX_LIMIT, _VIEW_KEY_ERROR, PagerType, Pagination class TestPaginationOptionValidation(TestCase): @@ -39,19 +39,35 @@ def test_valid_limits(self): def test_invalid_limits(self): test_limits = (_MIN_LIMIT - 1, _MAX_LIMIT + 1) for limit in test_limits: + if (limit == _MIN_LIMIT -1): + msg_regex = f'The provided limit {limit} is lower than the minimum page size value of 1.' + elif (limit == _MAX_LIMIT + 1): + msg_regex= f'The provided limit {limit} exceeds the maximum page size value of 200.' for pager_type in self.all_paginations: with self.subTest(pager_type): - with self.assertRaises(ValueError, msg='There should be a ValueError for invalid limits.'): + with self.assertRaisesRegex(ValueError, msg_regex): Pagination.new_pagination(None, pager_type, limit=limit) def test_keys_value_error_for_view_like(self): for pager_type in self.view_like_paginations: with self.subTest(pager_type): - with self.assertRaises(ValueError, msg=f'There should be a ValueError for {pager_type} with keys.'): + with self.assertRaisesRegex(ValueError, 'The option \'keys\' is invalid when using pagination.'): Pagination.new_pagination(None, pager_type, keys=['a','b','c']) def test_facet_value_errors_for_search(self): for invalid_opt in ('counts', 'group_field', 'group_limit', 'group_sort', 'ranges',): with self.subTest(invalid_opt): - with self.assertRaises(ValueError, msg=f'There should be a ValueError for search with option {invalid_opt}.'): + with self.assertRaisesRegex(ValueError, f'The option \'{invalid_opt}\' is invalid when using pagination.'): Pagination.new_pagination(None, PagerType.POST_SEARCH, **{invalid_opt: 'test value'}) + + def test_key_value_error_for_docs(self): + for pager_type in self.all_doc_paginations: + with self.subTest(pager_type): + with self.assertRaisesRegex(ValueError, f'The option \'key\' is invalid when using pagination. {_DOCS_KEY_ERROR}'): + Pagination.new_pagination(None, pager_type, key='a') + + def test_key_value_error_for_views(self): + for pager_type in self.view_paginations: + with self.subTest(pager_type): + with self.assertRaisesRegex(ValueError, f'The option \'key\' is invalid when using pagination. {_VIEW_KEY_ERROR}'): + Pagination.new_pagination(None, pager_type, key={})