Skip to content
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

Jump to bottom

fix: add field headers for other http verbs #443

Merged
merged 11 commits into from
Jun 9, 2020
Merged

fix: add field headers for other http verbs #443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5bb7d30
fix: add field headers for other http verbs
busunkim96 Jun 4, 2020
6c8aeeb
Merge branch 'master' into field-headers
busunkim96 Jun 4, 2020
797da8f
fix: fix custom path
busunkim96 Jun 4, 2020
78359ca
Merge branch 'field-headers' of github.com:googleapis/gapic-generator…
busunkim96 Jun 4, 2020
85ca6c9
fix: consolidate into one test
busunkim96 Jun 5, 2020
f569911
Merge branch 'master' into field-headers
busunkim96 Jun 5, 2020
224d81a
Merge branch 'master' into field-headers
busunkim96 Jun 9, 2020
38e8711
test: update generated unit tests
busunkim96 Jun 9, 2020
afd742a
test: update ads template
busunkim96 Jun 9, 2020
85f9699
fix: don't add field headers to streaming methods
busunkim96 Jun 9, 2020
8ed787d
test: exclude header check for client streaming method
busunkim96 Jun 9, 2020
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
2 changes: 2 additions & 0 deletions gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,9 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{%- for field_header in method.field_headers %}
{%- if not method.client_streaming %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this for a failing showcase test (see streaming method in proto and failing test).

Not sure if this is the right call though. What's the correct way to handle metadata/headers for streaming methods? @software-dov

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that headers for streaming methods don't make a lot of sense because they're not tightly bound to http semantics. If that's not correct, then I think we would need to ask someone who knows better.

('{{ field_header }}', request.{{ field_header }}),
{%- endif %}
{%- endfor %}
)),
)
Expand Down
35 changes: 21 additions & 14 deletions gapic/ads-templates/tests/unit/%name_%version/%sub/test_%service.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,25 +258,33 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'):
{% endfor %}
{% endif %}

{% if method.field_headers %}
{% if method.field_headers and not method.client_streaming %}
def test_{{ method.name|snake_case }}_field_headers():
client = {{ service.client_name }}(
credentials=credentials.AnonymousCredentials(),
)
)

# 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 }}(
{%- for field_header in method.field_headers %}
{{ field_header }}='{{ field_header }}/value',
{%- endfor %}
)
request = {{ method.input.ident }}()

{%- for field_header in method.field_headers %}
request.{{ field_header }} = '{{ field_header }}/value'
{%- endfor %}

# 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:
{% 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.name|snake_case }}(request)

# Establish that the underlying gRPC stub method was called.
Expand Down Expand Up @@ -471,7 +479,6 @@ def test_{{ method.name|snake_case }}_raw_page_lro():

{% endfor -%} {#- method in methods #}


def test_credentials_transport_error():
# It is an error to provide credentials and a transport instance.
transport = transports.{{ service.name }}GrpcTransport(
Expand Down Expand Up @@ -689,12 +696,12 @@ def test_{{ service.name|snake_case }}_grpc_lro_client():
{% for message in service.resource_messages -%}
{% with molluscs = cycler("squid", "clam", "whelk", "octopus", "oyster", "nudibranch", "cuttlefish", "mussel", "winkle", "nautilus", "scallop", "abalone") -%}
def test_{{ message.resource_type|snake_case }}_path():
{% for arg in message.resource_path_args -%}
{{ arg }} = "{{ molluscs.next() }}"
{% endfor %}
expected = "{{ message.resource_path }}".format({% for arg in message.resource_path_args %}{{ arg }}={{ arg }}, {% endfor %})
actual = {{ service.client_name }}.{{ message.resource_type|snake_case }}_path({{message.resource_path_args|join(", ") }})
assert expected == actual
{% for arg in message.resource_path_args -%}
{{ arg }} = "{{ molluscs.next() }}"
{% endfor %}
expected = "{{ message.resource_path }}".format({% for arg in message.resource_path_args %}{{ arg }}={{ arg }}, {% endfor %})
actual = {{ service.client_name }}.{{ message.resource_type|snake_case }}_path({{message.resource_path_args|join(", ") }})
assert expected == actual


def test_parse_{{ message.resource_type|snake_case }}_path():
Expand Down
16 changes: 13 additions & 3 deletions gapic/schema/wrappers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,9 +625,19 @@ def client_output(self):
def field_headers(self) -> Sequence[str]:
"""Return the field headers defined for this method."""
http = self.options.Extensions[annotations_pb2.http]
if http.get:
return tuple(re.findall(r'\{([a-z][\w\d_.]+)=', http.get))
return ()

pattern = re.compile(r'\{([a-z][\w\d_.]+)=')

potential_verbs = [
http.get,
http.put,
http.post,
http.delete,
http.patch,
http.custom.path,
]

return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ())

@utils.cached_property
def flattened_fields(self) -> Mapping[str, Field]:
Expand Down
2 changes: 2 additions & 0 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,9 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{%- for field_header in method.field_headers %}
{%- if not method.client_streaming %}
('{{ field_header }}', request.{{ field_header }}),
{%- endif %}
{%- endfor %}
)),
)
Expand Down
22 changes: 15 additions & 7 deletions gapic/templates/tests/unit/%name_%version/%sub/test_%service.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,25 +258,33 @@ def test_{{ method.name|snake_case }}(transport: str = 'grpc'):
{% endfor %}
{% endif %}

{% if method.field_headers %}
{% if method.field_headers and not method.client_streaming %}
def test_{{ method.name|snake_case }}_field_headers():
client = {{ service.client_name }}(
credentials=credentials.AnonymousCredentials(),
)
)

# 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 }}(
{%- for field_header in method.field_headers %}
{{ field_header }}='{{ field_header }}/value',
{%- endfor %}
)
request = {{ method.input.ident }}()

{%- for field_header in method.field_headers %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaking this slightly for field headers like "/v1beta1/{endpoint.name=projects/*/locations/*/namespaces/*/services/*/endpoints/*}"

https://github.com/googleapis/googleapis/blob/52701da10fec2a5f9796e8d12518c0fe574488fe/google/cloud/servicedirectory/v1beta1/registration_service.proto#L171-L177

request.{{ field_header }} = '{{ field_header }}/value'
{%- endfor %}

# 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:
{% if method.void -%}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-pasted from the unit test below this.

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.name|snake_case }}(request)

# Establish that the underlying gRPC stub method was called.
Expand Down
15 changes: 12 additions & 3 deletions tests/unit/schema/wrappers/test_method.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,18 @@ def test_method_field_headers_none():


def test_method_field_headers_present():
http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics')
method = make_method('DoSomething', http_rule=http_rule)
assert method.field_headers == ('parent',)
verbs = [
'get',
'put',
'post',
'delete',
'patch',
]

for v in verbs:
rule = http_pb2.HttpRule(**{v: '/v1/{parent=projects/*}/topics'})
method = make_method('DoSomething', http_rule=rule)
assert method.field_headers == ('parent',)


def test_method_idempotent_yes():
Expand Down