From 3a2314318de229a3353c984a8cb2766ae95cc968 Mon Sep 17 00:00:00 2001 From: Dov Shlachter Date: Tue, 28 Jul 2020 14:57:41 -0700 Subject: [PATCH] feat: bypass request copying in method calls (#557) If a proto-plus message is passed in, do not copy it. --- .../%sub/services/%service/client.py.j2 | 36 +++++++++-------- .../%name_%version/%sub/test_%service.py.j2 | 11 +++-- .../%sub/services/%service/client.py.j2 | 40 +++++++++++-------- .../%name_%version/%sub/test_%service.py.j2 | 10 +++-- 4 files changed, 58 insertions(+), 39 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 ffb77b3ce4..6fd1f72621 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 @@ -287,24 +287,28 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): request = {{ method.input.ident }}() {% endif -%}{# Cross-package req and flattened fields #} {%- else %} - request = {{ method.input.ident }}(request) + # Minor optimization to avoid making a copy if the user passes + # in a {{ method.input.ident }}. + # There's no risk of modifying the input as we've already verified + # there are no flattened fields. + if not isinstance(request, {{ method.input.ident }}): + request = {{ method.input.ident }}(request) {% endif %} {# different request package #} - {#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #} - {% if method.flattened_fields -%} - # If we have keyword arguments corresponding to fields on the - # request, apply these. - {% endif -%} - {%- 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 %} + {% if method.flattened_fields -%} + # If we have keyword arguments corresponding to fields on the + # request, apply these. + {% endif -%} + {%- 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, # and friendly error handling. 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 80d9f43214..fc5a596d6f 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 @@ -201,7 +201,7 @@ def test_{{ service.client_name|snake_case }}_client_options_from_dict(): {% for method in service.methods.values() -%} -def test_{{ method.name|snake_case }}(transport: str = 'grpc'): +def test_{{ method.name|snake_case }}(transport: str = 'grpc', request_type={{ method.input.ident }}): client = {{ service.client_name }}( credentials=credentials.AnonymousCredentials(), transport=transport, @@ -209,7 +209,7 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): # Everything is optional in proto3 as far as the runtime is concerned, # and we are mocking out the actual API, so just send an empty request. - request = {{ method.input.ident }}() + request = request_type() {% if method.client_streaming %} requests = [request] {% endif %} @@ -250,7 +250,7 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): {% if method.client_streaming %} assert next(args[0]) == request {% else %} - assert args[0] == request + assert args[0] == {{ method.input.ident }}() {% endif %} # Establish that the response is the type that we expect. @@ -275,6 +275,11 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): {% endfor %} {% endif %} + +def test_{{ method.name|snake_case }}_from_dict(): + test_{{ method.name|snake_case }}(request_type=dict) + + {% if method.field_headers and not method.client_streaming %} def test_{{ method.name|snake_case }}_field_headers(): client = {{ service.client_name }}( diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index e85a025ada..9f4ba15908 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -282,7 +282,8 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): {% if method.flattened_fields -%} # Sanity check: If we got a request object, we should *not* have # gotten any keyword arguments that map to the request. - if request is not None and any([{{ method.flattened_fields.values()|join(', ', attribute='name') }}]): + has_flattened_params = any([{{ method.flattened_fields.values()|join(', ', attribute='name') }}]) + if request is not None and has_flattened_params: raise ValueError('If the `request` argument is set, then none of ' 'the individual field arguments should be set.') @@ -297,24 +298,29 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): request = {{ method.input.ident }}() {% endif -%}{# Cross-package req and flattened fields #} {%- else %} - request = {{ method.input.ident }}(request) + # Minor optimization to avoid making a copy if the user passes + # in a {{ method.input.ident }}. + # There's no risk of modifying the input as we've already verified + # there are no flattened fields. + if not isinstance(request, {{ method.input.ident }}): + request = {{ method.input.ident }}(request) {% endif %} {# different request package #} - {#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #} - {% if method.flattened_fields -%} - # If we have keyword arguments corresponding to fields on the - # request, apply these. - {% endif -%} - {%- 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 %} + {#- Vanilla python protobuf wrapper types cannot _set_ repeated fields #} + {% if method.flattened_fields -%} + # If we have keyword arguments corresponding to fields on the + # request, apply these. + {% endif -%} + {%- 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, # and friendly error handling. 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 f07153ff3d..2fc3923b2a 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 @@ -299,7 +299,7 @@ def test_{{ service.client_name|snake_case }}_client_options_from_dict(): {% for method in service.methods.values() -%} -def test_{{ method.name|snake_case }}(transport: str = 'grpc'): +def test_{{ method.name|snake_case }}(transport: str = 'grpc', request_type={{ method.input.ident }}): client = {{ service.client_name }}( credentials=credentials.AnonymousCredentials(), transport=transport, @@ -307,7 +307,7 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): # Everything is optional in proto3 as far as the runtime is concerned, # and we are mocking out the actual API, so just send an empty request. - request = {{ method.input.ident }}() + request = request_type() {% if method.client_streaming %} requests = [request] {% endif %} @@ -348,7 +348,7 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): {% if method.client_streaming %} assert next(args[0]) == request {% else %} - assert args[0] == request + assert args[0] == {{ method.input.ident }}() {% endif %} # Establish that the response is the type that we expect. @@ -374,6 +374,10 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'): {% endif %} +def test_{{ method.name|snake_case }}_from_dict(): + test_{{ method.name|snake_case }}(request_type=dict) + + @pytest.mark.asyncio async def test_{{ method.name|snake_case }}_async(transport: str = 'grpc_asyncio'): client = {{ service.async_client_name }}(