Repository: googleapis/python-api-core (python)

Issue:
The page_iterator API proble, and proposed fix
It looks to me like the [page_iterator](https://github.com/googleapis/python-api-core/blob/f2600cce8bc8eaf24c2461908cfb5a2e9b489296/google/api_core/page_iterator.py) API has a problem. 

The `Iterator` class provides pagination, and yet invites clients to do their own pagination by taking a page token.  (The documentation for `page_token` in the BigQuery [list_tables](https://googleapis.dev/python/bigquery/latest/generated/google.cloud.bigquery.client.Client.html?highlight=list_tables#google.cloud.bigquery.client.Client.list_tables) method specifically says this.)  I would hope that this isn't the intent.

Empirically, the default pagination for `list_tables` is 50 rows per page, [which is arguably too low](https://github.com/googleapis/python-bigquery-sqlalchemy/issues/173), causing many REST API calls if there are many tables in a dataset.  If you pass a largish number for `max_results`, then the page size increases to 1000 rows, but no more than 1000 and you get no more than that many results.  Using `max_results` to influence the page size forces the client of `page_iterator` to do its own pagination if there is any chance of total number of items exceeding the maximum `max_results`, which for `list_tables` is 2147483647.  Arguably, a dataset wouldn't have more than that many tables, but this interface  is also used for [list_rows](https://googleapis.dev/python/bigquery/latest/generated/google.cloud.bigquery.client.Client.html?highlight=list_rows#google.cloud.bigquery.client.Client.list_rows), which also limits `max_results` to 2147483647.  One wouldn't want to limit table rows to 2147483647 just to affect the pagination, although empirically, `max_results` doesn't affect pagination in the case of `list_rows`. 

A straightforward way to address this would be to add a `page_size` argument to the iterator. Of course, to get the benefit, the option would need to be added to higher-level libraries.

**I'd be happy to create a PR to add this argument.**

BTW, it's weird to take `page_token`.  Is this a holdover from an earlier design? Should it be deprecated?



Pull request:
feat: HTTPIterator now accepts a page_size parameter to control page …
…size

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-api-core/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [x] Appropriate docs were updated (if necessary)

Fixes #194  🦕


Patch:
diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml
index 127c2cd..da616c9 100644
--- a/.github/.OwlBot.lock.yaml
+++ b/.github/.OwlBot.lock.yaml
@@ -1,3 +1,3 @@
 docker:
   image: gcr.io/repo-automation-bots/owlbot-python:latest
-  digest: sha256:0856ca711da1fd5ec9d6d7da6c50aa0bbf550fb94acb47b55159a640791987bf
+  digest: sha256:c66ba3c8d7bc8566f47df841f98cd0097b28fff0b1864c86f5817f4c8c3e8600
diff --git a/docs/conf.py b/docs/conf.py
index 9ec0452..1e73c44 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -366,6 +366,7 @@ intersphinx_mapping = {
     ),
     "grpc": ("https://grpc.github.io/grpc/python/", None),
     "proto-plus": ("https://proto-plus-python.readthedocs.io/en/latest/", None),
+    "protobuf": ("https://googleapis.dev/python/protobuf/latest/", None),
     
 }
 
diff --git a/google/api_core/page_iterator.py b/google/api_core/page_iterator.py
index 11a92d3..fff3b55 100644
--- a/google/api_core/page_iterator.py
+++ b/google/api_core/page_iterator.py
@@ -179,7 +179,7 @@ class Iterator(object):
             single item.
         """
         self.max_results = max_results
-        """int: The maximum number of results to fetch."""
+        """int: The maximum number of results to fetch"""
 
         # The attributes below will change over the life of the iterator.
         self.page_number = 0
@@ -298,7 +298,8 @@ class HTTPIterator(Iterator):
             can be found.
         page_token (str): A token identifying a page in a result set to start
             fetching results from.
-        max_results (int): The maximum number of results to fetch.
+        page_size (int): The maximum number of results to fetch per page
+        max_results (int): The maximum number of results to fetch
         extra_params (dict): Extra query string parameters for the
             API call.
         page_start (Callable[
@@ -329,6 +330,7 @@ class HTTPIterator(Iterator):
         item_to_value,
         items_key=_DEFAULT_ITEMS_KEY,
         page_token=None,
+        page_size=None,
         max_results=None,
         extra_params=None,
         page_start=_do_nothing_page_start,
@@ -341,6 +343,7 @@ class HTTPIterator(Iterator):
         self.path = path
         self._items_key = items_key
         self.extra_params = extra_params
+        self._page_size = page_size
         self._page_start = page_start
         self._next_token = next_token
         # Verify inputs / provide defaults.
@@ -399,8 +402,18 @@ class HTTPIterator(Iterator):
         result = {}
         if self.next_page_token is not None:
             result[self._PAGE_TOKEN] = self.next_page_token
+
+        page_size = None
         if self.max_results is not None:
-            result[self._MAX_RESULTS] = self.max_results - self.num_results
+            page_size = self.max_results - self.num_results
+            if self._page_size is not None:
+                page_size = min(page_size, self._page_size)
+        elif self._page_size is not None:
+            page_size = self._page_size
+
+        if page_size is not None:
+            result[self._MAX_RESULTS] = page_size
+
         result.update(self.extra_params)
         return result
 


Test patch:
diff --git a/tests/unit/test_page_iterator.py b/tests/unit/test_page_iterator.py
index 2bf7424..8359537 100644
--- a/tests/unit/test_page_iterator.py
+++ b/tests/unit/test_page_iterator.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import math
 import types
 
 import mock
@@ -235,6 +236,7 @@ class TestHTTPIterator(object):
         assert iterator.page_number == 0
         assert iterator.next_page_token is None
         assert iterator.num_results == 0
+        assert iterator._page_size is None
 
     def test_constructor_w_extra_param_collision(self):
         extra_params = {"pageToken": "val"}
@@ -432,6 +434,66 @@ class TestHTTPIterator(object):
         with pytest.raises(ValueError):
             iterator._get_next_page_response()
 
+    @pytest.mark.parametrize(
+        "page_size,max_results,pages",
+        [(3, None, False), (3, 8, False), (3, None, True), (3, 8, True)])
+    def test_page_size_items(self, page_size, max_results, pages):
+        path = "/foo"
+        NITEMS = 10
+
+        n = [0]  # blast you python 2!
+
+        def api_request(*args, **kw):
+            assert not args
+            query_params = dict(
+                maxResults=(
+                    page_size if max_results is None
+                    else min(page_size, max_results - n[0]))
+            )
+            if n[0]:
+                query_params.update(pageToken='test')
+            assert kw == {'method': 'GET', 'path': '/foo',
+                          'query_params': query_params}
+            n_items = min(kw['query_params']['maxResults'], NITEMS - n[0])
+            items = [dict(name=str(i + n[0])) for i in range(n_items)]
+            n[0] += n_items
+            result = dict(items=items)
+            if n[0] < NITEMS:
+                result.update(nextPageToken='test')
+            return result
+
+        iterator = page_iterator.HTTPIterator(
+            mock.sentinel.client,
+            api_request,
+            path=path,
+            item_to_value=page_iterator._item_to_value_identity,
+            page_size=page_size,
+            max_results=max_results,
+        )
+
+        assert iterator.num_results == 0
+
+        n_results = max_results if max_results is not None else NITEMS
+        if pages:
+            items_iter = iter(iterator.pages)
+            npages = int(math.ceil(float(n_results) / page_size))
+            for ipage in range(npages):
+                assert (
+                    list(six.next(items_iter)) == [
+                        dict(name=str(i))
+                        for i in range(ipage * page_size,
+                                       min((ipage + 1) * page_size, n_results),
+                                       )
+                    ])
+        else:
+            items_iter = iter(iterator)
+            for i in range(n_results):
+                assert six.next(items_iter) == dict(name=str(i))
+                assert iterator.num_results == i + 1
+
+        with pytest.raises(StopIteration):
+            six.next(items_iter)
+
 
 class TestGRPCIterator(object):
     def test_constructor(self):