fix: Fixed internal method generation naming issues (#2365)

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>

Author: 3 people

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.safe_name|snake_case }}(self,
+    def {{ method.client_method_name|snake_case }}(self,
     {% endif %}{# Extended Operations LRO #}
             {% if not method.client_streaming %}
             request: Optional[Union[{{ method.input.ident }}, dict]] = None,

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.safe_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.client_method_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.safe_name|snake_case }}(iter(requests))
+        response = client.{{ method.client_method_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.safe_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.client_method_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", [
@@ -1252,7 +1252,7 @@ def test_{{ method.name|snake_case }}_rest(request_type):
         req.return_value = response_value
         req.return_value.headers = {"header-1": "value-1", "header-2": "value-2"}
         {% if method.client_streaming %}
-        response = client.{{ method.safe_name|snake_case }}(iter(requests))
+        response = client.{{ method.client_method_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)
@@ -1550,7 +1550,7 @@ def test_{{ method_name }}_rest_bad_request(transport: str = 'rest', request_typ
         req.return_value = response_value
         req.return_value.headers = {"header-1": "value-1", "header-2": "value-2"}
         {% if method.client_streaming %}
-        client.{{ method.safe_name|snake_case }}(iter(requests))
+        client.{{ method.client_method_name|snake_case }}(iter(requests))
         {% else %}
         client.{{ method_name }}(request)
         {% endif %}
@@ -1818,7 +1818,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.safe_name|snake_case %}
+    not method.http_options %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.client_method_name|snake_case %}
 def test_{{ method_name }}_rest_error():
     client = {{ service.client_name }}(
         credentials=ga_credentials.AnonymousCredentials(),

gapic/samplegen/samplegen.py

@@ -1063,8 +1063,9 @@ def generate_sample_specs(
                 # [{START|END} ${apishortname}_${apiVersion}_generated_${serviceName}_${rpcName}_{sync|async|rest}]
                 region_tag = f"{api_short_name}_{api_version}_generated_{service_name}_{rpc_name}_{sync_or_async}"
 
-                # We assume that the only methods that start with an underscore are internal methods.
-                is_internal = rpc_name.startswith("_")
+                is_internal = api_schema.all_methods[
+                    f"{api_schema.naming.proto_package}.{service_name}.{rpc_name}"
+                ].is_internal
                 if is_internal:
                     region_tag += "_internal"
 
@@ -1119,7 +1120,9 @@ def _fill_sample_metadata(sample: dict, api_schema: api.API):
 
     # Client Method
     setattr(snippet_metadata.client_method, "async", async_)
-    snippet_metadata.client_method.short_name = utils.to_snake_case(method.name)
+    snippet_metadata.client_method.short_name = utils.to_snake_case(
+        method.client_method_name
+    )
     snippet_metadata.client_method.full_name = f"{snippet_metadata.client_method.client.full_name}.{snippet_metadata.client_method.short_name}"
 
     if not method.void:
@@ -1217,6 +1220,7 @@ def generate_sample(
         )
 
     calling_form = types.CallingForm.method_default(rpc)
+    sample["is_internal"] = rpc.is_internal
 
     v = Validator(rpc, api_schema)
     # Tweak some small aspects of the sample to set defaults for optional

gapic/schema/api.py

@@ -687,7 +687,7 @@ def gapic_metadata(self, options: Options) -> gapic_metadata_pb2.GapicMetadata:
                 transport.library_client = client_name
                 for method in methods:
                     method_desc = transport.rpcs.get_or_create(method.name)
-                    method_desc.methods.append(to_snake_case(method.name))
+                    method_desc.methods.append(to_snake_case(method.client_method_name))
 
         return gm
 

gapic/schema/wrappers.py

@@ -1499,11 +1499,14 @@ def __getattr__(self, name):
         return getattr(self.method_pb, name)
 
     @property
-    def safe_name(self) -> str:
-        # Used to prevent collisions with python keywords at the client level
+    def client_method_name(self) -> str:
+        """Returns the name of the generated method name.
 
-        name = self.name
-        return name + "_" if name.lower() in keyword.kwlist else name
+        This is used to prevent collisions with Python keywords at the
+        client level, as well as prefix internal method names with underscore.
+        """
+        name = self.name + "_" if self.name.lower() in keyword.kwlist else self.name
+        return make_private(name) if self.is_internal else name
 
     @property
     def transport_safe_name(self) -> str:
@@ -1534,14 +1537,6 @@ def is_operation_polling_method(self):
             and self.options.Extensions[ex_ops_pb2.operation_polling_method]
         )
 
-    @utils.cached_property
-    def name(self):
-        return (
-            make_private(self.method_pb.name)
-            if self.is_internal
-            else self.method_pb.name
-        )
-
     @utils.cached_property
     def client_output(self):
         return self._client_output(enable_asyncio=False)
@@ -2435,17 +2430,10 @@ def with_internal_methods(self, *, public_methods: Set[str]) -> "Service":
             Service: A version of this `Service` with `Method` objects corresponding to methods
                 not in `public_methods` marked as internal.
         """
-
-        # Internal methods need to be keyed with underscore prefixed method names
-        # (e.g. google.Service.Method -> google.Service._Method) in order for
-        # samplegen to work properly.
         return dataclasses.replace(
             self,
             methods={
-                meth.name: meth
-                for meth in (
-                    meth.with_internal_methods(public_methods=public_methods)
-                    for meth in self.methods.values()
-                )
+                k: v.with_internal_methods(public_methods=public_methods)
+                for k, v in self.methods.items()
             },
         )

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

@@ -266,7 +266,7 @@ class {{ service.async_client_name }}:
             )
 
     {% for method in service.methods.values() %}
-      {% with method_name = method.safe_name|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %}
+      {% with method_name = method.client_method_name|snake_case + "_unary" if method.operation_service else method.client_method_name|snake_case %}
     {%+ if not method.server_streaming %}async {% endif %}def {{ method_name }}(self,
       {% endwith %}
             {% if not method.client_streaming %}

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

@@ -654,11 +654,11 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
 
     {% for method in service.methods.values() %}
 {% if method.operation_service %}{# Uses extended operations #}
-{{ macros.client_method(method, method.safe_name|snake_case + "_unary", snippet_index, api, service) }}
+{{ macros.client_method(method, method.client_method_name|snake_case + "_unary", snippet_index, api, service) }}
 
-{{ macros.client_method(method, method.safe_name|snake_case, snippet_index, api, service, full_extended_lro=True) }}
+{{ macros.client_method(method, method.client_method_name|snake_case, snippet_index, api, service, full_extended_lro=True) }}
 {% else %}
-{{ macros.client_method(method, method.safe_name|snake_case, snippet_index, api, service) }}
+{{ macros.client_method(method, method.client_method_name|snake_case, snippet_index, api, service) }}
 {% endif %}
     {% endfor %}
 

gapic/templates/examples/feature_fragments.j2

@@ -235,10 +235,10 @@ await{{ " "}}
 {%- endif -%}
 {% if calling_form in [calling_form_enum.RequestStreamingBidi,
                        calling_form_enum.RequestStreamingClient] %}
-client.{{ sample.rpc|snake_case }}(requests=request_generator())
+client.{{ render_method_name(sample)|trim }}(requests=request_generator())
 {% else %}{# TODO: deal with flattening #}
 {# TODO: set up client streaming once some questions are answered #}
-client.{{ sample.rpc|snake_case }}({{ render_request_params_unary(sample.request)|trim }})
+client.{{ render_method_name(sample)|trim }}({{ render_request_params_unary(sample.request)|trim }})
 {% endif %}
 {% endmacro %}
 
@@ -305,11 +305,15 @@ response = {{ operation_text|trim }}.result()
 {% endif %}
 {% endmacro %}
 
-{% macro render_method_name(method_name) %}
-{{ method_name|snake_case }}
+{% macro render_method_name(sample) %}
+{% if sample.is_internal %}
+_{{ sample.rpc|snake_case }}
+{% else %}
+{{ sample.rpc|snake_case }}
+{% endif %}
 {% endmacro %}
 
-{% macro render_main_block(method_name, full_request) %}
+{% macro render_main_block(sample, full_request) %}
 def main():
     import argparse
 
@@ -330,7 +334,7 @@ def main():
 {% endfor %}
     args = parser.parse_args()
 
-    sample_{{ render_method_name(method_name)|trim }}({{ arg_list|join(", ") }})
+    sample_{{ render_method_name(sample)|trim }}({{ arg_list|join(", ") }})
 
 
 if __name__ == "__main__":

gapic/templates/examples/sample.py.j2

@@ -32,7 +32,7 @@
 
 
 {# also need calling form #}
-{% if sample.transport == "grpc-async" %}async {% endif %}def sample_{{ frags.render_method_name(sample.rpc)|trim }}({{ frags.print_input_params(sample.request)|trim }}):
+{% if sample.transport == "grpc-async" %}async {% endif %}def sample_{{ sample.rpc|snake_case|trim }}({{ frags.print_input_params(sample.request)|trim }}):
     {{ frags.render_client_setup(sample.module_name, sample.client_name)|indent }}
     {{ frags.render_request_setup(sample.request, sample.request_module_name, sample.request_type, calling_form, calling_form_enum)|indent }}
     {% with method_call = frags.render_method_call(sample, calling_form, calling_form_enum, sample.transport) %}
@@ -41,5 +41,5 @@
 
 # [END {{ sample.id }}]
 {# TODO: Enable main block (or decide to remove main block from python sample) #}
-{# {{ frags.render_main_block(sample.rpc, sample.request) }} #}
+{# {{ frags.render_main_block(sample, sample.request) }} #}
 {% endblock %}

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

@@ -927,7 +927,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|snake_case + "_unary" if method.operation_service else method.safe_name|snake_case %}
+    not method.http_options %}{% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.client_method_name|snake_case %}
 def test_{{ method_name }}_rest_error():
     client = {{ service.client_name }}(
         credentials=ga_credentials.AnonymousCredentials(),