Skip to content

Commit bbdd643

Browse files
fix(gapic-generator): do not emit empty routing metadata on client-streaming RPCs (#18591)
Previously, the generator would output empty routing metadata for client-streaming RPCs. Instead, we should skip adding routing metadata altogether when `client_streaming` is true. Addresses #16650 (will be fixed after next generator update)
1 parent fc6eae3 commit bbdd643

9 files changed

Lines changed: 156 additions & 32 deletions

File tree

‎packages/gapic-generator/gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,9 @@ except ImportError: # pragma: NO COVER
6464
{% endmacro %}
6565

6666
{% macro create_metadata(method) %}
67+
{% if not method.client_streaming %}
6768
{% if method.explicit_routing %}
6869
header_params: dict[str, str] = {}
69-
{% if not method.client_streaming %}
7070
{% for routing_param in method.routing_rule.routing_parameters %}
7171
{% if routing_param.path_template %} {# Need to match. #}
7272

@@ -82,7 +82,6 @@ except ImportError: # pragma: NO COVER
8282

8383
{% endif %}
8484
{% endfor %} {# method.routing_rule.routing_parameters #}
85-
{% endif %} {# if not method.client_streaming #}
8685

8786
if header_params:
8887
metadata = tuple(metadata) + (
@@ -94,14 +93,13 @@ except ImportError: # pragma: NO COVER
9493
# add these here.
9594
metadata = tuple(metadata) + (
9695
gapic_v1.routing_header.to_grpc_metadata((
97-
{% if not method.client_streaming %}
9896
{% for field_header in method.field_headers %}
9997
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
10098
{% endfor %}{# for field_header in method.field_headers #}
101-
{% endif %}{# not method.client_streaming #}
10299
)),
103100
)
104101
{% endif %}{# method.explicit_routing #}
102+
{% endif %}{# not method.client_streaming #}
105103
{% endmacro %}{# create_metadata #}
106104

107105
{% macro add_api_version_header_to_metadata(service_version) %}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -480,16 +480,14 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
480480
# Wrap the RPC method; this adds retry and timeout information,
481481
# and friendly error handling.
482482
rpc = self._transport._wrapped_methods[self._transport.{{ method.transport_safe_name|snake_case}}]
483-
{% if method.field_headers %}
483+
{% if method.field_headers and not method.client_streaming %}
484484

485485
# Certain fields should be provided within the metadata header;
486486
# add these here.
487487
metadata = tuple(metadata) + (
488488
gapic_v1.routing_header.to_grpc_metadata((
489489
{% for field_header in method.field_headers %}
490-
{% if not method.client_streaming %}
491490
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
492-
{% endif %}
493491
{% endfor %}
494492
)),
495493
)

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -948,13 +948,11 @@ def test_{{ method_name }}_pager(transport_name: str = "grpc"):
948948
expected_metadata = ()
949949
retry = retries.Retry()
950950
timeout = 5
951-
{% if method.field_headers %}
951+
{% if method.field_headers and not method.client_streaming %}
952952
expected_metadata = tuple(expected_metadata) + (
953953
gapic_v1.routing_header.to_grpc_metadata((
954954
{% for field_header in method.field_headers %}
955-
{% if not method.client_streaming %}
956955
('{{ field_header.raw }}', ''),
957-
{% endif %}
958956
{% endfor %}
959957
)),
960958
)

‎packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/services/%service/_shared_macros.j2‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ except ImportError: # pragma: NO COVER
5353
{% endmacro %}
5454

5555
{% macro create_metadata(method) %}
56+
{% if not method.client_streaming %}
5657
{% if method.explicit_routing %}
5758
header_params: dict[str, str] = {}
58-
{% if not method.client_streaming %}
5959
{% for routing_param in method.routing_rule.routing_parameters %}
6060
{% if routing_param.path_template %} {# Need to match. #}
6161

@@ -71,7 +71,6 @@ except ImportError: # pragma: NO COVER
7171

7272
{% endif %}
7373
{% endfor %} {# method.routing_rule.routing_parameters #}
74-
{% endif %} {# if not method.client_streaming #}
7574

7675
if header_params:
7776
metadata = tuple(metadata) + (
@@ -83,14 +82,13 @@ except ImportError: # pragma: NO COVER
8382
# add these here.
8483
metadata = tuple(metadata) + (
8584
gapic_v1.routing_header.to_grpc_metadata((
86-
{% if not method.client_streaming %}
8785
{% for field_header in method.field_headers %}
8886
("{{ field_header.raw }}", request.{{ field_header.disambiguated }}),
8987
{% endfor %}{# for field_header in method.field_headers #}
90-
{% endif %}{# not method.client_streaming #}
9188
)),
9289
)
9390
{% endif %}{# method.explicit_routing #}
91+
{% endif %}{# not method.client_streaming #}
9492
{% endmacro %}{# create_metadata #}
9593

9694
{% macro add_api_version_header_to_metadata(service_version) %}

‎packages/gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -733,13 +733,11 @@ def test_{{ method_name }}_pager(transport_name: str = "grpc"):
733733
expected_metadata = ()
734734
retry = retries.Retry()
735735
timeout = 5
736-
{% if not method.explicit_routing and method.field_headers %}
736+
{% if not method.explicit_routing and method.field_headers and not method.client_streaming %}
737737
expected_metadata = tuple(expected_metadata) + (
738738
gapic_v1.routing_header.to_grpc_metadata((
739739
{% for field_header in method.field_headers %}
740-
{% if not method.client_streaming %}
741740
('{{ field_header.raw }}', ''),
742-
{% endif %}
743741
{% endfor %}
744742
)),
745743
)

‎packages/gapic-generator/tests/fragments/test_dynamic_routing.proto‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,20 @@ service ServiceImplicitHeaders {
4646
get: "/v2/{resource.inner.inner_name=projects/*/instances/*/tables/*}:withMultipleLevels"
4747
};
4848
}
49+
50+
rpc ClientStreaming(stream Request) returns(Response) {
51+
option (google.api.http) = {
52+
post: "/v2/{table_name=projects/*/instances/*/tables/*}:clientStreaming"
53+
body: "*"
54+
};
55+
}
56+
57+
rpc BidiStreaming(stream Request) returns(stream Response) {
58+
option (google.api.http) = {
59+
post: "/v2/{table_name=projects/*/instances/*/tables/*}:bidiStreaming"
60+
body: "*"
61+
};
62+
}
4963
}
5064

5165
service ServiceExplicitHeaders {
@@ -135,6 +149,30 @@ service ServiceExplicitHeaders {
135149
}
136150
};
137151
}
152+
153+
rpc ClientStreaming(stream Request) returns(Response) {
154+
option (google.api.http) = {
155+
post: "/v2/{table_name=projects/*/instances/*/tables/*}:clientStreaming"
156+
body: "*"
157+
};
158+
option (google.api.routing) = {
159+
routing_parameters {
160+
field: "table_name"
161+
}
162+
};
163+
}
164+
165+
rpc BidiStreaming(stream Request) returns(stream Response) {
166+
option (google.api.http) = {
167+
post: "/v2/{table_name=projects/*/instances/*/tables/*}:bidiStreaming"
168+
body: "*"
169+
};
170+
option (google.api.routing) = {
171+
routing_parameters {
172+
field: "table_name"
173+
}
174+
};
175+
}
138176
}
139177

140178
message Request {

‎packages/gapic-generator/tests/integration/goldens/showcase/google/showcase_v1beta1/services/messaging/async_client.py‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1600,13 +1600,6 @@ def request_generator():
16001600
# and friendly error handling.
16011601
rpc = self._client._transport._wrapped_methods[self._client._transport.send_blurbs]
16021602

1603-
# Certain fields should be provided within the metadata header;
1604-
# add these here.
1605-
metadata = tuple(metadata) + (
1606-
gapic_v1.routing_header.to_grpc_metadata((
1607-
)),
1608-
)
1609-
16101603
# Validate the universe domain.
16111604
self._client._validate_universe_domain()
16121605

‎packages/gapic-generator/tests/integration/goldens/showcase/google/showcase_v1beta1/services/messaging/client.py‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1868,13 +1868,6 @@ def request_generator():
18681868
# and friendly error handling.
18691869
rpc = self._transport._wrapped_methods[self._transport.send_blurbs]
18701870

1871-
# Certain fields should be provided within the metadata header;
1872-
# add these here.
1873-
metadata = tuple(metadata) + (
1874-
gapic_v1.routing_header.to_grpc_metadata((
1875-
)),
1876-
)
1877-
18781871
# Validate the universe domain.
18791872
self._validate_universe_domain()
18801873

‎packages/gapic-generator/tests/unit/generator/test_generator.py‎

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,19 @@
1313
# limitations under the License.
1414

1515
import json
16+
import os
1617
from textwrap import dedent
1718
from typing import Mapping
1819
from unittest import mock
1920

2021
import jinja2
2122
import pytest
2223

23-
from google.api import service_pb2
24+
from google.api import annotations_pb2
2425
from google.api import client_pb2
26+
from google.api import http_pb2
27+
from google.api import routing_pb2
28+
from google.api import service_pb2
2529
from google.protobuf import descriptor_pb2
2630
from google.protobuf.compiler.plugin_pb2 import CodeGeneratorResponse
2731

@@ -912,6 +916,112 @@ def test_generator_duplicate_samples(fs):
912916
generator.get_response(api_schema=api_schema, opts=Options.build(""))
913917

914918

919+
@pytest.mark.parametrize("use_ads_templates", (False, True))
920+
@pytest.mark.parametrize("explicit_routing", (False, True))
921+
@pytest.mark.parametrize("client_streaming", (False, True))
922+
@pytest.mark.parametrize("server_streaming", (False, True))
923+
def test_routing_headers_client_streaming(
924+
use_ads_templates: bool,
925+
explicit_routing: bool,
926+
client_streaming: bool,
927+
server_streaming: bool,
928+
):
929+
"""Verify routing metadata is omitted for client-streaming RPCs and included otherwise."""
930+
ads_templates_dir = os.path.abspath(
931+
os.path.join(
932+
os.path.dirname(__file__), "..", "..", "..", "gapic", "ads-templates"
933+
)
934+
)
935+
opts_str = (
936+
f"autogen-snippets=false,python-gapic-templates={ads_templates_dir},old-naming"
937+
if use_ads_templates
938+
else "autogen-snippets=false"
939+
)
940+
opts = Options.build(opts_str)
941+
generator_obj = generator.Generator(opts)
942+
943+
method_options = descriptor_pb2.MethodOptions()
944+
method_options.Extensions[annotations_pb2.http].CopyFrom(
945+
http_pb2.HttpRule(
946+
post="/v1/{table_name=projects/*/tables/*}:someMethod",
947+
body="*",
948+
)
949+
)
950+
if explicit_routing:
951+
method_options.Extensions[routing_pb2.routing].CopyFrom(
952+
routing_pb2.RoutingRule(
953+
routing_parameters=[
954+
routing_pb2.RoutingParameter(
955+
field="table_name",
956+
path_template="{table_name=projects/*/tables/*}",
957+
)
958+
]
959+
)
960+
)
961+
962+
file_pb = descriptor_pb2.FileDescriptorProto(
963+
name="some_service.proto",
964+
package="google.cloud.hatstand.v1",
965+
message_type=[
966+
descriptor_pb2.DescriptorProto(
967+
name="SomeRequest",
968+
field=[
969+
descriptor_pb2.FieldDescriptorProto(
970+
name="table_name",
971+
number=1,
972+
type=descriptor_pb2.FieldDescriptorProto.Type.Value(
973+
"TYPE_STRING"
974+
),
975+
)
976+
],
977+
),
978+
descriptor_pb2.DescriptorProto(name="SomeResponse"),
979+
],
980+
service=[
981+
descriptor_pb2.ServiceDescriptorProto(
982+
name="SomeService",
983+
method=[
984+
descriptor_pb2.MethodDescriptorProto(
985+
name="SomeMethod",
986+
input_type=".google.cloud.hatstand.v1.SomeRequest",
987+
output_type=".google.cloud.hatstand.v1.SomeResponse",
988+
client_streaming=client_streaming,
989+
server_streaming=server_streaming,
990+
options=method_options,
991+
)
992+
],
993+
)
994+
],
995+
)
996+
api_schema = api.API.build([file_pb], package="google.cloud.hatstand.v1", opts=opts)
997+
998+
cgr = generator_obj.get_response(api_schema=api_schema, opts=opts)
999+
files_by_name = {f.name: f.content for f in cgr.file}
1000+
1001+
client_files = [
1002+
content
1003+
for name, content in files_by_name.items()
1004+
if name.endswith("/services/some_service/client.py")
1005+
or name.endswith("/services/some_service/async_client.py")
1006+
]
1007+
assert len(client_files) == (1 if use_ads_templates else 2)
1008+
1009+
for content in client_files:
1010+
if client_streaming:
1011+
assert "gapic_v1.routing_header.to_grpc_metadata" not in content
1012+
assert "header_params" not in content
1013+
else:
1014+
assert "gapic_v1.routing_header.to_grpc_metadata" in content
1015+
if explicit_routing and not use_ads_templates:
1016+
assert "header_params: dict[str, str] = {}" in content
1017+
assert (
1018+
"gapic_v1.routing_header.to_grpc_metadata(header_params)" in content
1019+
)
1020+
else:
1021+
assert "header_params" not in content
1022+
assert '("table_name", request.table_name)' in content
1023+
1024+
9151025
def make_generator(opts_str: str = "") -> generator.Generator:
9161026
return generator.Generator(Options.build(opts_str))
9171027

0 commit comments

Comments
 (0)