improve paging when server count is approximate

Author: jlwalke2

CHANGELOG.md

@@ -1,6 +1,7 @@
 Unreleased
 ----------
- -
+ **Improvements**
+ - `PagedList` handles situations where the server over-estimates the number of items available for paging.
 
  v1.5.5 (2021-03-26)
  -------------------

src/sasctl/core.py

@@ -797,9 +797,6 @@ def __init__(self, obj, session=None, threads=4):
         self._obj = obj
 
     def __next__(self):
-        return self.next()
-
-    def next(self):
         if self._pool is None:
             self._pool = concurrent.futures.ThreadPoolExecutor(max_workers=self._num_threads)
 
@@ -831,6 +828,8 @@ def __iter__(self):
         # All Iterators are also Iterables
         return self
 
+    next = __next__        # Python 2 compatible
+
     def _request_async(self, start):
         """Used by worker threads to retrieve next batch of items."""
 
@@ -847,6 +846,9 @@ def _request_async(self, start):
 class PagedItemIterator:
     """Iterates through a collection that must be "paged" from the server.
 
+    Uses `PageIterator` to transparently download pages of items from the server
+    as needed.
+
     Parameters
     ----------
     obj : RestObj
@@ -862,78 +864,135 @@ class PagedItemIterator:
     ------
     RestObj
 
+    Notes
+    -----
+    Value returned by len() is an approximate count of the items available.  The actual
+    number of items returned may be greater than or less than this number.
+
+    See Also
+    --------
+    PageIterator
+
     """
     def __init__(self, obj, session=None, threads=4):
         # Iterates over whole pages of items
         self._pager = PageIterator(obj, session, threads)
 
+        # Store items from latest page that haven't been returned yet.
+        self._cache = []
+
         # Total number of items to iterate over
         if 'count' in obj:
+            # NOTE: "count" may be an (over) estimate of the number of items available
+            #       since some may be inaccessible due to user permissions & won't actually be returned.
             self._count = int(obj.count)
         else:
             self._count = len(obj['items'])
 
-        self._cache = []
-
     def __len__(self):
         return self._count
 
     def __next__(self):
-        return self.next()
-
-    def next(self):
         # Get next page of items if we're currently out
         if not self._cache:
             self._cache = next(self._pager)
 
+            if len(self._cache) < self._pager._limit:
+                print('oops')
+                # number of items returned in page was less than expected
+                # might be last page, or items might have been filtered out by server.
+
         # Return the next item
         if self._cache:
+            self._count -= 1
             return self._cache.pop(0)
 
-        raise StopIteration
+        raise StopIteration()
+        # Out of items and out of pages
+        # self._count = 0
+        # raise StopIteration
 
     def __iter__(self):
         return self
 
+    next = __next__        # Python 2 compatible
+
+
+class PagedListIterator:
+    """Iterates over an instance of PagedList
+
+    Parameters
+    ----------
+    l : list-like
 
-class PagedListIterator():
+    """
     def __init__(self, l):
         self.__list = l
         self.__index = 0
 
     def __next__(self):
-        return self.next()
-
-    def next(self):
         if self.__index >= len(self.__list):
             raise StopIteration
 
-        item = self.__list[self.__index]
-        self.__index += 1
-        return item
+        try:
+            item = self.__list[self.__index]
+            self.__index += 1
+            return item
+        except IndexError:
+            # Because PagedList length is approximate, iterating can result in
+            # indexing outside the array.  Just stop the iteration if that occurs.
+            raise StopIteration
 
     def __iter__(self):
         return self
 
+    next = __next__     # Python 2 compatibility
+
 
 class PagedList(list):
-    def __init__(self, obj, **kwargs):
+    """List that dynamically loads items from the server.
+
+    Parameters
+    ----------
+    obj : RestObj
+        An instance of `RestObj` containing any initial items and a link to
+        retrieve additional items.
+    session : Session, optional
+        The `Session` instance to use for requesting additional items.  Defaults
+        to current_session()
+    threads : int, optional
+        Number of threads allocated to loading additional items.
+
+    Notes
+    -----
+    Value returned by len() is an approximate count of the items available.  The actual
+    length is not known until all items have been pulled from the server.
+
+    See Also
+    --------
+    PagedItemIterator
+
+    """
+    def __init__(self, obj, session=None, threads=4):
         super(PagedList, self).__init__()
-        self._pager = PagedItemIterator(obj, **kwargs)
+        self._pager = PagedItemIterator(obj, session=session, threads=threads)
 
-        # Force caching of first page
-        self.append(next(self._pager))
-        items = self._pager._cache
-        self.extend(items)
+        # Add the first page of items to the list
+        for _ in range(len(self._pager._cache)):
+            self.append(next(self._pager))
 
-        # clear the list (py27 compatible)
-        del self._pager._cache[:]
+        # Assume that server has more items available
+        self._has_more = True
 
     def __len__(self):
-        return len(self._pager)
+        if self._has_more:
+            # Estimate the total length as items downloaded + items still on server
+            return super(PagedList, self).__len__() + len(self._pager)
+        else:
+            # We've pulled everything from the server, so we have an exact length now.
+            return super(PagedList, self).__len__()
 
     def __iter__(self):
-        # return super(PagedList, self).__iter__()
         return PagedListIterator(self)
 
     def __getslice__(self, i, j):
@@ -949,15 +1008,22 @@ def __getitem__(self, item):
         else:
             idx = int(item)
 
+            # Support negative indexing.  Need to load items up to len() - idx.
+            if idx < 0:
+                idx = len(self) + idx
+
         try:
+            # Iterate through server-side pages until we've loaded
+            # the item at the requested index.
             while super(PagedList, self).__len__() <= idx:
                 n = next(self._pager)
                 self.append(n)
+
         except StopIteration:
-            # May cause if slice or index extends beyond array
-            # Ignore and let List handle IndexErrors if necessary.
-            pass
+            # We've hit the end of the paging so the server has no more items to retrieve.
+            self._has_more = False
 
+        # Get the item from the list
         return super(PagedList, self).__getitem__(item)
 
     def __str__(self):

tests/unit/test_pageditemiterator.py

@@ -9,6 +9,7 @@
 from sasctl.core import PagedItemIterator, RestObj
 from .test_pageiterator import paging
 
+
 def test_no_paging_required():
     """If "next" link not present, current items should be included."""
 
@@ -67,3 +68,51 @@ def test_paging(paging):
     for i, o in enumerate(pager):
         assert RestObj(items[i]) == o
 
+
+def test_paging_inflated_count():
+    """Test behavior when server overestimates the number of items available."""
+    import re
+
+    start = 10
+    limit = 10
+
+    # Only defines 20 items to return
+    pages = [
+        [{'name': x} for x in list('abcdefghi')],
+        [{'name': x} for x in list('klmnopqrs')],
+        [{'name': x} for x in list('uv')],
+    ]
+    actual_num_items = sum(len(page) for page in pages)
+
+    # services (like Files) may overestimate how many items are available.
+    # Simulate that behavior
+    num_items = 23
+
+    obj = RestObj(items=pages[0],
+                  count=num_items,
+                  links=[{'rel': 'next',
+                          'href': '/moaritems?start=%d&limit=%d' % (start, limit)}])
+
+    with mock.patch('sasctl.core.request') as req:
+        def side_effect(_, link, **kwargs):
+            assert 'limit=%d' % limit in link
+            start = int(re.search(r'(?<=start=)[\d]+', link).group())
+            if start == 10:
+                return RestObj(items=pages[1])
+            elif start == 20:
+                return RestObj(items=pages[2])
+            else:
+                return RestObj(items=[])
+
+        req.side_effect = side_effect
+
+        pager = PagedItemIterator(obj, threads=1)
+
+        # Initially, length is estimated based on how many items the server says it has
+        assert len(pager) == num_items
+
+        # Retrieve all of the items
+        items = [x for x in pager]
+
+    assert len(items) == actual_num_items
+    assert len(pager) == num_items - actual_num_items

tests/unit/test_pagedlist.py

@@ -9,7 +9,9 @@
 from sasctl.core import PagedList, RestObj
 from .test_pageiterator import paging
 
+
 def test_len_no_paging():
+    """len() should return the correct # of objects if no paging is required."""
     items = [{'name': 'a'}, {'name': 'b'}, {'name': 'c'}]
     obj = RestObj(items=items, count=len(items))
 
@@ -89,6 +91,80 @@ def test_getitem_paging(paging):
         assert item.name == RestObj(items[i]).name
 
 
+def test_get_item_inflated_len():
+    """Test behavior when server overestimates the number of items available."""
+    import re
+
+    start = 10
+    limit = 10
+
+    # Only defines 20 items to return
+    pages = [
+        [{'name': x} for x in list('abcdefghi')],
+        [{'name': x} for x in list('klmnopqrs')],
+        [{'name': x} for x in list('uv')],
+    ]
+    actual_num_items = sum(len(page) for page in pages)
+
+    # services (like Files) may overestimate how many items are available.
+    # Simulate that behavior
+    num_items = 23
+
+    obj = RestObj(items=pages[0],
+                  count=num_items,
+                  links=[{'rel': 'next',
+                          'href': '/moaritems?start=%d&limit=%d' % (start, limit)}])
+
+    with mock.patch('sasctl.core.request') as req:
+        def side_effect(_, link, **kwargs):
+            assert 'limit=%d' % limit in link
+            start = int(re.search(r'(?<=start=)[\d]+', link).group())
+            if start == 10:
+                return RestObj(items=pages[1])
+            elif start == 20:
+                return RestObj(items=pages[2])
+            else:
+                return RestObj(items=[])
+
+        req.side_effect = side_effect
+        pager = PagedList(obj, threads=1)
+
+        # Initially, length is estimated based on how many items the server says it has available
+        assert len(pager) == num_items
+
+        # Retrieve all of the items
+        items = [x for x in pager]
+
+    # Should have retrieved all of the items
+    assert len(items) == actual_num_items
+
+    # List length should now be updated to indicate the correct number of items
+    assert len(pager) == actual_num_items
+
+    # Recreate the pager
+    with mock.patch('sasctl.core.request') as req:
+        def side_effect(_, link, **kwargs):
+            assert 'limit=%d' % limit in link
+            start = int(re.search(r'(?<=start=)[\d]+', link).group())
+            if start == 10:
+                return RestObj(items=pages[1])
+            elif start == 20:
+                return RestObj(items=pages[2])
+            else:
+                return RestObj(items=[])
+
+        req.side_effect = side_effect
+
+        pager = PagedList(obj, threads=1)
+
+        # Requesting the last item should work & cause loading of all items
+        last_item = pager[-1]
+
+    # Make sure the last item is correct (even though server inflated item count)
+    assert last_item == pages[-1][-1]
+    assert len(pager) == actual_num_items
+
+
 def test_zip_paging(paging):
     """Check that zip() works correctly with the list."""
     obj, items, _ = paging