Skip to content

fix: resolve issue where explicit routing metadata was not sent in async clients #2133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
parthea merged 28 commits into from
Sep 18, 2024
Merged

fix: resolve issue where explicit routing metadata was not sent in async clients #2133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
46bf5cf
chore: refactor empty_call_test
parthea Sep 1, 2024
78acc42
method_name -> method.name
parthea Sep 3, 2024
0c1e5a2
Merge branch 'main' into refactor-empty-call-test
parthea Sep 5, 2024
ea9f823
method.name -> method.safe_name|snake_case
parthea Sep 5, 2024
d9a46bd
add with block
parthea Sep 7, 2024
4616a53
Merge branch 'main' into refactor-empty-call-test
parthea Sep 7, 2024
7288f40
use get_credentials and get_client macro
parthea Sep 7, 2024
99fb463
restore new line
parthea Sep 7, 2024
e53b87f
use common testing framework
parthea Sep 9, 2024
d9664af
test: improve routing parameter assert
parthea Sep 8, 2024
1691274
chore: refactor routing_parameters test
parthea Sep 9, 2024
fd74cbc
test: improve routing parameter assert
parthea Sep 8, 2024
fb0f9f4
Reviewer suggested edit
parthea Sep 14, 2024
f0ffb4c
optimize calls to RoutingParameter.to_regex
parthea Sep 14, 2024
1227c7c
@functools.lru_cache->@functools.cache
parthea Sep 14, 2024
e50bbd6
optimize calls to RoutingParameter.key
parthea Sep 14, 2024
5b9dbb3
update comment
parthea Sep 14, 2024
c3c683f
revert back to @functools.lru_cache
parthea Sep 14, 2024
dd28fce
fix build
parthea Sep 14, 2024
d6824b8
Merge branch 'main' into refactor-routing-parameters-test
parthea Sep 15, 2024
9eaa5b6
fix build
parthea Sep 15, 2024
0eaab3a
Merge branch 'fix-routing-parameter-assert' into refactor-routing-par…
parthea Sep 15, 2024
ce15214
restore assert
parthea Sep 15, 2024
c955243
See https://github.com/googleapis/gapic-generator-python/issues/2159
parthea Sep 15, 2024
850085e
Merge branch 'main' into refactor-routing-parameters-test
parthea Sep 16, 2024
3da4cc9
address review feedback
parthea Sep 16, 2024
5f945a6
typo
parthea Sep 18, 2024
5fa4a39
address review feedback
parthea Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 1 addition & 39 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/_client_macros.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,45 +144,7 @@
# and friendly error handling.
rpc = self._transport._wrapped_methods[self._transport.{{ method.transport_safe_name|snake_case}}]

{% if method.explicit_routing %}
header_params = {}
{% if not method.client_streaming %}
{% for routing_param in method.routing_rule.routing_parameters %}
{% if routing_param.path_template %} {# Need to match. #}

routing_param_regex = {{ routing_param.to_regex() }}
regex_match = routing_param_regex.match(request.{{ routing_param.field }})
if regex_match and regex_match.group("{{ routing_param.key }}"):
header_params["{{ routing_param.key }}"] = regex_match.group("{{ routing_param.key }}")

{% else %}

if request.{{ routing_param.field }}:
header_params["{{ routing_param.key }}"] = request.{{ routing_param.field }}

{% endif %}
{% endfor %} {# method.routing_rule.routing_parameters #}
{% endif %} {# if not method.client_streaming #}

if header_params:
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata(header_params),
)

{% elif method.field_headers %} {# implicit routing #}
# Certain fields should be provided within the metadata header;
# add these here.
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{% for field_header in method.field_headers %}
{% if not method.client_streaming %}
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
{% endif %}
{% endfor %}
)),
)
{% endif %} {# method.explicit_routing #}

{{ shared_macros.create_metadata(method) }}
{{ shared_macros.add_api_version_header_to_metadata(service.version) }}
{{ shared_macros.auto_populate_uuid4_fields(api, method) }}

Expand Down
41 changes: 41 additions & 0 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/_shared_macros.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,47 @@ except ImportError: # pragma: NO COVER
{% endif %}{# service_version #}
{% endmacro %}

{% macro create_metadata(method) %}
{% if method.explicit_routing %}
Copy link
Contributor

@daniel-sanche daniel-sanche Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To resolve #2078, we'll probably need to add an extra check here for passed-in routing metadata:

metadata = tuple(metadata)
# add routing headers if not passed in metadata
if not any (m[0] == gapic_v1.routing_header.ROUTING_METADATA_KEY for m in metadata):
   {% if method.explicit_routing %}
   ...

I have #2079 open to try to address this, and I plan to fix it up to match this new structure after this PR is merged. But I'm mentioning it now in case you think it makes more sense to just address these together here

Copy link
Contributor Author

@parthea parthea Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For #2078 we need to update AIPs first so there is consistent behavior across all language-specific generators: https://google.aip.dev/client-libraries/4222

I'm going to go ahead without #2078 as there is still work pending

header_params = {}
{% if not method.client_streaming %}
{% for routing_param in method.routing_rule.routing_parameters %}
{% if routing_param.path_template %} {# Need to match. #}

routing_param_regex = {{ routing_param.to_regex() }}
regex_match = routing_param_regex.match(request.{{ routing_param.field }})
if regex_match and regex_match.group("{{ routing_param.key }}"):
header_params["{{ routing_param.key }}"] = regex_match.group("{{ routing_param.key }}")

{% else %}

if request.{{ routing_param.field }}:
header_params["{{ routing_param.key }}"] = request.{{ routing_param.field }}

{% endif %}
{% endfor %} {# method.routing_rule.routing_parameters #}
{% endif %} {# if not method.client_streaming #}

if header_params:
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata(header_params),
)

{% elif method.field_headers %}{# implicit routing #}
# Certain fields should be provided within the metadata header;
# add these here.
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{% if not method.client_streaming %}
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I know this PR just moved the pre-existing code into this macro, but we could move the {% if %} outside the {% for %} since method is not altered by the loop.

Copy link
Contributor Author

@parthea parthea Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3da4cc9

{% for field_header in method.field_headers %}
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
{% endfor %}{# for field_header in method.field_headers #}
{% endif %}{# not method.client_streaming #}
)),
)
{% endif %}{# method.explicit_routing #}
{% endmacro %}{# create_metadata #}

{% macro add_api_version_header_to_metadata(service_version) %}
{#
Add API Version to metadata as per https://github.com/aip-dev/google.aip.dev/pull/1331.
Expand Down
15 changes: 1 addition & 14 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,20 +364,7 @@ class {{ service.async_client_name }}:
# and friendly error handling.
rpc = self._client._transport._wrapped_methods[self._client._transport.{{ method.transport_safe_name|snake_case }}]

{% if method.field_headers %}
# Certain fields should be provided within the metadata header;
# add these here.
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{% for field_header in method.field_headers %}
{% if not method.client_streaming %}
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
{% endif %}
{% endfor %}
)),
)
{% endif %}

{{ shared_macros.create_metadata(method) }}
{{ shared_macros.add_api_version_header_to_metadata(service.version) }}
{{ shared_macros.auto_populate_uuid4_fields(api, method) }}

Expand Down
2 changes: 2 additions & 0 deletions gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,8 @@ def test_transport_adc(transport_class):
{{ test_macros.transport_kind_test(**conf) }}

{{ test_macros.empty_call_test(**conf) }}

{{ test_macros.routing_parameter_test(**conf) }}
{% endfor %}

{% if 'grpc' in opts.transport %}
Expand Down
80 changes: 33 additions & 47 deletions gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,43 +386,6 @@ async def test_{{ method_name }}_async_from_dict():
await test_{{ method_name }}_async(request_type=dict)
{% endif %}{# full_extended_lro #}

{% if method.explicit_routing %}
def test_{{ method.name|snake_case }}_routing_parameters():
client = {{ service.client_name }}(
credentials=ga_credentials.AnonymousCredentials(),
)

{% for routing_param in method.routing_rule.routing_parameters %}
# Any value that is part of the HTTP/1.1 URI should be sent as
# a field header. Set these to a non-empty value.
request = {{ method.input.ident }}(**{{ routing_param.sample_request }})

# Mock the actual call within the gRPC stub, and fake the request.
with mock.patch.object(
type(client.transport.{{ method.transport_safe_name|snake_case }}),
'__call__') as 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 %}
client.{{ method.safe_name|snake_case }}(request)

# Establish that the underlying gRPC stub method was called.
assert len(call.mock_calls) == 1
_, args, kw = call.mock_calls[0]
assert args[0] == request

expected_headers = {{ method.routing_rule.resolve(method.routing_rule, routing_param.sample_request) }}
assert gapic_v1.routing_header.to_grpc_metadata(expected_headers) in kw['metadata']
{% endfor %}
{% endif %}


{% if method.field_headers and not method.client_streaming and not method.explicit_routing %}
def test_{{ method_name }}_field_headers():
client = {{ service.client_name }}(
Expand Down Expand Up @@ -1797,14 +1760,13 @@ def test_{{ method_name }}_rest_no_http_options():
{% endwith %}{# method_name #}
{% endmacro %}


{#
This is a generic macro for testing method calls. Ideally this macro can be used to avoid duplication
in Jinja templates. If this macro cannot be custimized for a specific method call test, consider
creating a new macro with the name `method_call_test_<with_customization>` for the macro which supports
a more customized method call.
#}
{% macro method_call_test_generic(test_name, method, service, api, transport, request, is_async=False) %}
{% 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 %}
{% set async_method_prefix = "async " if is_async else "" %}
Expand Down Expand Up @@ -1844,7 +1806,7 @@ def test_{{ method_name }}_rest_no_http_options():
{% endfor %}
))
{% endif %}{# method.void #}
await client.{{ method_name }}(request={{ request }})
await client.{{ method_name }}(request={{ request_dict }})
{% else %}{# if not is_async #}
{% if method.void %}
call.return_value = None
Expand All @@ -1855,12 +1817,12 @@ def test_{{ method_name }}_rest_no_http_options():
{% else %}
call.return_value = {{ method.output.ident }}()
{% endif %}
client.{{ method_name }}(request={{ request }})
client.{{ method_name }}(request={{ request_dict }})
{% endif %}{# is_async #}

# Establish that the underlying gRPC stub method was called.
call.assert_called()
_, args, _ = call.mock_calls[0]
_, args, {% if routing_param %}kw{% else %}_{% endif %} = call.mock_calls[0]
{% with method_settings = api.all_method_settings.get(method.meta.address.proto) %}
{% if method_settings is not none %}
{% for auto_populated_field in method_settings.auto_populated_fields %}
Expand All @@ -1871,11 +1833,17 @@ def test_{{ method_name }}_rest_no_http_options():
{% endfor %}{# for auto_populated_field in method_settings.auto_populated_fields #}
{% endif %}{# if method_settings is not none #}
{% endwith %}{# method_settings #}
{% if request %}
assert args[0] == {{ request }}
{% if request_dict %}
request_msg = {{ method.input.ident }}(**{{ request_dict }})
{% else %}
assert args[0] == {{ method.input.ident }}()
{% endif %}{# request #}
request_msg = {{ method.input.ident }}()
{% endif %}{# request_dict #}
assert args[0] == request_msg

{% if routing_param %}
expected_headers = {{ method.routing_rule.resolve(method.routing_rule, routing_param.sample_request) }}
assert gapic_v1.routing_header.to_grpc_metadata(expected_headers) in kw['metadata']
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code path tested? I don't find it in any of the goldens.

Copy link
Contributor Author

@parthea parthea Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can add an assert False when running the showcase unit tests and tests will fail. I attempted to add golden files for showcase in #1991 but ran into the issue in #1993

To test locally make the following change and run nox -s showcase_unit-3.9

(py392) partheniou@partheniou-vm-3:~/git/gapic-generator-python$ git diff
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 f3c20ea5..e9cb6c0c 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
@@ -1843,6 +1843,7 @@ def test_{{ method_name }}_rest_no_http_options():
         {% if routing_param %}
         expected_headers = {{ method.routing_rule.resolve(method.routing_rule, routing_param.sample_request) }}
         assert gapic_v1.routing_header.to_grpc_metadata(expected_headers) in kw['metadata']
+        assert False
         {% endif %}
 {% endwith %}{# method_name #}
 {% endmacro %}

(py392) partheniou@partheniou-vm-3:~/git/gapic-generator-python$ nox -s showcase_unit-3.9
nox > Running session showcase_unit-3.9
nox > Creating virtual environment (virtualenv) using python3.9 in .nox/showcase_unit-3-9
...
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_7_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_2_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_1_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_8_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_3_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_2_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_4_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_3_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_1_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_5_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_4_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_6_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_5_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_7_grpc_asyncio - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_6_grpc - assert False
FAILED tests/unit/gapic/showcase_v1beta1/test_echo.py::test_echo_routing_parameters_request_8_grpc_asyncio - assert False
16 failed, 1134 passed, 10 skipped, 58 warnings in 17.76s

{% endif %}
{% endwith %}{# method_name #}
{% endmacro %}

Expand Down Expand Up @@ -1910,7 +1878,7 @@ def test_transport_kind_{{ transport_name }}():
{% if not method.client_streaming %}
# This test is a coverage failsafe to make sure that totally empty calls,
# i.e. request == None and no flattened fields passed, work.
{{ method_call_test_generic("empty_call", method, service, api, transport, request=None, is_async=is_async) }}
{{ method_call_test_generic("empty_call", method, service, api, transport, request_dict=None, is_async=is_async) }}
{% endif %}{# not method.client_streaming #}
{% endfor %}{# method in service.methods.values() #}
{% endif %}{# 'rest' not in transport #}
Expand All @@ -1919,3 +1887,21 @@ def test_transport_kind_{{ transport_name }}():
{% macro get_uuid4_re() -%}
[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}
{%- endmacro %}{# uuid_re #}

{% macro routing_parameter_test(service, api, transport, is_async) %}
{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2159):
Currently this macro only supports gRPC. It should be updated to support REST
transport as well.
#}
{% if 'rest' not in transport %}
{% for method in service.methods.values() %}{# method #}
{% if method.explicit_routing %}
{# Any value that is part of the HTTP/1.1 URI should be sent as #}
{# a field header. Set these to a non-empty value. #}
{% for routing_param in method.routing_rule.routing_parameters %}
{{ method_call_test_generic("routing_parameters_request_" + loop.index|string, method, service, api, transport, request_dict=routing_param.sample_request, is_async=is_async, routing_param=routing_param) }}
{% endfor %}{# routing_param in method.routing_rule.routing_parameters #}
{% endif %}{# method.explicit_routing #}
{% endfor %}{# method in service.methods.values() #}
{% endif %}
{% endmacro %}{# routing_parameter_test #}
Loading