fix: update paging implementation to handle unconventional pagination #750

yon-mg · 2021-01-25T18:24:50Z

Pagination for GCE is unconventional in that it may use max_results instead of page_results and may return a map paginated resource instead of a repeated resource. Pagination otherwise does not change and even GCE may use conventional pagination for some rpcs.

Before this PR, pagination has only been implemented for the conventional case. This fix resolves that and adds the relevant testing to ensure coverage.

On an unrelated note, this PR now also includes a minor update to mypy cli flags due to the recent mypy update, https://mypy-lang.blogspot.com/2021/01/mypy-0800-released.html.

codecov · 2021-01-25T22:12:29Z

Codecov Report

Merging #750 (959e1a0) into master (4077b45) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #750   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines         1619      1623    +4     
  Branches       328       330    +2     
=========================================
+ Hits          1619      1623    +4

Impacted Files	Coverage Δ
gapic/schema/wrappers.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4077b45...959e1a0. Read the comment docs.

software-dov

Please add some comments in paged_result_field detailing the two options and how they work internally.

software-dov · 2021-01-26T18:30:30Z

gapic/schema/wrappers.py

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


Great extension. Since we're changing things here, can we tweak the loop so that we're destructuring into names instead of indexing the tuple?

for source, name in ((self.input, "page_token"), (self.output, "next_page_token")):

Something like that

Good idea. Done

software-dov · 2021-01-26T18:32:08Z

gapic/schema/wrappers.py

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


Nit: we don't have to explicitly check for None.

next((field for field in page_fields if field), None)

I just wanted to be extra explicit in case there was some false-ish value field might have. I suppose that's not the case for the Field class though.

vchudnov-g

Looks good! Minor comments and some questions for my own learning.

vchudnov-g · 2021-01-26T18:47:55Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+        response = response + response
+
+        # Wrap the values into proper Response objs
+        response = tuple(map(lambda x: {{ method.output.ident }}.to_json(x), response))


Python question: response and side_effect only contain one element. Why do you need to make them tuples? (I see zip() requires iterables, but so does tuple())

Nit: you don't need to call both map and tuple: tuple can take generator expressions as its iterable.

response = tuple({{ method.output.ident }}.to_json(x) for x in response)

Thanks for that. Not sure why I was dead set on using map.

vchudnov-g · 2021-01-26T18:49:53Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+
+        # 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))


Being inexperienced with Python mocks, I found the name side_effect confusing here. IIUC, this could be called return_values (and then assigned to req.side_effect)

(Note to self: https://docs.python.org/3/library/unittest.mock-examples.html#side-effect-functions-and-iterables)

We can be a little bit clever here:

side_effect = (Response(),) * len(response)

I think this does not work because new Response objects are not made. Instead it is aliases to the same object and editing a value within one changes it for all.

Aha, good point.

vchudnov-g · 2021-01-26T19:08:09Z

tests/unit/schema/wrappers/test_method.py

+                             make_field(name='next_page_token', type=9),  # str
+                         ))
+                         )
+    assert method.paged_result_field is None


Do we add tests in this file for max_results and for mapped responses?

Mapped responses are treated the same here. Checking for repeated fields should be sufficient since mapped fields are also repeated. Test for max_results is now also added.

vchudnov-g · 2021-01-26T19:14:35Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+            assert page_.raw_page.next_page_token == token
+
+
+{% endif %} {# paged methods #}


Are the results of this template visible in a generated file within this repo? (just asking, the repo may not be structured that way)

No. It may be useful to include a generated client but that might clog the repo. The showcase client is generated as part of CircleCI tests though.

software-dov · 2021-01-26T21:09:58Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+        response = response + response
+
+        # Wrap the values into proper Response objs
+        response = tuple(map(lambda x: {{ method.output.ident }}.to_json(x), response))


Nit: you don't need to call both map and tuple: tuple can take generator expressions as its iterable.

response = tuple({{ method.output.ident }}.to_json(x) for x in response)

software-dov · 2021-01-26T21:14:13Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+
+        # 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))


We can be a little bit clever here:

side_effect = (Response(),) * len(response)

software-dov · 2021-01-26T21:14:59Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+        assert pager.get('h') is None
+        {% endif %}
+
+        results = [i for i in pager]


Judgement call: can also do results = list(pager)

This is cleaner. Done.

software-dov · 2021-01-26T21:18:54Z

gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2

+            tuple(map(lambda x: type(x), results[0])) == (str, {{ method.paged_result_field.type.fields.get('value').ident }})
+                   for i in results)


Am I misreading this expression, or is this a loop invariant? If so, let's lift it up. In either case, let's clarifiy it to

tuple(type(r) for r in results[0])

Actually, the whole line looks like a loop invariant.

software-dov · 2021-01-26T21:19:53Z

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


Formatting nit: please add a newline and then indent here for better readability

Done.. I think.

vam-google · 2021-01-29T19:51:23Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2

@@ -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(']', '') }}]]:


Please fix the typing issue as we discussed offline.

vam-google · 2021-01-29T19:54:49Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/pagers.py.j2

+        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') }}:


yon-mg · 2021-02-01T23:41:49Z

@software-dov friendly ping for review.

yon-mg added 2 commits January 25, 2021 18:14

fix: update paging implementation to handle unconventional pagination

1141010

Merge remote-tracking branch 'upstream/master'

058b55e

yon-mg requested review from software-dov and busunkim96 January 25, 2021 18:24

yon-mg requested a review from a team as a code owner January 25, 2021 18:24

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 25, 2021

yon-mg requested review from vchudnov-g and vam-google January 25, 2021 18:25

yon-mg added 2 commits January 25, 2021 21:58

fix: typing errors, mypy cli update

d226f36

fix: mypy cli flag

01adad1

fix: delete __init__.py, remove -p mypy flag

0f3e63d

software-dov reviewed Jan 26, 2021

View reviewed changes

vchudnov-g reviewed Jan 26, 2021

View reviewed changes

software-dov reviewed Jan 26, 2021

View reviewed changes

fix: clearing up statements, tests, minor bug in filter usage

48253d3

vam-google reviewed Jan 29, 2021

View reviewed changes

fix: wrong genereated type hints

86e2c43

Merge branch 'master' into master

959e1a0

software-dov approved these changes Feb 3, 2021

View reviewed changes

yon-mg merged commit eaac3e6 into googleapis:master Feb 4, 2021

This was referenced Jan 18, 2022

chore(master): release reverted #1133

Merged

chore(master): release 0.40.0 #1138

Closed

chore(master): release 0.39.1 #1146

Closed

This was referenced Jan 26, 2022

chore(main): release 0.39.1 #1160

Closed

chore(main): release 0.39.1 #1162

Closed

chore(main): release 0.61.0 #1163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update paging implementation to handle unconventional pagination #750

fix: update paging implementation to handle unconventional pagination #750

yon-mg commented Jan 25, 2021 •

edited

Loading

codecov bot commented Jan 25, 2021 •

edited

Loading

software-dov left a comment

software-dov Jan 26, 2021

yon-mg Jan 26, 2021

software-dov Jan 26, 2021

yon-mg Jan 26, 2021

vchudnov-g left a comment

vchudnov-g Jan 26, 2021

software-dov Jan 26, 2021

yon-mg Jan 27, 2021

vchudnov-g Jan 26, 2021

software-dov Jan 26, 2021

yon-mg Jan 27, 2021

software-dov Jan 28, 2021

vchudnov-g Jan 26, 2021

yon-mg Jan 27, 2021

vchudnov-g Jan 26, 2021

yon-mg Jan 27, 2021

software-dov Jan 26, 2021

software-dov Jan 26, 2021

software-dov Jan 26, 2021

yon-mg Jan 27, 2021

software-dov Jan 26, 2021

software-dov Jan 26, 2021

yon-mg Jan 27, 2021

vam-google Jan 29, 2021

vam-google Jan 29, 2021

yon-mg commented Feb 1, 2021

		assert page_.raw_page.next_page_token == token


		{% endif %} {# paged methods #}

		tuple(map(lambda x: type(x), results[0])) == (str, {{ method.paged_result_field.type.fields.get('value').ident }})
		for i in results)

fix: update paging implementation to handle unconventional pagination #750

fix: update paging implementation to handle unconventional pagination #750

Conversation

yon-mg commented Jan 25, 2021 • edited Loading

codecov bot commented Jan 25, 2021 • edited Loading

Codecov Report

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg commented Feb 1, 2021

yon-mg commented Jan 25, 2021 •

edited

Loading

codecov bot commented Jan 25, 2021 •

edited

Loading