Pagination fixes

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,27 +174,32 @@ 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:
       return Pagination(client, _FindPartitionPageIterator, 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
 

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()

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)

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')

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={})