Visitar URL original
fix: numerous small fixes to allow bigtable-admin (#660) · googleapis/google-cloud-python@2eb36f9 · GitHub
Skip to content

Commit 2eb36f9

Browse files
software-dovsoftware-dov
authored
fix: numerous small fixes to allow bigtable-admin (#660)
Includes: * tweaked logic around defining recursive message types * more sophisticated logic for generating unit tests using recursive message types * flattened map-y fields are handled properly * fixed a corner case where a method has a third-party request object and flattened fields
1 parent 364c7c3 commit 2eb36f9

9 files changed

Lines changed: 96 additions & 34 deletions

File tree

‎packages/gapic-generator/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
333333
request = {{ method.input.ident }}(**request)
334334
{% if method.flattened_fields -%}{# Cross-package req and flattened fields #}
335335
elif not request:
336-
request = {{ method.input.ident }}()
336+
request = {{ method.input.ident }}({% if method.input.ident.package != method.ident.package %}{% for f in method.flattened_fields.values() %}{{ f.name }}={{ f.name }}, {% endfor %}{% endif %})
337337
{% endif -%}{# Cross-package req and flattened fields #}
338338
{%- else %}
339339
# Minor optimization to avoid making a copy if the user passes
@@ -344,16 +344,22 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
344344
request = {{ method.input.ident }}(request)
345345
{% endif %} {# different request package #}
346346
{#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #}
347-
{% if method.flattened_fields -%}
347+
{% if method.flattened_fields and method.input.ident.package == method.ident.package -%}
348348
# If we have keyword arguments corresponding to fields on the
349349
# request, apply these.
350350
{% endif -%}
351351
{%- for key, field in method.flattened_fields.items() if not(field.repeated and method.input.ident.package != method.ident.package) %}
352352
if {{ field.name }} is not None:
353353
request.{{ key }} = {{ field.name }}
354354
{%- endfor %}
355-
{# They can be _extended_, however -#}
356-
{%- for key, field in method.flattened_fields.items() if (field.repeated and method.input.ident.package != method.ident.package) %}
355+
{# Map-y fields can be _updated_, however #}
356+
{%- for key, field in method.flattened_fields.items() if field.map and method.input.ident.package == method.ident.package %}
357+
358+
if {{ field.name }}:
359+
request.{{ key }}.update({{ field.name }})
360+
{%- endfor %}
361+
{# And list-y fields can be _extended_ -#}
362+
{%- for key, field in method.flattened_fields.items() if field.repeated and not field.map and method.input.ident.package == method.ident.package %}
357363
if {{ field.name }}:
358364
request.{{ key }}.extend({{ field.name }})
359365
{%- endfor %}

‎packages/gapic-generator/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,10 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc', request_type={{ m
297297
for message in response:
298298
assert isinstance(message, {{ method.output.ident }})
299299
{% else -%}
300+
{% if "next_page_token" in method.output.fields.values()|map(attribute='name') and not method.paged_result_field %}
301+
{# Cheeser assertion to force code coverage for bad paginated methods #}
302+
assert response.raw_page is response
303+
{% endif %}
300304
assert isinstance(response, {{ method.client_output.ident }})
301305
{% for field in method.output.fields.values() | rejectattr('message') -%}{% if not field.oneof or field.proto3_optional %}
302306
{% if field.field_pb.type in [1, 2] -%} {# Use approx eq for floats -#}

‎packages/gapic-generator/gapic/schema/metadata.py‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -242,12 +242,15 @@ def rel(self, address: 'Address') -> str:
242242
# It is possible that a field references a message that has
243243
# not yet been declared. If so, send its name enclosed in quotes
244244
# (a string) instead.
245-
if self.module_path > address.module_path or self == address:
246-
return f"'{'.'.join(self.parent + (self.name,))}'"
247-
248-
# This is a message in the same module, already declared.
249-
# Send its name.
250-
return '.'.join(self.parent + (self.name,))
245+
#
246+
# Note: this is a conservative construction; it generates a stringy
247+
# identifier all the time when it may be possible to use a regular
248+
# module lookup.
249+
# On the other hand, there's no reason _not_ to use a stringy
250+
# identifier. It is guaranteed to work all the time because
251+
# it bumps name resolution until a time when all types in a module
252+
# are guaranteed to be fully defined.
253+
return f"'{'.'.join(self.parent + (self.name,))}'"
251254

252255
# Return the usual `module.Name`.
253256
return str(self)

‎packages/gapic-generator/gapic/schema/wrappers.py‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,6 @@ class Field:
5555
)
5656
oneof: Optional[str] = None
5757

58-
# Arbitrary cap set via heuristic rule of thumb.
59-
MAX_MOCK_DEPTH: int = 20
60-
6158
def __getattr__(self, name):
6259
return getattr(self.field_pb, name)
6360

@@ -93,17 +90,16 @@ def map(self) -> bool:
9390

9491
@utils.cached_property
9592
def mock_value(self) -> str:
96-
depth = 0
93+
visited_fields: Set["Field"] = set()
9794
stack = [self]
9895
answer = "{}"
9996
while stack:
10097
expr = stack.pop()
101-
answer = answer.format(expr.inner_mock(stack, depth))
102-
depth += 1
98+
answer = answer.format(expr.inner_mock(stack, visited_fields))
10399

104100
return answer
105101

106-
def inner_mock(self, stack, depth):
102+
def inner_mock(self, stack, visited_fields):
107103
"""Return a repr of a valid, usually truthy mock value."""
108104
# For primitives, send a truthy value computed from the
109105
# field name.
@@ -137,10 +133,11 @@ def inner_mock(self, stack, depth):
137133
and isinstance(self.type, MessageType)
138134
and len(self.type.fields)
139135
# Nested message types need to terminate eventually
140-
and depth < self.MAX_MOCK_DEPTH
136+
and self not in visited_fields
141137
):
142138
sub = next(iter(self.type.fields.values()))
143139
stack.append(sub)
140+
visited_fields.add(self)
144141
# Don't do the recursive rendering here, just set up
145142
# where the nested value should go with the double {}.
146143
answer = f'{self.type.ident}({sub.name}={{}})'

‎packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,8 @@ class {{ service.async_client_name }}:
169169
{% if method.flattened_fields -%}
170170
# Sanity check: If we got a request object, we should *not* have
171171
# gotten any keyword arguments that map to the request.
172-
if request is not None and any([{{ method.flattened_fields.values()|join(', ', attribute='name') }}]):
172+
has_flattened_params = any([{{ method.flattened_fields.values()|join(', ', attribute='name') }}])
173+
if request is not None and has_flattened_params:
173174
raise ValueError('If the `request` argument is set, then none of '
174175
'the individual field arguments should be set.')
175176

@@ -181,23 +182,29 @@ class {{ service.async_client_name }}:
181182
request = {{ method.input.ident }}(**request)
182183
{% if method.flattened_fields -%}{# Cross-package req and flattened fields #}
183184
elif not request:
184-
request = {{ method.input.ident }}()
185+
request = {{ method.input.ident }}({% if method.input.ident.package != method.ident.package %}{% for f in method.flattened_fields.values() %}{{ f.name }}={{ f.name }}, {% endfor %}{% endif %})
185186
{% endif -%}{# Cross-package req and flattened fields #}
186187
{%- else %}
187188
request = {{ method.input.ident }}(request)
188189
{% endif %} {# different request package #}
189190

190191
{#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #}
191-
{% if method.flattened_fields -%}
192+
{% if method.flattened_fields and method.input.ident.package == method.ident.package -%}
192193
# If we have keyword arguments corresponding to fields on the
193194
# request, apply these.
194195
{% endif -%}
195-
{%- for key, field in method.flattened_fields.items() if not(field.repeated and method.input.ident.package != method.ident.package) %}
196+
{%- for key, field in method.flattened_fields.items() if not field.repeated and method.input.ident.package == method.ident.package %}
196197
if {{ field.name }} is not None:
197198
request.{{ key }} = {{ field.name }}
198199
{%- endfor %}
199-
{# They can be _extended_, however -#}
200-
{%- for key, field in method.flattened_fields.items() if (field.repeated and method.input.ident.package != method.ident.package) %}
200+
{# Map-y fields can be _updated_, however #}
201+
{%- for key, field in method.flattened_fields.items() if field.map and method.input.ident.package == method.ident.package %}
202+
203+
if {{ field.name }}:
204+
request.{{ key }}.update({{ field.name }})
205+
{%- endfor %}
206+
{# And list-y fields can be _extended_ -#}
207+
{%- for key, field in method.flattened_fields.items() if field.repeated and not field.map and method.input.ident.package == method.ident.package %}
201208
if {{ field.name }}:
202209
request.{{ key }}.extend({{ field.name }})
203210
{%- endfor %}

‎packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
345345
request = {{ method.input.ident }}(**request)
346346
{% if method.flattened_fields -%}{# Cross-package req and flattened fields #}
347347
elif not request:
348-
request = {{ method.input.ident }}()
348+
request = {{ method.input.ident }}({% if method.input.ident.package != method.ident.package %}{% for f in method.flattened_fields.values() %}{{ f.name }}={{ f.name }}, {% endfor %}{% endif %})
349349
{% endif -%}{# Cross-package req and flattened fields #}
350350
{%- else %}
351351
# Minor optimization to avoid making a copy if the user passes
@@ -357,16 +357,22 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
357357
{% endif %} {# different request package #}
358358

359359
{#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #}
360-
{% if method.flattened_fields -%}
360+
{% if method.flattened_fields and method.input.ident.package == method.ident.package -%}
361361
# If we have keyword arguments corresponding to fields on the
362362
# request, apply these.
363363
{% endif -%}
364-
{%- for key, field in method.flattened_fields.items() if not(field.repeated or method.input.ident.package != method.ident.package) %}
364+
{%- for key, field in method.flattened_fields.items() if not field.repeated and method.input.ident.package == method.ident.package %}
365365
if {{ field.name }} is not None:
366366
request.{{ key }} = {{ field.name }}
367367
{%- endfor %}
368-
{# They can be _extended_, however -#}
369-
{%- for key, field in method.flattened_fields.items() if field.repeated %}
368+
{# Map-y fields can be _updated_, however #}
369+
{%- for key, field in method.flattened_fields.items() if field.map and method.input.ident.package == method.ident.package %}
370+
371+
if {{ field.name }}:
372+
request.{{ key }}.update({{ field.name }})
373+
{%- endfor %}
374+
{# And list-y fields can be _extended_ -#}
375+
{%- for key, field in method.flattened_fields.items() if field.repeated and not field.map and method.input.ident.package == method.ident.package %}
370376
if {{ field.name }}:
371377
request.{{ key }}.extend({{ field.name }})
372378
{%- endfor %}

‎packages/gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,10 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc', request_type={{ m
398398
for message in response:
399399
assert isinstance(message, {{ method.output.ident }})
400400
{% else -%}
401+
{% if "next_page_token" in method.output.fields.values()|map(attribute='name') and not method.paged_result_field %}
402+
{# Cheeser assertion to force code coverage for bad paginated methods #}
403+
assert response.raw_page is response
404+
{% endif %}
401405
assert isinstance(response, {{ method.client_output.ident }})
402406
{% for field in method.output.fields.values() | rejectattr('message') -%}{% if not field.oneof or field.proto3_optional %}
403407
{% if field.field_pb.type in [1, 2] -%} {# Use approx eq for floats -#}
@@ -417,15 +421,15 @@ def test_{{ method.name|snake_case }}_from_dict():
417421

418422

419423
@pytest.mark.asyncio
420-
async def test_{{ method.name|snake_case }}_async(transport: str = 'grpc_asyncio'):
424+
async def test_{{ method.name|snake_case }}_async(transport: str = 'grpc_asyncio', request_type={{ method.input.ident }}):
421425
client = {{ service.async_client_name }}(
422426
credentials=credentials.AnonymousCredentials(),
423427
transport=transport,
424428
)
425429

426430
# Everything is optional in proto3 as far as the runtime is concerned,
427431
# and we are mocking out the actual API, so just send an empty request.
428-
request = {{ method.input.ident }}()
432+
request = request_type()
429433
{% if method.client_streaming %}
430434
requests = [request]
431435
{% endif %}
@@ -474,7 +478,7 @@ async def test_{{ method.name|snake_case }}_async(transport: str = 'grpc_asyncio
474478
{% if method.client_streaming %}
475479
assert next(args[0]) == request
476480
{% else %}
477-
assert args[0] == request
481+
assert args[0] == {{ method.input.ident }}()
478482
{% endif %}
479483

480484
# Establish that the response is the type that we expect.
@@ -500,6 +504,11 @@ async def test_{{ method.name|snake_case }}_async(transport: str = 'grpc_asyncio
500504
{% endif %}
501505

502506

507+
@pytest.mark.asyncio
508+
async def test_{{ method.name|snake_case }}_async_from_dict():
509+
await test_{{ method.name|snake_case }}_async(request_type=dict)
510+
511+
503512
{% if method.field_headers and not method.client_streaming %}
504513
def test_{{ method.name|snake_case }}_field_headers():
505514
client = {{ service.client_name }}(
@@ -592,7 +601,7 @@ async def test_{{ method.name|snake_case }}_field_headers_async():
592601
{% endif %}
593602

594603
{% if method.ident.package != method.input.ident.package %}
595-
def test_{{ method.name|snake_case }}_from_dict():
604+
def test_{{ method.name|snake_case }}_from_dict_foreign():
596605
client = {{ service.client_name }}(
597606
credentials=credentials.AnonymousCredentials(),
598607
)

‎packages/gapic-generator/tests/unit/schema/test_metadata.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def test_address_rel():
7070
addr = metadata.Address(package=('foo', 'bar'), module='baz', name='Bacon')
7171
assert addr.rel(
7272
metadata.Address(package=('foo', 'bar'), module='baz'),
73-
) == 'Bacon'
73+
) == "'Bacon'"
7474

7575

7676
def test_address_rel_other():

‎packages/gapic-generator/tests/unit/schema/wrappers/test_field.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from google.api import field_behavior_pb2
2020
from google.protobuf import descriptor_pb2
2121

22+
from gapic.schema import api
2223
from gapic.schema import metadata
2324
from gapic.schema import wrappers
2425

@@ -250,6 +251,35 @@ def test_mock_value_message():
250251
assert field.mock_value == 'bogus.Message(foo=324)'
251252

252253

254+
def test_mock_value_recursive():
255+
# The elaborate setup is an unfortunate requirement.
256+
file_pb = descriptor_pb2.FileDescriptorProto(
257+
name="turtle.proto",
258+
package="animalia.chordata.v2",
259+
message_type=(
260+
descriptor_pb2.DescriptorProto(
261+
# It's turtles all the way down ;)
262+
name="Turtle",
263+
field=(
264+
descriptor_pb2.FieldDescriptorProto(
265+
name="turtle",
266+
type="TYPE_MESSAGE",
267+
type_name=".animalia.chordata.v2.Turtle",
268+
number=1,
269+
),
270+
),
271+
),
272+
),
273+
)
274+
my_api = api.API.build([file_pb], package="animalia.chordata.v2")
275+
turtle_field = my_api.messages["animalia.chordata.v2.Turtle"].fields["turtle"]
276+
277+
# If not handled properly, this will run forever and eventually OOM.
278+
actual = turtle_field.mock_value
279+
expected = "ac_turtle.Turtle(turtle=ac_turtle.Turtle(turtle=turtle.Turtle(turtle=None)))"
280+
assert actual == expected
281+
282+
253283
def test_field_name_kword_disambiguation():
254284
from_field = make_field(
255285
name="from",

0 commit comments

Comments
 (0)