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.

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 rest transport generation for clients with optional flag #688

Merged
merged 16 commits into from
Nov 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ _lazy_type_to_package_map = {
'{{ enum.name }}': '{{ enum.ident.package|join('.') }}.types.{{enum.ident.module }}',
{%- endfor %}

{# TODO(yon-mg): add rest transport service once I know what this is #}
# Client classes and transports
{%- for service in api.services.values() %}
'{{ service.client_name }}': '{{ service.meta.address.package|join('.') }}.services.{{ service.meta.address.module }}',
Expand Down
1 change: 1 addition & 0 deletions gapic/ads-templates/%namespace/%name/__init__.py.j2
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ _lazy_type_to_package_map = {
'{{ enum.name }}': '{{ enum.ident.package|join('.') }}.types.{{enum.ident.module }}',
{%- endfor %}

{# TODO(yon-mg): add rest transport service once I know what this is #}
# Client classes and transports
{%- for service in api.services.values() %}
'{{ service.client_name }}': '{{ service.meta.address.package|join('.') }}.services.{{ service.meta.address.module }}',
Expand Down
12 changes: 10 additions & 2 deletions gapic/generator/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,11 @@ def _render_template(
if "%service" in template_name:
for service in api_schema.services.values():
if (
skip_subpackages
and service.meta.address.subpackage != api_schema.subpackage_view
(skip_subpackages
and service.meta.address.subpackage != api_schema.subpackage_view)
or
('transport' in template_name
and not self._is_desired_transport(template_name, opts))
):
continue

Expand All @@ -293,6 +296,11 @@ def _render_template(
template_name, api_schema=api_schema, opts=opts))
return answer

def _is_desired_transport(self, template_name: str, opts: Options) -> bool:
"""Returns true if template name contains a desired transport"""
desired_transports = ['__init__', 'base'] + opts.transport
return any(transport in template_name for transport in desired_transports)

def _get_file(
self,
template_name: str,
Expand Down
36 changes: 36 additions & 0 deletions gapic/schema/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,8 @@ def _client_output(self, enable_asyncio: bool):
# Return the usual output.
return self.output

# TODO(yon-mg): remove or rewrite: don't think it performs as intended
# e.g. doesn't work with basic case of gRPC transcoding
@property
def field_headers(self) -> Sequence[str]:
"""Return the field headers defined for this method."""
Expand All @@ -737,6 +739,35 @@ def field_headers(self) -> Sequence[str]:

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

@property
def http_opt(self) -> Optional[Dict[str, str]]:
"""Return the http option for this method.

e.g. {'verb': 'post'
'url': '/some/path'
'body': '*'}

"""
http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]]
http = self.options.Extensions[annotations_pb2.http].ListFields()
Copy link
Contributor

Choose a reason for hiding this comment

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

I do think a type declaration or a comment saying this is a List[Tuple[FieldDescriptor, str]] would be helpful.


if len(http) < 1:
return None

http_method = http[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our offline discussion, I suggest s/http_method/primary_binding/ for clarity.

answer: Dict[str, str] = {
'verb': http_method[0].name,
'url': http_method[1],
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having "template" in the name: maybe 'path_template' would be more descriptive and accurate.

}
if len(http) > 1:
body_spec = http[1]
answer[body_spec[0].name] = body_spec[1]

# TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings'
# TODO(yon-mg): enums for http verbs?
return answer

# TODO(yon-mg): refactor as there may be more than one method signature
@utils.cached_property
def flattened_fields(self) -> Mapping[str, Field]:
"""Return the signature defined for this method."""
Expand Down Expand Up @@ -786,6 +817,7 @@ def grpc_stub_type(self) -> str:
server='stream' if self.server_streaming else 'unary',
)

# TODO(yon-mg): figure out why idempotent is reliant on http annotation
@utils.cached_property
def idempotent(self) -> bool:
"""Return True if we know this method is idempotent, False otherwise.
Expand Down Expand Up @@ -980,6 +1012,10 @@ def grpc_transport_name(self):
def grpc_asyncio_transport_name(self):
return self.name + "GrpcAsyncIOTransport"

@property
def rest_transport_name(self):
return self.name + "RestTransport"

@property
def has_lro(self) -> bool:
"""Return whether the service has a long-running method."""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ from .transports.grpc_asyncio import {{ service.grpc_asyncio_transport_name }}
from .client import {{ service.client_name }}


{# TODO(yon-mg): handle rest transport async client interaction #}
class {{ service.async_client_name }}:
"""{{ service.meta.doc|rst(width=72, indent=4) }}"""

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,13 @@ from google.iam.v1 import policy_pb2 as policy # type: ignore
{% endif %}
{% endfilter %}
from .transports.base import {{ service.name }}Transport, DEFAULT_CLIENT_INFO
{%- if 'grpc' in opts.transport %}
from .transports.grpc import {{ service.grpc_transport_name }}
from .transports.grpc_asyncio import {{ service.grpc_asyncio_transport_name }}
{%- endif %}
{%- if 'rest' in opts.transport %}
from .transports.rest import {{ service.name }}RestTransport
{%- endif %}


class {{ service.client_name }}Meta(type):
Expand All @@ -42,8 +47,13 @@ class {{ service.client_name }}Meta(type):
objects.
"""
_transport_registry = OrderedDict() # type: Dict[str, Type[{{ service.name }}Transport]]
{%- if 'grpc' in opts.transport %}
_transport_registry['grpc'] = {{ service.grpc_transport_name }}
_transport_registry['grpc_asyncio'] = {{ service.grpc_asyncio_transport_name }}
{%- endif %}
{%- if 'rest' in opts.transport %}
_transport_registry['rest'] = {{ service.name }}RestTransport
{%- endif %}

def get_transport_class(cls,
label: str = None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,34 @@ from collections import OrderedDict
from typing import Dict, Type

from .base import {{ service.name }}Transport
{%- if 'grpc' in opts.transport %}
from .grpc import {{ service.name }}GrpcTransport
from .grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport
{%- endif %}
{%- if 'rest' in opts.transport %}
from .rest import {{ service.name }}RestTransport
{%- endif %}



# Compile a registry of transports.
_transport_registry = OrderedDict() # type: Dict[str, Type[{{ service.name }}Transport]]
{%- if 'grpc' in opts.transport %}
_transport_registry['grpc'] = {{ service.name }}GrpcTransport
_transport_registry['grpc_asyncio'] = {{ service.name }}GrpcAsyncIOTransport

{%- endif %}
{%- if 'rest' in opts.transport %}
_transport_registry['rest'] = {{ service.name }}RestTransport
{%- endif %}

__all__ = (
'{{ service.name }}Transport',
{%- if 'grpc' in opts.transport %}
'{{ service.name }}GrpcTransport',
'{{ service.name }}GrpcAsyncIOTransport',
{%- endif %}
{%- if 'rest' in opts.transport %}
'{{ service.name }}RestTransport',
{%- endif %}
)
{% endblock %}
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
)

self._stubs = {} # type: Dict[str, Callable]
{%- if service.has_lro %}
self._operations_client = None
{%- endif %}

# Run the base constructor.
super().__init__(
Expand All @@ -172,7 +175,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
**kwargs) -> grpc.Channel:
"""Create and return a gRPC channel object.
Args:
address (Optionsl[str]): The host for the channel to use.
software-dov marked this conversation as resolved.
Show resolved Hide resolved
address (Optional[str]): The host for the channel to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed (looks unrelated to your changes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor

Choose a reason for hiding this comment

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

I approve of fixing typos on the fly!

credentials (Optional[~.Credentials]): The
authorization credentials to attach to requests. These
credentials identify this application to the service. If
Expand Down Expand Up @@ -220,13 +223,13 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
client.
"""
# Sanity check: Only create a new client if we do not already have one.
if 'operations_client' not in self.__dict__:
self.__dict__['operations_client'] = operations_v1.OperationsClient(
if self._operations_client is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

won't if not self._operation_client work? I thought it was more idiomatic for python.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little bit complicated. Some values can be falsy even if they are non-null, e.g. empty lists and strings.
In this particular case, it's slightly more idiomatic to do if self._operations_client, but I personally am fine with explicit checks in cases like this.

self._operations_client = operations_v1.OperationsClient(
self.grpc_channel
)

# Return the client from cache.
return self.__dict__['operations_client']
return self._operations_client
{%- endif %}
{%- for method in service.methods.values() %}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport):
)

self._stubs = {}
{%- if service.has_lro %}
self._operations_client = None
{%- endif %}

@property
def grpc_channel(self) -> aio.Channel:
Expand All @@ -225,13 +228,13 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport):
client.
"""
# Sanity check: Only create a new client if we do not already have one.
if 'operations_client' not in self.__dict__:
self.__dict__['operations_client'] = operations_v1.OperationsAsyncClient(
if self._operations_client is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you changing this code (looks unrelated to http transport stuff)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#688 (comment)
I decided to update the other templates as well for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for consistency

self._operations_client = operations_v1.OperationsAsyncClient(
self.grpc_channel
)

# Return the client from cache.
return self.__dict__['operations_client']
return self._operations_client
{%- endif %}
{%- for method in service.methods.values() %}

Expand Down
Loading