From 1141010c3de40202a65c93fe0c1801d6309f9816 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 25 Jan 2021 18:14:00 +0000 Subject: [PATCH 1/6] fix: update paging implementation to handle unconventional pagination --- gapic/schema/wrappers.py | 9 +- .../%sub/services/%service/pagers.py.j2 | 14 ++- .../%name_%version/%sub/test_%service.py.j2 | 119 +++++++++++++++++- tests/unit/schema/wrappers/test_method.py | 14 ++- 4 files changed, 149 insertions(+), 7 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index eefe0cdc7e..94f29be8a0 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -866,12 +866,17 @@ def paged_result_field(self) -> Optional[Field]: """Return the response pagination field if the method is paginated.""" # If the request field lacks any of the expected pagination fields, # then the method is not paginated. - for page_field in ((self.input, int, 'page_size'), - (self.input, str, 'page_token'), + for page_field in ((self.input, str, 'page_token'), (self.output, str, 'next_page_token')): field = page_field[0].fields.get(page_field[2], None) if not field or field.type != page_field[1]: return None + page_fields = [self.input.fields.get('max_results', None), + self.input.fields.get('page_size', None)] + page_field = next( + (field for field in page_fields if field is not None), None) + if not page_field or page_field.type != int: + return None # Return the first repeated field. for field in self.output.fields.values(): diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 index ea08466ba0..5f04eed61e 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 @@ -6,7 +6,7 @@ {# This lives within the loop in order to ensure that this template is empty if there are no paged methods. -#} -from typing import Any, AsyncIterable, Awaitable, Callable, Iterable, Sequence, Tuple +from typing import Any, AsyncIterable, Awaitable, Callable, Iterable, Sequence, Tuple, Optional {% filter sort_lines -%} {% for method in service.methods.values() | selectattr('paged_result_field') -%} @@ -68,14 +68,25 @@ class {{ method.name }}Pager: self._response = self._method(self._request, metadata=self._metadata) yield self._response + {% if method.paged_result_field.map %} + def __iter__(self) -> Iterable[Tuple[str, {{ method.paged_result_field.ident | replace('Sequence[', '') | replace(']', '') }}]]: + for page in self.pages: + yield from page.{{ method.paged_result_field.name}}.items() + + def get(self, key: str) -> {{ method.paged_result_field.ident | replace('Sequence', 'Optional') }}: + return self._response.items.get(key) + {% else %} def __iter__(self) -> {{ method.paged_result_field.ident | replace('Sequence', 'Iterable') }}: for page in self.pages: yield from page.{{ method.paged_result_field.name }} + {% endif %} def __repr__(self) -> str: return '{0}<{1!r}>'.format(self.__class__.__name__, self._response) +{# TODO(yon-mg): remove on rest async transport impl #} +{% if 'grpc' in opts.transport %} class {{ method.name }}AsyncPager: """A pager for iterating through ``{{ method.name|snake_case }}`` requests. @@ -138,5 +149,6 @@ class {{ method.name }}AsyncPager: def __repr__(self) -> str: return '{0}<{1!r}>'.format(self.__class__.__name__, self._response) +{% endif %} {% endfor %} {% endblock %} diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 6affa40bc8..b99aa34262 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -1029,7 +1029,7 @@ def test_{{ method.name|snake_case }}_raw_page_lro(): assert response.raw_page is response {% endif %} {#- method.paged_result_field #} -{% endfor -%} {#- method in methods #} +{% endfor -%} {#- method in methods for grpc #} {% for method in service.methods.values() if 'rest' in opts.transport -%} def test_{{ method.name|snake_case }}_rest(transport: str = 'rest', request_type={{ method.input.ident }}): @@ -1169,7 +1169,122 @@ def test_{{ method.name|snake_case }}_rest_flattened_error(): ) -{% endfor -%} +{% if method.paged_result_field %} +def test_{{ method.name|snake_case }}_pager(): + client = {{ service.client_name }}( + credentials=credentials.AnonymousCredentials(), + ) + + # Mock the http request call within the method and fake a response. + with mock.patch.object(Session, 'request') as req: + # Set the response as a series of pages + {% if method.paged_result_field.map%} + response = ( + {{ method.output.ident }}( + {{ method.paged_result_field.name }}={ + 'a':{{ method.paged_result_field.type.fields.get('value').ident }}(), + 'b':{{ method.paged_result_field.type.fields.get('value').ident }}(), + 'c':{{ method.paged_result_field.type.fields.get('value').ident }}(), + }, + next_page_token='abc', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}={}, + next_page_token='def', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}={ + 'g':{{ method.paged_result_field.type.fields.get('value').ident }}(), + }, + next_page_token='ghi', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}={ + 'h':{{ method.paged_result_field.type.fields.get('value').ident }}(), + 'i':{{ method.paged_result_field.type.fields.get('value').ident }}(), + }, + ), + ) + {% else %} + response = ( + {{ method.output.ident }}( + {{ method.paged_result_field.name }}=[ + {{ method.paged_result_field.type.ident }}(), + {{ method.paged_result_field.type.ident }}(), + {{ method.paged_result_field.type.ident }}(), + ], + next_page_token='abc', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}=[], + next_page_token='def', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}=[ + {{ method.paged_result_field.type.ident }}(), + ], + next_page_token='ghi', + ), + {{ method.output.ident }}( + {{ method.paged_result_field.name }}=[ + {{ method.paged_result_field.type.ident }}(), + {{ method.paged_result_field.type.ident }}(), + ], + ), + ) + {% endif %} + # Two responses for two calls + response = response + response + + # Wrap the values into proper Response objs + response = tuple(map(lambda x: {{ method.output.ident }}.to_json(x), response)) + side_effect = tuple(map(lambda x: Response(), response)) + for return_val, response_val in zip(side_effect, response): + return_val._content = response_val.encode('UTF-8') + req.side_effect = side_effect + + metadata = () + {% if method.field_headers -%} + metadata = tuple(metadata) + ( + gapic_v1.routing_header.to_grpc_metadata(( + {%- for field_header in method.field_headers %} + {%- if not method.client_streaming %} + ('{{ field_header }}', ''), + {%- endif %} + {%- endfor %} + )), + ) + {% endif -%} + pager = client.{{ method.name|snake_case }}(request={}) + + assert pager._metadata == metadata + + {% if method.paged_result_field.map %} + assert isinstance(pager.get('a'), {{ method.paged_result_field.type.fields.get('value').ident }}) + assert pager.get('h') is None + {% endif %} + + results = [i for i in pager] + assert len(results) == 6 + {% if method.paged_result_field.map %} + assert all( + isinstance(i, tuple) and + tuple(map(lambda x: type(x), results[0])) == (str, {{ method.paged_result_field.type.fields.get('value').ident }}) + for i in results) + assert pager.get('a') is None + assert isinstance(pager.get('h'), {{ method.paged_result_field.type.fields.get('value').ident }}) + {% else %} + assert all(isinstance(i, {{ method.paged_result_field.type.ident }}) + for i in results) + {% endif %} + + pages = list(client.{{ method.name|snake_case }}(request={}).pages) + for page_, token in zip(pages, ['abc','def','ghi', '']): + assert page_.raw_page.next_page_token == token + + +{% endif %} {# paged methods #} +{% endfor -%} {#- method in methods for rest #} def test_credentials_transport_error(): # It is an error to provide credentials and a transport instance. transport = transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport( diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index bcaeb68800..256ef27951 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -123,6 +123,16 @@ def test_method_paged_result_field_no_page_field(): ) assert method.paged_result_field is None + method = make_method('Foo', + input_message=make_message(name='FooRequest', fields=( + make_field(name='page_token', type=9), # str + )), + output_message=make_message(name='FooResponse', fields=( + make_field(name='next_page_token', type=9), # str + )) + ) + assert method.paged_result_field == None + def test_method_paged_result_ref_types(): input_msg = make_message( @@ -139,7 +149,7 @@ def test_method_paged_result_ref_types(): name='ListMolluscsResponse', fields=( make_field(name='molluscs', message=mollusc_msg, repeated=True), - make_field(name='next_page_token', type=9) + make_field(name='next_page_token', type=9) # str ), module='mollusc' ) @@ -207,7 +217,7 @@ def test_flattened_ref_types(): def test_method_paged_result_primitive(): - paged = make_field(name='squids', type=9, repeated=True) + paged = make_field(name='squids', type=9, repeated=True) # str input_msg = make_message( name='ListSquidsRequest', fields=( From d226f360ea3c191d49754a4140826c311371a2c7 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 25 Jan 2021 21:58:22 +0000 Subject: [PATCH 2/6] fix: typing errors, mypy cli update --- gapic/schema/wrappers.py | 15 ++++++++------- gapic/templates/noxfile.py.j2 | 1 + noxfile.py | 2 +- tests/unit/schema/wrappers/test_method.py | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 94f29be8a0..6d3a4c7080 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -866,16 +866,17 @@ def paged_result_field(self) -> Optional[Field]: """Return the response pagination field if the method is paginated.""" # If the request field lacks any of the expected pagination fields, # then the method is not paginated. - for page_field in ((self.input, str, 'page_token'), + for page_field_token in ((self.input, str, 'page_token'), (self.output, str, 'next_page_token')): - field = page_field[0].fields.get(page_field[2], None) - if not field or field.type != page_field[1]: + field = page_field_token[0].fields.get(page_field_token[2], None) + if not field or field.type != page_field_token[1]: return None - page_fields = [self.input.fields.get('max_results', None), - self.input.fields.get('page_size', None)] - page_field = next( + + page_fields = (self.input.fields.get('max_results', None), + self.input.fields.get('page_size', None)) + page_field_size = next( (field for field in page_fields if field is not None), None) - if not page_field or page_field.type != int: + if not page_field_size or page_field_size.type != int: return None # Return the first repeated field. diff --git a/gapic/templates/noxfile.py.j2 b/gapic/templates/noxfile.py.j2 index ee97ea01cb..b6225d867d 100644 --- a/gapic/templates/noxfile.py.j2 +++ b/gapic/templates/noxfile.py.j2 @@ -32,6 +32,7 @@ def mypy(session): session.install('.') session.run( 'mypy', + '--explicit-package-bases', {%- if api.naming.module_namespace %} '{{ api.naming.module_namespace[0] }}', {%- else %} diff --git a/noxfile.py b/noxfile.py index a50376efe1..ca83576363 100644 --- a/noxfile.py +++ b/noxfile.py @@ -262,4 +262,4 @@ def mypy(session): session.install("mypy") session.install(".") - session.run("mypy", "gapic") + session.run("mypy", "-p", "gapic") diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 256ef27951..86f72a65b5 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -131,7 +131,7 @@ def test_method_paged_result_field_no_page_field(): make_field(name='next_page_token', type=9), # str )) ) - assert method.paged_result_field == None + assert method.paged_result_field is None def test_method_paged_result_ref_types(): From 01adad1f91b623e5844b8c0dc70e641788508197 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 25 Jan 2021 22:08:22 +0000 Subject: [PATCH 3/6] fix: mypy cli flag --- noxfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index ca83576363..e1b881ac57 100644 --- a/noxfile.py +++ b/noxfile.py @@ -227,7 +227,7 @@ def showcase_mypy( session.chdir(lib) # Run the tests. - session.run("mypy", "google") + session.run("mypy", "--explicit-package-bases", "google") @nox.session(python="3.8") From 0f3e63d08f71cddd0ef81ebb894c119b17a098ca Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 25 Jan 2021 23:30:42 +0000 Subject: [PATCH 4/6] fix: delete __init__.py, remove -p mypy flag --- .../tests/unit/gapic/%name_%version/%sub/__init__.py | 0 noxfile.py | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/__init__.py diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/__init__.py b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/noxfile.py b/noxfile.py index e1b881ac57..7dbe33ebc3 100644 --- a/noxfile.py +++ b/noxfile.py @@ -262,4 +262,4 @@ def mypy(session): session.install("mypy") session.install(".") - session.run("mypy", "-p", "gapic") + session.run("mypy", "gapic") From 48253d346b0aab050f0b0853471fa27a585c528c Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 27 Jan 2021 22:43:35 +0000 Subject: [PATCH 5/6] fix: clearing up statements, tests, minor bug in filter usage --- gapic/schema/wrappers.py | 11 ++-- .../services/%service/transports/rest.py.j2 | 4 +- .../%name_%version/%sub/test_%service.py.j2 | 18 ++++--- tests/unit/schema/wrappers/test_method.py | 52 +++++++++++++------ 4 files changed, 56 insertions(+), 29 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 6d3a4c7080..812630720b 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -866,16 +866,19 @@ def paged_result_field(self) -> Optional[Field]: """Return the response pagination field if the method is paginated.""" # If the request field lacks any of the expected pagination fields, # then the method is not paginated. - for page_field_token in ((self.input, str, 'page_token'), + + # The request must have page_token and next_page_token as they keep track of pages + for source, source_type, name in ((self.input, str, 'page_token'), (self.output, str, 'next_page_token')): - field = page_field_token[0].fields.get(page_field_token[2], None) - if not field or field.type != page_field_token[1]: + field = source.fields.get(name, None) + if not field or field.type != source_type: return None + # The request must have max_results or page_size page_fields = (self.input.fields.get('max_results', None), self.input.fields.get('page_size', None)) page_field_size = next( - (field for field in page_fields if field is not None), None) + (field for field in page_fields if field), None) if not page_field_size or page_field_size.type != int: return None diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index 54ec5ca92e..c21ad5b27e 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -182,11 +182,9 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): # TODO(yon-mg): handle nested fields corerctly rather than using only top level fields # not required for GCE query_params = { - {% filter sort_lines -%} - {%- for field in method.query_params %} + {%- for field in method.query_params | sort%} '{{ field|camel_case }}': request.{{ field }}, {%- endfor %} - {% endfilter -%} } # TODO(yon-mg): further discussion needed whether 'python truthiness' is appropriate here # discards default values diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index ea36c6fe35..59611cfd33 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -1230,11 +1230,12 @@ def test_{{ method.name|snake_case }}_pager(): response = response + response # Wrap the values into proper Response objs - response = tuple(map(lambda x: {{ method.output.ident }}.to_json(x), response)) - side_effect = tuple(map(lambda x: Response(), response)) - for return_val, response_val in zip(side_effect, response): + response = tuple({{ method.output.ident }}.to_json(x) for x in response) + return_values = tuple(Response() for i in response) + for return_val, response_val in zip(return_values, response): return_val._content = response_val.encode('UTF-8') - req.side_effect = side_effect + return_val.status_code = 200 + req.side_effect = return_values metadata = () {% if method.field_headers -%} @@ -1257,13 +1258,16 @@ def test_{{ method.name|snake_case }}_pager(): assert pager.get('h') is None {% endif %} - results = [i for i in pager] + results = list(pager) assert len(results) == 6 {% if method.paged_result_field.map %} assert all( - isinstance(i, tuple) and - tuple(map(lambda x: type(x), results[0])) == (str, {{ method.paged_result_field.type.fields.get('value').ident }}) + isinstance(i, tuple) for i in results) + for result in results: + assert isinstance(result, tuple) + assert tuple(type(t) for t in result) == (str, {{ method.paged_result_field.type.fields.get('value').ident }}) + assert pager.get('a') is None assert isinstance(pager.get('h'), {{ method.paged_result_field.type.fields.get('value').ident }}) {% else %} diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 86f72a65b5..2162effbbb 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -66,19 +66,38 @@ def test_method_client_output_empty(): def test_method_client_output_paged(): paged = make_field(name='foos', message=make_message('Foo'), repeated=True) + parent = make_field(name='parent', type=9) # str + page_size = make_field(name='page_size', type=5) # int + page_token = make_field(name='page_token', type=9) # str + input_msg = make_message(name='ListFoosRequest', fields=( - make_field(name='parent', type=9), # str - make_field(name='page_size', type=5), # int - make_field(name='page_token', type=9), # str + parent, + page_size, + page_token, )) output_msg = make_message(name='ListFoosResponse', fields=( paged, make_field(name='next_page_token', type=9), # str )) - method = make_method('ListFoos', - input_message=input_msg, - output_message=output_msg, - ) + method = make_method( + 'ListFoos', + input_message=input_msg, + output_message=output_msg, + ) + assert method.paged_result_field == paged + assert method.client_output.ident.name == 'ListFoosPager' + + max_results = make_field(name='max_results', type=5) # int + input_msg = make_message(name='ListFoosRequest', fields=( + parent, + max_results, + page_token, + )) + method = make_method( + 'ListFoos', + input_message=input_msg, + output_message=output_msg, + ) assert method.paged_result_field == paged assert method.client_output.ident.name == 'ListFoosPager' @@ -123,14 +142,17 @@ def test_method_paged_result_field_no_page_field(): ) assert method.paged_result_field is None - method = make_method('Foo', - input_message=make_message(name='FooRequest', fields=( - make_field(name='page_token', type=9), # str - )), - output_message=make_message(name='FooResponse', fields=( - make_field(name='next_page_token', type=9), # str - )) - ) + method = make_method( + name='Foo', + input_message=make_message( + name='FooRequest', + fields=(make_field(name='page_token', type=9),) # str + ), + output_message=make_message( + name='FooResponse', + fields=(make_field(name='next_page_token', type=9),) # str + ) + ) assert method.paged_result_field is None From 86e2c43737433093b339becc7a31633c46e81089 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 29 Jan 2021 20:03:32 +0000 Subject: [PATCH 6/6] fix: wrong genereated type hints --- .../%name_%version/%sub/services/%service/pagers.py.j2 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 index 5f04eed61e..ca3cc8d40e 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2 @@ -69,11 +69,11 @@ class {{ method.name }}Pager: yield self._response {% if method.paged_result_field.map %} - def __iter__(self) -> Iterable[Tuple[str, {{ method.paged_result_field.ident | replace('Sequence[', '') | replace(']', '') }}]]: + def __iter__(self) -> Iterable[Tuple[str, {{ method.paged_result_field.type.fields.get('value').ident }}]]: for page in self.pages: yield from page.{{ method.paged_result_field.name}}.items() - def get(self, key: str) -> {{ method.paged_result_field.ident | replace('Sequence', 'Optional') }}: + def get(self, key: str) -> Optional[{{ method.paged_result_field.type.fields.get('value').ident }}]: return self._response.items.get(key) {% else %} def __iter__(self) -> {{ method.paged_result_field.ident | replace('Sequence', 'Iterable') }}: