From 17a8fc38002bddd282f630112603560e2c526f53 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 12:44:53 -0700 Subject: [PATCH 01/14] Renaming Iterator to HTTPIterator. --- bigquery/google/cloud/bigquery/client.py | 21 +++++++++-------- core/google/cloud/iterator.py | 2 +- core/unit_tests/test_iterator.py | 6 ++--- dns/google/cloud/dns/client.py | 9 ++++---- dns/google/cloud/dns/zone.py | 16 ++++++------- pubsub/google/cloud/pubsub/_gax.py | 8 +++---- pubsub/google/cloud/pubsub/connection.py | 9 ++++---- pubsub/unit_tests/test_client.py | 10 ++++---- .../google/cloud/resource_manager/client.py | 4 ++-- resource_manager/unit_tests/test_client.py | 4 ++-- .../google/cloud/runtimeconfig/config.py | 6 ++--- runtimeconfig/unit_tests/test_client.py | 1 - runtimeconfig/unit_tests/test_config.py | 14 +++++++---- runtimeconfig/unit_tests/test_variable.py | 23 +++++++++++++++---- storage/google/cloud/storage/bucket.py | 4 ++-- storage/google/cloud/storage/client.py | 4 ++-- 16 files changed, 80 insertions(+), 61 deletions(-) diff --git a/bigquery/google/cloud/bigquery/client.py b/bigquery/google/cloud/bigquery/client.py index 7548681c448b..785cc842e56f 100644 --- a/bigquery/google/cloud/bigquery/client.py +++ b/bigquery/google/cloud/bigquery/client.py @@ -23,7 +23,7 @@ from google.cloud.bigquery.job import LoadTableFromStorageJob from google.cloud.bigquery.job import QueryJob from google.cloud.bigquery.query import QueryResults -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator class Project(object): @@ -92,9 +92,10 @@ def list_projects(self, max_results=None, page_token=None): :returns: Iterator of :class:`~google.cloud.bigquery.client.Project` accessible to the current client. """ - return Iterator(client=self, path='/projects', - items_key='projects', item_to_value=_item_to_project, - page_token=page_token, max_results=max_results) + return HTTPIterator( + client=self, path='/projects', item_to_value=_item_to_project, + items_key='projects', page_token=page_token, + max_results=max_results) def list_datasets(self, include_all=False, max_results=None, page_token=None): @@ -123,9 +124,9 @@ def list_datasets(self, include_all=False, max_results=None, if include_all: extra_params['all'] = True path = '/projects/%s/datasets' % (self.project,) - return Iterator( - client=self, path=path, items_key='datasets', - item_to_value=_item_to_dataset, page_token=page_token, + return HTTPIterator( + client=self, path=path, item_to_value=_item_to_dataset, + items_key='datasets', page_token=page_token, max_results=max_results, extra_params=extra_params) def dataset(self, dataset_name): @@ -204,9 +205,9 @@ def list_jobs(self, max_results=None, page_token=None, all_users=None, extra_params['stateFilter'] = state_filter path = '/projects/%s/jobs' % (self.project,) - return Iterator( - client=self, path=path, items_key='jobs', - item_to_value=_item_to_job, page_token=page_token, + return HTTPIterator( + client=self, path=path, item_to_value=_item_to_job, + items_key='jobs', page_token=page_token, max_results=max_results, extra_params=extra_params) def load_table_from_storage(self, job_name, destination, *source_uris): diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 87a938022a55..3cfb5c61ad74 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -166,7 +166,7 @@ def next(self): __next__ = next -class Iterator(object): +class HTTPIterator(object): """A generic class for iterating through Cloud JSON APIs list responses. :type client: :class:`~google.cloud.client.Client` diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 9a7285cc20d7..d6c651376dbb 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -91,11 +91,11 @@ def item_to_value(self, item): self.assertEqual(page.remaining, 97) -class TestIterator(unittest.TestCase): +class TestHTTPIterator(unittest.TestCase): def _getTargetClass(self): - from google.cloud.iterator import Iterator - return Iterator + from google.cloud.iterator import HTTPIterator + return HTTPIterator def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) diff --git a/dns/google/cloud/dns/client.py b/dns/google/cloud/dns/client.py index 4637c8dae1d0..aa285562fca7 100644 --- a/dns/google/cloud/dns/client.py +++ b/dns/google/cloud/dns/client.py @@ -18,7 +18,7 @@ from google.cloud.client import JSONClient from google.cloud.dns.connection import Connection from google.cloud.dns.zone import ManagedZone -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator class Client(JSONClient): @@ -81,9 +81,10 @@ def list_zones(self, max_results=None, page_token=None): belonging to this project. """ path = '/projects/%s/managedZones' % (self.project,) - return Iterator(client=self, path=path, items_key='managedZones', - item_to_value=_item_to_zone, page_token=page_token, - max_results=max_results) + return HTTPIterator( + client=self, path=path, item_to_value=_item_to_zone, + items_key='managedZones', page_token=page_token, + max_results=max_results) def zone(self, name, dns_name=None, description=None): """Construct a zone bound to this client. diff --git a/dns/google/cloud/dns/zone.py b/dns/google/cloud/dns/zone.py index ec47a97027db..b7b5c594c320 100644 --- a/dns/google/cloud/dns/zone.py +++ b/dns/google/cloud/dns/zone.py @@ -20,7 +20,7 @@ from google.cloud.exceptions import NotFound from google.cloud.dns.changes import Changes from google.cloud.dns.resource_record_set import ResourceRecordSet -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator class ManagedZone(object): @@ -347,9 +347,9 @@ def list_resource_record_sets(self, max_results=None, page_token=None, client = self._require_client(client) path = '/projects/%s/managedZones/%s/rrsets' % ( self.project, self.name) - iterator = Iterator( - client=client, path=path, items_key='rrsets', - item_to_value=_item_to_resource_record_set, + iterator = HTTPIterator( + client=client, path=path, + item_to_value=_item_to_resource_record_set, items_key='rrsets', page_token=page_token, max_results=max_results) iterator.zone = self return iterator @@ -381,10 +381,10 @@ def list_changes(self, max_results=None, page_token=None, client=None): client = self._require_client(client) path = '/projects/%s/managedZones/%s/changes' % ( self.project, self.name) - iterator = Iterator( - client=client, path=path, items_key='changes', - item_to_value=_item_to_changes, - page_token=page_token, max_results=max_results) + iterator = HTTPIterator( + client=client, path=path, item_to_value=_item_to_changes, + items_key='changes', page_token=page_token, + max_results=max_results) iterator.zone = self return iterator diff --git a/pubsub/google/cloud/pubsub/_gax.py b/pubsub/google/cloud/pubsub/_gax.py index 1a39c0078b69..06b8cec1faeb 100644 --- a/pubsub/google/cloud/pubsub/_gax.py +++ b/pubsub/google/cloud/pubsub/_gax.py @@ -31,7 +31,7 @@ from google.cloud._helpers import _pb_timestamp_to_rfc3339 from google.cloud.exceptions import Conflict from google.cloud.exceptions import NotFound -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator from google.cloud.iterator import Page from google.cloud.pubsub.topic import Topic @@ -83,9 +83,9 @@ def list_topics(self, project, page_size=0, page_token=None): path, page_size=page_size, options=options) page_iter = functools.partial(_recast_page_iterator, page_iter) - return Iterator(client=self._client, path=path, - item_to_value=_item_to_topic, - page_iter=page_iter) + return HTTPIterator( + client=self._client, path=path, item_to_value=_item_to_topic, + page_iter=page_iter) def topic_create(self, topic_path): """API call: create a topic diff --git a/pubsub/google/cloud/pubsub/connection.py b/pubsub/google/cloud/pubsub/connection.py index 722b28102dfa..53f57f6c7435 100644 --- a/pubsub/google/cloud/pubsub/connection.py +++ b/pubsub/google/cloud/pubsub/connection.py @@ -19,7 +19,7 @@ from google.cloud import connection as base_connection from google.cloud.environment_vars import PUBSUB_EMULATOR -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator from google.cloud.pubsub.topic import Topic @@ -134,9 +134,10 @@ def list_topics(self, project, page_size=None, page_token=None): extra_params['pageSize'] = page_size path = '/projects/%s/topics' % (project,) - return Iterator(client=self._client, path=path, - items_key='topics', item_to_value=_item_to_topic, - page_token=page_token, extra_params=extra_params) + return HTTPIterator( + client=self._client, path=path, item_to_value=_item_to_topic, + items_key='topics', page_token=page_token, + extra_params=extra_params) def topic_create(self, topic_path): """API call: create a topic diff --git a/pubsub/unit_tests/test_client.py b/pubsub/unit_tests/test_client.py index 5b12aee800e3..4f4d597591fa 100644 --- a/pubsub/unit_tests/test_client.py +++ b/pubsub/unit_tests/test_client.py @@ -79,12 +79,11 @@ def __init__(self, _wrapped, client): self._client = client creds = _Credentials() - client = self._makeOne(project=self.PROJECT, credentials=creds) - with _Monkey(MUT, - _USE_GAX=True, + with _Monkey(MUT, _USE_GAX=True, make_gax_publisher_api=_generated_api, GAXPublisherAPI=_GaxPublisherAPI): + client = self._makeOne(project=self.PROJECT, credentials=creds) api = client.publisher_api self.assertIsInstance(api, _GaxPublisherAPI) @@ -131,12 +130,11 @@ def __init__(self, _wrapped): self._wrapped = _wrapped creds = _Credentials() - client = self._makeOne(project=self.PROJECT, credentials=creds) - with _Monkey(MUT, - _USE_GAX=True, + with _Monkey(MUT, _USE_GAX=True, make_gax_subscriber_api=_generated_api, GAXSubscriberAPI=_GaxSubscriberAPI): + client = self._makeOne(project=self.PROJECT, credentials=creds) api = client.subscriber_api self.assertIsInstance(api, _GaxSubscriberAPI) diff --git a/resource_manager/google/cloud/resource_manager/client.py b/resource_manager/google/cloud/resource_manager/client.py index 0c0341948aab..dca9446acbb6 100644 --- a/resource_manager/google/cloud/resource_manager/client.py +++ b/resource_manager/google/cloud/resource_manager/client.py @@ -16,7 +16,7 @@ from google.cloud.client import Client as BaseClient -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator from google.cloud.resource_manager.connection import Connection from google.cloud.resource_manager.project import Project @@ -154,7 +154,7 @@ def list_projects(self, filter_params=None, page_size=None): if filter_params is not None: extra_params['filter'] = filter_params - return Iterator( + return HTTPIterator( client=self, path='/projects', item_to_value=_item_to_project, items_key='projects', extra_params=extra_params) diff --git a/resource_manager/unit_tests/test_client.py b/resource_manager/unit_tests/test_client.py index cdc4159dd8ab..6a18614cdc5a 100644 --- a/resource_manager/unit_tests/test_client.py +++ b/resource_manager/unit_tests/test_client.py @@ -78,7 +78,7 @@ def test_fetch_project(self): self.assertEqual(project.labels, labels) def test_list_projects_return_type(self): - from google.cloud.iterator import Iterator + from google.cloud.iterator import HTTPIterator credentials = _Credentials() client = self._makeOne(credentials=credentials) @@ -86,7 +86,7 @@ def test_list_projects_return_type(self): client.connection = _Connection({}) results = client.list_projects() - self.assertIsInstance(results, Iterator) + self.assertIsInstance(results, HTTPIterator) def test_list_projects_no_paging(self): credentials = _Credentials() diff --git a/runtimeconfig/google/cloud/runtimeconfig/config.py b/runtimeconfig/google/cloud/runtimeconfig/config.py index b73897dd02b1..7a259df23f09 100644 --- a/runtimeconfig/google/cloud/runtimeconfig/config.py +++ b/runtimeconfig/google/cloud/runtimeconfig/config.py @@ -17,7 +17,7 @@ from google.cloud.exceptions import NotFound from google.cloud.runtimeconfig._helpers import config_name_from_full_name from google.cloud.runtimeconfig.variable import Variable -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator class Config(object): @@ -238,9 +238,9 @@ def list_variables(self, page_size=None, page_token=None, client=None): belonging to this project. """ path = '%s/variables' % (self.path,) - iterator = Iterator( + iterator = HTTPIterator( client=self._require_client(client), path=path, - items_key='variables', item_to_value=_item_to_variable, + item_to_value=_item_to_variable, items_key='variables', page_token=page_token, max_results=page_size) iterator._MAX_RESULTS = 'pageSize' iterator.config = self diff --git a/runtimeconfig/unit_tests/test_client.py b/runtimeconfig/unit_tests/test_client.py index 350a825f6cdf..943385230e8b 100644 --- a/runtimeconfig/unit_tests/test_client.py +++ b/runtimeconfig/unit_tests/test_client.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. - import unittest diff --git a/runtimeconfig/unit_tests/test_config.py b/runtimeconfig/unit_tests/test_config.py index 5eed1482539d..d58184bd58b6 100644 --- a/runtimeconfig/unit_tests/test_config.py +++ b/runtimeconfig/unit_tests/test_config.py @@ -14,9 +14,6 @@ import unittest -from google.cloud._helpers import _rfc3339_to_datetime -from google.cloud.runtimeconfig._helpers import config_name_from_full_name - class TestConfig(unittest.TestCase): PROJECT = 'PROJECT' @@ -31,6 +28,9 @@ def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) def _verifyResourceProperties(self, config, resource): + from google.cloud.runtimeconfig._helpers import ( + config_name_from_full_name) + if 'name' in resource: self.assertEqual(config.full_name, resource['name']) self.assertEqual( @@ -51,7 +51,7 @@ def test_ctor_w_no_name(self): client = _Client(project=self.PROJECT) config = self._makeOne(name=None, client=client) with self.assertRaises(ValueError): - _ = config.full_name + getattr(config, 'full_name') def test_exists_miss_w_bound_client(self): conn = _Connection() @@ -143,6 +143,8 @@ def test_variable(self): self.assertEqual(len(conn._requested), 0) def test_get_variable_w_bound_client(self): + from google.cloud._helpers import _rfc3339_to_datetime + VARIABLE_NAME = 'my-variable/abcd' VARIABLE_PATH = '%s/variables/%s' % (self.CONFIG_PATH, VARIABLE_NAME) RESOURCE = { @@ -178,6 +180,8 @@ def test_get_variable_w_notfound(self): self.assertIsNone(variable) def test_get_variable_w_alternate_client(self): + from google.cloud._helpers import _rfc3339_to_datetime + VARIABLE_NAME = 'my-variable/abcd' VARIABLE_PATH = '%s/variables/%s' % (self.CONFIG_PATH, VARIABLE_NAME) RESOURCE = { @@ -230,6 +234,7 @@ def test_list_variables_empty(self): def test_list_variables_defaults(self): import six + from google.cloud._helpers import _rfc3339_to_datetime from google.cloud.runtimeconfig.variable import Variable VARIABLE_1 = 'variable-one' @@ -273,6 +278,7 @@ def test_list_variables_defaults(self): def test_list_variables_explicit(self): import six + from google.cloud._helpers import _rfc3339_to_datetime from google.cloud.runtimeconfig.variable import Variable VARIABLE_1 = 'variable-one' diff --git a/runtimeconfig/unit_tests/test_variable.py b/runtimeconfig/unit_tests/test_variable.py index 5f9d441884c8..dc5df9a0d716 100644 --- a/runtimeconfig/unit_tests/test_variable.py +++ b/runtimeconfig/unit_tests/test_variable.py @@ -12,12 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -import base64 import unittest -from google.cloud.runtimeconfig.config import Config -from google.cloud._helpers import _rfc3339_to_datetime - class TestVariable(unittest.TestCase): PROJECT = 'PROJECT' @@ -34,6 +30,9 @@ def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) def _verifyResourceProperties(self, variable, resource): + import base64 + from google.cloud._helpers import _rfc3339_to_datetime + if 'name' in resource: self.assertEqual(variable.full_name, resource['name']) @@ -54,6 +53,8 @@ def _verifyResourceProperties(self, variable, resource): self.assertIsNone(variable.update_time) def test_ctor(self): + from google.cloud.runtimeconfig.config import Config + client = _Client(project=self.PROJECT) config = Config(name=self.CONFIG_NAME, client=client) variable = self._makeOne(name=self.VARIABLE_NAME, config=config) @@ -63,13 +64,17 @@ def test_ctor(self): self.assertIs(variable.client, client) def test_ctor_w_no_name(self): + from google.cloud.runtimeconfig.config import Config + client = _Client(project=self.PROJECT) config = Config(name=self.CONFIG_NAME, client=client) variable = self._makeOne(name=None, config=config) with self.assertRaises(ValueError): - _ = variable.full_name + getattr(variable, 'full_name') def test_exists_miss_w_bound_client(self): + from google.cloud.runtimeconfig.config import Config + conn = _Connection() client = _Client(project=self.PROJECT, connection=conn) config = Config(name=self.CONFIG_NAME, client=client) @@ -84,6 +89,8 @@ def test_exists_miss_w_bound_client(self): self.assertEqual(req['query_params'], {'fields': 'name'}) def test_exists_hit_w_alternate_client(self): + from google.cloud.runtimeconfig.config import Config + conn1 = _Connection() CLIENT1 = _Client(project=self.PROJECT, connection=conn1) CONFIG1 = Config(name=self.CONFIG_NAME, client=CLIENT1) @@ -101,6 +108,8 @@ def test_exists_hit_w_alternate_client(self): self.assertEqual(req['query_params'], {'fields': 'name'}) def test_reload_w_bound_client(self): + from google.cloud.runtimeconfig.config import Config + RESOURCE = { 'name': self.PATH, 'value': 'bXktdmFyaWFibGUtdmFsdWU=', # base64 my-variable-value @@ -121,6 +130,8 @@ def test_reload_w_bound_client(self): self._verifyResourceProperties(variable, RESOURCE) def test_reload_w_empty_resource(self): + from google.cloud.runtimeconfig.config import Config + RESOURCE = {} conn = _Connection(RESOURCE) client = _Client(project=self.PROJECT, connection=conn) @@ -139,6 +150,8 @@ def test_reload_w_empty_resource(self): self._verifyResourceProperties(variable, RESOURCE) def test_reload_w_alternate_client(self): + from google.cloud.runtimeconfig.config import Config + RESOURCE = { 'name': self.PATH, 'value': 'bXktdmFyaWFibGUtdmFsdWU=', # base64 my-variable-value diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 9f8da3e75b7e..b2ffafb1311d 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -20,7 +20,7 @@ from google.cloud._helpers import _rfc3339_to_datetime from google.cloud.exceptions import NotFound -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator from google.cloud.storage._helpers import _PropertyMixin from google.cloud.storage._helpers import _scalar_property from google.cloud.storage.acl import BucketACL @@ -306,7 +306,7 @@ def list_blobs(self, max_results=None, page_token=None, prefix=None, client = self._require_client(client) path = self.path + '/o' - iterator = Iterator( + iterator = HTTPIterator( client=client, path=path, item_to_value=_item_to_blob, page_token=page_token, max_results=max_results, extra_params=extra_params, page_start=_blobs_page_start) diff --git a/storage/google/cloud/storage/client.py b/storage/google/cloud/storage/client.py index 721f5dd962ec..4255c43720c4 100644 --- a/storage/google/cloud/storage/client.py +++ b/storage/google/cloud/storage/client.py @@ -18,7 +18,7 @@ from google.cloud._helpers import _LocalStack from google.cloud.client import JSONClient from google.cloud.exceptions import NotFound -from google.cloud.iterator import Iterator +from google.cloud.iterator import HTTPIterator from google.cloud.storage.batch import Batch from google.cloud.storage.bucket import Bucket from google.cloud.storage.connection import Connection @@ -265,7 +265,7 @@ def list_buckets(self, max_results=None, page_token=None, prefix=None, if fields is not None: extra_params['fields'] = fields - return Iterator( + return HTTPIterator( client=self, path='/b', item_to_value=_item_to_bucket, page_token=page_token, max_results=max_results, extra_params=extra_params) From b894dfa4cfc808f10c31882f3dfc811242427407 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 14:00:21 -0700 Subject: [PATCH 02/14] Making Iterator base class. For this commit, moving some features of HTTPIterator constructor onto base class. --- core/google/cloud/iterator.py | 39 +++++++++++++++++++++++--------- core/unit_tests/test_iterator.py | 26 +++++++++++++++++++++ 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 3cfb5c61ad74..e12f857f8d9c 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -166,15 +166,37 @@ def next(self): __next__ = next -class HTTPIterator(object): +class Iterator(object): + """A generic class for iterating through API list responses. + + :type client: :class:`~google.cloud.client.Client` + :param client: The client used to identify the application. + + :type page_token: str + :param page_token: (Optional) A token identifying a page in a result set. + + :type max_results: int + :param max_results: (Optional) The maximum number of results to fetch. + """ + + def __init__(self, client, page_token=None, max_results=None): + self._started = False + self.client = client + self.max_results = max_results + # The attributes below will change over the life of the iterator. + self.page_number = 0 + self.next_page_token = page_token + self.num_results = 0 + + +class HTTPIterator(Iterator): """A generic class for iterating through Cloud JSON APIs list responses. :type client: :class:`~google.cloud.client.Client` - :param client: The client, which owns a connection to make requests. + :param client: The client used to identify the application. :type path: str - :param path: The path to query for the list of items. Defaults - to :attr:`PATH` on the current iterator class. + :param path: The path to query for the list of items. :type item_to_value: callable :param item_to_value: Callable to convert an item from JSON @@ -220,12 +242,11 @@ def __init__(self, client, path, item_to_value, items_key=DEFAULT_ITEMS_KEY, page_token=None, max_results=None, extra_params=None, page_start=_do_nothing_page_start, page_iter=None): - self._started = False - self.client = client + super(HTTPIterator, self).__init__( + client, page_token=page_token, max_results=max_results) self.path = path self._item_to_value = item_to_value self._items_key = items_key - self.max_results = max_results self.extra_params = extra_params self._page_start = page_start self._page_iter = None @@ -237,10 +258,6 @@ def __init__(self, client, path, item_to_value, else: self._page_iter = page_iter(self) self._verify_params() - # The attributes below will change over the life of the iterator. - self.page_number = 0 - self.next_page_token = page_token - self.num_results = 0 def _verify_params(self): """Verifies the parameters don't use any reserved parameter. diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index d6c651376dbb..bd57c6009a7a 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -91,6 +91,32 @@ def item_to_value(self, item): self.assertEqual(page.remaining, 97) +class TestIterator(unittest.TestCase): + + def _getTargetClass(self): + from google.cloud.iterator import Iterator + return Iterator + + def _makeOne(self, *args, **kw): + return self._getTargetClass()(*args, **kw) + + def test_constructor(self): + connection = _Connection() + client = _Client(connection) + token = 'ab13nceor03' + max_results = 1337 + iterator = self._makeOne(client, page_token=token, + max_results=max_results) + + self.assertFalse(iterator._started) + self.assertIs(iterator.client, client) + self.assertEqual(iterator.max_results, max_results) + # Changing attributes. + self.assertEqual(iterator.page_number, 0) + self.assertEqual(iterator.next_page_token, token) + self.assertEqual(iterator.num_results, 0) + + class TestHTTPIterator(unittest.TestCase): def _getTargetClass(self): From 264b1ce0337959e98ce15c9f8c169babcf6102c7 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 14:49:32 -0700 Subject: [PATCH 03/14] Moving pages/_items_iter/__iter__ onto Iterator base class. --- core/google/cloud/iterator.py | 77 +++++++++-------- core/unit_tests/test_iterator.py | 136 ++++++++++++++++++++----------- 2 files changed, 129 insertions(+), 84 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index e12f857f8d9c..dcd3ff7291a0 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -183,11 +183,50 @@ def __init__(self, client, page_token=None, max_results=None): self._started = False self.client = client self.max_results = max_results + # NOTE: The _page_iter is not intended to come through the + # constructor, instead subclasses should over-ride + # this property. + self._page_iter = iter(()) # The attributes below will change over the life of the iterator. self.page_number = 0 self.next_page_token = page_token self.num_results = 0 + @property + def pages(self): + """Iterator of pages in the response. + + :rtype: :class:`~types.GeneratorType` + :returns: A generator of :class:`Page` instances. + :raises ValueError: If the iterator has already been started. + """ + if self._started: + raise ValueError('Iterator has already started', self) + self._started = True + return self._page_iter + + def _items_iter(self): + """Iterator for each item returned.""" + for page in self._page_iter: + # Decrement the total results since the pages iterator adds + # to it when each page is encountered. + self.num_results -= page.num_items + for item in page: + self.num_results += 1 + yield item + + def __iter__(self): + """Iterator for each item returned. + + :rtype: :class:`~types.GeneratorType` + :returns: A generator of items from the API. + :raises ValueError: If the iterator has already been started. + """ + if self._started: + raise ValueError('Iterator has already started', self) + self._started = True + return self._items_iter() + class HTTPIterator(Iterator): """A generic class for iterating through Cloud JSON APIs list responses. @@ -232,6 +271,8 @@ class HTTPIterator(Iterator): the HTTP pages iterator. Meant to provide a custom way to create pages (potentially with a custom transport such as gRPC). + + .. autoattribute:: pages """ _PAGE_TOKEN = 'pageToken' @@ -249,7 +290,6 @@ def __init__(self, client, path, item_to_value, self._items_key = items_key self.extra_params = extra_params self._page_start = page_start - self._page_iter = None # Verify inputs / provide defaults. if self.extra_params is None: self.extra_params = {} @@ -283,41 +323,6 @@ def _default_page_iter(self): self.num_results += page.num_items yield page - @property - def pages(self): - """Iterator of pages in the response. - - :rtype: :class:`~types.GeneratorType` - :returns: A generator of :class:`Page` instances. - :raises ValueError: If the iterator has already been started. - """ - if self._started: - raise ValueError('Iterator has already started', self) - self._started = True - return self._page_iter - - def _items_iter(self): - """Iterator for each item returned.""" - for page in self._page_iter: - # Decrement the total results since the pages iterator adds - # to it when each page is encountered. - self.num_results -= page.num_items - for item in page: - self.num_results += 1 - yield item - - def __iter__(self): - """Iterator for each item returned. - - :rtype: :class:`~types.GeneratorType` - :returns: A generator of items from the API. - :raises ValueError: If the iterator has already been started. - """ - if self._started: - raise ValueError('Iterator has already started', self) - self._started = True - return self._items_iter() - def _has_next_page(self): """Determines whether or not there are more pages with results. diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index bd57c6009a7a..747f739530b2 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -111,11 +111,99 @@ def test_constructor(self): self.assertFalse(iterator._started) self.assertIs(iterator.client, client) self.assertEqual(iterator.max_results, max_results) + self.assertEqual(list(iterator._page_iter), []) # Changing attributes. self.assertEqual(iterator.page_number, 0) self.assertEqual(iterator.next_page_token, token) self.assertEqual(iterator.num_results, 0) + def test_pages_property(self): + iterator = self._makeOne(None) + self.assertFalse(iterator._started) + mock_iter = object() + iterator._page_iter = mock_iter + self.assertIs(iterator.pages, mock_iter) + # Check the side-effect. + self.assertTrue(iterator._started) + + def test_pages_property_started(self): + iterator = self._makeOne(None) + self.assertEqual(list(iterator.pages), []) + # Make sure we cannot restart. + with self.assertRaises(ValueError): + getattr(iterator, 'pages') + + def test_pages_property_items_started(self): + iterator = self._makeOne(None) + self.assertEqual(list(iterator), []) + with self.assertRaises(ValueError): + getattr(iterator, 'pages') + + @staticmethod + def _do_nothing(parent, value): + return parent, value + + def test__items_iter(self): + import types + import six + from google.cloud.iterator import Page + + # Items to be returned. + item1 = 17 + item2 = 100 + item3 = 211 + + # Make pages from mock responses + mock_key = 'mock' + parent = object() + page1 = Page(parent, {mock_key: [item1, item2]}, + mock_key, self._do_nothing) + page2 = Page(parent, {mock_key: [item3]}, + mock_key, self._do_nothing) + # Spoof the number of items in each page to offset the + # ``num_results -= page.num_items`` in _items_iter(). + page1._num_items = page2._num_items = 0 + + iterator = self._makeOne(None) + # Fake the page iterator on the object. + iterator._page_iter = iter((page1, page2)) + + items_iter = iterator._items_iter() + # Make sure it is a generator. + self.assertIsInstance(items_iter, types.GeneratorType) + + # Consume items and check the state of the iterator. + self.assertEqual(iterator.num_results, 0) + self.assertEqual(six.next(items_iter), (parent, item1)) + self.assertEqual(iterator.num_results, 1) + self.assertEqual(six.next(items_iter), (parent, item2)) + self.assertEqual(iterator.num_results, 2) + self.assertEqual(six.next(items_iter), (parent, item3)) + self.assertEqual(iterator.num_results, 3) + with self.assertRaises(StopIteration): + six.next(items_iter) + + def test___iter__(self): + iterator = self._makeOne(None) + self.assertFalse(iterator._started) + mock_iter = object() + iterator._page_iter = mock_iter + self.assertIs(iterator.pages, mock_iter) + # Check the side-effect. + self.assertTrue(iterator._started) + + def test___iter___started(self): + iterator = self._makeOne(None) + self.assertEqual(list(iterator), []) + with self.assertRaises(ValueError): + iter(iterator) + + def test___iter___pages_started(self): + iterator = self._makeOne(None) + self.assertEqual(list(iterator.pages), []) + with self.assertRaises(ValueError): + iter(iterator) + class TestHTTPIterator(unittest.TestCase): @@ -196,54 +284,6 @@ def dummy_page_class(*args): self.assertEqual( page_args, [(iterator, {}, items_key, iterator._item_to_value)]) - def test_pages_property(self): - import types - - iterator = self._makeOne(None, None, None) - self.assertIsInstance(iterator.pages, types.GeneratorType) - - def test_pages_property_started(self): - import types - - iterator = self._makeOne(None, None, None) - pages_iter = iterator.pages - self.assertIsInstance(pages_iter, types.GeneratorType) - with self.assertRaises(ValueError): - getattr(iterator, 'pages') - - def test_pages_property_items_started(self): - import types - - iterator = self._makeOne(None, None, None) - items_iter = iter(iterator) - self.assertIsInstance(items_iter, types.GeneratorType) - with self.assertRaises(ValueError): - getattr(iterator, 'pages') - - def test___iter__(self): - import types - - iterator = self._makeOne(None, None, None) - self.assertIsInstance(iter(iterator), types.GeneratorType) - - def test___iter___started(self): - import types - - iterator = self._makeOne(None, None, None) - iter_obj = iter(iterator) - self.assertIsInstance(iter_obj, types.GeneratorType) - with self.assertRaises(ValueError): - iter(iterator) - - def test___iter___pages_started(self): - import types - - iterator = self._makeOne(None, None, None) - pages_iter = iterator.pages - self.assertIsInstance(pages_iter, types.GeneratorType) - with self.assertRaises(ValueError): - iter(iterator) - def test_iterate(self): import six From 251484fee2f7cb7026c3d8d6a8d3ee79d02945e4 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 15:08:50 -0700 Subject: [PATCH 04/14] Adding instance flag to determine if page/items are iterating. --- core/google/cloud/iterator.py | 12 ++++++++---- core/unit_tests/test_iterator.py | 4 +--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index dcd3ff7291a0..ebef82bc410c 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -187,6 +187,11 @@ def __init__(self, client, page_token=None, max_results=None): # constructor, instead subclasses should over-ride # this property. self._page_iter = iter(()) + # NOTE: This flag indicates if the total number of results should be + # incremented. This is useful since a page iterator will + # want to increment by results per page while an items + # iterator will want to increment per item. + self._page_increment = False # The attributes below will change over the life of the iterator. self.page_number = 0 self.next_page_token = page_token @@ -203,14 +208,12 @@ def pages(self): if self._started: raise ValueError('Iterator has already started', self) self._started = True + self._page_increment = True return self._page_iter def _items_iter(self): """Iterator for each item returned.""" for page in self._page_iter: - # Decrement the total results since the pages iterator adds - # to it when each page is encountered. - self.num_results -= page.num_items for item in page: self.num_results += 1 yield item @@ -320,7 +323,8 @@ def _default_page_iter(self): page = Page(self, response, self._items_key, self._item_to_value) self._page_start(self, page, response) - self.num_results += page.num_items + if self._page_increment: + self.num_results += page.num_items yield page def _has_next_page(self): diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 747f739530b2..38f0d1376d20 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -112,6 +112,7 @@ def test_constructor(self): self.assertIs(iterator.client, client) self.assertEqual(iterator.max_results, max_results) self.assertEqual(list(iterator._page_iter), []) + self.assertFalse(iterator._page_increment) # Changing attributes. self.assertEqual(iterator.page_number, 0) self.assertEqual(iterator.next_page_token, token) @@ -160,9 +161,6 @@ def test__items_iter(self): mock_key, self._do_nothing) page2 = Page(parent, {mock_key: [item3]}, mock_key, self._do_nothing) - # Spoof the number of items in each page to offset the - # ``num_results -= page.num_items`` in _items_iter(). - page1._num_items = page2._num_items = 0 iterator = self._makeOne(None) # Fake the page iterator on the object. From 2dbdb3a8f82a064a7416003d4e56bad57d58d9f6 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 15:25:14 -0700 Subject: [PATCH 05/14] Adding GAXIterator subclass. --- core/google/cloud/iterator.py | 25 +++++++++++++++++++++++++ core/unit_tests/test_iterator.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index ebef82bc410c..8649a2753c01 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -370,3 +370,28 @@ def _get_next_page_response(self): self.next_page_token = response.get('nextPageToken') return response + + +class GAXIterator(Iterator): + """A generic class for iterating through Cloud gRPC APIs list responses. + + :type client: :class:`~google.cloud.client.Client` + :param client: The client used to identify the application. + + :type page_iter: :class:`~google.gax.PageIterator` + :param page_iter: A GAX page iterator to be wrapped and conform to the + :class:`~google.cloud.iterator.Iterator` surface. + + :type page_token: str + :param page_token: (Optional) A token identifying a page in a result set. + + :type max_results: int + :param max_results: (Optional) The maximum number of results to fetch. + + .. autoattribute:: pages + """ + + def __init__(self, client, page_iter, page_token=None, max_results=None): + super(GAXIterator, self).__init__( + client, page_token=page_token, max_results=max_results) + self._page_iter = page_iter diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 38f0d1376d20..0fb166af7ff9 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -427,6 +427,34 @@ def test__get_next_page_response_new_no_token_in_response(self): self.assertEqual(kw['query_params'], {}) +class TestGAXIterator(unittest.TestCase): + + def _getTargetClass(self): + from google.cloud.iterator import GAXIterator + return GAXIterator + + def _makeOne(self, *args, **kw): + return self._getTargetClass()(*args, **kw) + + def test_constructor(self): + client = _Client(None) + page_iter = object() + token = 'zzzyy78kl' + max_results = 1337 + iterator = self._makeOne(client, page_iter, page_token=token, + max_results=max_results) + + self.assertFalse(iterator._started) + self.assertIs(iterator.client, client) + self.assertEqual(iterator.max_results, max_results) + self.assertIs(iterator._page_iter, page_iter) + self.assertFalse(iterator._page_increment) + # Changing attributes. + self.assertEqual(iterator.page_number, 0) + self.assertEqual(iterator.next_page_token, token) + self.assertEqual(iterator.num_results, 0) + + class _Connection(object): def __init__(self, *responses): From 083110bada86260b0c7a9e8892c71467daca219d Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 15:37:50 -0700 Subject: [PATCH 06/14] Moving item_to_value into base Iterator class. --- core/google/cloud/iterator.py | 26 +++++++++++++++++++++----- core/unit_tests/test_iterator.py | 24 ++++++++++++++---------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 8649a2753c01..6efb2d08a514 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -172,6 +172,12 @@ class Iterator(object): :type client: :class:`~google.cloud.client.Client` :param client: The client used to identify the application. + :type item_to_value: callable + :param item_to_value: Callable to convert an item from the type in the + raw API response into the native object. + Assumed signature takes an :class:`Iterator` and a + raw API response with a single item. + :type page_token: str :param page_token: (Optional) A token identifying a page in a result set. @@ -179,9 +185,11 @@ class Iterator(object): :param max_results: (Optional) The maximum number of results to fetch. """ - def __init__(self, client, page_token=None, max_results=None): + def __init__(self, client, item_to_value, + page_token=None, max_results=None): self._started = False self.client = client + self._item_to_value = item_to_value self.max_results = max_results # NOTE: The _page_iter is not intended to come through the # constructor, instead subclasses should over-ride @@ -287,9 +295,9 @@ def __init__(self, client, path, item_to_value, page_token=None, max_results=None, extra_params=None, page_start=_do_nothing_page_start, page_iter=None): super(HTTPIterator, self).__init__( - client, page_token=page_token, max_results=max_results) + client, item_to_value, page_token=page_token, + max_results=max_results) self.path = path - self._item_to_value = item_to_value self._items_key = items_key self.extra_params = extra_params self._page_start = page_start @@ -382,6 +390,12 @@ class GAXIterator(Iterator): :param page_iter: A GAX page iterator to be wrapped and conform to the :class:`~google.cloud.iterator.Iterator` surface. + :type item_to_value: callable + :param item_to_value: Callable to convert an item from a protobuf + into the native object. Assumed signature + takes an :class:`Iterator` and a single item + from the API response as a protobuf. + :type page_token: str :param page_token: (Optional) A token identifying a page in a result set. @@ -391,7 +405,9 @@ class GAXIterator(Iterator): .. autoattribute:: pages """ - def __init__(self, client, page_iter, page_token=None, max_results=None): + def __init__(self, client, page_iter, item_to_value, + page_token=None, max_results=None): super(GAXIterator, self).__init__( - client, page_token=page_token, max_results=max_results) + client, item_to_value, page_token=page_token, + max_results=max_results) self._page_iter = page_iter diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 0fb166af7ff9..2a01d8b6df55 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -103,13 +103,15 @@ def _makeOne(self, *args, **kw): def test_constructor(self): connection = _Connection() client = _Client(connection) + item_to_value = object() token = 'ab13nceor03' max_results = 1337 - iterator = self._makeOne(client, page_token=token, + iterator = self._makeOne(client, item_to_value, page_token=token, max_results=max_results) self.assertFalse(iterator._started) self.assertIs(iterator.client, client) + self.assertIs(iterator._item_to_value, item_to_value) self.assertEqual(iterator.max_results, max_results) self.assertEqual(list(iterator._page_iter), []) self.assertFalse(iterator._page_increment) @@ -119,7 +121,7 @@ def test_constructor(self): self.assertEqual(iterator.num_results, 0) def test_pages_property(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertFalse(iterator._started) mock_iter = object() iterator._page_iter = mock_iter @@ -128,14 +130,14 @@ def test_pages_property(self): self.assertTrue(iterator._started) def test_pages_property_started(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertEqual(list(iterator.pages), []) # Make sure we cannot restart. with self.assertRaises(ValueError): getattr(iterator, 'pages') def test_pages_property_items_started(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertEqual(list(iterator), []) with self.assertRaises(ValueError): getattr(iterator, 'pages') @@ -162,7 +164,7 @@ def test__items_iter(self): page2 = Page(parent, {mock_key: [item3]}, mock_key, self._do_nothing) - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) # Fake the page iterator on the object. iterator._page_iter = iter((page1, page2)) @@ -182,7 +184,7 @@ def test__items_iter(self): six.next(items_iter) def test___iter__(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertFalse(iterator._started) mock_iter = object() iterator._page_iter = mock_iter @@ -191,13 +193,13 @@ def test___iter__(self): self.assertTrue(iterator._started) def test___iter___started(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertEqual(list(iterator), []) with self.assertRaises(ValueError): iter(iterator) def test___iter___pages_started(self): - iterator = self._makeOne(None) + iterator = self._makeOne(None, None) self.assertEqual(list(iterator.pages), []) with self.assertRaises(ValueError): iter(iterator) @@ -439,13 +441,15 @@ def _makeOne(self, *args, **kw): def test_constructor(self): client = _Client(None) page_iter = object() + item_to_value = object() token = 'zzzyy78kl' max_results = 1337 - iterator = self._makeOne(client, page_iter, page_token=token, - max_results=max_results) + iterator = self._makeOne(client, page_iter, item_to_value, + page_token=token, max_results=max_results) self.assertFalse(iterator._started) self.assertIs(iterator.client, client) + self.assertIs(iterator._item_to_value, item_to_value) self.assertEqual(iterator.max_results, max_results) self.assertIs(iterator._page_iter, page_iter) self.assertFalse(iterator._page_increment) From 04b0932439ab0ece646af7c86de97d977ddb126b Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 16:00:10 -0700 Subject: [PATCH 07/14] Removing items key usage from Page helper. This was HTTP/JSON specific and belongs in the HTTP subclass. --- core/google/cloud/iterator.py | 24 ++++++++----------- core/unit_tests/test_iterator.py | 27 +++++++++------------- pubsub/google/cloud/pubsub/_gax.py | 7 +----- resource_manager/unit_tests/test_client.py | 2 +- storage/unit_tests/test_bucket.py | 2 +- storage/unit_tests/test_client.py | 2 +- 6 files changed, 25 insertions(+), 39 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 6efb2d08a514..8dcc3eba7e13 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -109,23 +109,19 @@ class Page(object): :type parent: :class:`Iterator` :param parent: The iterator that owns the current page. - :type response: dict - :param response: The JSON API response for a page. - - :type items_key: str - :param items_key: The dictionary key used to retrieve items - from the response. + :type items: iterable + :param items: An iterable (that also defines __len__) of items + from a raw API response. :type item_to_value: callable - :param item_to_value: Callable to convert an item from JSON - into the native object. Assumed signature - takes an :class:`Iterator` and a dictionary - holding a single item. + :param item_to_value: Callable to convert an item from the type in the + raw API response into the native object. + Assumed signature takes an :class:`Iterator` and a + raw API response with a single item. """ - def __init__(self, parent, response, items_key, item_to_value): + def __init__(self, parent, items, item_to_value): self._parent = parent - items = response.get(items_key, ()) self._num_items = len(items) self._remaining = self._num_items self._item_iter = iter(items) @@ -328,8 +324,8 @@ def _default_page_iter(self): """ while self._has_next_page(): response = self._get_next_page_response() - page = Page(self, response, self._items_key, - self._item_to_value) + items = response.get(self._items_key, ()) + page = Page(self, items, self._item_to_value) self._page_start(self, page, response) if self._page_increment: self.num_results += page.num_items diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 2a01d8b6df55..e1f1965bc0c6 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -37,27 +37,27 @@ def _makeOne(self, *args, **kw): def test_constructor(self): parent = object() - items_key = 'potatoes' - response = {items_key: (1, 2, 3)} - page = self._makeOne(parent, response, items_key, None) + item_to_value = object() + page = self._makeOne(parent, (1, 2, 3), item_to_value) self.assertIs(page._parent, parent) self.assertEqual(page._num_items, 3) self.assertEqual(page._remaining, 3) + self.assertIs(page._item_to_value, item_to_value) def test_num_items_property(self): - page = self._makeOne(None, {}, '', None) + page = self._makeOne(None, (), None) num_items = 42 page._num_items = num_items self.assertEqual(page.num_items, num_items) def test_remaining_property(self): - page = self._makeOne(None, {}, '', None) + page = self._makeOne(None, (), None) remaining = 1337 page._remaining = remaining self.assertEqual(page.remaining, remaining) def test___iter__(self): - page = self._makeOne(None, {}, '', None) + page = self._makeOne(None, (), None) self.assertIs(iter(page), page) def test_iterator_calls__item_to_value(self): @@ -71,10 +71,8 @@ def item_to_value(self, item): self.calls += 1 return item - items_key = 'turkeys' - response = {items_key: [10, 11, 12]} parent = Parent() - page = self._makeOne(parent, response, items_key, + page = self._makeOne(parent, (10, 11, 12), Parent.item_to_value) page._remaining = 100 @@ -157,12 +155,9 @@ def test__items_iter(self): item3 = 211 # Make pages from mock responses - mock_key = 'mock' parent = object() - page1 = Page(parent, {mock_key: [item1, item2]}, - mock_key, self._do_nothing) - page2 = Page(parent, {mock_key: [item3]}, - mock_key, self._do_nothing) + page1 = Page(parent, (item1, item2), self._do_nothing) + page2 = Page(parent, (item3,), self._do_nothing) iterator = self._makeOne(None, None) # Fake the page iterator on the object. @@ -266,7 +261,7 @@ def test_pages_iter_empty_then_another(self): items_key = 'its-key' iterator = self._makeOne(None, None, None, items_key=items_key) # Fake the next page class. - fake_page = MUT.Page(None, {}, '', None) + fake_page = MUT.Page(None, (), None) page_args = [] def dummy_response(): @@ -282,7 +277,7 @@ def dummy_page_class(*args): page = six.next(pages_iter) self.assertIs(page, fake_page) self.assertEqual( - page_args, [(iterator, {}, items_key, iterator._item_to_value)]) + page_args, [(iterator, (), iterator._item_to_value)]) def test_iterate(self): import six diff --git a/pubsub/google/cloud/pubsub/_gax.py b/pubsub/google/cloud/pubsub/_gax.py index 06b8cec1faeb..2fd5810b6582 100644 --- a/pubsub/google/cloud/pubsub/_gax.py +++ b/pubsub/google/cloud/pubsub/_gax.py @@ -36,9 +36,6 @@ from google.cloud.pubsub.topic import Topic -_FAKE_ITEMS_KEY = 'not-a-key' - - class _PublisherAPI(object): """Helper mapping publisher-related APIs. @@ -589,9 +586,7 @@ def _recast_page_iterator(page_iter, iterator): :param iterator: The iterator that owns each page. """ for items in page_iter: - fake_response = {_FAKE_ITEMS_KEY: items} - page = Page( - iterator, fake_response, _FAKE_ITEMS_KEY, _item_to_topic) + page = Page(iterator, items, _item_to_topic) iterator.next_page_token = page_iter.page_token or None iterator.num_results += page.num_items yield page diff --git a/resource_manager/unit_tests/test_client.py b/resource_manager/unit_tests/test_client.py index 6a18614cdc5a..dd40f8b6877f 100644 --- a/resource_manager/unit_tests/test_client.py +++ b/resource_manager/unit_tests/test_client.py @@ -222,7 +222,7 @@ def test_page_empty_response(self): credentials = _Credentials() client = self._makeOne(credentials=credentials) iterator = client.list_projects() - page = Page(iterator, {}, iterator._items_key, None) + page = Page(iterator, (), None) iterator._page = page self.assertEqual(page.num_items, 0) self.assertEqual(page.remaining, 0) diff --git a/storage/unit_tests/test_bucket.py b/storage/unit_tests/test_bucket.py index 263a09e9df09..98fe5fcc4c2e 100644 --- a/storage/unit_tests/test_bucket.py +++ b/storage/unit_tests/test_bucket.py @@ -984,7 +984,7 @@ def test_page_empty_response(self): name = 'name' bucket = self._makeOne(client=client, name=name) iterator = bucket.list_blobs() - page = Page(iterator, {}, iterator._items_key, None) + page = Page(iterator, (), None) iterator._page = page blobs = list(page) self.assertEqual(blobs, []) diff --git a/storage/unit_tests/test_client.py b/storage/unit_tests/test_client.py index 2b55fff71968..61eeda051db5 100644 --- a/storage/unit_tests/test_client.py +++ b/storage/unit_tests/test_client.py @@ -376,7 +376,7 @@ def test_page_empty_response(self): credentials = _Credentials() client = self._makeOne(project=project, credentials=credentials) iterator = client.list_buckets() - page = Page(iterator, {}, iterator._items_key, None) + page = Page(iterator, (), None) iterator._page = page self.assertEqual(list(page), []) From 1ca98f1e116e169f4d3da2a2c0f6cd37b9b87619 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 16:43:27 -0700 Subject: [PATCH 08/14] Adding GAXIterator._wrap_gax for wrapping the GAX iterator. Also updating the _GAXPageIterator mock to allow multiple pages. --- core/google/cloud/_testing.py | 12 ++--- core/google/cloud/iterator.py | 20 ++++++- core/unit_tests/test_iterator.py | 91 +++++++++++++++++++++++++++++++- logging/unit_tests/test__gax.py | 14 ++--- pubsub/unit_tests/test__gax.py | 25 +++++---- 5 files changed, 136 insertions(+), 26 deletions(-) diff --git a/core/google/cloud/_testing.py b/core/google/cloud/_testing.py index ddaeb6c8c5a4..3c01825fa6f8 100644 --- a/core/google/cloud/_testing.py +++ b/core/google/cloud/_testing.py @@ -76,15 +76,13 @@ def _make_grpc_failed_precondition(self): class _GAXPageIterator(object): - def __init__(self, items, page_token): - self._items = items - self.page_token = page_token + def __init__(self, *pages, **kwargs): + self._pages = iter(pages) + self.page_token = kwargs.get('page_token') def next(self): - if self._items is None: - raise StopIteration - items, self._items = self._items, None - return items + import six + return six.next(self._pages) __next__ = next diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 8dcc3eba7e13..00d488bdfd23 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -406,4 +406,22 @@ def __init__(self, client, page_iter, item_to_value, super(GAXIterator, self).__init__( client, item_to_value, page_token=page_token, max_results=max_results) - self._page_iter = page_iter + self._page_iter = self._wrap_gax(page_iter) + + def _wrap_gax(self, page_iter): + """Generator of pages of API responses. + + Wraps each response from the :class:`~google.gax.PageIterator` in a + :class:`Page` instance and captures some state at each page. + + :type page_iter: :class:`~google.gax.PageIterator` + :param page_iter: The GAX page iterator to wrap. + + Yields :class:`Page` instances. + """ + for items in page_iter: + page = Page(self, items, self._item_to_value) + self.next_page_token = page_iter.page_token or None + if self._page_increment: + self.num_results += page.num_items + yield page diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index e1f1965bc0c6..5c636381d088 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -434,6 +434,8 @@ def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) def test_constructor(self): + import types + client = _Client(None) page_iter = object() item_to_value = object() @@ -446,13 +448,100 @@ def test_constructor(self): self.assertIs(iterator.client, client) self.assertIs(iterator._item_to_value, item_to_value) self.assertEqual(iterator.max_results, max_results) - self.assertIs(iterator._page_iter, page_iter) + self.assertIsInstance(iterator._page_iter, types.GeneratorType) self.assertFalse(iterator._page_increment) # Changing attributes. self.assertEqual(iterator.page_number, 0) self.assertEqual(iterator.next_page_token, token) self.assertEqual(iterator.num_results, 0) + @staticmethod + def _do_nothing(parent, value): + return parent, value + + def _wrap_gax_helper(self, page_increment=None): + import types + from google.cloud._testing import _GAXPageIterator + from google.cloud.iterator import Page + + iterator = self._makeOne(None, (), self._do_nothing) + if page_increment is not None: + iterator._page_increment = page_increment + # Make a mock ``google.gax.PageIterator`` + page_items = (29, 31) # Items for just one page. + page_token = '2sde98ds2s0hh' + page_iter = _GAXPageIterator(page_items, page_token=page_token) + wrapped = iterator._wrap_gax(page_iter) + self.assertIsInstance(wrapped, types.GeneratorType) + + pages = list(wrapped) + # First check the page token. + self.assertEqual(iterator.next_page_token, page_token) + # Then check the pages of results. + self.assertEqual(len(pages), 1) + page = pages[0] + self.assertIsInstance(page, Page) + # _do_nothing will throw the iterator in front. + expected = zip((iterator, iterator), page_items) + self.assertEqual(list(page), list(expected)) + return iterator + + def test__wrap_gax(self): + iterator = self._wrap_gax_helper() + # Make sure no page incrementing happend. + self.assertFalse(iterator._page_increment) + self.assertEqual(iterator.num_results, 0) + + def test__wrap_gax_with_increment(self): + iterator = self._wrap_gax_helper(True) + # Make sure no page incrementing happend. + self.assertTrue(iterator._page_increment) + self.assertEqual(iterator.num_results, 2) + + def test_iterate(self): + import six + from google.cloud._testing import _GAXPageIterator + + item1 = object() + item2 = object() + item3 = object() + token1 = 'smkdme30e2e32r' + token2 = '39cm9csl123dck' + + # Make a mock ``google.gax.PageIterator`` + page1 = (item1,) + page2 = (item2, item3) + page_iter = _GAXPageIterator(page1, page2, page_token=token1) + iterator = self._makeOne(None, page_iter, self._do_nothing) + + self.assertEqual(iterator.num_results, 0) + + items_iter = iter(iterator) + val1 = six.next(items_iter) + self.assertEqual(val1, (iterator, item1)) + self.assertEqual(iterator.num_results, 1) + self.assertEqual(iterator.next_page_token, token1) + + # Before grabbing the next page, hot-swap the token + # on the ``page_iter``. + page_iter.page_token = token2 + + # Grab the next item (which will cause the next page). + val2 = six.next(items_iter) + self.assertEqual(val2, (iterator, item2)) + self.assertEqual(iterator.num_results, 2) + self.assertEqual(iterator.next_page_token, token2) + + # Grab the final item from the final / current page. + val3 = six.next(items_iter) + self.assertEqual(val3, (iterator, item3)) + self.assertEqual(iterator.num_results, 3) + # Make sure the token did not change. + self.assertEqual(iterator.next_page_token, token2) + + with self.assertRaises(StopIteration): + six.next(items_iter) + class _Connection(object): diff --git a/logging/unit_tests/test__gax.py b/logging/unit_tests/test__gax.py index f0c3ec6595d3..1fd5d2fc9a22 100644 --- a/logging/unit_tests/test__gax.py +++ b/logging/unit_tests/test__gax.py @@ -69,7 +69,7 @@ def test_list_entries_no_paging(self): resource=resource_pb, timestamp=timestamp_pb, text_payload=TEXT) - response = _GAXPageIterator([entry_pb], TOKEN) + response = _GAXPageIterator([entry_pb], page_token=TOKEN) gax_api = _GAXLoggingAPI(_list_log_entries_response=response) api = self._makeOne(gax_api) @@ -110,7 +110,7 @@ def _list_entries_with_paging_helper(self, payload, struct_pb): resource=resource_pb, timestamp=timestamp_pb, json_payload=struct_pb) - response = _GAXPageIterator([entry_pb], NEW_TOKEN) + response = _GAXPageIterator([entry_pb], page_token=NEW_TOKEN) gax_api = _GAXLoggingAPI(_list_log_entries_response=response) api = self._makeOne(gax_api) @@ -237,7 +237,7 @@ def test_list_entries_with_extra_properties(self): entry_pb = self._make_log_entry_with_extras( LABELS, IID, bool_type_url, NOW) - response = _GAXPageIterator([entry_pb], NEW_TOKEN) + response = _GAXPageIterator([entry_pb], page_token=NEW_TOKEN) gax_api = _GAXLoggingAPI(_list_log_entries_response=response) api = self._makeOne(gax_api) @@ -598,7 +598,7 @@ def test_list_sinks_no_paging(self): sink_pb = LogSink(name=self.SINK_PATH, destination=self.DESTINATION_URI, filter=self.FILTER) - response = _GAXPageIterator([sink_pb], TOKEN) + response = _GAXPageIterator([sink_pb], page_token=TOKEN) gax_api = _GAXSinksAPI(_list_sinks_response=response) api = self._makeOne(gax_api) @@ -626,7 +626,7 @@ def test_list_sinks_w_paging(self): sink_pb = LogSink(name=self.SINK_PATH, destination=self.DESTINATION_URI, filter=self.FILTER) - response = _GAXPageIterator([sink_pb], None) + response = _GAXPageIterator([sink_pb]) gax_api = _GAXSinksAPI(_list_sinks_response=response) api = self._makeOne(gax_api) @@ -813,7 +813,7 @@ def test_list_metrics_no_paging(self): metric_pb = LogMetric(name=self.METRIC_PATH, description=self.DESCRIPTION, filter=self.FILTER) - response = _GAXPageIterator([metric_pb], TOKEN) + response = _GAXPageIterator([metric_pb], page_token=TOKEN) gax_api = _GAXMetricsAPI(_list_log_metrics_response=response) api = self._makeOne(gax_api) @@ -841,7 +841,7 @@ def test_list_metrics_w_paging(self): metric_pb = LogMetric(name=self.METRIC_PATH, description=self.DESCRIPTION, filter=self.FILTER) - response = _GAXPageIterator([metric_pb], None) + response = _GAXPageIterator([metric_pb]) gax_api = _GAXMetricsAPI(_list_log_metrics_response=response) api = self._makeOne(gax_api) diff --git a/pubsub/unit_tests/test__gax.py b/pubsub/unit_tests/test__gax.py index 907bdecb5c89..06e2b87adb78 100644 --- a/pubsub/unit_tests/test__gax.py +++ b/pubsub/unit_tests/test__gax.py @@ -61,7 +61,8 @@ def test_list_topics_no_paging(self): from google.cloud.pubsub.topic import Topic TOKEN = 'TOKEN' - response = _GAXPageIterator([_TopicPB(self.TOPIC_PATH)], TOKEN) + response = _GAXPageIterator([_TopicPB(self.TOPIC_PATH)], + page_token=TOKEN) gax_api = _GAXPublisherAPI(_list_topics_response=response) client = _Client(self.PROJECT) api = self._makeOne(gax_api, client) @@ -90,7 +91,7 @@ def test_list_topics_with_paging(self): TOKEN = 'TOKEN' NEW_TOKEN = 'NEW_TOKEN' response = _GAXPageIterator( - [_TopicPB(self.TOPIC_PATH)], NEW_TOKEN) + [_TopicPB(self.TOPIC_PATH)], page_token=NEW_TOKEN) gax_api = _GAXPublisherAPI(_list_topics_response=response) client = _Client(self.PROJECT) api = self._makeOne(gax_api, client) @@ -291,8 +292,8 @@ def test_topic_publish_error(self): def test_topic_list_subscriptions_no_paging(self): from google.gax import INITIAL_PAGE from google.cloud._testing import _GAXPageIterator - response = _GAXPageIterator([ - {'name': self.SUB_PATH, 'topic': self.TOPIC_PATH}], None) + response = _GAXPageIterator( + [{'name': self.SUB_PATH, 'topic': self.TOPIC_PATH}]) gax_api = _GAXPublisherAPI(_list_topic_subscriptions_response=response) client = _Client(self.PROJECT) api = self._makeOne(gax_api, client) @@ -318,8 +319,9 @@ def test_topic_list_subscriptions_with_paging(self): SIZE = 23 TOKEN = 'TOKEN' NEW_TOKEN = 'NEW_TOKEN' - response = _GAXPageIterator([ - {'name': self.SUB_PATH, 'topic': self.TOPIC_PATH}], NEW_TOKEN) + response = _GAXPageIterator( + [{'name': self.SUB_PATH, 'topic': self.TOPIC_PATH}], + page_token=NEW_TOKEN) gax_api = _GAXPublisherAPI(_list_topic_subscriptions_response=response) client = _Client(self.PROJECT) api = self._makeOne(gax_api, client) @@ -390,8 +392,10 @@ def test_ctor(self): def test_list_subscriptions_no_paging(self): from google.gax import INITIAL_PAGE from google.cloud._testing import _GAXPageIterator - response = _GAXPageIterator([_SubscriptionPB( - self.SUB_PATH, self.TOPIC_PATH, self.PUSH_ENDPOINT, 0)], None) + + sub_pb = _SubscriptionPB( + self.SUB_PATH, self.TOPIC_PATH, self.PUSH_ENDPOINT, 0) + response = _GAXPageIterator([sub_pb]) gax_api = _GAXSubscriberAPI(_list_subscriptions_response=response) api = self._makeOne(gax_api) @@ -417,8 +421,9 @@ def test_list_subscriptions_with_paging(self): SIZE = 23 TOKEN = 'TOKEN' NEW_TOKEN = 'NEW_TOKEN' - response = _GAXPageIterator([_SubscriptionPB( - self.SUB_PATH, self.TOPIC_PATH, self.PUSH_ENDPOINT, 0)], NEW_TOKEN) + sub_pb = _SubscriptionPB( + self.SUB_PATH, self.TOPIC_PATH, self.PUSH_ENDPOINT, 0) + response = _GAXPageIterator([sub_pb], page_token=NEW_TOKEN) gax_api = _GAXSubscriberAPI(_list_subscriptions_response=response) api = self._makeOne(gax_api) From f1126c7dc602e03c73d133df77a26002fd46530c Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 16:54:18 -0700 Subject: [PATCH 09/14] Moving pubsub _gax impl. of list_topics to GAXIterator. --- pubsub/google/cloud/pubsub/_gax.py | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/pubsub/google/cloud/pubsub/_gax.py b/pubsub/google/cloud/pubsub/_gax.py index 2fd5810b6582..fcc56838bff7 100644 --- a/pubsub/google/cloud/pubsub/_gax.py +++ b/pubsub/google/cloud/pubsub/_gax.py @@ -14,8 +14,6 @@ """GAX wrapper for Pubsub API requests.""" -import functools - from google.cloud.gapic.pubsub.v1.publisher_api import PublisherApi from google.cloud.gapic.pubsub.v1.subscriber_api import SubscriberApi from google.gax import CallOptions @@ -31,7 +29,7 @@ from google.cloud._helpers import _pb_timestamp_to_rfc3339 from google.cloud.exceptions import Conflict from google.cloud.exceptions import NotFound -from google.cloud.iterator import HTTPIterator +from google.cloud.iterator import GAXIterator from google.cloud.iterator import Page from google.cloud.pubsub.topic import Topic @@ -78,11 +76,8 @@ def list_topics(self, project, page_size=0, page_token=None): path = 'projects/%s' % (project,) page_iter = self._gax_api.list_topics( path, page_size=page_size, options=options) - page_iter = functools.partial(_recast_page_iterator, page_iter) - return HTTPIterator( - client=self._client, path=path, item_to_value=_item_to_topic, - page_iter=page_iter) + return GAXIterator(self._client, page_iter, _item_to_topic) def topic_create(self, topic_path): """API call: create a topic @@ -569,24 +564,3 @@ def _item_to_topic(iterator, resource): """ return Topic.from_api_repr( {'name': resource.name}, iterator.client) - - -def _recast_page_iterator(page_iter, iterator): - """Wrap GAX pages generator. - - In particular, wrap each page and capture some state from the - GAX iterator. - - Yields :class:`~google.cloud.iterator.Page` instances - - :type page_iter: :class:`~google.gax.PageIterator` - :param page_iter: The iterator to wrap. - - :type iterator: :class:`~google.cloud.iterator.Iterator` - :param iterator: The iterator that owns each page. - """ - for items in page_iter: - page = Page(iterator, items, _item_to_topic) - iterator.next_page_token = page_iter.page_token or None - iterator.num_results += page.num_items - yield page From a909eca53dd852a8a5976a8d479b73bfe51e617f Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 16:58:44 -0700 Subject: [PATCH 10/14] Removing page_iter over-ride from HTTPIterator. --- core/google/cloud/iterator.py | 17 +++-------------- core/unit_tests/test_iterator.py | 16 ---------------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index 00d488bdfd23..d4c12491ce25 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -271,14 +271,6 @@ class HTTPIterator(Iterator): the :class:`Page` that was started and the dictionary containing the page response. - :type page_iter: callable - :param page_iter: (Optional) Callable to produce a pages iterator from the - current iterator. Assumed signature takes the - :class:`Iterator` that started the page. By default uses - the HTTP pages iterator. Meant to provide a custom - way to create pages (potentially with a custom - transport such as gRPC). - .. autoattribute:: pages """ @@ -289,7 +281,7 @@ class HTTPIterator(Iterator): def __init__(self, client, path, item_to_value, items_key=DEFAULT_ITEMS_KEY, page_token=None, max_results=None, extra_params=None, - page_start=_do_nothing_page_start, page_iter=None): + page_start=_do_nothing_page_start): super(HTTPIterator, self).__init__( client, item_to_value, page_token=page_token, max_results=max_results) @@ -300,10 +292,7 @@ def __init__(self, client, path, item_to_value, # Verify inputs / provide defaults. if self.extra_params is None: self.extra_params = {} - if page_iter is None: - self._page_iter = self._default_page_iter() - else: - self._page_iter = page_iter(self) + self._page_iter = self._http_page_iter() self._verify_params() def _verify_params(self): @@ -317,7 +306,7 @@ def _verify_params(self): raise ValueError('Using a reserved parameter', reserved_in_use) - def _default_page_iter(self): + def _http_page_iter(self): """Generator of pages of API responses. Yields :class:`Page` instances. diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 5c636381d088..025af7f76f57 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -237,22 +237,6 @@ def test_constructor_w_extra_param_collision(self): with self.assertRaises(ValueError): self._makeOne(client, path, None, extra_params=extra_params) - def test_constructor_non_default_page_iter(self): - connection = _Connection() - client = _Client(connection) - path = '/foo' - result = object() - called = [] - - def page_iter(iterator): - called.append(iterator) - return result - - iterator = self._makeOne(client, path, None, - page_iter=page_iter) - self.assertEqual(called, [iterator]) - self.assertIs(iterator._page_iter, result) - def test_pages_iter_empty_then_another(self): import six from google.cloud._testing import _Monkey From 5741e158807d2589aae44dcfd787d70e376e15bf Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 17:23:20 -0700 Subject: [PATCH 11/14] Removing page_token from GAXIterator constructor. Instead, using the page token directly from the page iterator passed in (this may occasionally be strange to a user, e.g. if the token is INITIAL_PAGE). --- core/google/cloud/iterator.py | 8 ++------ core/unit_tests/test_iterator.py | 14 ++++++++++---- pubsub/google/cloud/pubsub/_gax.py | 6 +++++- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index d4c12491ce25..ba161192709a 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -381,19 +381,15 @@ class GAXIterator(Iterator): takes an :class:`Iterator` and a single item from the API response as a protobuf. - :type page_token: str - :param page_token: (Optional) A token identifying a page in a result set. - :type max_results: int :param max_results: (Optional) The maximum number of results to fetch. .. autoattribute:: pages """ - def __init__(self, client, page_iter, item_to_value, - page_token=None, max_results=None): + def __init__(self, client, page_iter, item_to_value, max_results=None): super(GAXIterator, self).__init__( - client, item_to_value, page_token=page_token, + client, item_to_value, page_token=page_iter.page_token, max_results=max_results) self._page_iter = self._wrap_gax(page_iter) diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 025af7f76f57..bc64f80af5fd 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -421,12 +421,12 @@ def test_constructor(self): import types client = _Client(None) - page_iter = object() - item_to_value = object() token = 'zzzyy78kl' + page_iter = SimpleIter(token) + item_to_value = object() max_results = 1337 iterator = self._makeOne(client, page_iter, item_to_value, - page_token=token, max_results=max_results) + max_results=max_results) self.assertFalse(iterator._started) self.assertIs(iterator.client, client) @@ -448,7 +448,7 @@ def _wrap_gax_helper(self, page_increment=None): from google.cloud._testing import _GAXPageIterator from google.cloud.iterator import Page - iterator = self._makeOne(None, (), self._do_nothing) + iterator = self._makeOne(None, SimpleIter(), self._do_nothing) if page_increment is not None: iterator._page_increment = page_increment # Make a mock ``google.gax.PageIterator`` @@ -543,3 +543,9 @@ class _Client(object): def __init__(self, connection): self.connection = connection + + +class SimpleIter(object): + + def __init__(self, page_token=None): + self.page_token = page_token diff --git a/pubsub/google/cloud/pubsub/_gax.py b/pubsub/google/cloud/pubsub/_gax.py index fcc56838bff7..a392cc679bf5 100644 --- a/pubsub/google/cloud/pubsub/_gax.py +++ b/pubsub/google/cloud/pubsub/_gax.py @@ -77,7 +77,11 @@ def list_topics(self, project, page_size=0, page_token=None): page_iter = self._gax_api.list_topics( path, page_size=page_size, options=options) - return GAXIterator(self._client, page_iter, _item_to_topic) + iter_kwargs = {} + if page_size: # page_size can be 0 or explicit None. + iter_kwargs['max_results'] = page_size + return GAXIterator(self._client, page_iter, _item_to_topic, + **iter_kwargs) def topic_create(self, topic_path): """API call: create a topic From f17783d68f6d2d5705a54aeef61f27af6f827d4c Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 18:01:10 -0700 Subject: [PATCH 12/14] Adding _next_page() methods in iterator subclasses. This somewhat of a cart-before-the-horse change, but is done this way to make the commit easier to understand, before unifying the two approaches via _next_page(). --- core/google/cloud/iterator.py | 46 +++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index ba161192709a..b51b8d1924d9 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -306,19 +306,33 @@ def _verify_params(self): raise ValueError('Using a reserved parameter', reserved_in_use) - def _http_page_iter(self): - """Generator of pages of API responses. + def _next_page(self): + """Get the next page in the iterator. - Yields :class:`Page` instances. + :rtype: :class:`Page` + :returns: The next page in the iterator (or :data:`None` if + there are no pages left). """ - while self._has_next_page(): + if self._has_next_page(): response = self._get_next_page_response() items = response.get(self._items_key, ()) page = Page(self, items, self._item_to_value) self._page_start(self, page, response) + return page + else: + return None + + def _http_page_iter(self): + """Generator of pages of API responses. + + Yields :class:`Page` instances. + """ + page = self._next_page() + while page is not None: if self._page_increment: self.num_results += page.num_items yield page + page = self._next_page() def _has_next_page(self): """Determines whether or not there are more pages with results. @@ -393,6 +407,24 @@ def __init__(self, client, page_iter, item_to_value, max_results=None): max_results=max_results) self._page_iter = self._wrap_gax(page_iter) + def _next_page(self, page_iter): + """Get the next page in the iterator. + + :type page_iter: :class:`~google.gax.PageIterator` + :param page_iter: The GAX page iterator to consume. + + :rtype: :class:`Page` + :returns: The next page in the iterator (or :data:`None` if + there are no pages left). + """ + try: + items = six.next(page_iter) + page = Page(self, items, self._item_to_value) + self.next_page_token = page_iter.page_token or None + return page + except StopIteration: + return None + def _wrap_gax(self, page_iter): """Generator of pages of API responses. @@ -404,9 +436,9 @@ def _wrap_gax(self, page_iter): Yields :class:`Page` instances. """ - for items in page_iter: - page = Page(self, items, self._item_to_value) - self.next_page_token = page_iter.page_token or None + page = self._next_page(page_iter) + while page is not None: if self._page_increment: self.num_results += page.num_items yield page + page = self._next_page(page_iter) From 95bdd255c0ddb5ef408c0347adac37902e32bcce Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 18:06:14 -0700 Subject: [PATCH 13/14] Moving Iterator page incrementing into page iterator. Also moving the token setting behavior from HTTPIterator._get_next_page_response() into _next_page(). --- core/google/cloud/iterator.py | 10 ++++------ core/unit_tests/test_iterator.py | 2 -- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index b51b8d1924d9..d1b240a1b467 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -318,6 +318,7 @@ def _next_page(self): items = response.get(self._items_key, ()) page = Page(self, items, self._item_to_value) self._page_start(self, page, response) + self.next_page_token = response.get('nextPageToken') return page else: return None @@ -329,6 +330,7 @@ def _http_page_iter(self): """ page = self._next_page() while page is not None: + self.page_number += 1 if self._page_increment: self.num_results += page.num_items yield page @@ -369,15 +371,10 @@ def _get_next_page_response(self): :rtype: dict :returns: The parsed JSON response of the next page's contents. """ - response = self.client.connection.api_request( + return self.client.connection.api_request( method='GET', path=self.path, query_params=self._get_query_params()) - self.page_number += 1 - self.next_page_token = response.get('nextPageToken') - - return response - class GAXIterator(Iterator): """A generic class for iterating through Cloud gRPC APIs list responses. @@ -438,6 +435,7 @@ def _wrap_gax(self, page_iter): """ page = self._next_page(page_iter) while page is not None: + self.page_number += 1 if self._page_increment: self.num_results += page.num_items yield page diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index bc64f80af5fd..55423641dce0 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -400,8 +400,6 @@ def test__get_next_page_response_new_no_token_in_response(self): iterator = self._makeOne(client, path, None) response = iterator._get_next_page_response() self.assertEqual(response['items'], [{'name': key1}, {'name': key2}]) - self.assertEqual(iterator.page_number, 1) - self.assertEqual(iterator.next_page_token, token) kw, = connection._requested self.assertEqual(kw['method'], 'GET') self.assertEqual(kw['path'], path) From 9e71744ecc1b6f6c5931bb02cb3abdb08c7fca8c Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Oct 2016 18:36:58 -0700 Subject: [PATCH 14/14] Unifying Iterator paging via _next_page(). Also a lint fix for an unimorted member and a unit test fix adding a page token to allow more paging. --- core/google/cloud/iterator.py | 90 ++++++++++++---------------- core/unit_tests/test_iterator.py | 95 ++++++++++++++++++------------ pubsub/google/cloud/pubsub/_gax.py | 1 - storage/unit_tests/test_bucket.py | 1 + 4 files changed, 98 insertions(+), 89 deletions(-) diff --git a/core/google/cloud/iterator.py b/core/google/cloud/iterator.py index d1b240a1b467..78a781188063 100644 --- a/core/google/cloud/iterator.py +++ b/core/google/cloud/iterator.py @@ -187,15 +187,6 @@ def __init__(self, client, item_to_value, self.client = client self._item_to_value = item_to_value self.max_results = max_results - # NOTE: The _page_iter is not intended to come through the - # constructor, instead subclasses should over-ride - # this property. - self._page_iter = iter(()) - # NOTE: This flag indicates if the total number of results should be - # incremented. This is useful since a page iterator will - # want to increment by results per page while an items - # iterator will want to increment per item. - self._page_increment = False # The attributes below will change over the life of the iterator. self.page_number = 0 self.next_page_token = page_token @@ -212,12 +203,11 @@ def pages(self): if self._started: raise ValueError('Iterator has already started', self) self._started = True - self._page_increment = True - return self._page_iter + return self._page_iter(increment=True) def _items_iter(self): """Iterator for each item returned.""" - for page in self._page_iter: + for page in self._page_iter(increment=False): for item in page: self.num_results += 1 yield item @@ -234,6 +224,37 @@ def __iter__(self): self._started = True return self._items_iter() + def _page_iter(self, increment): + """Generator of pages of API responses. + + :type increment: bool + :param increment: Flag indicating if the total number of results + should be incremented on each page. This is useful + since a page iterator will want to increment by + results per page while an items iterator will want + to increment per item. + + Yields :class:`Page` instances. + """ + page = self._next_page() + while page is not None: + self.page_number += 1 + if increment: + self.num_results += page.num_items + yield page + page = self._next_page() + + @staticmethod + def _next_page(): + """Get the next page in the iterator. + + This does nothing and is intended to be over-ridden by subclasses + to return the next :class:`Page`. + + :raises NotImplementedError: Always. + """ + raise NotImplementedError + class HTTPIterator(Iterator): """A generic class for iterating through Cloud JSON APIs list responses. @@ -292,7 +313,6 @@ def __init__(self, client, path, item_to_value, # Verify inputs / provide defaults. if self.extra_params is None: self.extra_params = {} - self._page_iter = self._http_page_iter() self._verify_params() def _verify_params(self): @@ -323,19 +343,6 @@ def _next_page(self): else: return None - def _http_page_iter(self): - """Generator of pages of API responses. - - Yields :class:`Page` instances. - """ - page = self._next_page() - while page is not None: - self.page_number += 1 - if self._page_increment: - self.num_results += page.num_items - yield page - page = self._next_page() - def _has_next_page(self): """Determines whether or not there are more pages with results. @@ -402,41 +409,22 @@ def __init__(self, client, page_iter, item_to_value, max_results=None): super(GAXIterator, self).__init__( client, item_to_value, page_token=page_iter.page_token, max_results=max_results) - self._page_iter = self._wrap_gax(page_iter) + self._gax_page_iter = page_iter - def _next_page(self, page_iter): + def _next_page(self): """Get the next page in the iterator. - :type page_iter: :class:`~google.gax.PageIterator` - :param page_iter: The GAX page iterator to consume. + Wraps the response from the :class:`~google.gax.PageIterator` in a + :class:`Page` instance and captures some state at each page. :rtype: :class:`Page` :returns: The next page in the iterator (or :data:`None` if there are no pages left). """ try: - items = six.next(page_iter) + items = six.next(self._gax_page_iter) page = Page(self, items, self._item_to_value) - self.next_page_token = page_iter.page_token or None + self.next_page_token = self._gax_page_iter.page_token or None return page except StopIteration: return None - - def _wrap_gax(self, page_iter): - """Generator of pages of API responses. - - Wraps each response from the :class:`~google.gax.PageIterator` in a - :class:`Page` instance and captures some state at each page. - - :type page_iter: :class:`~google.gax.PageIterator` - :param page_iter: The GAX page iterator to wrap. - - Yields :class:`Page` instances. - """ - page = self._next_page(page_iter) - while page is not None: - self.page_number += 1 - if self._page_increment: - self.num_results += page.num_items - yield page - page = self._next_page(page_iter) diff --git a/core/unit_tests/test_iterator.py b/core/unit_tests/test_iterator.py index 55423641dce0..fa54a13be28f 100644 --- a/core/unit_tests/test_iterator.py +++ b/core/unit_tests/test_iterator.py @@ -111,8 +111,6 @@ def test_constructor(self): self.assertIs(iterator.client, client) self.assertIs(iterator._item_to_value, item_to_value) self.assertEqual(iterator.max_results, max_results) - self.assertEqual(list(iterator._page_iter), []) - self.assertFalse(iterator._page_increment) # Changing attributes. self.assertEqual(iterator.page_number, 0) self.assertEqual(iterator.next_page_token, token) @@ -122,21 +120,32 @@ def test_pages_property(self): iterator = self._makeOne(None, None) self.assertFalse(iterator._started) mock_iter = object() - iterator._page_iter = mock_iter + incremented = [] + + def page_iter(increment): + incremented.append(increment) + return mock_iter + + iterator._page_iter = page_iter self.assertIs(iterator.pages, mock_iter) + self.assertEqual(incremented, [True]) # Check the side-effect. self.assertTrue(iterator._started) def test_pages_property_started(self): + import types + iterator = self._makeOne(None, None) - self.assertEqual(list(iterator.pages), []) + self.assertIsInstance(iterator.pages, types.GeneratorType) # Make sure we cannot restart. with self.assertRaises(ValueError): getattr(iterator, 'pages') def test_pages_property_items_started(self): + import types + iterator = self._makeOne(None, None) - self.assertEqual(list(iterator), []) + self.assertIsInstance(iter(iterator), types.GeneratorType) with self.assertRaises(ValueError): getattr(iterator, 'pages') @@ -161,8 +170,13 @@ def test__items_iter(self): iterator = self._makeOne(None, None) # Fake the page iterator on the object. - iterator._page_iter = iter((page1, page2)) + incremented = [] + + def page_iter(increment): + incremented.append(increment) + return iter((page1, page2)) + iterator._page_iter = page_iter items_iter = iterator._items_iter() # Make sure it is a generator. self.assertIsInstance(items_iter, types.GeneratorType) @@ -178,27 +192,44 @@ def test__items_iter(self): with self.assertRaises(StopIteration): six.next(items_iter) + # Make sure our page_iter() was called correctly. + self.assertEqual(incremented, [False]) + def test___iter__(self): iterator = self._makeOne(None, None) self.assertFalse(iterator._started) - mock_iter = object() - iterator._page_iter = mock_iter - self.assertIs(iterator.pages, mock_iter) + incremented = [] + + def page_iter(increment): + incremented.append(increment) + return iter(()) + + iterator._page_iter = page_iter + self.assertEqual(list(iterator), []) # Check the side-effect. self.assertTrue(iterator._started) def test___iter___started(self): + import types + iterator = self._makeOne(None, None) - self.assertEqual(list(iterator), []) + self.assertIsInstance(iter(iterator), types.GeneratorType) with self.assertRaises(ValueError): iter(iterator) def test___iter___pages_started(self): + import types + iterator = self._makeOne(None, None) - self.assertEqual(list(iterator.pages), []) + self.assertIsInstance(iterator.pages, types.GeneratorType) with self.assertRaises(ValueError): iter(iterator) + def test__next_page_virtual(self): + iterator = self._makeOne(None, None) + with self.assertRaises(NotImplementedError): + iterator._next_page() + class TestHTTPIterator(unittest.TestCase): @@ -416,8 +447,6 @@ def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) def test_constructor(self): - import types - client = _Client(None) token = 'zzzyy78kl' page_iter = SimpleIter(token) @@ -430,8 +459,7 @@ def test_constructor(self): self.assertIs(iterator.client, client) self.assertIs(iterator._item_to_value, item_to_value) self.assertEqual(iterator.max_results, max_results) - self.assertIsInstance(iterator._page_iter, types.GeneratorType) - self.assertFalse(iterator._page_increment) + self.assertIs(iterator._gax_page_iter, page_iter) # Changing attributes. self.assertEqual(iterator.page_number, 0) self.assertEqual(iterator.next_page_token, token) @@ -441,44 +469,37 @@ def test_constructor(self): def _do_nothing(parent, value): return parent, value - def _wrap_gax_helper(self, page_increment=None): - import types + def test__next_page(self): from google.cloud._testing import _GAXPageIterator from google.cloud.iterator import Page - iterator = self._makeOne(None, SimpleIter(), self._do_nothing) - if page_increment is not None: - iterator._page_increment = page_increment # Make a mock ``google.gax.PageIterator`` page_items = (29, 31) # Items for just one page. page_token = '2sde98ds2s0hh' page_iter = _GAXPageIterator(page_items, page_token=page_token) - wrapped = iterator._wrap_gax(page_iter) - self.assertIsInstance(wrapped, types.GeneratorType) + # Wrap the GAX iterator. + iterator = self._makeOne(None, page_iter, self._do_nothing) - pages = list(wrapped) + page = iterator._next_page() # First check the page token. self.assertEqual(iterator.next_page_token, page_token) - # Then check the pages of results. - self.assertEqual(len(pages), 1) - page = pages[0] + # Then check the page. self.assertIsInstance(page, Page) # _do_nothing will throw the iterator in front. expected = zip((iterator, iterator), page_items) self.assertEqual(list(page), list(expected)) - return iterator - def test__wrap_gax(self): - iterator = self._wrap_gax_helper() - # Make sure no page incrementing happend. - self.assertFalse(iterator._page_increment) - self.assertEqual(iterator.num_results, 0) + def test__next_page_empty(self): + from google.cloud._testing import _GAXPageIterator - def test__wrap_gax_with_increment(self): - iterator = self._wrap_gax_helper(True) - # Make sure no page incrementing happend. - self.assertTrue(iterator._page_increment) - self.assertEqual(iterator.num_results, 2) + # Make a mock ``google.gax.PageIterator`` + page_iter = _GAXPageIterator() + # Wrap the GAX iterator. + iterator = self._makeOne(None, page_iter, self._do_nothing) + + page = iterator._next_page() + self.assertIsNone(page) + self.assertIsNone(iterator.next_page_token) def test_iterate(self): import six diff --git a/pubsub/google/cloud/pubsub/_gax.py b/pubsub/google/cloud/pubsub/_gax.py index a392cc679bf5..b6b357cbdc04 100644 --- a/pubsub/google/cloud/pubsub/_gax.py +++ b/pubsub/google/cloud/pubsub/_gax.py @@ -30,7 +30,6 @@ from google.cloud.exceptions import Conflict from google.cloud.exceptions import NotFound from google.cloud.iterator import GAXIterator -from google.cloud.iterator import Page from google.cloud.pubsub.topic import Topic diff --git a/storage/unit_tests/test_bucket.py b/storage/unit_tests/test_bucket.py index 98fe5fcc4c2e..53a79884f0b6 100644 --- a/storage/unit_tests/test_bucket.py +++ b/storage/unit_tests/test_bucket.py @@ -1024,6 +1024,7 @@ def test_cumulative_prefixes(self): response1 = { 'items': [{'name': BLOB_NAME}], 'prefixes': ['foo'], + 'nextPageToken': 's39rmf9', } response2 = { 'items': [],