Skip to content

Commit 0744ed5

Browse files
software-dovsoftware-dov
authored
fix: disambiguate method names that are reserved in transport classes (#1187)
In addition to the method specific stubs they provide, the generated transports expose other methods to e.g. create a gRPC channel. This presents the opportunity for a naming collision if an API has a CreateChannel method. This PR disambiguates colliding method names at the transport level. Client level method names are unchanged for ergonomic reasons.
1 parent 012f60b commit 0744ed5

20 files changed

Lines changed: 629 additions & 85 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
443443

444444
# Wrap the RPC method; this adds retry and timeout information,
445445
# and friendly error handling.
446-
rpc = self._transport._wrapped_methods[self._transport.{{ method.name|snake_case}}]
446+
rpc = self._transport._wrapped_methods[self._transport.{{ method.transport_safe_name|snake_case}}]
447447
{% if method.field_headers %}
448448

449449
# Certain fields should be provided within the metadata header;

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@ class {{ service.name }}Transport(abc.ABC):
120120
# Precompute the wrapped methods.
121121
self._wrapped_methods = {
122122
{% for method in service.methods.values() %}
123-
self.{{ method.name|snake_case }}: gapic_v1.method.wrap_method(
124-
self.{{ method.name|snake_case }},
123+
self.{{ method.transport_safe_name|snake_case }}: gapic_v1.method.wrap_method(
124+
self.{{ method.transport_safe_name|snake_case }},
125125
{% if method.retry %}
126126
default_retry=retries.Retry(
127127
{% if method.retry.initial_backoff %}initial={{ method.retry.initial_backoff }},{% endif %}
@@ -160,7 +160,7 @@ class {{ service.name }}Transport(abc.ABC):
160160
{% for method in service.methods.values() %}
161161

162162
@property
163-
def {{ method.name|snake_case }}(self) -> Callable[
163+
def {{ method.transport_safe_name|snake_case }}(self) -> Callable[
164164
[{{ method.input.ident }}],
165165
Union[
166166
{{ method.output.ident }},

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
249249
{% for method in service.methods.values() %}
250250

251251
@property
252-
def {{ method.name|snake_case }}(self) -> Callable[
252+
def {{ method.transport_safe_name|snake_case }}(self) -> Callable[
253253
[{{ method.input.ident }}],
254254
{{ method.output.ident }}]:
255255
r"""Return a callable for the{{ ' ' }}
@@ -269,13 +269,13 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
269269
# the request.
270270
# gRPC handles serialization and deserialization, so we just need
271271
# to pass in the functions for each.
272-
if '{{ method.name|snake_case }}' not in self._stubs:
273-
self._stubs['{{ method.name|snake_case }}'] = self.grpc_channel.{{ method.grpc_stub_type }}(
272+
if '{{ method.transport_safe_name|snake_case }}' not in self._stubs:
273+
self._stubs['{{ method.transport_safe_name|snake_case }}'] = self.grpc_channel.{{ method.grpc_stub_type }}(
274274
'/{{ '.'.join(method.meta.address.package) }}.{{ service.name }}/{{ method.name }}',
275275
request_serializer={{ method.input.ident }}.{% if method.input.ident.python_import.module.endswith('_pb2') %}SerializeToString{% else %}serialize{% endif %},
276276
response_deserializer={{ method.output.ident }}.{% if method.output.ident.python_import.module.endswith('_pb2') %}FromString{% else %}deserialize{% endif %},
277277
)
278-
return self._stubs['{{ method.name|snake_case }}']
278+
return self._stubs['{{ method.transport_safe_name|snake_case }}']
279279
{% endfor %}
280280

281281
{% if opts.add_iam_methods %}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -396,12 +396,12 @@ class {{service.name}}RestTransport({{service.name}}Transport):
396396
{% for method in service.methods.values()|sort(attribute="name") %}
397397

398398
@property
399-
def {{method.name | snake_case}}(self) -> Callable[
399+
def {{method.transport_safe_name | snake_case}}(self) -> Callable[
400400
[{{method.input.ident}}],
401401
{{method.output.ident}}]:
402-
stub = self._STUBS.get("{{method.name | snake_case}}")
402+
stub = self._STUBS.get("{{method.transport_safe_name | snake_case}}")
403403
if not stub:
404-
stub = self._STUBS["{{method.name | snake_case}}"] = self._{{method.name}}(self._session, self._host, self._interceptor)
404+
stub = self._STUBS["{{method.transport_safe_name | snake_case}}"] = self._{{method.name}}(self._session, self._host, self._interceptor)
405405

406406
# The return type is fine, but mypy isn't sophisticated enough to determine what's going on here.
407407
# In C++ this would require a dynamic_cast

‎packages/gapic-generator/gapic/ads-templates/setup.py.j2‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ setuptools.setup(
1919
install_requires=(
2020
{# TODO(dovs): remove when 1.x deprecation is complete #}
2121
{% if 'rest' in opts.transport %}
22-
'google-api-core[grpc] >= 2.1.0, < 3.0.0dev',
22+
'google-api-core[grpc] >= 2.4.0, < 3.0.0dev',
2323
{% else %}
2424
'google-api-core[grpc] >= 1.28.0, < 3.0.0dev',
2525
{% endif %}

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ def test_{{ method_name }}(request_type, transport: str = 'grpc'):
494494

495495
# Mock the actual call within the gRPC stub, and fake the request.
496496
with mock.patch.object(
497-
type(client.transport.{{ method.name|snake_case }}),
497+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
498498
'__call__') as call:
499499
# Designate an appropriate return value for the call.
500500
{% if method.void %}
@@ -571,7 +571,7 @@ def test_{{ method_name }}_empty_call():
571571

572572
# Mock the actual call within the gRPC stub, and fake the request.
573573
with mock.patch.object(
574-
type(client.transport.{{ method.name|snake_case }}),
574+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
575575
'__call__') as call:
576576
client.{{ method_name }}()
577577
call.assert_called()
@@ -600,7 +600,7 @@ def test_{{ method_name }}_field_headers():
600600

601601
# Mock the actual call within the gRPC stub, and fake the request.
602602
with mock.patch.object(
603-
type(client.transport.{{ method.name|snake_case }}),
603+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
604604
'__call__') as call:
605605
{% if method.void %}
606606
call.return_value = None
@@ -638,7 +638,7 @@ def test_{{ method_name }}_from_dict_foreign():
638638
)
639639
# Mock the actual call within the gRPC stub, and fake the request.
640640
with mock.patch.object(
641-
type(client.transport.{{ method.name|snake_case }}),
641+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
642642
'__call__') as call:
643643
# Designate an appropriate return value for the call.
644644
{% if method.void %}
@@ -668,7 +668,7 @@ def test_{{ method_name }}_flattened():
668668

669669
# Mock the actual call within the gRPC stub, and fake the request.
670670
with mock.patch.object(
671-
type(client.transport.{{ method.name|snake_case }}),
671+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
672672
'__call__') as call:
673673
# Designate an appropriate return value for the call.
674674
{% if method.void %}
@@ -746,7 +746,7 @@ def test_{{ method_name }}_pager(transport_name: str = "grpc"):
746746

747747
# Mock the actual call within the gRPC stub, and fake the request.
748748
with mock.patch.object(
749-
type(client.transport.{{ method.name|snake_case }}),
749+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
750750
'__call__') as call:
751751
# Set the response to a series of pages.
752752
call.side_effect = (
@@ -808,7 +808,7 @@ def test_{{ method_name }}_pages(transport_name: str = "grpc"):
808808

809809
# Mock the actual call within the gRPC stub, and fake the request.
810810
with mock.patch.object(
811-
type(client.transport.{{ method.name|snake_case }}),
811+
type(client.transport.{{ method.transport_safe_name|snake_case }}),
812812
'__call__') as call:
813813
# Set the response to a series of pages.
814814
{% if method.paged_result_field.map%}
@@ -1184,7 +1184,7 @@ def test_{{ method_name }}_rest_required_fields(request_type={{ method.input.ide
11841184
def test_{{ method_name }}_rest_unset_required_fields():
11851185
transport = transports.{{ service.rest_transport_name }}(credentials=ga_credentials.AnonymousCredentials)
11861186

1187-
unset_fields = transport.{{ method.name|snake_case }}._get_unset_required_fields({})
1187+
unset_fields = transport.{{ method.transport_safe_name|snake_case }}._get_unset_required_fields({})
11881188
assert set(unset_fields) == (set(({% for param in method.query_params|sort %}"{{ param|camel_case }}", {% endfor %})) & set(({% for param in method.input.required_fields %}"{{param.name|camel_case}}", {% endfor %})))
11891189

11901190

@@ -1645,7 +1645,7 @@ def test_{{ service.name|snake_case }}_base_transport():
16451645
# raise NotImplementedError.
16461646
methods = (
16471647
{% for method in service.methods.values() %}
1648-
'{{ method.name|snake_case }}',
1648+
'{{ method.transport_safe_name|snake_case }}',
16491649
{% endfor %}
16501650
{% if opts.add_iam_methods %}
16511651
'set_iam_policy',

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,6 +948,21 @@ class Method:
948948
def __getattr__(self, name):
949949
return getattr(self.method_pb, name)
950950

951+
@property
952+
def transport_safe_name(self) -> str:
953+
# These names conflict with other methods in the transport.
954+
# We don't want to disambiguate the names at the client level
955+
# because the disambiguated name is less convenient and user friendly.
956+
#
957+
# Note: this should really be a class variable,
958+
# but python 3.6 can't handle that.
959+
TRANSPORT_UNSAFE_NAMES = {
960+
"CreateChannel",
961+
"GrpcChannel",
962+
"OperationsClient",
963+
}
964+
return f"{self.name}_" if self.name in TRANSPORT_UNSAFE_NAMES else self.name
965+
951966
@property
952967
def is_operation_polling_method(self):
953968
return self.output.is_extended_operation and self.options.Extensions[ex_ops_pb2.operation_polling_method]

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,15 +92,15 @@ class {{ service.async_client_name }}:
9292

9393
@classmethod
9494
def get_mtls_endpoint_and_cert_source(cls, client_options: Optional[ClientOptions] = None):
95-
"""Return the API endpoint and client cert source for mutual TLS.
95+
"""Return the API endpoint and client cert source for mutual TLS.
9696

9797
The client cert source is determined in the following order:
9898
(1) if `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is not "true", the
9999
client cert source is None.
100100
(2) if `client_options.client_cert_source` is provided, use the provided one; if the
101101
default client cert source exists, use the default one; otherwise the client cert
102102
source is None.
103-
103+
104104
The API endpoint is determined in the following order:
105105
(1) if `client_options.api_endpoint` if provided, use the provided one.
106106
(2) if `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is "always", use the
@@ -118,7 +118,7 @@ class {{ service.async_client_name }}:
118118
Returns:
119119
Tuple[str, Callable[[], Tuple[bytes, bytes]]]: returns the API endpoint and the
120120
client cert source to use.
121-
121+
122122
Raises:
123123
google.auth.exceptions.MutualTLSChannelError: If any errors happen.
124124
"""
@@ -302,7 +302,7 @@ class {{ service.async_client_name }}:
302302
# Wrap the RPC method; this adds retry and timeout information,
303303
# and friendly error handling.
304304
rpc = gapic_v1.method_async.wrap_method(
305-
self._client._transport.{{ method.name|snake_case }},
305+
self._client._transport.{{ method.transport_safe_name|snake_case }},
306306
{% if method.retry %}
307307
default_retry=retries.Retry(
308308
{% if method.retry.initial_backoff %}initial={{ method.retry.initial_backoff }},{% endif %}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
477477

478478
# Wrap the RPC method; this adds retry and timeout information,
479479
# and friendly error handling.
480-
rpc = self._transport._wrapped_methods[self._transport.{{ method.name|snake_case}}]
480+
rpc = self._transport._wrapped_methods[self._transport.{{ method.transport_safe_name|snake_case}}]
481481

482482
{% if method.explicit_routing %}
483483
header_params = {}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@ class {{ service.name }}Transport(abc.ABC):
120120
# Precompute the wrapped methods.
121121
self._wrapped_methods = {
122122
{% for method in service.methods.values() %}
123-
self.{{ method.name|snake_case }}: gapic_v1.method.wrap_method(
124-
self.{{ method.name|snake_case }},
123+
self.{{ method.transport_safe_name|snake_case }}: gapic_v1.method.wrap_method(
124+
self.{{ method.transport_safe_name|snake_case }},
125125
{% if method.retry %}
126126
default_retry=retries.Retry(
127127
{% if method.retry.initial_backoff %}initial={{ method.retry.initial_backoff }},{% endif %}
@@ -160,7 +160,7 @@ class {{ service.name }}Transport(abc.ABC):
160160
{% for method in service.methods.values() %}
161161

162162
@property
163-
def {{ method.name|snake_case }}(self) -> Callable[
163+
def {{ method.transport_safe_name|snake_case }}(self) -> Callable[
164164
[{{ method.input.ident }}],
165165
Union[
166166
{{ method.output.ident }},

0 commit comments

Comments
 (0)