Cross package request messages are constructed specially (#317)

A cross package request is almost certainly not a proto-plus wrapped
type, which puts certain restrictions on its construction and
manipulation due to constraints in the Python protobuf API.

It is bad practice but neither forbidden nor impossible to write a
method whose request message definitionlives in a different
package. A recurring example is IAM Policy Requests.

This change detects when a method's request lives in a different
package and constructs it either via keyword expansion (a dict was
passed in) or with no ctor params.

Also, in the above scenario, fields are not eligible for flattening if
they point to non-primitive types.

Generated unit tests for services that use out-of-package requests include from-dict construction.

Adds the grpc-google-iam-v1 requirement as a special case in generated setup.py

Author: software-dov

packages/gapic-generator/gapic/schema/api.py

@@ -279,6 +279,13 @@ def subpackages(self) -> Mapping[str, 'API']:
                                                       )
         return answer
 
+    def requires_package(self, pkg: Tuple[str, ...]) -> bool:
+        return any(
+            message.ident.package == pkg
+            for proto in self.all_protos.values()
+            for message in proto.all_messages.values()
+        )
+
 
 class _ProtoBuilder:
     """A "builder class" for Proto objects.

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

@@ -551,12 +551,26 @@ def field_headers(self) -> Sequence[str]:
     def flattened_fields(self) -> Mapping[str, Field]:
         """Return the signature defined for this method."""
         signatures = self.options.Extensions[client_pb2.method_signature]
+        cross_pkg_request = self.input.ident.package != self.ident.package
+
+        def filter_fields(sig):
+            for f in sig.split(','):
+                if not f:
+                    # Special case for an empty signature
+                    continue
+                name = f.strip()
+                field = self.input.get_field(*name.split('.'))
+                if cross_pkg_request and not field.is_primitive:
+                    # This is not a proto-plus wrapped message type,
+                    # and setting a non-primitive field directly is verboten.
+                    continue
+
+                yield name, field
 
         answer: Dict[str, Field] = collections.OrderedDict(
-            (f.strip(), self.input.get_field(*f.strip().split('.')))
+            name_and_field
             for sig in signatures
-            # Special case for an empty signature check
-            for f in sig.split(',') if f
+            for name_and_field in filter_fields(sig)
         )
 
         return answer

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

@@ -188,14 +188,30 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
             raise ValueError('If the `request` argument is set, then none of '
                              'the individual field arguments should be set.')
 
-        # If we have keyword arguments corresponding to fields on the
-        # request, apply these.
         {% endif -%}
+        {% if method.input.ident.package != method.ident.package -%} {# request lives in a different package, so there is no proto wrapper #}
+        # The request isn't a proto-plus wrapped type,
+        # so it must be constructed via keyword expansion.
+        if isinstance(request, dict):
+            request = {{ method.input.ident }}(**request)
+        elif not request:
+            request = {{ method.input.ident }}()
+        {%- else %}
         request = {{ method.input.ident }}(request)
-        {%- for key, field in method.flattened_fields.items() %}
+        {% endif %} {# different request package #}
+
+        # If we have keyword arguments corresponding to fields on the
+        # request, apply these.
+        {#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #}
+        {%- for key, field in method.flattened_fields.items() if not(field.repeated and method.input.ident.package != method.ident.package) %}
         if {{ field.name }} is not None:
             request.{{ key }} = {{ field.name }}
         {%- endfor %}
+        {# They can be _extended_, however -#}
+        {%- for key, field in method.flattened_fields.items() if (field.repeated and method.input.ident.package != method.ident.package) %}
+        if {{ field.name }}:
+            request.{{ key }}.extend({{ field.name }})
+        {%- endfor %}
         {%- endif %}
 
         # Wrap the RPC method; this adds retry and timeout information,

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

@@ -20,6 +20,9 @@ setuptools.setup(
         'googleapis-common-protos >= 1.5.8',
         'grpcio >= 1.10.0',
         'proto-plus >= 0.4.0',
+    {%- if api.requires_package(('google', 'iam', 'v1')) %}
+        'grpc-google-iam-v1',
+    {%- endif %}
     ),
     python_requires='>={% if opts.lazy_import %}3.7{% else %}3.6{% endif %}',{# Lazy import requires module-level getattr #}
     setup_requires=[

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

@@ -165,6 +165,35 @@ def test_{{ method.name|snake_case }}_field_headers():
     ) in kw['metadata']
 {% endif %}
 
+{% if method.ident.package != method.input.ident.package %}
+def test_{{ method.name|snake_case }}_from_dict():
+    client = {{ service.client_name }}(
+        credentials=credentials.AnonymousCredentials(),
+    )
+    # Mock the actual call within the gRPC stub, and fake the request.
+    with mock.patch.object(
+            type(client._transport.{{ method.name|snake_case }}),
+            '__call__') as call:
+        # Designate an appropriate return value for the call.
+        {% if method.void -%}
+        call.return_value = None
+        {% elif method.lro -%}
+        call.return_value = operations_pb2.Operation(name='operations/op')
+        {% elif method.server_streaming -%}
+        call.return_value = iter([{{ method.output.ident }}()])
+        {% else -%}
+        call.return_value = {{ method.output.ident }}()
+        {% endif %}
+        response = client.{{ method.name|snake_case }}(request={
+            {%- for field in method.input.fields.values() %}
+            '{{ field.name }}': {{ field.mock_value }},
+            {%- endfor %}
+            }
+        )
+        call.assert_called()
+
+{% endif %}
+
 {% if method.flattened_fields %} 
 def test_{{ method.name|snake_case }}_flattened(): 
     client = {{ service.client_name }}( 

packages/gapic-generator/tests/unit/schema/test_api.py

@@ -87,6 +87,9 @@ def test_api_build():
         imp.Import(package=('google', 'dep'), module='dep_pb2'),
     )
 
+    assert api_schema.requires_package(('google', 'example', 'v1'))
+    assert not api_schema.requires_package(('elgoog', 'example', 'v1'))
+
     # Establish that the subpackages work.
     assert 'common' in api_schema.subpackages
     sub = api_schema.subpackages['common']

packages/gapic-generator/tests/unit/schema/wrappers/test_method.py

@@ -179,9 +179,35 @@ def test_method_flattened_fields_empty_sig():
     assert len(method.flattened_fields) == 0
 
 
+def test_method_flattened_fields_different_package_non_primitive():
+    # This test verifies that method flattening handles a special case where:
+    # * the method's request message type lives in a different package and
+    # * a field in the method_signature is a non-primitive.
+    #
+    # If the message is defined in a different package it is not guaranteed to
+    # be a proto-plus wrapped type, which puts restrictions on assigning
+    # directly to its fields, which complicates request construction.
+    # The easiest solution in this case is to just prohibit these fields
+    # in the method flattening.
+    message = make_message('Mantle',
+                           package="mollusc.cephalopod.v1", module="squid")
+    mantle = make_field('mantle', type=11, type_name='Mantle',
+                        message=message, meta=message.meta)
+    arms_count = make_field('arms_count', type=5, meta=message.meta)
+    input_message = make_message(
+        'Squid', fields=(mantle, arms_count),
+        package=".".join(message.meta.address.package),
+        module=message.meta.address.module
+    )
+    method = make_method('PutSquid', input_message=input_message,
+                         package="remote.package.v1", module="module", signatures=("mantle,arms_count",))
+    assert set(method.flattened_fields) == {'arms_count'}
+
+
 def test_method_include_flattened_message_fields():
     a = make_field('a', type=5)
-    b = make_field('b', type=11, message=make_message('Eggs'))
+    b = make_field('b', type=11, type_name='Eggs',
+                   message=make_message('Eggs'))
     input_msg = make_message('Z', fields=(a, b))
     method = make_method('F', input_message=input_msg, signatures=('a,b',))
     assert len(method.flattened_fields) == 2
@@ -274,7 +300,7 @@ def make_method(
         output=output_message,
         meta=metadata.Metadata(address=metadata.Address(
             name=name,
-            package=package,
+            package=tuple(package.split('.')),
             module=module,
             parent=(f'{name}Service',),
         )),

packages/gapic-generator/tests/unit/schema/wrappers/test_service.py

@@ -280,6 +280,7 @@ def get_method(name: str,
         input=input_,
         output=output,
         lro=lro,
+        meta=input_.meta,
     )