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 12 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(yonmg): add rest transport service once I know what this is #}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use your github id for TODOs (here and in the other places)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

# 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(yonmg): 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: 0 additions & 1 deletion gapic/cli/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ def generate(

# Pull apart arguments in the request.
opts = Options.build(req.parameter)

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I like having the blank line here for readability.

# Determine the appropriate package.
# This generator uses a slightly different mechanism for determining
# which files to generate; it tracks at package level rather than file
Expand Down
8 changes: 8 additions & 0 deletions gapic/generator/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ def _render_template(
if (
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put parentheses around this chunk, just to make it clear that it's a cohesive unit?

):
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
27 changes: 27 additions & 0 deletions gapic/schema/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,7 @@ def _client_output(self, enable_asyncio: bool):
# Return the usual output.
return self.output

# TODO(yonmg): remove or rewrite. don't think it performs as intended
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Suggest adding just a tiny bit more detail. It often happens that the person working on the TODO is not the person who wrote it, so adding some context is a helpful practice.

@property
def field_headers(self) -> Sequence[str]:
"""Return the field headers defined for this method."""
Expand All @@ -737,6 +738,27 @@ 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."""
yon-mg marked this conversation as resolved.
Show resolved Hide resolved
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] = {
'method': http_method[0].name,
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, let's s/'method'/'verb'/, since "method" is overloaded (service method, object method, HTTP method)

Copy link
Contributor

Choose a reason for hiding this comment

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

If this be madness, there be method to it! 💀

'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:
http_opt = http[1]
answer[http_opt[0].name] = http_opt[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity I would suggest

body_spec = http[1]
answer[body_spec[0].name] = body_spec[1]


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

# TODO(yonmg): 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 +808,7 @@ def grpc_stub_type(self) -> str:
server='stream' if self.server_streaming else 'unary',
)

# TODO(yonmg): figure out why idempotent is reliant on http annotation
Copy link
Contributor

Choose a reason for hiding this comment

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

http GET requests are automatically considered idempotent because they do not change data on the server. Post requests are usually non-idempotent, because it is usually unsafe retry write operation if it failed, because it may lead for example into writing the same information twice on the server.

Copy link
Contributor Author

@yon-mg yon-mg Nov 12, 2020

Choose a reason for hiding this comment

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

I get that GET is idempotent but couldn't there be methods without an http annotation that are also idempotent? are grpc only methods inherently not so?

@utils.cached_property
def idempotent(self) -> bool:
"""Return True if we know this method is idempotent, False otherwise.
Expand Down Expand Up @@ -980,6 +1003,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(yonmg): 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
from .transports.grpc import {{ service.grpc_transport_name }}
from .transports.grpc_asyncio import {{ service.grpc_asyncio_transport_name }}
{%- if 'grpc' in opts.transport %}
from .transports.grpc import {{ service.name }}GrpcTransport
yon-mg marked this conversation as resolved.
Show resolved Hide resolved
from .transports.grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport
{%- 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]]
_transport_registry['grpc'] = {{ service.grpc_transport_name }}
_transport_registry['grpc_asyncio'] = {{ service.grpc_asyncio_transport_name }}
{%- 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 %}

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