From 03b3189826b59a827b7e45d70fd2cd44e4f6c373 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 10 Oct 2024 17:31:16 +0000 Subject: [PATCH 1/2] fix: use disambiguated name for rpcs to avoid collisions --- .../%sub/services/%service/client.py.j2 | 2 +- .../%name_%version/%sub/test_%service.py.j2 | 12 +++++----- .../%name_%version/%sub/test_%service.py.j2 | 2 +- .../gapic/%name_%version/%sub/test_macros.j2 | 22 +++++++++---------- .../test_reserved_method_names.proto | 15 +++++++++++++ 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 b/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 index 064e17924..5555339c4 100644 --- a/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 +++ b/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2 @@ -345,7 +345,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): {% if method.operation_service %}{# Extended Operations LRO #} def {{ method.name|snake_case }}_unary(self, {% else %} - def {{ method.name|snake_case }}(self, + def {{ method.safe_name|snake_case }}(self, {% endif %}{# Extended Operations LRO #} {% if not method.client_streaming %} request: Optional[Union[{{ method.input.ident }}, dict]] = None, diff --git a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 9387a124e..c84936dc8 100644 --- a/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -516,7 +516,7 @@ def test_{{ service.client_name|snake_case }}_create_channel_credentials_file(cl {% endif %} -{% for method in service.methods.values() if 'grpc' in opts.transport %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.name|snake_case %} +{% for method in service.methods.values() if 'grpc' in opts.transport %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %} @pytest.mark.parametrize("request_type", [ {{ method.input.ident }}, dict, @@ -579,7 +579,7 @@ def test_{{ method_name }}(request_type, transport: str = 'grpc'): ) {% endif %} {% if method.client_streaming %} - response = client.{{ method.name|snake_case }}(iter(requests)) + response = client.{{ method.safe_name|snake_case }}(iter(requests)) {% else %} response = client.{{ method_name }}(request) {% endif %} @@ -1053,7 +1053,7 @@ def test_{{ method_name }}_raw_page_lro(): {% endfor %} {# method in methods for grpc #} -{% for method in service.methods.values() if 'rest' in opts.transport %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.name|snake_case %}{% if method.http_options %} +{% for method in service.methods.values() if 'rest' in opts.transport %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %}{% if method.http_options %} {# TODO(kbandes): remove this if condition when client streaming are supported. #} {% if not method.client_streaming %} @pytest.mark.parametrize("request_type", [ @@ -1250,7 +1250,7 @@ def test_{{ method.name|snake_case }}_rest(request_type): response_value._content = json_return_value.encode('UTF-8') req.return_value = response_value {% if method.client_streaming %} - response = client.{{ method.name|snake_case }}(iter(requests)) + response = client.{{ method.safe_name|snake_case }}(iter(requests)) {% elif method.server_streaming %} with mock.patch.object(response_value, 'iter_content') as iter_content: iter_content.return_value = iter(json_return_value) @@ -1546,7 +1546,7 @@ def test_{{ method_name }}_rest_bad_request(transport: str = 'rest', request_typ response_value.request = Request() req.return_value = response_value {% if method.client_streaming %} - client.{{ method.name|snake_case }}(iter(requests)) + client.{{ method.safe_name|snake_case }}(iter(requests)) {% else %} client.{{ method_name }}(request) {% endif %} @@ -1814,7 +1814,7 @@ def test_{{ method_name }}_rest_no_http_options(): {% endfor -%} {#- method in methods for rest #} {% for method in service.methods.values() if 'rest' in opts.transport and - not method.http_options %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.name|snake_case %} + not method.http_options %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %} def test_{{ method_name }}_rest_error(): client = {{ service.client_name }}( credentials=ga_credentials.AnonymousCredentials(), 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 0a3b1d21f..17603ca6d 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 @@ -946,7 +946,7 @@ def test_{{ service.client_name|snake_case }}_create_channel_credentials_file(cl {% endfor -%} {#- method in methods for rest #} {% for method in service.methods.values() if 'rest' in opts.transport and - not method.http_options %}{% with method_name = (method.name + ("_unary" if method.operation_service else "")) | snake_case %} + not method.http_options %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %} def test_{{ method_name }}_rest_error(): client = {{ service.client_name }}( credentials=ga_credentials.AnonymousCredentials(), diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index d76cd91a8..70f5af67b 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -1,5 +1,5 @@ {% macro grpc_required_tests(method, service, api, full_extended_lro=False) %} -{% with method_name = method.safe_name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %} +{% with method_name = method.name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %} @pytest.mark.parametrize("request_type", [ {{ method.input.ident }}, dict, @@ -238,7 +238,7 @@ async def test_{{ method_name }}_async_use_cached_wrapped_rpc(transport: str = " {% if method.client_streaming %} request = [{}] - await client.{{ method.name|snake_case }}(request) + await client.{{ method.safe_name|snake_case }}(request) {% else %} request = {} await client.{{ method_name }}(request) @@ -255,7 +255,7 @@ async def test_{{ method_name }}_async_use_cached_wrapped_rpc(transport: str = " {% endif %} {% if method.client_streaming %} - await client.{{ method.name|snake_case }}(request) + await client.{{ method.safe_name|snake_case }}(request) {% else %} await client.{{ method_name }}(request) {% endif %} @@ -321,9 +321,9 @@ async def test_{{ method_name }}_async(transport: str = 'grpc_asyncio', request_ )) {% endif %} {% if method.client_streaming and method.server_streaming %} - response = await client.{{ method.name|snake_case }}(iter(requests)) + response = await client.{{ method.safe_name|snake_case }}(iter(requests)) {% elif method.client_streaming and not method.server_streaming %} - response = await (await client.{{ method.name|snake_case }}(iter(requests))) + response = await (await client.{{ method.safe_name|snake_case }}(iter(requests))) {% else %} response = await client.{{ method_name }}(request) {% endif %} @@ -1001,7 +1001,7 @@ def test_{{ method_name }}_raw_page_lro(): {% endmacro %} {% macro rest_required_tests(method, service, numeric_enums=False, full_extended_lro=False) %} -{% with method_name = method.safe_name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %}{% if method.http_options %} +{% with method_name = method.name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %}{% if method.http_options %} {# TODO(kbandes): remove this if condition when lro and client streaming are supported. #} {% if not method.client_streaming %} def test_{{ method_name }}_rest_use_cached_wrapped_rpc(): @@ -1460,7 +1460,7 @@ def test_{{ method_name }}_rest_no_http_options(): #} {% macro method_call_test_generic(test_name, method, service, api, transport, request_dict, is_async=False, routing_param=None) %} {% set transport_name = get_transport_name(transport, is_async) %} -{% with method_name = (method.name + ("_unary" if method.operation_service else "")) | snake_case %} +{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %} {% set async_method_prefix = "async " if is_async else "" %} {% if is_async %} @pytest.mark.asyncio @@ -1713,7 +1713,7 @@ def test_unsupported_parameter_rest_asyncio(): {% set async_prefix = get_async_prefix(is_async) %} {% set async_decorator = get_async_decorator(is_async) %} {% set transport_name = get_transport_name(transport, is_async) %} -{% set method_name = method.name|snake_case %} +{% set method_name = method.safe_name|snake_case %} {{async_decorator}} {{async_prefix}}def test_{{ method_name }}_{{transport_name}}_error(): {% if transport_name == 'rest_asyncio' %} @@ -1763,7 +1763,7 @@ def test_initialize_client_w_{{transport_name}}(): {% set async_prefix = get_async_prefix(is_async) %} {% set async_decorator = get_async_decorator(is_async) %} {% set transport_name = get_transport_name(transport, is_async) %} -{% set method_name = method.name|snake_case %} +{% set method_name = method.safe_name|snake_case %} {% set mocked_session = "AsyncAuthorizedSession" if is_async else "Session" %} {{ async_decorator }} {{ async_prefix }}def test_{{ method_name }}_{{transport_name}}_bad_request(request_type={{ method.input.ident }}): @@ -1862,7 +1862,7 @@ def test_initialize_client_w_{{transport_name}}(): {% set async_prefix = get_async_prefix(is_async) %} {% set async_decorator = get_async_decorator(is_async) %} {% set transport_name = get_transport_name(transport, is_async) %} -{% set method_name = method.name|snake_case %} +{% set method_name = method.safe_name|snake_case %} {# NOTE: set method_output to method.extended_lro.operation_type for the following method types: # (method.extended_lro and not full_extended_lro) #} @@ -2183,7 +2183,7 @@ def test_initialize_client_w_{{transport_name}}(): {% set async_prefix = get_async_prefix(is_async) %} {% set async_decorator = get_async_decorator(is_async) %} {% set transport_name = get_transport_name(transport, is_async) %} -{% set method_name = method.name|snake_case %} +{% set method_name = method.safe_name|snake_case %} {% set async_method_prefix = "Async" if is_async else "" %} {{async_decorator}} @pytest.mark.parametrize("null_interceptor", [True, False]) diff --git a/tests/fragments/test_reserved_method_names.proto b/tests/fragments/test_reserved_method_names.proto index d8f23494f..ba89ef0f2 100644 --- a/tests/fragments/test_reserved_method_names.proto +++ b/tests/fragments/test_reserved_method_names.proto @@ -30,6 +30,13 @@ service MyService { }; }; + rpc Import(CreateImportRequest) returns (CreateImportResponse) { + option (google.api.http) = { + body: "*" + post: "/import/v1" + }; + }; + rpc GrpcChannel(GrpcChannelRequest) returns (GrpcChannelResponse) { option (google.api.http) = { body: "*" @@ -59,6 +66,14 @@ message CreateChannelResponse { string info = 1; } +message CreateImportRequest { + string info = 1; +} + +message CreateImportResponse { + string info = 1; +} + message GrpcChannelRequest { string grpc_channel = 1; string info = 2; From 637fb94c78cf86453f7e93e86b87a58826285956 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 10 Oct 2024 18:33:57 +0000 Subject: [PATCH 2/2] revert --- .../tests/unit/gapic/%name_%version/%sub/test_macros.j2 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 index 70f5af67b..38c43b0ba 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2 @@ -1,5 +1,5 @@ {% macro grpc_required_tests(method, service, api, full_extended_lro=False) %} -{% with method_name = method.name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %} +{% with method_name = method.safe_name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %} @pytest.mark.parametrize("request_type", [ {{ method.input.ident }}, dict, @@ -1001,7 +1001,7 @@ def test_{{ method_name }}_raw_page_lro(): {% endmacro %} {% macro rest_required_tests(method, service, numeric_enums=False, full_extended_lro=False) %} -{% with method_name = method.name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %}{% if method.http_options %} +{% with method_name = method.safe_name|snake_case + "_unary" if method.extended_lro and not full_extended_lro else method.safe_name|snake_case, method_output = method.extended_lro.operation_type if method.extended_lro and not full_extended_lro else method.output %}{% if method.http_options %} {# TODO(kbandes): remove this if condition when lro and client streaming are supported. #} {% if not method.client_streaming %} def test_{{ method_name }}_rest_use_cached_wrapped_rpc():