From c1a6f891db8ce80a64d293ae8d8bed836b63a3c7 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 2 Nov 2020 17:49:42 +0000 Subject: [PATCH 01/24] feat: add rest transport generation for clients --- .../%namespace/%name/%version/__init__.py.j2 | 1 + .../%namespace/%name/__init__.py.j2 | 1 + gapic/schema/wrappers.py | 4 + .../%sub/services/%service/client.py.j2 | 2 + .../%service/transports/__init__.py.j2 | 3 + .../services/%service/transports/grpc.py.j2 | 2 +- .../services/%service/transports/rest.py.j2 | 200 ++++++++++++++++++ 7 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 diff --git a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 index 749a408c42..ac517161fd 100644 --- a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 @@ -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 }}', diff --git a/gapic/ads-templates/%namespace/%name/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/__init__.py.j2 index 749a408c42..ac517161fd 100644 --- a/gapic/ads-templates/%namespace/%name/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/__init__.py.j2 @@ -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 }}', diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index b5ae6fd0e9..7cebbe30e2 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -980,6 +980,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.""" diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index c3093aa1cf..60ea884727 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -32,6 +32,7 @@ from google.iam.v1 import policy_pb2 as policy # type: ignore 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 }} +from .transports.rest import {{ service.rest_transport_name }} class {{ service.client_name }}Meta(type): @@ -44,6 +45,7 @@ class {{ service.client_name }}Meta(type): _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 }} + _transport_registry['rest'] = {{ service.name }}RestTransport def get_transport_class(cls, label: str = None, diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 index fa97f46164..8ff320d88a 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 @@ -7,17 +7,20 @@ from typing import Dict, Type from .base import {{ service.name }}Transport from .grpc import {{ service.name }}GrpcTransport from .grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport +from .rest import {{ service.name }}RestTransport # Compile a registry of transports. _transport_registry = OrderedDict() # type: Dict[str, Type[{{ service.name }}Transport]] _transport_registry['grpc'] = {{ service.name }}GrpcTransport _transport_registry['grpc_asyncio'] = {{ service.name }}GrpcAsyncIOTransport +_transport_registry['rest'] = {{ service.name }}RestTransport __all__ = ( '{{ service.name }}Transport', '{{ service.name }}GrpcTransport', '{{ service.name }}GrpcAsyncIOTransport', + '{{ service.name }}RestTransport', ) {% endblock %} diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 index e2c68c483d..8be8de3b67 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 @@ -172,7 +172,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. + address (Optional[str]): The host for the channel to use. credentials (Optional[~.Credentials]): The authorization credentials to attach to requests. These credentials identify this application to the service. If diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 new file mode 100644 index 0000000000..dbd1055335 --- /dev/null +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -0,0 +1,200 @@ +{% extends '_base.py.j2' %} + +{% block content %} +import warnings +from typing import Callable, Dict, Optional, Sequence, Tuple + +{% if service.has_lro %} +from google.api_core import operations_v1 +{%- 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 + +import grpc # type: ignore + +from google.auth.transport.requests import AuthorizedSession + +{# TODO: re-add python_import/ python_modules from removed diff/current grpc template code #} +{% filter sort_lines -%} +{% for method in service.methods.values() -%} +{{ method.input.ident.python_import }} +{{ method.output.ident.python_import }} +{% endfor -%} +{% if opts.add_iam_methods %} +from google.iam.v1 import iam_policy_pb2 as iam_policy # type: ignore +from google.iam.v1 import policy_pb2 as policy # type: ignore +{% endif %} +{% endfilter %} + +from .base import {{ service.name }}Transport, DEFAULT_CLIENT_INFO + + +class {{ service.name }}RestTransport({{ service.name }}Transport): + """REST backend transport for {{ service.name }}. + + {{ service.meta.doc|rst(width=72, indent=4) }} + + This class defines the same methods as the primary client, so the + primary client can load the underlying transport implementation + and call it. + + It sends JSON representations of protocol buffers over HTTP/1.1 + """ + def __init__(self, *, + host: str = 'compute.googleapis.com', + credentials: credentials.Credentials = None, + credentials_file: str = None, + scopes: Sequence[str] = None, + ssl_channel_credentials: grpc.ChannelCredentials = None, + quota_project_id: Optional[str] = None, + client_info: gapic_v1.client_info.ClientInfo = DEFAULT_CLIENT_INFO, + ) -> None: + """Instantiate the transport. + + Args: + host ({% if service.host %}Optional[str]{% else %}str{% endif %}): + {{- ' ' }}The hostname to connect to. + credentials (Optional[google.auth.credentials.Credentials]): The + authorization credentials to attach to requests. These + credentials identify the application to the service; if none + are specified, the client will attempt to ascertain the + credentials from the environment. + + credentials_file (Optional[str]): A file with credentials that can + be loaded with :func:`google.auth.load_credentials_from_file`. + This argument is ignored if ``channel`` is provided. + scopes (Optional(Sequence[str])): A list of scopes. This argument is + ignored if ``channel`` is provided. + ssl_channel_credentials (grpc.ChannelCredentials): SSL credentials + for grpc channel. It is ignored if ``channel`` is provided. + quota_project_id (Optional[str]): An optional project to use for billing + and quota. + client_info (google.api_core.gapic_v1.client_info.ClientInfo): + The client info used to send a user-agent string along with + API requests. If ``None``, then default info will be used. + Generally, you only need to set this if you're developing + your own client library. + """ + super().__init__(host=host, credentials=credentials) + self._session = AuthorizedSession(self._credentials) + {%- if service.has_lro %} + + @property + def operations_client(self) -> operations_v1.OperationsClient: + """Create the client designed to process long-running operations. + + This property caches on the instance; repeated calls return the same + client. + """ + # Sanity check: Only create a new client if we do not already have one. + if 'operations_client' not in self.__dict__: + from google.api_core import grpc_helpers + self.__dict__['operations_client'] = operations_v1.OperationsClient( + grpc_helpers.create_channel( + self._host, + credentials=self._credentials, + scopes=self.AUTH_SCOPES, + ) + ) + + # Return the client from cache. + return self.__dict__['operations_client'] + {%- endif %} + + + {%- for method in service.methods.values() %} + {# TODO(yonmg): consider using enums #} + {%- set ns = namespace(http=None) %} + {%- for option in method.options.ListFields() %} + {%- if option[0].name == 'http' %} + {%- set ns.http = option[1] %} + {%- endif %} + {%- endfor %} + + {%- if ns.http %} + + + def {{ method.name|snake_case }}(self, + request: {{ method.input.ident }}, *, + metadata: Sequence[Tuple[str, str]] = (), + ) -> {{ method.output.ident }}: + r"""Call the {{- ' ' -}} + {{ (method.name|snake_case).replace('_',' ')|wrap( + width=70, offset=45, indent=8) }} + {{- ' ' -}} method over HTTP. + + Args: + request (~.{{ method.input.ident }}): + The request object. + {{ method.input.meta.doc|rst(width=72, indent=16) }} + metadata (Sequence[Tuple[str, str]]): Strings which should be + sent along with the request as metadata. + {%- if not method.void %} + + Returns: + ~.{{ method.output.ident }}: + {{ method.output.meta.doc|rst(width=72, indent=16) }} + {%- endif %} + """ + + # Check which http method to use + {%- if ns.http.get != '' %} + {%- set ns.httpMethod = {'method':'get','url':ns.http.get} %} + {%- elif ns.http.put != '' %} + {%- set ns.httpMethod = {'method':'put','url':ns.http.put} %} + {%- elif ns.http.post != '' %} + {%- set ns.httpMethod = {'method':'post','url':ns.http.post} %} + {%- elif ns.http.delete != '' %} + {%- set ns.httpMethod = {'method':'delete','url':ns.http.delete} %} + {%- elif ns.http.patch != '' %} + {%- set ns.httpMethod = {'method':'patch','url':ns.http.patch} %} + {%- elif ns.http.custom != '' %} + {%- set ns.httpMethod = {'method':'custom','url':ns.http.custom} %} + {%- else %} + {%- set ns.httpMethod = None %} + {%- endif %} + + {%- if ns.http.body != '' %} + # Jsonify the input + data = {{ method.output.ident }}.to_json( + {%- if ns.http.body == '*' %} + request + {%- else %} + request.body + {%- endif %} + ) + {%- endif %} + + {# TODO(yonmg): Write helper method for handling grpc transcoding url #} + # TODO(yonmg): need to handle grpc transcoding and parse url correctly + # current impl assumes simpler version of grpc transcoding + # Send the request + url = 'https://{host}{{ ns.httpMethod['url'] }}'.format( + host=self._host, + {%- for field in method.input.fields.keys() %} + {{ field }}=request.{{ field }}, + {%- endfor %} + ) + {% if not method.void %}response = {% endif %}self._session.{{ ns.httpMethod['method'] }}( + url, + {%- if ns.http.body != '' %} + json=data, + {%- endif %} + ) + {%- if not method.void %} + {%- endif %} + + # Return the response + return {{ method.output.ident }}.from_json(response.content) + {%- else %} + # No http annotation found for method {{ method.name|snake_case }}() + {%- endif %} + {%- endfor %} + + +__all__ = ( + '{{ service.name }}RestTransport', +) +{% endblock %} \ No newline at end of file From bd7c32dfd665ce2332898481b89f9c1c0f71d64d Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 2 Nov 2020 17:49:42 +0000 Subject: [PATCH 02/24] feat: add rest transport generation for clients --- .../%namespace/%name/%version/__init__.py.j2 | 1 + .../%namespace/%name/__init__.py.j2 | 1 + gapic/schema/wrappers.py | 4 + .../%sub/services/%service/client.py.j2 | 2 + .../%service/transports/__init__.py.j2 | 3 + .../services/%service/transports/grpc.py.j2 | 2 +- .../services/%service/transports/rest.py.j2 | 200 ++++++++++++++++++ tests/unit/schema/wrappers/test_service.py | 1 + 8 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 diff --git a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 index 749a408c42..ac517161fd 100644 --- a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 @@ -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 }}', diff --git a/gapic/ads-templates/%namespace/%name/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/__init__.py.j2 index 749a408c42..ac517161fd 100644 --- a/gapic/ads-templates/%namespace/%name/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/__init__.py.j2 @@ -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 }}', diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index b5ae6fd0e9..7cebbe30e2 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -980,6 +980,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.""" diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index c3093aa1cf..60ea884727 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -32,6 +32,7 @@ from google.iam.v1 import policy_pb2 as policy # type: ignore 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 }} +from .transports.rest import {{ service.rest_transport_name }} class {{ service.client_name }}Meta(type): @@ -44,6 +45,7 @@ class {{ service.client_name }}Meta(type): _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 }} + _transport_registry['rest'] = {{ service.name }}RestTransport def get_transport_class(cls, label: str = None, diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 index fa97f46164..8ff320d88a 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 @@ -7,17 +7,20 @@ from typing import Dict, Type from .base import {{ service.name }}Transport from .grpc import {{ service.name }}GrpcTransport from .grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport +from .rest import {{ service.name }}RestTransport # Compile a registry of transports. _transport_registry = OrderedDict() # type: Dict[str, Type[{{ service.name }}Transport]] _transport_registry['grpc'] = {{ service.name }}GrpcTransport _transport_registry['grpc_asyncio'] = {{ service.name }}GrpcAsyncIOTransport +_transport_registry['rest'] = {{ service.name }}RestTransport __all__ = ( '{{ service.name }}Transport', '{{ service.name }}GrpcTransport', '{{ service.name }}GrpcAsyncIOTransport', + '{{ service.name }}RestTransport', ) {% endblock %} diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 index e2c68c483d..8be8de3b67 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 @@ -172,7 +172,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. + address (Optional[str]): The host for the channel to use. credentials (Optional[~.Credentials]): The authorization credentials to attach to requests. These credentials identify this application to the service. If diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 new file mode 100644 index 0000000000..dbd1055335 --- /dev/null +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -0,0 +1,200 @@ +{% extends '_base.py.j2' %} + +{% block content %} +import warnings +from typing import Callable, Dict, Optional, Sequence, Tuple + +{% if service.has_lro %} +from google.api_core import operations_v1 +{%- 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 + +import grpc # type: ignore + +from google.auth.transport.requests import AuthorizedSession + +{# TODO: re-add python_import/ python_modules from removed diff/current grpc template code #} +{% filter sort_lines -%} +{% for method in service.methods.values() -%} +{{ method.input.ident.python_import }} +{{ method.output.ident.python_import }} +{% endfor -%} +{% if opts.add_iam_methods %} +from google.iam.v1 import iam_policy_pb2 as iam_policy # type: ignore +from google.iam.v1 import policy_pb2 as policy # type: ignore +{% endif %} +{% endfilter %} + +from .base import {{ service.name }}Transport, DEFAULT_CLIENT_INFO + + +class {{ service.name }}RestTransport({{ service.name }}Transport): + """REST backend transport for {{ service.name }}. + + {{ service.meta.doc|rst(width=72, indent=4) }} + + This class defines the same methods as the primary client, so the + primary client can load the underlying transport implementation + and call it. + + It sends JSON representations of protocol buffers over HTTP/1.1 + """ + def __init__(self, *, + host: str = 'compute.googleapis.com', + credentials: credentials.Credentials = None, + credentials_file: str = None, + scopes: Sequence[str] = None, + ssl_channel_credentials: grpc.ChannelCredentials = None, + quota_project_id: Optional[str] = None, + client_info: gapic_v1.client_info.ClientInfo = DEFAULT_CLIENT_INFO, + ) -> None: + """Instantiate the transport. + + Args: + host ({% if service.host %}Optional[str]{% else %}str{% endif %}): + {{- ' ' }}The hostname to connect to. + credentials (Optional[google.auth.credentials.Credentials]): The + authorization credentials to attach to requests. These + credentials identify the application to the service; if none + are specified, the client will attempt to ascertain the + credentials from the environment. + + credentials_file (Optional[str]): A file with credentials that can + be loaded with :func:`google.auth.load_credentials_from_file`. + This argument is ignored if ``channel`` is provided. + scopes (Optional(Sequence[str])): A list of scopes. This argument is + ignored if ``channel`` is provided. + ssl_channel_credentials (grpc.ChannelCredentials): SSL credentials + for grpc channel. It is ignored if ``channel`` is provided. + quota_project_id (Optional[str]): An optional project to use for billing + and quota. + client_info (google.api_core.gapic_v1.client_info.ClientInfo): + The client info used to send a user-agent string along with + API requests. If ``None``, then default info will be used. + Generally, you only need to set this if you're developing + your own client library. + """ + super().__init__(host=host, credentials=credentials) + self._session = AuthorizedSession(self._credentials) + {%- if service.has_lro %} + + @property + def operations_client(self) -> operations_v1.OperationsClient: + """Create the client designed to process long-running operations. + + This property caches on the instance; repeated calls return the same + client. + """ + # Sanity check: Only create a new client if we do not already have one. + if 'operations_client' not in self.__dict__: + from google.api_core import grpc_helpers + self.__dict__['operations_client'] = operations_v1.OperationsClient( + grpc_helpers.create_channel( + self._host, + credentials=self._credentials, + scopes=self.AUTH_SCOPES, + ) + ) + + # Return the client from cache. + return self.__dict__['operations_client'] + {%- endif %} + + + {%- for method in service.methods.values() %} + {# TODO(yonmg): consider using enums #} + {%- set ns = namespace(http=None) %} + {%- for option in method.options.ListFields() %} + {%- if option[0].name == 'http' %} + {%- set ns.http = option[1] %} + {%- endif %} + {%- endfor %} + + {%- if ns.http %} + + + def {{ method.name|snake_case }}(self, + request: {{ method.input.ident }}, *, + metadata: Sequence[Tuple[str, str]] = (), + ) -> {{ method.output.ident }}: + r"""Call the {{- ' ' -}} + {{ (method.name|snake_case).replace('_',' ')|wrap( + width=70, offset=45, indent=8) }} + {{- ' ' -}} method over HTTP. + + Args: + request (~.{{ method.input.ident }}): + The request object. + {{ method.input.meta.doc|rst(width=72, indent=16) }} + metadata (Sequence[Tuple[str, str]]): Strings which should be + sent along with the request as metadata. + {%- if not method.void %} + + Returns: + ~.{{ method.output.ident }}: + {{ method.output.meta.doc|rst(width=72, indent=16) }} + {%- endif %} + """ + + # Check which http method to use + {%- if ns.http.get != '' %} + {%- set ns.httpMethod = {'method':'get','url':ns.http.get} %} + {%- elif ns.http.put != '' %} + {%- set ns.httpMethod = {'method':'put','url':ns.http.put} %} + {%- elif ns.http.post != '' %} + {%- set ns.httpMethod = {'method':'post','url':ns.http.post} %} + {%- elif ns.http.delete != '' %} + {%- set ns.httpMethod = {'method':'delete','url':ns.http.delete} %} + {%- elif ns.http.patch != '' %} + {%- set ns.httpMethod = {'method':'patch','url':ns.http.patch} %} + {%- elif ns.http.custom != '' %} + {%- set ns.httpMethod = {'method':'custom','url':ns.http.custom} %} + {%- else %} + {%- set ns.httpMethod = None %} + {%- endif %} + + {%- if ns.http.body != '' %} + # Jsonify the input + data = {{ method.output.ident }}.to_json( + {%- if ns.http.body == '*' %} + request + {%- else %} + request.body + {%- endif %} + ) + {%- endif %} + + {# TODO(yonmg): Write helper method for handling grpc transcoding url #} + # TODO(yonmg): need to handle grpc transcoding and parse url correctly + # current impl assumes simpler version of grpc transcoding + # Send the request + url = 'https://{host}{{ ns.httpMethod['url'] }}'.format( + host=self._host, + {%- for field in method.input.fields.keys() %} + {{ field }}=request.{{ field }}, + {%- endfor %} + ) + {% if not method.void %}response = {% endif %}self._session.{{ ns.httpMethod['method'] }}( + url, + {%- if ns.http.body != '' %} + json=data, + {%- endif %} + ) + {%- if not method.void %} + {%- endif %} + + # Return the response + return {{ method.output.ident }}.from_json(response.content) + {%- else %} + # No http annotation found for method {{ method.name|snake_case }}() + {%- endif %} + {%- endfor %} + + +__all__ = ( + '{{ service.name }}RestTransport', +) +{% endblock %} \ No newline at end of file diff --git a/tests/unit/schema/wrappers/test_service.py b/tests/unit/schema/wrappers/test_service.py index c4c8d9b838..ef14e27a6f 100644 --- a/tests/unit/schema/wrappers/test_service.py +++ b/tests/unit/schema/wrappers/test_service.py @@ -59,6 +59,7 @@ def test_service_properties(): assert service.transport_name == 'ThingDoerTransport' assert service.grpc_transport_name == 'ThingDoerGrpcTransport' assert service.grpc_asyncio_transport_name == 'ThingDoerGrpcAsyncIOTransport' + assert service.rest_transport_name == 'ThingDoerRestTransport' def test_service_host(): From b41db3136f5fd4499fccf1ff7ec9c8efc4ba5395 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 6 Nov 2020 19:42:25 +0000 Subject: [PATCH 03/24] feat: add transport flag --- gapic/generator/generator.py | 4 ++++ .../%sub/services/%service/async_client.py.j2 | 1 + .../%sub/services/%service/client.py.j2 | 18 +++++++++++++----- .../%service/transports/__init__.py.j2 | 14 +++++++++++++- gapic/utils/options.py | 9 +++++++-- 5 files changed, 38 insertions(+), 8 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 5bd8a4f3c9..92e8571eae 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -274,6 +274,10 @@ def _render_template( if ( skip_subpackages and service.meta.address.subpackage != api_schema.subpackage_view + or + 'grpc' in template_name and 'grpc' not in opts.transport + or + 'rest' in template_name and 'rest' not in opts.transport ): continue diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 index 9d5150d869..d851dec06f 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 @@ -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) }}""" diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index 60ea884727..5c58fdf5f7 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -30,9 +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 }} -from .transports.rest import {{ service.rest_transport_name }} +{%- 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 %} class {{ service.client_name }}Meta(type): @@ -43,9 +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, diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 index 8ff320d88a..bd7981387f 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/__init__.py.j2 @@ -5,22 +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 %} diff --git a/gapic/utils/options.py b/gapic/utils/options.py index c3a1ef322e..21f059bede 100644 --- a/gapic/utils/options.py +++ b/gapic/utils/options.py @@ -14,7 +14,7 @@ from collections import defaultdict from os import path -from typing import Any, DefaultDict, Dict, FrozenSet, List, Optional, Tuple +from typing import Any, DefaultDict, Dict, FrozenSet, List, Optional, Tuple, Set import dataclasses import json @@ -40,6 +40,8 @@ class Options: lazy_import: bool = False old_naming: bool = False add_iam_methods: bool = False + # TODO(yonmg): should there be an enum for transport type? + transport: List[str] = None # Class constants PYTHON_GAPIC_PREFIX: str = 'python-gapic-' @@ -49,6 +51,7 @@ class Options: 'samples', # output dir 'lazy-import', # requires >= 3.7 'add-iam-methods', # microgenerator implementation for `reroute_to_grpc_interface` + 'transport', # transport type (i.e. grpc, rest, custom.[something], etc?) )) @classmethod @@ -86,7 +89,7 @@ def build(cls, opt_string: str) -> 'Options': # Throw away other options not meant for us. if not opt.startswith(cls.PYTHON_GAPIC_PREFIX): continue - + # Set the option, using a key with the "python-gapic-" prefix # stripped. # @@ -121,6 +124,7 @@ def tweak_path(p): # Build the options instance. sample_paths = opts.pop('samples', []) + answer = Options( name=opts.pop('name', ['']).pop(), namespace=tuple(opts.pop('namespace', [])), @@ -134,6 +138,7 @@ def tweak_path(p): lazy_import=bool(opts.pop('lazy-import', False)), old_naming=bool(opts.pop('old-naming', False)), add_iam_methods=bool(opts.pop('add-iam-methods', False)), + transport=opts.pop('transport', ['grpc'])[0].split('+') ) # Note: if we ever need to recursively check directories for sample From ce69e7de76543f74f5cd2d2b67d2623c33e8c2ff Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 6 Nov 2020 19:44:42 +0000 Subject: [PATCH 04/24] refactor: moved template logic outside --- gapic/cli/generate.py | 1 - gapic/schema/wrappers.py | 23 ++++++++ .../services/%service/transports/grpc.py.j2 | 7 ++- .../%service/transports/grpc_asyncio.py.j2 | 7 ++- .../services/%service/transports/rest.py.j2 | 57 ++++++------------- 5 files changed, 48 insertions(+), 47 deletions(-) diff --git a/gapic/cli/generate.py b/gapic/cli/generate.py index b0e50ccf2e..7950e462ff 100644 --- a/gapic/cli/generate.py +++ b/gapic/cli/generate.py @@ -42,7 +42,6 @@ def generate( # Pull apart arguments in the request. opts = Options.build(req.parameter) - # 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 diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 7cebbe30e2..1da269c9ce 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -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 @property def field_headers(self) -> Sequence[str]: """Return the field headers defined for this method.""" @@ -736,7 +737,28 @@ def field_headers(self) -> Sequence[str]: ] return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) + + @property + def http_opt(self) -> Dict[str, str]: + """Return the http option for this method.""" + http = self.options.Extensions[annotations_pb2.http].ListFields() + if len(http) < 1: + return None + + http_method = http[0] + answer: Dict[str, str] = { + 'method':http_method[0].name, + 'url':http_method[1], + } + if len(http) > 1: + http_opt = http[1] + answer[http_opt[0].name] = http_opt[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.""" @@ -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 @utils.cached_property def idempotent(self) -> bool: """Return True if we know this method is idempotent, False otherwise. diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 index 8be8de3b67..c7bd0cfc1e 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 @@ -151,6 +151,7 @@ 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__( @@ -220,13 +221,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: + 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() %} diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 index 6399f1f1cd..d2b162d4e3 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 @@ -205,6 +205,7 @@ 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: @@ -225,13 +226,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: + 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() %} diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index dbd1055335..a5d9c5dd34 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -42,8 +42,9 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): It sends JSON representations of protocol buffers over HTTP/1.1 """ + {# TODO(yonmg): handle mtls stuff if that's relevant for rest transport #} def __init__(self, *, - host: str = 'compute.googleapis.com', + host: str{% if service.host %} = '{{ service.host }}'{% endif %}, credentials: credentials.Credentials = None, credentials_file: str = None, scopes: Sequence[str] = None, @@ -79,8 +80,12 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): """ super().__init__(host=host, credentials=credentials) self._session = AuthorizedSession(self._credentials) - {%- if service.has_lro %} + {%- if service.has_lro %} + self._operations_client = None + {%- endif %} + {%- if service.has_lro %} + @property def operations_client(self) -> operations_v1.OperationsClient: """Create the client designed to process long-running operations. @@ -89,9 +94,9 @@ class {{ service.name }}RestTransport({{ 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__: + if self._operations_client is None: from google.api_core import grpc_helpers - self.__dict__['operations_client'] = operations_v1.OperationsClient( + self._operations_client = operations_v1.OperationsClient( grpc_helpers.create_channel( self._host, credentials=self._credentials, @@ -100,22 +105,12 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): ) # Return the client from cache. - return self.__dict__['operations_client'] + return self._operations_client {%- endif %} {%- for method in service.methods.values() %} - {# TODO(yonmg): consider using enums #} - {%- set ns = namespace(http=None) %} - {%- for option in method.options.ListFields() %} - {%- if option[0].name == 'http' %} - {%- set ns.http = option[1] %} - {%- endif %} - {%- endfor %} - - {%- if ns.http %} - - + {%- if method.http_opt %} def {{ method.name|snake_case }}(self, request: {{ method.input.ident }}, *, metadata: Sequence[Tuple[str, str]] = (), @@ -139,27 +134,10 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {%- endif %} """ - # Check which http method to use - {%- if ns.http.get != '' %} - {%- set ns.httpMethod = {'method':'get','url':ns.http.get} %} - {%- elif ns.http.put != '' %} - {%- set ns.httpMethod = {'method':'put','url':ns.http.put} %} - {%- elif ns.http.post != '' %} - {%- set ns.httpMethod = {'method':'post','url':ns.http.post} %} - {%- elif ns.http.delete != '' %} - {%- set ns.httpMethod = {'method':'delete','url':ns.http.delete} %} - {%- elif ns.http.patch != '' %} - {%- set ns.httpMethod = {'method':'patch','url':ns.http.patch} %} - {%- elif ns.http.custom != '' %} - {%- set ns.httpMethod = {'method':'custom','url':ns.http.custom} %} - {%- else %} - {%- set ns.httpMethod = None %} - {%- endif %} - - {%- if ns.http.body != '' %} + {%- if 'body' in method.http_opt.keys() %} # Jsonify the input data = {{ method.output.ident }}.to_json( - {%- if ns.http.body == '*' %} + {%- if method.http_opt['body'] == '*' %} request {%- else %} request.body @@ -171,15 +149,15 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): # TODO(yonmg): need to handle grpc transcoding and parse url correctly # current impl assumes simpler version of grpc transcoding # Send the request - url = 'https://{host}{{ ns.httpMethod['url'] }}'.format( + url = 'https://{host}{{ method.http_opt['url'] }}'.format( host=self._host, {%- for field in method.input.fields.keys() %} {{ field }}=request.{{ field }}, {%- endfor %} ) - {% if not method.void %}response = {% endif %}self._session.{{ ns.httpMethod['method'] }}( + {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['method'] }}( url, - {%- if ns.http.body != '' %} + {%- if 'body' in method.http_opt.keys() %} json=data, {%- endif %} ) @@ -189,8 +167,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): # Return the response return {{ method.output.ident }}.from_json(response.content) {%- else %} - # No http annotation found for method {{ method.name|snake_case }}() - {%- endif %} + {%- endif %} {%- endfor %} From 89fa08b3032acb5de8d806bbba0876f92561de1c Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 9 Nov 2020 20:19:40 +0000 Subject: [PATCH 05/24] fix: small fixes in transport option logic --- gapic/generator/generator.py | 13 ++++++++++--- gapic/schema/wrappers.py | 9 +++++---- .../%sub/services/%service/transports/rest.py.j2 | 6 ++---- gapic/utils/options.py | 5 +++-- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 92e8571eae..6460c3b182 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -275,9 +275,8 @@ def _render_template( skip_subpackages and service.meta.address.subpackage != api_schema.subpackage_view or - 'grpc' in template_name and 'grpc' not in opts.transport - or - 'rest' in template_name and 'rest' not in opts.transport + 'transport' in template_name + and not self._is_desired_transport(template_name, opts) ): continue @@ -297,6 +296,14 @@ 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 + for transport in desired_transports: + if transport in template_name: + return True + return False + def _get_file( self, template_name: str, diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 1da269c9ce..6b91a2cc40 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -742,13 +742,14 @@ def field_headers(self) -> Sequence[str]: def http_opt(self) -> Dict[str, str]: """Return the http option for this method.""" http = self.options.Extensions[annotations_pb2.http].ListFields() + answer: Dict[str, str] = None if len(http) < 1: - return None + return answer http_method = http[0] - answer: Dict[str, str] = { - 'method':http_method[0].name, - 'url':http_method[1], + answer = { + 'method': http_method[0].name, + 'url': http_method[1], } if len(http) > 1: http_opt = http[1] diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index a5d9c5dd34..913f6a774e 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -107,10 +107,9 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): # Return the client from cache. return self._operations_client {%- endif %} - - {%- for method in service.methods.values() %} {%- if method.http_opt %} + def {{ method.name|snake_case }}(self, request: {{ method.input.ident }}, *, metadata: Sequence[Tuple[str, str]] = (), @@ -162,11 +161,10 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {%- endif %} ) {%- if not method.void %} - {%- endif %} # Return the response return {{ method.output.ident }}.from_json(response.content) - {%- else %} + {%- endif %} {%- endif %} {%- endfor %} diff --git a/gapic/utils/options.py b/gapic/utils/options.py index 21f059bede..df18c35619 100644 --- a/gapic/utils/options.py +++ b/gapic/utils/options.py @@ -40,8 +40,8 @@ class Options: lazy_import: bool = False old_naming: bool = False add_iam_methods: bool = False - # TODO(yonmg): should there be an enum for transport type? - transport: List[str] = None + # TODO(yon-mg): should there be an enum for transport type? + transport: List[str] = dataclasses.field(default_factory=lambda: []) # Class constants PYTHON_GAPIC_PREFIX: str = 'python-gapic-' @@ -138,6 +138,7 @@ def tweak_path(p): lazy_import=bool(opts.pop('lazy-import', False)), old_naming=bool(opts.pop('old-naming', False)), add_iam_methods=bool(opts.pop('add-iam-methods', False)), + # transport should include desired transports delimited by '+', e.g. transport='grpc+rest' transport=opts.pop('transport', ['grpc'])[0].split('+') ) From 86f3a9c15fe03d15447c2adb864abee7c021ce7a Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 9 Nov 2020 22:59:28 +0000 Subject: [PATCH 06/24] test: added unit test for transport flag --- gapic/generator/generator.py | 5 +---- gapic/schema/wrappers.py | 7 +++--- tests/unit/generator/test_generator.py | 31 ++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 6460c3b182..9ee6d97cca 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -299,10 +299,7 @@ def _render_template( 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 - for transport in desired_transports: - if transport in template_name: - return True - return False + return any(transport in template_name for transport in desired_transports) def _get_file( self, diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 6b91a2cc40..5c793deb31 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -739,15 +739,14 @@ def field_headers(self) -> Sequence[str]: return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) @property - def http_opt(self) -> Dict[str, str]: + def http_opt(self) -> Optional[Dict[str, str]]: """Return the http option for this method.""" http = self.options.Extensions[annotations_pb2.http].ListFields() - answer: Dict[str, str] = None if len(http) < 1: - return answer + return None http_method = http[0] - answer = { + answer: Dict[str, str] = { 'method': http_method[0].name, 'url': http_method[1], } diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index a258c294cf..8df0167c52 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -115,6 +115,37 @@ def test_get_response_fails_invalid_file_paths(): ex_str = str(ex.value) assert "%proto" in ex_str and "%service" in ex_str +def test_get_response_ignores_unwanted_transports(): + g = make_generator() + with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt: + lt.return_value = [ + "foo/%service/transports/river.py.j2", + "foo/%service/transports/car.py.j2", + "foo/%service/transports/grpc.py.j2", + "foo/%service/transports/__init__.py.j2", + "foo/%service/transports/base.py.j2", + "mollusks/squid/sample.py.j2", + ] + with mock.patch.object(jinja2.Environment, "get_template") as gt: + gt.return_value = jinja2.Template("Service: {{ service.name }}") + cgr = g.get_response(api_schema=make_api( + make_proto( + descriptor_pb2.FileDescriptorProto( + service=[ + descriptor_pb2.ServiceDescriptorProto( + name="SomeService"), + ] + ), + ) + ), + opts=Options.build("transport=river+car")) + assert len(cgr.file) == 4 + assert {i.name for i in cgr.file} == { + "foo/some_service/transports/river.py", + "foo/some_service/transports/car.py", + "foo/some_service/transports/__init__.py", + "foo/some_service/transports/base.py", + } def test_get_response_enumerates_services(): g = make_generator() From 234cc09a12b4e5fe46ebfbebe5093472711d6061 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 11 Nov 2020 13:25:20 +0000 Subject: [PATCH 07/24] test: add unit test for http option method --- .../services/%service/transports/grpc.py.j2 | 4 +++- tests/unit/generator/test_generator.py | 24 ++++++++++--------- tests/unit/schema/wrappers/test_method.py | 14 +++++++++++ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 index c7bd0cfc1e..3d1f5ca9b2 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2 @@ -151,7 +151,9 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport): ) self._stubs = {} # type: Dict[str, Callable] - {%- if service.has_lro %} self._operations_client = None {%- endif %} + {%- if service.has_lro %} + self._operations_client = None + {%- endif %} # Run the base constructor. super().__init__( diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index 8df0167c52..e783a89fec 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -128,17 +128,19 @@ def test_get_response_ignores_unwanted_transports(): ] with mock.patch.object(jinja2.Environment, "get_template") as gt: gt.return_value = jinja2.Template("Service: {{ service.name }}") - cgr = g.get_response(api_schema=make_api( - make_proto( - descriptor_pb2.FileDescriptorProto( - service=[ - descriptor_pb2.ServiceDescriptorProto( - name="SomeService"), - ] - ), - ) - ), - opts=Options.build("transport=river+car")) + cgr = g.get_response( + api_schema=make_api( + make_proto( + descriptor_pb2.FileDescriptorProto( + service=[ + descriptor_pb2.ServiceDescriptorProto( + name="SomeService"), + ] + ), + ) + ), + opts=Options.build("transport=river+car") + ) assert len(cgr.file) == 4 assert {i.name for i in cgr.file} == { "foo/some_service/transports/river.py", diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 8b551df560..366b014220 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -248,6 +248,20 @@ def test_method_field_headers_present(): method = make_method('DoSomething', http_rule=rule) assert method.field_headers == ('parent',) +# TODO(yonmg) to test: grpc transcoding, +# correct handling of path/query params +# correct handling of body & additional binding +def test_method_http_opt(): + http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics', body='*') + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_opt == {'method': 'post', + 'url': '/v1/{parent=projects/*}/topics', + 'body': '*'} + +def test_method_http_opt_no_http_rule(): + method = make_method('DoSomething') + assert method.http_opt == None + def test_method_idempotent_yes(): http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics') From 35ac2768bf667415ba24f0e11b3eddd4525359c6 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 11 Nov 2020 14:01:58 +0000 Subject: [PATCH 08/24] test: add unit test for http option method branch --- .../%service/transports/grpc_asyncio.py.j2 | 4 +++- tests/unit/schema/wrappers/test_method.py | 21 +++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 index d2b162d4e3..5ea7031162 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2 @@ -205,7 +205,9 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport): ) self._stubs = {} - {%- if service.has_lro %} self._operations_client = None {%- endif %} + {%- if service.has_lro %} + self._operations_client = None + {%- endif %} @property def grpc_channel(self) -> aio.Channel: diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 366b014220..62a546476c 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -252,11 +252,24 @@ def test_method_field_headers_present(): # correct handling of path/query params # correct handling of body & additional binding def test_method_http_opt(): - http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics', body='*') + http_rule = http_pb2.HttpRule( + post='/v1/{parent=projects/*}/topics', + body='*' + ) + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_opt == { + 'method': 'post', + 'url': '/v1/{parent=projects/*}/topics', + 'body': '*' + } + +def test_method_http_opt_no_body(): + http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics') method = make_method('DoSomething', http_rule=http_rule) - assert method.http_opt == {'method': 'post', - 'url': '/v1/{parent=projects/*}/topics', - 'body': '*'} + assert method.http_opt == { + 'method': 'post', + 'url': '/v1/{parent=projects/*}/topics' + } def test_method_http_opt_no_http_rule(): method = make_method('DoSomething') From c8155a5fdea724a8df57b0b2feeb26d6964ceb58 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 11 Nov 2020 14:19:54 +0000 Subject: [PATCH 09/24] fix: fix import paths --- .../%name_%version/%sub/services/%service/client.py.j2 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index 5c58fdf5f7..cdbacde3c7 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -31,11 +31,11 @@ from google.iam.v1 import policy_pb2 as policy # type: ignore {% endfilter %} from .transports.base import {{ service.name }}Transport, DEFAULT_CLIENT_INFO {%- if 'grpc' in opts.transport %} -from .grpc import {{ service.name }}GrpcTransport -from .grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport +from .transports.grpc import {{ service.name }}GrpcTransport +from .transports.grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport {%- endif %} {%- if 'rest' in opts.transport %} -from .rest import {{ service.name }}RestTransport +from .transports.rest import {{ service.name }}RestTransport {%- endif %} From 55a815cc83428d24136ecd1da231e92dead95e7c Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 11 Nov 2020 14:32:28 +0000 Subject: [PATCH 10/24] fix: style check issues --- gapic/schema/wrappers.py | 2 +- gapic/utils/options.py | 2 +- tests/unit/generator/test_generator.py | 4 +++- tests/unit/schema/wrappers/test_method.py | 9 ++++++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 5c793deb31..ea57fbe54f 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -737,7 +737,7 @@ 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.""" diff --git a/gapic/utils/options.py b/gapic/utils/options.py index df18c35619..25a0a8f09f 100644 --- a/gapic/utils/options.py +++ b/gapic/utils/options.py @@ -89,7 +89,7 @@ def build(cls, opt_string: str) -> 'Options': # Throw away other options not meant for us. if not opt.startswith(cls.PYTHON_GAPIC_PREFIX): continue - + # Set the option, using a key with the "python-gapic-" prefix # stripped. # diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index e783a89fec..18a64e15fa 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -115,6 +115,7 @@ def test_get_response_fails_invalid_file_paths(): ex_str = str(ex.value) assert "%proto" in ex_str and "%service" in ex_str + def test_get_response_ignores_unwanted_transports(): g = make_generator() with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt: @@ -128,7 +129,7 @@ def test_get_response_ignores_unwanted_transports(): ] with mock.patch.object(jinja2.Environment, "get_template") as gt: gt.return_value = jinja2.Template("Service: {{ service.name }}") - cgr = g.get_response( + cgr = g.get_response( api_schema=make_api( make_proto( descriptor_pb2.FileDescriptorProto( @@ -149,6 +150,7 @@ def test_get_response_ignores_unwanted_transports(): "foo/some_service/transports/base.py", } + def test_get_response_enumerates_services(): g = make_generator() with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt: diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 62a546476c..04946b3c52 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -248,9 +248,7 @@ def test_method_field_headers_present(): method = make_method('DoSomething', http_rule=rule) assert method.field_headers == ('parent',) -# TODO(yonmg) to test: grpc transcoding, -# correct handling of path/query params -# correct handling of body & additional binding + def test_method_http_opt(): http_rule = http_pb2.HttpRule( post='/v1/{parent=projects/*}/topics', @@ -262,6 +260,10 @@ def test_method_http_opt(): 'url': '/v1/{parent=projects/*}/topics', 'body': '*' } +# TODO(yonmg) to test: grpc transcoding, +# correct handling of path/query params +# correct handling of body & additional binding + def test_method_http_opt_no_body(): http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics') @@ -271,6 +273,7 @@ def test_method_http_opt_no_body(): 'url': '/v1/{parent=projects/*}/topics' } + def test_method_http_opt_no_http_rule(): method = make_method('DoSomething') assert method.http_opt == None From ac0bdef557be6b0d8dc5d21edd59266da1775a57 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 11 Nov 2020 14:34:44 +0000 Subject: [PATCH 11/24] fix: more style check issues --- gapic/utils/options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gapic/utils/options.py b/gapic/utils/options.py index 25a0a8f09f..a327fa69b6 100644 --- a/gapic/utils/options.py +++ b/gapic/utils/options.py @@ -51,7 +51,8 @@ class Options: 'samples', # output dir 'lazy-import', # requires >= 3.7 'add-iam-methods', # microgenerator implementation for `reroute_to_grpc_interface` - 'transport', # transport type (i.e. grpc, rest, custom.[something], etc?) + # transport type (i.e. grpc, rest, custom.[something], etc?) + 'transport', )) @classmethod From e79e8c744d5728bcc299c8287b22209344780376 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 13 Nov 2020 20:04:42 +0000 Subject: [PATCH 12/24] fix: addressing pr reviews --- .../%namespace/%name/%version/__init__.py.j2 | 2 +- .../%namespace/%name/__init__.py.j2 | 2 +- gapic/cli/generate.py | 1 + gapic/generator/generator.py | 8 +++--- gapic/schema/wrappers.py | 27 ++++++++++++------- .../%sub/services/%service/async_client.py.j2 | 2 +- .../%sub/services/%service/client.py.j2 | 8 +++--- .../services/%service/transports/rest.py.j2 | 12 ++++----- gapic/utils/options.py | 4 +-- tests/unit/generator/test_generator.py | 2 ++ tests/unit/schema/wrappers/test_method.py | 2 +- 11 files changed, 41 insertions(+), 29 deletions(-) diff --git a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 index ac517161fd..aa12751852 100644 --- a/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/%version/__init__.py.j2 @@ -20,7 +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 #} + {# 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 }}', diff --git a/gapic/ads-templates/%namespace/%name/__init__.py.j2 b/gapic/ads-templates/%namespace/%name/__init__.py.j2 index ac517161fd..aa12751852 100644 --- a/gapic/ads-templates/%namespace/%name/__init__.py.j2 +++ b/gapic/ads-templates/%namespace/%name/__init__.py.j2 @@ -20,7 +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 #} + {# 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 }}', diff --git a/gapic/cli/generate.py b/gapic/cli/generate.py index 7950e462ff..921b2e9f11 100644 --- a/gapic/cli/generate.py +++ b/gapic/cli/generate.py @@ -42,6 +42,7 @@ def generate( # Pull apart arguments in the request. opts = Options.build(req.parameter) + # 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 diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 9ee6d97cca..45b204ce83 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -272,11 +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) + ('transport' in template_name + and not self._is_desired_transport(template_name, opts)) ): continue diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index ea57fbe54f..5a1763ec46 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -719,7 +719,8 @@ 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 + # 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.""" @@ -740,25 +741,33 @@ def field_headers(self) -> Sequence[str]: @property def http_opt(self) -> Optional[Dict[str, str]]: - """Return the http option for this method.""" + """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() + if len(http) < 1: return None http_method = http[0] answer: Dict[str, str] = { - 'method': http_method[0].name, + 'verb': http_method[0].name, 'url': http_method[1], } if len(http) > 1: - http_opt = http[1] - answer[http_opt[0].name] = http_opt[1] + 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? + # TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings' + # TODO(yon-mg): enums for http verbs? return answer - # TODO(yonmg): refactor as there may be more than one method signature + # 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.""" @@ -808,7 +817,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 + # 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. diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 index d851dec06f..c6320144ef 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2 @@ -30,7 +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 #} +{# TODO(yon-mg): handle rest transport async client interaction #} class {{ service.async_client_name }}: """{{ service.meta.doc|rst(width=72, indent=4) }}""" diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index cdbacde3c7..6ec3d5d879 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 @@ -31,8 +31,8 @@ from google.iam.v1 import policy_pb2 as policy # type: ignore {% endfilter %} from .transports.base import {{ service.name }}Transport, DEFAULT_CLIENT_INFO {%- if 'grpc' in opts.transport %} -from .transports.grpc import {{ service.name }}GrpcTransport -from .transports.grpc_asyncio import {{ service.name }}GrpcAsyncIOTransport +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 @@ -48,8 +48,8 @@ class {{ service.client_name }}Meta(type): """ _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 + _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 diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index 913f6a774e..d26856dd2c 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -16,7 +16,7 @@ import grpc # type: ignore from google.auth.transport.requests import AuthorizedSession -{# TODO: re-add python_import/ python_modules from removed diff/current grpc template code #} +{# TODO(yon-mg): re-add python_import/ python_modules from removed diff/current grpc template code #} {% filter sort_lines -%} {% for method in service.methods.values() -%} {{ method.input.ident.python_import }} @@ -42,7 +42,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): It sends JSON representations of protocol buffers over HTTP/1.1 """ - {# TODO(yonmg): handle mtls stuff if that's relevant for rest transport #} + {# TODO(yon-mg): handle mtls stuff if that's relevant for rest transport #} def __init__(self, *, host: str{% if service.host %} = '{{ service.host }}'{% endif %}, credentials: credentials.Credentials = None, @@ -144,8 +144,8 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): ) {%- endif %} - {# TODO(yonmg): Write helper method for handling grpc transcoding url #} - # TODO(yonmg): need to handle grpc transcoding and parse url correctly + {# 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 simpler version of grpc transcoding # Send the request url = 'https://{host}{{ method.http_opt['url'] }}'.format( @@ -154,7 +154,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {{ field }}=request.{{ field }}, {%- endfor %} ) - {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['method'] }}( + {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( url, {%- if 'body' in method.http_opt.keys() %} json=data, @@ -172,4 +172,4 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): __all__ = ( '{{ service.name }}RestTransport', ) -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/gapic/utils/options.py b/gapic/utils/options.py index a327fa69b6..d99e34c631 100644 --- a/gapic/utils/options.py +++ b/gapic/utils/options.py @@ -14,7 +14,7 @@ from collections import defaultdict from os import path -from typing import Any, DefaultDict, Dict, FrozenSet, List, Optional, Tuple, Set +from typing import Any, DefaultDict, Dict, FrozenSet, List, Optional, Tuple import dataclasses import json @@ -51,7 +51,7 @@ class Options: 'samples', # output dir 'lazy-import', # requires >= 3.7 'add-iam-methods', # microgenerator implementation for `reroute_to_grpc_interface` - # transport type (i.e. grpc, rest, custom.[something], etc?) + # transport type(s) delineated by '+' (i.e. grpc, rest, custom.[something], etc?) 'transport', )) diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index 18a64e15fa..e68bf5bb66 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -127,6 +127,7 @@ def test_get_response_ignores_unwanted_transports(): "foo/%service/transports/base.py.j2", "mollusks/squid/sample.py.j2", ] + with mock.patch.object(jinja2.Environment, "get_template") as gt: gt.return_value = jinja2.Template("Service: {{ service.name }}") cgr = g.get_response( @@ -142,6 +143,7 @@ def test_get_response_ignores_unwanted_transports(): ), opts=Options.build("transport=river+car") ) + assert len(cgr.file) == 4 assert {i.name for i in cgr.file} == { "foo/some_service/transports/river.py", diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 04946b3c52..d462a7b5f1 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -260,7 +260,7 @@ def test_method_http_opt(): 'url': '/v1/{parent=projects/*}/topics', 'body': '*' } -# TODO(yonmg) to test: grpc transcoding, +# TODO(yon-mg) to test: grpc transcoding, # correct handling of path/query params # correct handling of body & additional binding From 824ad1167dd0ccb150b97f5f5659013f4b848bda Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 13 Nov 2020 22:48:14 +0000 Subject: [PATCH 13/24] fix: typo in test_method --- tests/unit/schema/wrappers/test_method.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index d462a7b5f1..f6db3c044b 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -256,7 +256,7 @@ def test_method_http_opt(): ) method = make_method('DoSomething', http_rule=http_rule) assert method.http_opt == { - 'method': 'post', + 'verb': 'post', 'url': '/v1/{parent=projects/*}/topics', 'body': '*' } @@ -269,7 +269,7 @@ def test_method_http_opt_no_body(): http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics') method = make_method('DoSomething', http_rule=http_rule) assert method.http_opt == { - 'method': 'post', + 'verb': 'post', 'url': '/v1/{parent=projects/*}/topics' } From 2d11e19cb40dddef404f7ec7335c5fea30f85b91 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 13 Nov 2020 22:59:27 +0000 Subject: [PATCH 14/24] fix: style check fixes --- gapic/cli/generate.py | 2 +- gapic/schema/wrappers.py | 2 +- tests/unit/generator/test_generator.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/cli/generate.py b/gapic/cli/generate.py index 921b2e9f11..b0e50ccf2e 100644 --- a/gapic/cli/generate.py +++ b/gapic/cli/generate.py @@ -42,7 +42,7 @@ def generate( # Pull apart arguments in the request. opts = Options.build(req.parameter) - + # 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 diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 5a1763ec46..664f240ffa 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -746,7 +746,7 @@ def http_opt(self) -> Optional[Dict[str, str]]: e.g. {'verb': 'post' 'url': '/some/path' 'body': '*'} - + """ http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]] http = self.options.Extensions[annotations_pb2.http].ListFields() diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index e68bf5bb66..97793e4433 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -143,7 +143,7 @@ def test_get_response_ignores_unwanted_transports(): ), opts=Options.build("transport=river+car") ) - + assert len(cgr.file) == 4 assert {i.name for i in cgr.file} == { "foo/some_service/transports/river.py", From 221cd446c4552ab19f3589b6eeda1df5e8ec7339 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 20 Nov 2020 22:17:02 +0000 Subject: [PATCH 15/24] feat: add proper handling of query/path/body parameters for rest transport --- gapic/generator/generator.py | 3 ++ gapic/schema/wrappers.py | 17 +++++++ .../services/%service/transports/rest.py.j2 | 32 ++++++++---- gapic/utils/__init__.py | 2 + gapic/utils/case.py | 14 ++++++ tests/unit/schema/wrappers/test_method.py | 49 +++++++++++++++++++ 6 files changed, 108 insertions(+), 9 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 45b204ce83..143e1eebf5 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -54,6 +54,7 @@ def __init__(self, opts: Options) -> None: # Add filters which templates require. self._env.filters["rst"] = utils.rst self._env.filters["snake_case"] = utils.to_snake_case + self._env.filters["camel_case"] = utils.to_camel_case self._env.filters["sort_lines"] = utils.sort_lines self._env.filters["wrap"] = utils.wrap self._env.filters["coerce_response_name"] = coerce_response_name @@ -288,6 +289,8 @@ def _render_template( opts=opts, ) ) + #for method in service.methods.values(): + #breakpoint() return answer # This file is not iterating over anything else; return back diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 664f240ffa..9a71360e86 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -767,6 +767,23 @@ def http_opt(self) -> Optional[Dict[str, str]]: # TODO(yon-mg): enums for http verbs? return answer + @property + def path_params(self) -> Sequence[str]: + if self.http_opt is None: + return [] + pattern = r'\{\w+\}' + return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])] + + @property + def query_params(self) -> Set[str]: + if self.http_opt is None: + return {} + body = [] + if 'body' in self.http_opt.keys(): + body.append(self.http_opt['body']) + all_params = self.input.fields.keys() + return set(all_params) ^ set(body + self.path_params) + # TODO(yon-mg): refactor as there may be more than one method signature @utils.cached_property def flattened_fields(self) -> Mapping[str, Field]: diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index d26856dd2c..1df38f9aac 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -135,28 +135,42 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {%- if 'body' in method.http_opt.keys() %} # Jsonify the input - data = {{ method.output.ident }}.to_json( - {%- if method.http_opt['body'] == '*' %} + {%- if method.http_opt['body'] != '*' %} + data = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json( + request.{{ method.http_opt['body'] }}, + including_default_value_fields=False + ) + {%- else %} + data = {{ method.input.ident }}.to_json( request - {%- else %} - request.body - {%- endif %} ) {%- 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 simpler version of grpc transcoding + # current impl assumes basic case of grpc transcoding # Send the request url = 'https://{host}{{ method.http_opt['url'] }}'.format( host=self._host, - {%- for field in method.input.fields.keys() %} + {%- for field in method.path_params %} {{ field }}=request.{{ field }}, {%- endfor %} ) + + potentialParams = { + {%- for field in method.query_params %} + '{{ field|camel_case }}': request.{{ field }}, + {%- endfor %} + } + potentialParams = {k: v for k, v in potentialParams.items() if v} + for i, (param_name, param_value) in enumerate(potentialParams.items()): + q = '?' if i == 0 else '&' + url += q + param_name + '=' + str(param_value).replace(' ', '+') + {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( - url, - {%- if 'body' in method.http_opt.keys() %} + url + {%- if 'body' in method.http_opt.keys() %}, json=data, {%- endif %} ) diff --git a/gapic/utils/__init__.py b/gapic/utils/__init__.py index 9719a8f7a2..9729591c3c 100644 --- a/gapic/utils/__init__.py +++ b/gapic/utils/__init__.py @@ -14,6 +14,7 @@ from gapic.utils.cache import cached_property from gapic.utils.case import to_snake_case +from gapic.utils.case import to_camel_case from gapic.utils.code import empty from gapic.utils.code import nth from gapic.utils.code import partition @@ -38,6 +39,7 @@ 'rst', 'sort_lines', 'to_snake_case', + 'to_camel_case', 'to_valid_filename', 'to_valid_module_name', 'wrap', diff --git a/gapic/utils/case.py b/gapic/utils/case.py index a7552e2aba..61700925d5 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -45,3 +45,17 @@ def to_snake_case(s: str) -> str: # Done; return the camel-cased string. return s.lower() + +def to_camel_case(s: str) -> str: + '''Convert any string to camel case. + + This is provided to templates as the ``camel_case`` filter. + + Args: + s (str): The input string, provided in snake case. + + Returns: + str: The string in camel case with the first letter unchanged. + ''' + items = s.split('_') + return items[0] + "".join([x.capitalize() for x in items[1:]]) \ No newline at end of file diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index f6db3c044b..c03c7be8bf 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -279,6 +279,55 @@ def test_method_http_opt_no_http_rule(): assert method.http_opt == None +def test_method_path_params(): + # tests only the basic case of grpc transcoding + http_rule = http_pb2.HttpRule(post='/v1/{project}/topics') + method = make_method('DoSomething', http_rule=http_rule) + assert method.path_params == ['project'] + + +def test_method_path_params_no_http_rule(): + method = make_method('DoSomething') + assert method.path_params == [] + + +def test_method_query_params(): + # tests only the basic case of grpc transcoding + http_rule = http_pb2.HttpRule( + post='/v1/{project}/topics', + body='address' + ) + input_message = make_message( + 'MethodInput', + fields=( + make_field('region'), + make_field('project'), + make_field('address') + ) + ) + method = make_method('DoSomething', http_rule=http_rule, input_message=input_message) + assert method.query_params == {'region'} + + +def test_method_query_params_no_body(): + # tests only the basic case of grpc transcoding + http_rule = http_pb2.HttpRule(post='/v1/{project}/topics') + input_message = make_message( + 'MethodInput', + fields=( + make_field('region'), + make_field('project'), + ) + ) + method = make_method('DoSomething', http_rule=http_rule, input_message=input_message) + assert method.query_params == {'region'} + + +def test_method_query_params_no_http_rule(): + method = make_method('DoSomething') + assert method.query_params == {} + + def test_method_idempotent_yes(): http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics') method = make_method('DoSomething', http_rule=http_rule) From efa0ed62b5fe2e0f6f77a7d4ea10b085690d7355 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 20 Nov 2020 22:57:08 +0000 Subject: [PATCH 16/24] fix: typing errors --- gapic/schema/wrappers.py | 4 ++-- tests/unit/schema/wrappers/test_method.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 9a71360e86..c3ddd7a56f 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -777,12 +777,12 @@ def path_params(self) -> Sequence[str]: @property def query_params(self) -> Set[str]: if self.http_opt is None: - return {} + return set() body = [] if 'body' in self.http_opt.keys(): body.append(self.http_opt['body']) all_params = self.input.fields.keys() - return set(all_params) ^ set(body + self.path_params) + return set(all_params) ^ set(body + list(self.path_params)) # TODO(yon-mg): refactor as there may be more than one method signature @utils.cached_property diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index c03c7be8bf..53df09dcd5 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -325,7 +325,7 @@ def test_method_query_params_no_body(): def test_method_query_params_no_http_rule(): method = make_method('DoSomething') - assert method.query_params == {} + assert method.query_params == set() def test_method_idempotent_yes(): From d3e08c00bbd532671d8cf2c73988475e8b6b8f6f Mon Sep 17 00:00:00 2001 From: Dov Shlachter Date: Fri, 20 Nov 2020 15:39:34 -0800 Subject: [PATCH 17/24] Update case.py --- gapic/utils/case.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/utils/case.py b/gapic/utils/case.py index 61700925d5..fff80897f6 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -58,4 +58,4 @@ def to_camel_case(s: str) -> str: str: The string in camel case with the first letter unchanged. ''' items = s.split('_') - return items[0] + "".join([x.capitalize() for x in items[1:]]) \ No newline at end of file + return items[0] + "".join([x.capitalize() for x in items[1:]]) From df88ef900688e91fcc917ad63d205c3de454535e Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 30 Nov 2020 23:09:26 +0000 Subject: [PATCH 18/24] fix: minor changes adding a test, refactor and style check --- .circleci/config.yml | 1 + gapic/generator/generator.py | 2 -- gapic/schema/wrappers.py | 16 +++++++++++----- .../%sub/services/%service/transports/rest.py.j2 | 10 +++++----- gapic/utils/case.py | 11 ++++++----- noxfile.py | 6 ++++++ tests/unit/schema/wrappers/test_method.py | 6 ++++-- tests/unit/utils/test_case.py | 16 ++++++++++++++++ 8 files changed, 49 insertions(+), 19 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 77375976fc..6fef7704cf 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -364,6 +364,7 @@ jobs: cd .. nox -s showcase_mtls_alternative_templates + # TODO(yon-mg): add compute unit tests showcase-unit-3.6: docker: - image: python:3.6-slim diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 143e1eebf5..6a3446cbf8 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -289,8 +289,6 @@ def _render_template( opts=opts, ) ) - #for method in service.methods.values(): - #breakpoint() return answer # This file is not iterating over anything else; return back diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index c3ddd7a56f..e31eb4f135 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -769,20 +769,26 @@ def http_opt(self) -> Optional[Dict[str, str]]: @property def path_params(self) -> Sequence[str]: + """Return the path parameters found in the http annotation url""" + # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case) if self.http_opt is None: return [] pattern = r'\{\w+\}' + return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])] @property def query_params(self) -> Set[str]: + """Return query parameters for API call as determined by http annotation and grpc transcoding""" + # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case) if self.http_opt is None: return set() - body = [] - if 'body' in self.http_opt.keys(): - body.append(self.http_opt['body']) - all_params = self.input.fields.keys() - return set(all_params) ^ set(body + list(self.path_params)) + params = set(self.path_params) + body = self.http_opt.get('body') + if body: + params.add(body) + + return set(self.input.fields) ^ params # TODO(yon-mg): refactor as there may be more than one method signature @utils.cached_property diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index 1df38f9aac..57b5df8691 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -133,7 +133,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {%- endif %} """ - {%- if 'body' in method.http_opt.keys() %} + {%- if 'body' in method.http_opt %} # Jsonify the input {%- if method.http_opt['body'] != '*' %} data = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json( @@ -163,14 +163,14 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): '{{ field|camel_case }}': request.{{ field }}, {%- endfor %} } - potentialParams = {k: v for k, v in potentialParams.items() if v} - for i, (param_name, param_value) in enumerate(potentialParams.items()): + potentialParams = ((k, v) for k, v in potentialParams.items() if v) + for i, (param_name, param_value) in enumerate(potentialParams): q = '?' if i == 0 else '&' - url += q + param_name + '=' + str(param_value).replace(' ', '+') + url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+')) {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( url - {%- if 'body' in method.http_opt.keys() %}, + {%- if 'body' in method.http_opt %}, json=data, {%- endif %} ) diff --git a/gapic/utils/case.py b/gapic/utils/case.py index 61700925d5..3c0449f953 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -46,16 +46,17 @@ def to_snake_case(s: str) -> str: # Done; return the camel-cased string. return s.lower() + def to_camel_case(s: str) -> str: '''Convert any string to camel case. This is provided to templates as the ``camel_case`` filter. Args: - s (str): The input string, provided in snake case. - + s (str): The input string, provided in any sane case system + Returns: - str: The string in camel case with the first letter unchanged. + str: The string in lower camel case with the first letter unchanged. ''' - items = s.split('_') - return items[0] + "".join([x.capitalize() for x in items[1:]]) \ No newline at end of file + items = re.split(r'[_-]', s) + return items[0] + "".join(x.capitalize() for x in items[1:]) diff --git a/noxfile.py b/noxfile.py index 37a2be048a..a50376efe1 100644 --- a/noxfile.py +++ b/noxfile.py @@ -52,6 +52,10 @@ def unit(session): ) +# TODO(yon-mg): -add compute context manager that includes rest transport +# -add compute unit tests +# (to test against temporarily while rest transport is incomplete) +# (to be removed once all features are complete) @contextmanager def showcase_library( session, templates="DEFAULT", other_opts: typing.Iterable[str] = () @@ -87,6 +91,8 @@ def showcase_library( # Write out a client library for Showcase. template_opt = f"python-gapic-templates={templates}" + # TODO(yon-mg): add "transports=grpc+rest" when all rest features required for + # Showcase are implemented i.e. (grpc transcoding, LROs, etc) opts = "--python_gapic_opt=" opts += ",".join(other_opts + (f"{template_opt}",)) cmd_tup = ( diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 53df09dcd5..bcaeb68800 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -305,7 +305,8 @@ def test_method_query_params(): make_field('address') ) ) - method = make_method('DoSomething', http_rule=http_rule, input_message=input_message) + method = make_method('DoSomething', http_rule=http_rule, + input_message=input_message) assert method.query_params == {'region'} @@ -319,7 +320,8 @@ def test_method_query_params_no_body(): make_field('project'), ) ) - method = make_method('DoSomething', http_rule=http_rule, input_message=input_message) + method = make_method('DoSomething', http_rule=http_rule, + input_message=input_message) assert method.query_params == {'region'} diff --git a/tests/unit/utils/test_case.py b/tests/unit/utils/test_case.py index 93b86ea763..62b4c56180 100644 --- a/tests/unit/utils/test_case.py +++ b/tests/unit/utils/test_case.py @@ -25,3 +25,19 @@ def test_camel_to_snake(): def test_constant_to_snake(): assert case.to_snake_case('CONSTANT_CASE_THING') == 'constant_case_thing' + + +def test_pascal_to_camel(): + assert case.to_camel_case('PascalCaseThing') == 'PascalCaseThing' + + +def test_snake_to_camel(): + assert case.to_camel_case('snake_case_thing') == 'snakeCaseThing' + + +def test_constant_to_camel(): + assert case.to_camel_case('CONSTANT_CASE_THING') == 'ConstantCaseThing' + + +def test_kebab_to_camel(): + assert case.to_camel_case('kebab-case-thing') == 'kebabCaseThing' From 37e3d284454f3ba961baa2977320a993fc722665 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 30 Nov 2020 23:46:32 +0000 Subject: [PATCH 19/24] fix: camel_case bug with constant case --- gapic/utils/case.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/utils/case.py b/gapic/utils/case.py index 3c0449f953..3e48f7a81a 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -59,4 +59,4 @@ def to_camel_case(s: str) -> str: str: The string in lower camel case with the first letter unchanged. ''' items = re.split(r'[_-]', s) - return items[0] + "".join(x.capitalize() for x in items[1:]) + return items[0].capitalize() + "".join(x.capitalize() for x in items[1:]) From c964fba4a1652cfeadde7f5f22547d2e39ae8aba Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Tue, 1 Dec 2020 00:45:32 +0000 Subject: [PATCH 20/24] fix: to_camel_case to produce lower camel case instead of PascalCase where relevant --- gapic/utils/case.py | 7 ++++--- tests/unit/utils/test_case.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/gapic/utils/case.py b/gapic/utils/case.py index 3e48f7a81a..f58aa4adc6 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -56,7 +56,8 @@ def to_camel_case(s: str) -> str: s (str): The input string, provided in any sane case system Returns: - str: The string in lower camel case with the first letter unchanged. + str: The string in lower camel case. ''' - items = re.split(r'[_-]', s) - return items[0].capitalize() + "".join(x.capitalize() for x in items[1:]) + + items = re.split(r'[_-]', to_snake_case(s)) + return items[0].lower() + "".join(x.capitalize() for x in items[1:]) diff --git a/tests/unit/utils/test_case.py b/tests/unit/utils/test_case.py index 62b4c56180..83406ca43e 100644 --- a/tests/unit/utils/test_case.py +++ b/tests/unit/utils/test_case.py @@ -28,7 +28,7 @@ def test_constant_to_snake(): def test_pascal_to_camel(): - assert case.to_camel_case('PascalCaseThing') == 'PascalCaseThing' + assert case.to_camel_case('PascalCaseThing') == 'pascalCaseThing' def test_snake_to_camel(): @@ -36,7 +36,7 @@ def test_snake_to_camel(): def test_constant_to_camel(): - assert case.to_camel_case('CONSTANT_CASE_THING') == 'ConstantCaseThing' + assert case.to_camel_case('CONSTANT_CASE_THING') == 'constantCaseThing' def test_kebab_to_camel(): From 85be88d7fa680c91cbedb3195e0b26715e633e21 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Thu, 3 Dec 2020 19:33:57 +0000 Subject: [PATCH 21/24] fix: addressing pr comments --- gapic/schema/wrappers.py | 3 ++- .../services/%service/transports/rest.py.j2 | 21 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index e31eb4f135..859f9b5f35 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -781,6 +781,7 @@ def path_params(self) -> Sequence[str]: def query_params(self) -> Set[str]: """Return query parameters for API call as determined by http annotation and grpc transcoding""" # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case) + # TODO(yon-mg): remove this method and move logic to generated client if self.http_opt is None: return set() params = set(self.path_params) @@ -788,7 +789,7 @@ def query_params(self) -> Set[str]: if body: params.add(body) - return set(self.input.fields) ^ params + return set(self.input.fields) - params # TODO(yon-mg): refactor as there may be more than one method signature @utils.cached_property diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index 57b5df8691..9e9e205ed5 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -133,15 +133,21 @@ 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 %} - # Jsonify the input + # Jsonify the request body {%- if method.http_opt['body'] != '*' %} - data = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json( + body = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json( request.{{ method.http_opt['body'] }}, including_default_value_fields=False ) {%- else %} - data = {{ method.input.ident }}.to_json( + body = {{ method.input.ident }}.to_json( request ) {%- endif %} @@ -150,7 +156,6 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {# 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 - # Send the request url = 'https://{host}{{ method.http_opt['url'] }}'.format( host=self._host, {%- for field in method.path_params %} @@ -158,6 +163,11 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): {%- 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 potentialParams = { {%- for field in method.query_params %} '{{ field|camel_case }}': request.{{ field }}, @@ -168,10 +178,11 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): q = '?' if i == 0 else '&' url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+')) + # Send the request {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( url {%- if 'body' in method.http_opt %}, - json=data, + json=body, {%- endif %} ) {%- if not method.void %} From 5d1c6a3bf6f79cb1f6ee07b7359626405db108dd Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 4 Dec 2020 18:34:09 +0000 Subject: [PATCH 22/24] fix: adding appropriate todos, addressing comments --- gapic/schema/wrappers.py | 6 +++--- .../%sub/services/%service/transports/rest.py.j2 | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 859f9b5f35..ac16e3bdb5 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -769,13 +769,13 @@ def http_opt(self) -> Optional[Dict[str, str]]: @property def path_params(self) -> Sequence[str]: - """Return the path parameters found in the http annotation url""" + """Return the path parameters found in the http annotation path template""" # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case) if self.http_opt is None: return [] - pattern = r'\{\w+\}' - return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])] + pattern = r'\{(\w+)\}' + return re.findall(pattern, self.http_opt['url']) @property def query_params(self) -> Set[str]: diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 index 9e9e205ed5..a25f66e2a7 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2 @@ -168,13 +168,16 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): #} # TODO(yon-mg): handle nested fields corerctly rather than using only top level fields # not required for GCE - potentialParams = { + query_params = { {%- for field in method.query_params %} '{{ field|camel_case }}': request.{{ field }}, {%- endfor %} } - potentialParams = ((k, v) for k, v in potentialParams.items() if v) - for i, (param_name, param_value) in enumerate(potentialParams): + # 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) for k, v in query_params.items() if v) + for i, (param_name, param_value) in enumerate(query_params): q = '?' if i == 0 else '&' url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+')) From f6c64cc31a55a1c418bf2a9e575e203dcc8f938d Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Tue, 8 Dec 2020 00:49:45 +0000 Subject: [PATCH 23/24] fix: dataclass dependency issue --- setup.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setup.py b/setup.py index 16fef93d78..409dea8b65 100644 --- a/setup.py +++ b/setup.py @@ -51,9 +51,9 @@ "protobuf >= 3.12.0", "pypandoc >= 1.4", "PyYAML >= 5.1.1", - "dataclasses<0.8; python_version < '3.7'" + "dataclasses < 0.8; python_version < '3.7'" ), - extras_require={':python_version<"3.7"': ("dataclasses >= 0.4",),}, + extras_require={':python_version<"3.7"': ("dataclasses >= 0.4, < 0.8",),}, tests_require=("pyfakefs >= 3.6",), python_requires=">=3.6", classifiers=( From a4f34e7a96d10789fa0004ee5c9daec09ec8321f Mon Sep 17 00:00:00 2001 From: Dov Shlachter Date: Mon, 7 Dec 2020 17:01:06 -0800 Subject: [PATCH 24/24] Update wrappers.py --- gapic/schema/wrappers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index ac16e3bdb5..00f22d6da2 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -784,6 +784,7 @@ def query_params(self) -> Set[str]: # TODO(yon-mg): remove this method and move logic to generated client if self.http_opt is None: return set() + params = set(self.path_params) body = self.http_opt.get('body') if body: