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

feat: add grpc transcoding, clean up unimplemented rest method/test generation #776

Closed
wants to merge 12 commits into from
Closed

feat: add grpc transcoding, clean up unimplemented rest method/test generation #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1141010
fix: update paging implementation to handle unconventional pagination
yon-mg Jan 25, 2021
058b55e
Merge remote-tracking branch 'upstream/master'
yon-mg Jan 25, 2021
d226f36
fix: typing errors, mypy cli update
yon-mg Jan 25, 2021
01adad1
fix: mypy cli flag
yon-mg Jan 25, 2021
0f3e63d
fix: delete __init__.py, remove -p mypy flag
yon-mg Jan 25, 2021
48253d3
fix: clearing up statements, tests, minor bug in filter usage
yon-mg Jan 27, 2021
86e2c43
fix: wrong genereated type hints
yon-mg Jan 29, 2021
959e1a0
Merge branch 'master' into master
yon-mg Feb 3, 2021
5f1ba34
feat: add grpc_transcoding, clean up rest generation unimplemented me…
yon-mg Feb 13, 2021
82c91b8
Merge branch 'master' of github.com:yon-mg/gapic-generator-python
yon-mg Feb 13, 2021
31a442b
Merge remote-tracking branch 'upstream/master'
yon-mg Feb 13, 2021
73b6044
Merge branch 'master' into master
software-dov Feb 17, 2021
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: 1 addition & 1 deletion gapic/generator/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def __init__(self, opts: Options) -> None:
self._env.filters["wrap"] = utils.wrap
self._env.filters["coerce_response_name"] = coerce_response_name

# Add tests to determine type of expressions stored in strings
# Add tests to determine FieldDescriptorProto type
self._env.tests["str_field_pb"] = utils.is_str_field_pb
self._env.tests["msg_field_pb"] = utils.is_msg_field_pb

Expand Down
36 changes: 36 additions & 0 deletions gapic/schema/wrappers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import collections
import dataclasses
import re
import copy
from itertools import chain
from typing import (cast, Dict, FrozenSet, Iterable, List, Mapping,
ClassVar, Optional, Sequence, Set, Tuple, Union)
Expand Down Expand Up @@ -741,6 +742,41 @@ def field_headers(self) -> Sequence[str]:

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

@property
def http_options(self) -> List[Dict[str, str]]:
"""Return a list of the http options for this method.

e.g. [{'method': 'post'
'uri': '/some/path'
'body': '*'},]

"""
http = self.options.Extensions[annotations_pb2.http]
http_options = copy.deepcopy(http.additional_bindings)
http_options.append(http)
answers : List[Dict[str, str]] = []

for http_rule in http_options:
try:
method, uri = next((method, uri) for method,uri in [
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting nit: for method, uri

('get',http_rule.get),
('put',http_rule.put),
('post',http_rule.post),
('delete',http_rule.delete),
('patch',http_rule.patch),
('custom.path',http_rule.custom.path),
] if uri
)
except StopIteration:
continue
Comment on lines +761 to +771
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better not to catch a StopIteration and instead use explicit control flow.
This is a fairly common pattern when using next:

thing = next((generator_expression_or_iterable), None) # Or (None, None) if detupling
if not thing:
    continue # Or do some other action

answer : Dict[str, str] = {}
answer['method'] = method
answer['uri'] = uri
if http_rule.body:
answer['body'] = http_rule.body
answers.append(answer)
return answers

@property
def http_opt(self) -> Optional[Dict[str, str]]:
"""Return the http option for this method.
Expand Down
122 changes: 71 additions & 51 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ import warnings
from typing import Callable, Dict, Optional, Sequence, Tuple

{% if service.has_lro %}
from google.api_core import operations_v1
from google.api_core import operations_v1 # type: ignore
{%- endif %}
from google.api_core import gapic_v1 # type: ignore
from google import auth # type: ignore
from google.auth import credentials # type: ignore
from google.auth.transport.grpc import SslCredentials # type: ignore
from google.api_core import gapic_v1, path_template # type: ignore
from google import auth # type: ignore
from google.auth import credentials # type: ignore
from google.auth.transport.grpc import SslCredentials # type: ignore

import grpc # type: ignore
import json

from google.auth.transport.requests import AuthorizedSession
from google.auth.transport.requests import AuthorizedSession # type: ignore

{# TODO(yon-mg): re-add python_import/ python_modules from removed diff/current grpc template code #}
{% filter sort_lines -%}
Expand Down Expand Up @@ -121,9 +122,9 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
return self._operations_client
{%- endif %}
{%- for method in service.methods.values() %}
{%- if method.http_opt %}
{%- if method.http_options and not method.lro and not (method.server_streaming or method.client_streaming) %}

def {{ method.name|snake_case }}(self,
def _{{ method.name|snake_case }}(self,
request: {{ method.input.ident }}, *,
metadata: Sequence[Tuple[str, str]] = (),
) -> {{ method.output.ident }}:
Expand All @@ -146,57 +147,47 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
{%- endif %}
"""

{# TODO(yon-mg): refactor when implementing grpc transcoding
- parse request pb & assign body, path params
- shove leftovers into query params
- make sure dotted nested fields preserved
- format url and send the request
#}
{%- if 'body' in method.http_opt %}
http_options = [
{%- for rule in method.http_options %}{
{%- for field, argument in rule.items() | sort %}
'{{ field }}':'{{ argument }}',
{%- endfor %}
},
{%- endfor %}]

request_kwargs = {
field.name:value
for field, value
in {{ method.input.ident }}.pb(request).ListFields()
}
transcoded_request = path_template.transcode(http_options, **request_kwargs)
{%- if 'body' in method.http_options[0] %}

# Jsonify the request body
{%- if method.http_opt['body'] != '*' %}
body = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json(
request.{{ method.http_opt['body'] }},
body = {% if method.http_options[0]['body'] == '*' -%}
{{ method.input.ident }}.to_json(
{{ method.input.ident }}(transcoded_request['body']),
{%- else -%}
{{ method.input.fields[method.http_opt['body']].type.ident }}.to_json(
{{ method.input.fields[method.http_opt['body']].type.ident }}(transcoded_request['body']),
{%- endif %}
including_default_value_fields=False,
use_integers_for_enums=False
)
{%- else %}
body = {{ method.input.ident }}.to_json(
request,
use_integers_for_enums=False
)
{%- endif %}
{%- endif %}

{# TODO(yon-mg): Write helper method for handling grpc transcoding url #}
# TODO(yon-mg): need to handle grpc transcoding and parse url correctly
# current impl assumes basic case of grpc transcoding
url = 'https://{host}{{ method.http_opt['url'] }}'.format(
host=self._host,
{%- for field in method.path_params %}
{{ field }}=request.{{ method.input.get_field(field).name }},
{%- endfor %}
)

{# TODO(yon-mg): move all query param logic out of wrappers into here to handle
nested fields correctly (can't just use set of top level fields
#}
# TODO(yon-mg): handle nested fields corerctly rather than using only top level fields
# not required for GCE
query_params = {
{%- for field in method.query_params | sort%}
'{{ field|camel_case }}': request.{{ field }},
{%- endfor %}
}
# TODO(yon-mg): further discussion needed whether 'python truthiness' is appropriate here
# discards default values
# TODO(yon-mg): add test for proper url encoded strings
query_params = ['{k}={v}'.format(k=k, v=v) for k, v in query_params.items() if v]
url += '?{}'.format('&'.join(query_params)).replace(' ', '+')
# Jsonify the query params
query_params = json.loads({{ method.input.ident }}.to_json(
{{ method.input.ident }}(transcoded_request['query_params']),
including_default_value_fields=False,
use_integers_for_enums=False
))
Comment on lines +179 to +184
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, this is turning the transcoded request back into a Request message object, turning it into a JSON string, and then turning it back into a python dict. Is that correct? Can we use to_dict instead of the indirection?
If the above is correct, can you add some comments describing the dataflow? If it's not correct, then could you describe what the dataflow is?

Copy link
Contributor

Choose a reason for hiding this comment

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

for the record: this is actually not redundant, provided we want to rely on proto+ to do the proper case conversion. to_dict doesn't do the conversion from snake_case to camelCase, only to_json does that. But the result of to_json is a string that can't be used as-is to construct the query string.


# Send the request
{% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}(
url
response = self._session.request(
transcoded_request['method'],
self._host.join(('https://', transcoded_request['uri'])),
params=query_params
{%- if 'body' in method.http_opt %},
data=body,
{%- endif %}
Expand All @@ -208,9 +199,38 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):

# Return the response
return {{ method.output.ident }}.from_json(response.content)
{%- else %}

# Returh the response
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Return the response

return {{ method.output.ident }}()
{%- endif %}
{%- else %}

def _{{ method.name|snake_case }}(self,
request: {{ method.input.ident }}, *,
metadata: Sequence[Tuple[str, str]] = (),
) -> {{ method.output.ident }}:
r"""Placeholder: Unable to implement over REST
"""
{%- if not method.http_options %}
raise RuntimeError('Cannot define a method without a valid `google.api.http` annotation.')
{%- elif method.lro %}
raise NotImplementedError('LRO over REST is not yet defined for python client.')
{%- elif method.server_streaming or method.client_streaming %}
raise NotImplementedError('Streaming over REST is not yet defined for python client')
{%- else %}
raise NotImplementedError()
{%- endif %}
{%- endif %}
{%- endfor %}
{%- for method in service.methods.values() %}

@property
def {{ method.name|snake_case }}(self) -> Callable[
[{{ method.input.ident }}],
{{ method.output.ident }}]:
return self._{{ method.name|snake_case }}
Comment on lines +228 to +232
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra indirection? Is it removable?

{%- endfor %}


__all__ = (
Expand Down
Loading