From c1a6f891db8ce80a64d293ae8d8bed836b63a3c7 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 2 Nov 2020 17:49:42 +0000 Subject: [PATCH 01/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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: From 872e01db60bc9054aa04fed6091771df3c817d69 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Fri, 18 Dec 2020 01:51:29 +0000 Subject: [PATCH 25/32] fix: updating testing, rest-only generation, & minor bug-fixes --- gapic/generator/generator.py | 4 + gapic/schema/wrappers.py | 3 +- .../templates/%namespace/%name/__init__.py.j2 | 4 + .../%sub/services/%service/__init__.py.j2 | 4 + .../services/%service/transports/rest.py.j2 | 14 +- .../%name_%version/%sub/test_%service.py.j2 | 220 ++++++++++++++++-- gapic/utils/case.py | 4 +- 7 files changed, 223 insertions(+), 30 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 6a3446cbf8..679b85176e 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -278,6 +278,10 @@ def _render_template( or ('transport' in template_name and not self._is_desired_transport(template_name, opts)) + or + # TODO(yon-mg) - remove when rest async implementation resolved + # temporarily stop async client gen while rest async is unkown + ('async' in template_name and 'grpc' not in opts.transport) ): continue diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 00f22d6da2..6f64348990 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -411,7 +411,8 @@ def get_field(self, *field_path: str, collisions = collisions or self.meta.address.collisions # Get the first field in the path. - cursor = self.fields[field_path[0]] + first_field = field_path[0] + cursor = self.fields[first_field+('_' if first_field in utils.RESERVED_NAMES else '')] # Base case: If this is the last field in the path, return it outright. if len(field_path) == 1: diff --git a/gapic/templates/%namespace/%name/__init__.py.j2 b/gapic/templates/%namespace/%name/__init__.py.j2 index d777dc86e3..7ffe67b3fe 100644 --- a/gapic/templates/%namespace/%name/__init__.py.j2 +++ b/gapic/templates/%namespace/%name/__init__.py.j2 @@ -12,8 +12,10 @@ from {% if api.naming.module_namespace %}{{ api.naming.module_namespace|join('.' if service.meta.address.subpackage == api.subpackage_view -%} from {% if api.naming.module_namespace %}{{ api.naming.module_namespace|join('.') }}.{% endif -%} {{ api.naming.versioned_module_name }}.services.{{ service.name|snake_case }}.client import {{ service.client_name }} +{%- if 'grpc' in opts.transport %} from {% if api.naming.module_namespace %}{{ api.naming.module_namespace|join('.') }}.{% endif -%} {{ api.naming.versioned_module_name }}.services.{{ service.name|snake_case }}.async_client import {{ service.async_client_name }} +{%- endif %} {% endfor -%} {# Import messages and enums from each proto. @@ -50,7 +52,9 @@ __all__ = ( {% for service in api.services.values()|sort(attribute='name') if service.meta.address.subpackage == api.subpackage_view -%} '{{ service.client_name }}', + {%- if 'grpc' in opts.transport %} '{{ service.async_client_name }}', + {%- endif %} {% endfor -%} {% for proto in api.protos.values()|sort(attribute='module_name') if proto.meta.address.subpackage == api.subpackage_view -%} diff --git a/gapic/templates/%namespace/%name_%version/%sub/services/%service/__init__.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/__init__.py.j2 index c99b2a5f91..e0112041c3 100644 --- a/gapic/templates/%namespace/%name_%version/%sub/services/%service/__init__.py.j2 +++ b/gapic/templates/%namespace/%name_%version/%sub/services/%service/__init__.py.j2 @@ -2,10 +2,14 @@ {% block content %} from .client import {{ service.client_name }} +{%- if 'grpc' in opts.transport %} from .async_client import {{ service.async_client_name }} +{%- endif %} __all__ = ( '{{ service.client_name }}', + {%- if 'grpc' in opts.transport %} '{{ service.async_client_name }}', + {%- endif %} ) {% endblock %} 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 ad7e4051b9..4f44807583 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 @@ -78,7 +78,13 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): Generally, you only need to set this if you're developing your own client library. """ - super().__init__(host=host, credentials=credentials) + # Run the base constructor + # TODO(yon-mg): resolve other ctor params i.e. scopes, quota, etc. + super().__init__( + host=host, + credentials=credentials, + client_info=client_info, + ) self._session = AuthorizedSession(self._credentials) {%- if service.has_lro %} self._operations_client = None @@ -180,10 +186,8 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): # 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(' ', '+')) + query_params = ['{k}={v}'.format(k=k, v=v) for k, v in query_params.items() if v] + url += '?{}'.format('&'.join(query_params)).replace(' ', '+') # Send the request {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 570e997a6f..4a309de52b 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -10,6 +10,9 @@ import math import pytest from proto.marshal.rules.dates import DurationRule, TimestampRule +from requests import Response +from requests.sessions import Session + {# Import the service itself as well as every proto module that it imports. -#} {% filter sort_lines -%} from google import auth @@ -17,7 +20,9 @@ from google.auth import credentials from google.auth.exceptions import MutualTLSChannelError from google.oauth2 import service_account from {{ (api.naming.module_namespace + (api.naming.versioned_module_name,) + service.meta.address.subpackage)|join(".") }}.services.{{ service.name|snake_case }} import {{ service.client_name }} +{%- if 'grpc' in opts.transport %} from {{ (api.naming.module_namespace + (api.naming.versioned_module_name,) + service.meta.address.subpackage)|join(".") }}.services.{{ service.name|snake_case }} import {{ service.async_client_name }} +{%- endif %} from {{ (api.naming.module_namespace + (api.naming.versioned_module_name,) + service.meta.address.subpackage)|join(".") }}.services.{{ service.name|snake_case }} import transports from google.api_core import client_options from google.api_core import exceptions @@ -81,7 +86,12 @@ def test_{{ service.client_name|snake_case }}_from_service_account_info(): {% if service.host %}assert client.transport._host == '{{ service.host }}{% if ":" not in service.host %}:443{% endif %}'{% endif %} -@pytest.mark.parametrize("client_class", [{{ service.client_name }}, {{ service.async_client_name }}]) +@pytest.mark.parametrize("client_class", [ + {{ service.client_name }}, + {%- if 'grpc' in opts.transport %} + {{ service.async_client_name }}, + {%- endif %} +]) def test_{{ service.client_name|snake_case }}_from_service_account_file(client_class): creds = credentials.AnonymousCredentials() with mock.patch.object(service_account.Credentials, 'from_service_account_file') as factory: @@ -97,18 +107,29 @@ def test_{{ service.client_name|snake_case }}_from_service_account_file(client_c def test_{{ service.client_name|snake_case }}_get_transport_class(): transport = {{ service.client_name }}.get_transport_class() - assert transport == transports.{{ service.name }}GrpcTransport + available_transports = [ + {%- for transport_name in opts.transport %} + transports.{{ service.name }}{{ transport_name.capitalize() }}Transport, + {%- endfor %} + ] + assert transport in available_transports - transport = {{ service.client_name }}.get_transport_class("grpc") - assert transport == transports.{{ service.name }}GrpcTransport + transport = {{ service.client_name }}.get_transport_class("{{ opts.transport[0] }}") + assert transport == transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport @pytest.mark.parametrize("client_class,transport_class,transport_name", [ + {%- if 'grpc' in opts.transport %} ({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc"), - ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio") + ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio"), + {%- elif 'rest' in opts.transport %} + ({{ service.client_name }}, transports.{{ service.rest_transport_name }}, "rest"), + {%- endif %} ]) @mock.patch.object({{ service.client_name }}, "DEFAULT_ENDPOINT", modify_default_endpoint({{ service.client_name }})) +{%- if 'grpc' in opts.transport %} @mock.patch.object({{ service.async_client_name }}, "DEFAULT_ENDPOINT", modify_default_endpoint({{ service.async_client_name }})) +{%- endif %} def test_{{ service.client_name|snake_case }}_client_options(client_class, transport_class, transport_name): # Check that if channel is provided we won't create a new one. with mock.patch.object({{ service.client_name }}, 'get_transport_class') as gtc: @@ -197,13 +218,20 @@ def test_{{ service.client_name|snake_case }}_client_options(client_class, trans ) @pytest.mark.parametrize("client_class,transport_class,transport_name,use_client_cert_env", [ + {% if 'grpc' in opts.transport %} ({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc", "true"), ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio", "true"), ({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc", "false"), - ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio", "false") + ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio", "false"), + {% elif 'rest' in opts.transport %} + ({{ service.client_name }}, transports.{{ service.rest_transport_name }}, "rest", "true"), + ({{ service.client_name }}, transports.{{ service.rest_transport_name }}, "rest", "false"), + {%- endif %} ]) @mock.patch.object({{ service.client_name }}, "DEFAULT_ENDPOINT", modify_default_endpoint({{ service.client_name }})) +{%- if 'grpc' in opts.transport %} @mock.patch.object({{ service.async_client_name }}, "DEFAULT_ENDPOINT", modify_default_endpoint({{ service.async_client_name }})) +{%- endif %} @mock.patch.dict(os.environ, {"GOOGLE_API_USE_MTLS_ENDPOINT": "auto"}) def test_{{ service.client_name|snake_case }}_mtls_env_auto(client_class, transport_class, transport_name, use_client_cert_env): # This tests the endpoint autoswitch behavior. Endpoint is autoswitched to the default @@ -286,8 +314,12 @@ def test_{{ service.client_name|snake_case }}_mtls_env_auto(client_class, transp @pytest.mark.parametrize("client_class,transport_class,transport_name", [ + {%- if 'grpc' in opts.transport %} ({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc"), - ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio") + ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio"), + {%- elif 'rest' in opts.transport %} + ({{ service.client_name }}, transports.{{ service.rest_transport_name }}, "rest"), + {%- endif %} ]) def test_{{ service.client_name|snake_case }}_client_options_scopes(client_class, transport_class, transport_name): # Check the case scopes are provided. @@ -308,8 +340,12 @@ def test_{{ service.client_name|snake_case }}_client_options_scopes(client_class ) @pytest.mark.parametrize("client_class,transport_class,transport_name", [ + {%- if 'grpc' in opts.transport %} ({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc"), - ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio") + ({{ service.async_client_name }}, transports.{{ service.grpc_asyncio_transport_name }}, "grpc_asyncio"), + {%- elif 'rest' in opts.transport %} + ({{ service.client_name }}, transports.{{ service.rest_transport_name }}, "rest"), + {%- endif %} ]) def test_{{ service.client_name|snake_case }}_client_options_credentials_file(client_class, transport_class, transport_name): # Check the case credentials file is provided. @@ -328,6 +364,7 @@ def test_{{ service.client_name|snake_case }}_client_options_credentials_file(cl quota_project_id=None, client_info=transports.base.DEFAULT_CLIENT_INFO, ) +{%- if 'grpc' in opts.transport %} def test_{{ service.client_name|snake_case }}_client_options_from_dict(): @@ -345,9 +382,10 @@ def test_{{ service.client_name|snake_case }}_client_options_from_dict(): quota_project_id=None, client_info=transports.base.DEFAULT_CLIENT_INFO, ) +{%- endif %} -{% for method in service.methods.values() -%} +{% for method in service.methods.values() if 'grpc' in opts.transport -%} def test_{{ method.name|snake_case }}(transport: str = 'grpc', request_type={{ method.input.ident }}): client = {{ service.client_name }}( credentials=credentials.AnonymousCredentials(), @@ -991,9 +1029,142 @@ def test_{{ method.name|snake_case }}_raw_page_lro(): {% endfor -%} {#- method in methods #} +{% for method in service.methods.values() if 'rest' in opts.transport -%} +def test_{{ method.name|snake_case }}_rest(transport: str = 'rest', request_type={{ method.input.ident }}): + client = {{ service.client_name }}( + credentials=credentials.AnonymousCredentials(), + transport=transport, + ) + + # Everything is optional in proto3 as far as the runtime is concerned, + # and we are mocking out the actual API, so just send an empty request. + request = request_type() + {% if method.client_streaming %} + requests = [request] + {% endif %} + + # Mock the http request call within the method and fake a response. + with mock.patch.object(Session, 'request') as req: + # Designate an appropriate value for the returned response. + {% if method.void -%} + return_value = None + {% elif method.lro -%} + return_value = operations_pb2.Operation(name='operations/spam') + {% elif method.server_streaming -%} + return_value = iter([{{ method.output.ident }}()]) + {% else -%} + return_value = {{ method.output.ident }}( + {%- for field in method.output.fields.values() %} + {{ field.name }}={{ field.mock_value }}, + {%- endfor %} + ) + {% endif -%} + + # Wrap the value into a proper Response obj + json_return_value = {{ method.output.ident }}.to_json(return_value) + response_value = Response() + response_value._content = json_return_value.encode('UTF-8') + req.return_value = response_value + {% if method.client_streaming %} + response = client.{{ method.name|snake_case }}(iter(requests)) + {% else %} + response = client.{{ method.name|snake_case }}(request) + {% endif %} + + {% if "next_page_token" in method.output.fields.values()|map(attribute='name') and not method.paged_result_field %} + {# Cheeser assertion to force code coverage for bad paginated methods #} + assert response.raw_page is response + {% endif %} + + # Establish that the response is the type that we expect. + {% if method.void -%} + assert response is None + {% else %} + assert isinstance(response, {{ method.client_output.ident }}) + {% for field in method.output.fields.values() -%} + {% if field.field_pb.type in [1, 2] -%} {# Use approx eq for floats -#} + assert math.isclose(response.{{ field.name }}, {{ field.mock_value }}, rel_tol=1e-6) + {% elif field.field_pb.type == 8 -%} {# Use 'is' for bools #} + assert response.{{ field.name }} is {{ field.mock_value }} + {% else -%} + assert response.{{ field.name }} == {{ field.mock_value }} + {% endif -%} + {% endfor %} + {% endif %} + + +def test_{{ method.name|snake_case }}_rest_from_dict(): + test_{{ method.name|snake_case }}_rest(request_type=dict) + + +def test_{{ method.name|snake_case }}_rest_flattened(): + client = {{ service.client_name }}( + credentials=credentials.AnonymousCredentials(), + ) + + # Mock the http request call within the method and fake a response. + with mock.patch.object(Session, 'request') as req: + # Designate an appropriate value for the returned response. + {% if method.void -%} + return_value = None + {% elif method.lro -%} + return_value = operations_pb2.Operation(name='operations/spam') + {% elif method.server_streaming -%} + return_value = iter([{{ method.output.ident }}()]) + {% else -%} + return_value = {{ method.output.ident }}() + {% endif %} + + # Wrap the value into a proper Response obj + json_return_value = {{ method.output.ident }}.to_json(return_value) + response_value = Response() + response_value._content = json_return_value.encode('UTF-8') + req.return_value = response_value + + # Call the method with a truthy value for each flattened field, + # using the keyword arguments to the method. + client.{{ method.name|snake_case }}( + {%- for field in method.flattened_fields.values() %} + {{ field.name }}={{ field.mock_value }}, + {%- endfor %} + ) + + # Establish that the underlying call was made with the expected + # request object values. + assert len(req.mock_calls) == 1 + _, args, _ = req.mock_calls[0] + {% for key, field in method.flattened_fields.items() -%}{%- if not field.oneof or field.proto3_optional %} + {% if field.ident|string() == 'timestamp.Timestamp' -%} + assert TimestampRule().to_proto(args[0].{{ key }}) == {{ field.mock_value }} + {% elif field.ident|string() == 'duration.Duration' -%} + assert DurationRule().to_proto(args[0].{{ key }}) == {{ field.mock_value }} + {% else -%} + {# TODO(yon-mg): 'is string' test not working as expected; Investigate #} + assert {% if field.mock_value is string %}str({{ field.mock_value|string() }}){%- else %}str({{ field.mock_value }}){%- endif %} in args[1] + {% endif %} + {% endif %}{% endfor %} + + +def test_{{ method.name|snake_case }}_rest_flattened_error(): + client = {{ service.client_name }}( + credentials=credentials.AnonymousCredentials(), + ) + + # Attempting to call a method with both a request object and flattened + # fields is an error. + with pytest.raises(ValueError): + client.{{ method.name|snake_case }}( + {{ method.input.ident }}(), + {%- for field in method.flattened_fields.values() %} + {{ field.name }}={{ field.mock_value }}, + {%- endfor %} + ) + + +{% endfor -%} def test_credentials_transport_error(): # It is an error to provide credentials and a transport instance. - transport = transports.{{ service.name }}GrpcTransport( + transport = transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport( credentials=credentials.AnonymousCredentials(), ) with pytest.raises(ValueError): @@ -1003,7 +1174,7 @@ def test_credentials_transport_error(): ) # It is an error to provide a credentials file and a transport instance. - transport = transports.{{ service.name }}GrpcTransport( + transport = transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport( credentials=credentials.AnonymousCredentials(), ) with pytest.raises(ValueError): @@ -1013,7 +1184,7 @@ def test_credentials_transport_error(): ) # It is an error to provide scopes and a transport instance. - transport = transports.{{ service.name }}GrpcTransport( + transport = transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport( credentials=credentials.AnonymousCredentials(), ) with pytest.raises(ValueError): @@ -1023,16 +1194,15 @@ def test_credentials_transport_error(): ) - def test_transport_instance(): # A client may be instantiated with a custom transport instance. - transport = transports.{{ service.name }}GrpcTransport( + transport = transports.{{ service.name }}{{ opts.transport[0].capitalize() }}Transport( credentials=credentials.AnonymousCredentials(), ) client = {{ service.client_name }}(transport=transport) assert client.transport is transport - +{% if 'grpc' in opts.transport %} def test_transport_get_channel(): # A client may be instantiated with a custom transport instance. transport = transports.{{ service.name }}GrpcTransport( @@ -1046,11 +1216,15 @@ def test_transport_get_channel(): ) channel = transport.grpc_channel assert channel - +{% endif %} @pytest.mark.parametrize("transport_class", [ + {%- if 'grpc' in opts.transport %} transports.{{ service.grpc_transport_name }}, - transports.{{ service.grpc_asyncio_transport_name }} + transports.{{ service.grpc_asyncio_transport_name }}, + {%- elif 'rest' in opts.transport %} + transports.{{ service.rest_transport_name }}, + {%- endif %} ]) def test_transport_adc(transport_class): # Test default credentials are used if not provided. @@ -1059,7 +1233,7 @@ def test_transport_adc(transport_class): transport_class() adc.assert_called_once() - +{% if 'grpc' in opts.transport %} def test_transport_grpc_default(): # A client should use the gRPC transport by default. client = {{ service.client_name }}( @@ -1069,7 +1243,7 @@ def test_transport_grpc_default(): client.transport, transports.{{ service.name }}GrpcTransport, ) - +{% endif %} def test_{{ service.name|snake_case }}_base_transport_error(): # Passing both a credentials object and credentials_file should raise an error @@ -1151,7 +1325,7 @@ def test_{{ service.name|snake_case }}_auth_adc(): quota_project_id=None, ) - +{% if 'grpc' in opts.transport %} def test_{{ service.name|snake_case }}_transport_auth_adc(): # If credentials and host are not provided, the transport class should use # ADC credentials. @@ -1164,6 +1338,7 @@ def test_{{ service.name|snake_case }}_transport_auth_adc(): {%- endfor %}), quota_project_id="octopus", ) +{% endif %} def test_{{ service.name|snake_case }}_host_no_port(): {% with host = (service.host|default('localhost', true)).split(':')[0] -%} @@ -1184,7 +1359,7 @@ def test_{{ service.name|snake_case }}_host_with_port(): assert client.transport._host == '{{ host }}:8000' {% endwith %} - +{% if 'grpc' in opts.transport %} def test_{{ service.name|snake_case }}_grpc_transport_channel(): channel = grpc.insecure_channel('http://localhost/') @@ -1334,6 +1509,7 @@ def test_{{ service.name|snake_case }}_grpc_lro_async_client(): assert transport.operations_client is transport.operations_client {% endif -%} +{% endif %} {# if grpc in opts #} {% with molluscs = cycler("squid", "clam", "whelk", "octopus", "oyster", "nudibranch", "cuttlefish", "mussel", "winkle", "nautilus", "scallop", "abalone") -%} {% for message in service.resource_messages|sort(attribute="resource_type") -%} @@ -1404,7 +1580,7 @@ def test_client_withDEFAULT_CLIENT_INFO(): prep.assert_called_once_with(client_info) -{% if opts.add_iam_methods %} +{% if opts.add_iam_methods and 'grpc' in opts.transport %} def test_set_iam_policy(transport: str = "grpc"): client = {{ service.client_name }}( credentials=credentials.AnonymousCredentials(), transport=transport, diff --git a/gapic/utils/case.py b/gapic/utils/case.py index f58aa4adc6..635d2945c5 100644 --- a/gapic/utils/case.py +++ b/gapic/utils/case.py @@ -21,7 +21,7 @@ def to_snake_case(s: str) -> str: This is provided to templates as the ``snake_case`` filter. Args: - s (str): The input string, provided in any sane case system. + s (str): The input string, provided in any sane case system without spaces. Returns: str: The string in snake case (and all lower-cased). @@ -53,7 +53,7 @@ def to_camel_case(s: str) -> str: This is provided to templates as the ``camel_case`` filter. Args: - s (str): The input string, provided in any sane case system + s (str): The input string, provided in any sane case system without spaces. Returns: str: The string in lower camel case. From 0f8a9033715b63334613a5317957b35e47682a16 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 21 Dec 2020 15:52:28 +0000 Subject: [PATCH 26/32] test: test async client generation --- tests/unit/generator/test_generator.py | 44 ++++++++++++++++++-------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index 97793e4433..27653ebf1f 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -116,7 +116,7 @@ def test_get_response_fails_invalid_file_paths(): assert "%proto" in ex_str and "%service" in ex_str -def test_get_response_ignores_unwanted_transports(): +def test_get_response_ignores_unwanted_transports_and_clients(): g = make_generator() with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt: lt.return_value = [ @@ -125,31 +125,49 @@ def test_get_response_ignores_unwanted_transports(): "foo/%service/transports/grpc.py.j2", "foo/%service/transports/__init__.py.j2", "foo/%service/transports/base.py.j2", + "foo/%service/async_client.py.j2", + "foo/%service/client.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 }}") + api_schema=make_api( + make_proto( + descriptor_pb2.FileDescriptorProto( + service=[ + descriptor_pb2.ServiceDescriptorProto( + name="SomeService"), + ] + ), + ) + ) + cgr = g.get_response( - api_schema=make_api( - make_proto( - descriptor_pb2.FileDescriptorProto( - service=[ - descriptor_pb2.ServiceDescriptorProto( - name="SomeService"), - ] - ), - ) - ), + api_schema=api_schema, opts=Options.build("transport=river+car") ) - - assert len(cgr.file) == 4 + assert len(cgr.file) == 5 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", + # Only generate async client with grpc transport + "foo/some_service/client.py", + } + + cgr = g.get_response( + api_schema=api_schema, + opts=Options.build("transport=grpc") + ) + assert len(cgr.file) == 5 + assert {i.name for i in cgr.file} == { + "foo/some_service/transports/grpc.py", + "foo/some_service/transports/__init__.py", + "foo/some_service/transports/base.py", + "foo/some_service/client.py", + "foo/some_service/async_client.py", } From cc5ae23cb2832fd0a151bff2747fae02eafabeaf Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Tue, 22 Dec 2020 22:07:28 +0000 Subject: [PATCH 27/32] fix: fixed reserved keyword bug, fixed bugs in gapic tests --- gapic/generator/generator.py | 7 +++++++ gapic/schema/wrappers.py | 7 ++++--- .../%sub/services/%service/client.py.j2 | 6 +++--- .../services/%service/transports/rest.py.j2 | 2 +- gapic/templates/noxfile.py.j2 | 4 ++-- .../%name_%version/%sub/test_%service.py.j2 | 20 +++++++++++++------ tests/unit/generator/test_generator.py | 2 +- 7 files changed, 32 insertions(+), 16 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 679b85176e..5c8bf4ac17 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -59,6 +59,13 @@ def __init__(self, opts: Options) -> None: self._env.filters["wrap"] = utils.wrap self._env.filters["coerce_response_name"] = coerce_response_name + # Add tests to determine type of expressions stored in strings + self._env.tests["str"] = lambda val: re.fullmatch( + r'\'.*\'|\".*\"', val) + self._env.tests["int"] = lambda val: re.fullmatch(r'[0-9]+', val) + self._env.tests["obj"] = lambda val: re.fullmatch( + r'\w+(\.\w+)*\(.*\)', val) + self._sample_configs = opts.sample_configs def get_response( diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 6f64348990..6d0b96a72e 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -412,7 +412,8 @@ def get_field(self, *field_path: str, # Get the first field in the path. first_field = field_path[0] - cursor = self.fields[first_field+('_' if first_field in utils.RESERVED_NAMES else '')] + cursor = self.fields[first_field + + ('_' if first_field in utils.RESERVED_NAMES else '')] # Base case: If this is the last field in the path, return it outright. if len(field_path) == 1: @@ -769,14 +770,14 @@ def http_opt(self) -> Optional[Dict[str, str]]: return answer @property - def path_params(self) -> Sequence[str]: + def path_params(self) -> Sequence[Field]: """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 re.findall(pattern, self.http_opt['url']) + return [self.input.get_field(field) for field in 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/client.py.j2 b/gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2 index 6d703fb2de..1977f3cfb7 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 @@ -389,18 +389,18 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): {% endif -%} {%- for key, field in method.flattened_fields.items() if not field.repeated and method.input.ident.package == method.ident.package %} if {{ field.name }} is not None: - request.{{ key }} = {{ field.name }} + request.{{ field.name }} = {{ field.name }} {%- endfor %} {# Map-y fields can be _updated_, however #} {%- for key, field in method.flattened_fields.items() if field.map and method.input.ident.package == method.ident.package %} if {{ field.name }}: - request.{{ key }}.update({{ field.name }}) + request.{{ field.name }}.update({{ field.name }}) {%- endfor %} {# And list-y fields can be _extended_ -#} {%- for key, field in method.flattened_fields.items() if field.repeated and not field.map and method.input.ident.package == method.ident.package %} if {{ field.name }}: - request.{{ key }}.extend({{ field.name }}) + request.{{ field.name }}.extend({{ field.name }}) {%- endfor %} {%- endif %} 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 4f44807583..3851ecc486 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 @@ -169,7 +169,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): url = 'https://{host}{{ method.http_opt['url'] }}'.format( host=self._host, {%- for field in method.path_params %} - {{ field }}=request.{{ field }}, + {{ field.field_pb.name }}=request.{{ field.name }}, {%- endfor %} ) diff --git a/gapic/templates/noxfile.py.j2 b/gapic/templates/noxfile.py.j2 index 5fde488f00..4a301477ba 100644 --- a/gapic/templates/noxfile.py.j2 +++ b/gapic/templates/noxfile.py.j2 @@ -6,7 +6,7 @@ import os import nox # type: ignore -@nox.session(python=['3.6', '3.7']) +@nox.session(python=['3.6', '3.7', '3.8']) def unit(session): """Run the unit test suite.""" @@ -20,7 +20,7 @@ def unit(session): '--cov-config=.coveragerc', '--cov-report=term', '--cov-report=html', - os.path.join('tests', 'unit',) + os.path.join('tests', 'unit', ''.join(session.posargs)) ) diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 4a309de52b..25ad182746 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -10,8 +10,10 @@ import math import pytest from proto.marshal.rules.dates import DurationRule, TimestampRule +{%- if 'rest' in opts.transport %} from requests import Response from requests.sessions import Session +{%- endif %} {# Import the service itself as well as every proto module that it imports. -#} {% filter sort_lines -%} @@ -1123,24 +1125,30 @@ def test_{{ method.name|snake_case }}_rest_flattened(): # Call the method with a truthy value for each flattened field, # using the keyword arguments to the method. + {%- for field in method.flattened_fields.values() if field.mock_value is obj %} + {{ field.name }} = {{ field.mock_value }} + {% endfor %} client.{{ method.name|snake_case }}( {%- for field in method.flattened_fields.values() %} - {{ field.name }}={{ field.mock_value }}, + {% if field.mock_value is obj %}{{ field.name }}={{ field.name }},{% else %}{{ field.name }}={{ field.mock_value }},{% endif %} {%- endfor %} ) # Establish that the underlying call was made with the expected # request object values. assert len(req.mock_calls) == 1 - _, args, _ = req.mock_calls[0] + _, http_call, http_params = req.mock_calls[0] + body = http_params.get('json') {% for key, field in method.flattened_fields.items() -%}{%- if not field.oneof or field.proto3_optional %} {% if field.ident|string() == 'timestamp.Timestamp' -%} - assert TimestampRule().to_proto(args[0].{{ key }}) == {{ field.mock_value }} + assert TimestampRule().to_proto(http_call[0].{{ key }}) == {{ field.mock_value }} {% elif field.ident|string() == 'duration.Duration' -%} - assert DurationRule().to_proto(args[0].{{ key }}) == {{ field.mock_value }} + assert DurationRule().to_proto(http_call[0].{{ key }}) == {{ field.mock_value }} {% else -%} - {# TODO(yon-mg): 'is string' test not working as expected; Investigate #} - assert {% if field.mock_value is string %}str({{ field.mock_value|string() }}){%- else %}str({{ field.mock_value }}){%- endif %} in args[1] + assert {% if field.mock_value is obj %}{{ field.ident }}.to_json({{ field.name }}, including_default_value_fields=False) + {%- elif field.mock_value is str %}{{ field.mock_value }} + {%- else %}str({{ field.mock_value }}) + {%- endif %} in http_call[1] + str(body) {% endif %} {% endif %}{% endfor %} diff --git a/tests/unit/generator/test_generator.py b/tests/unit/generator/test_generator.py index 27653ebf1f..3f66033d42 100644 --- a/tests/unit/generator/test_generator.py +++ b/tests/unit/generator/test_generator.py @@ -132,7 +132,7 @@ def test_get_response_ignores_unwanted_transports_and_clients(): with mock.patch.object(jinja2.Environment, "get_template") as gt: gt.return_value = jinja2.Template("Service: {{ service.name }}") - api_schema=make_api( + api_schema = make_api( make_proto( descriptor_pb2.FileDescriptorProto( service=[ From 34b4b5d2ab36e28e9f4f20032057299d0dcdddba Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 23 Dec 2020 02:19:22 +0000 Subject: [PATCH 28/32] fix: reverted bug causing change to , refactored template tests --- gapic/generator/generator.py | 8 ++-- gapic/schema/wrappers.py | 4 +- .../services/%service/transports/rest.py.j2 | 2 +- .../%name_%version/%sub/test_%service.py.j2 | 6 +-- gapic/utils/__init__.py | 6 +++ gapic/utils/checks.py | 42 +++++++++++++++++++ tests/unit/utils/test_checks.py | 30 +++++++++++++ 7 files changed, 87 insertions(+), 11 deletions(-) create mode 100644 gapic/utils/checks.py create mode 100644 tests/unit/utils/test_checks.py diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index 5c8bf4ac17..c101bc57f8 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -60,11 +60,9 @@ def __init__(self, opts: Options) -> None: self._env.filters["coerce_response_name"] = coerce_response_name # Add tests to determine type of expressions stored in strings - self._env.tests["str"] = lambda val: re.fullmatch( - r'\'.*\'|\".*\"', val) - self._env.tests["int"] = lambda val: re.fullmatch(r'[0-9]+', val) - self._env.tests["obj"] = lambda val: re.fullmatch( - r'\w+(\.\w+)*\(.*\)', val) + self._env.tests["int"] = utils.is_int + self._env.tests["call"] = utils.is_call + self._env.tests["str"] = utils.is_str self._sample_configs = opts.sample_configs diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 6d0b96a72e..2b321c8872 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -770,14 +770,14 @@ def http_opt(self) -> Optional[Dict[str, str]]: return answer @property - def path_params(self) -> Sequence[Field]: + def path_params(self) -> Sequence[str]: """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 [self.input.get_field(field) for field in re.findall(pattern, self.http_opt['url'])] + 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 3851ecc486..338bfcbaff 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 @@ -169,7 +169,7 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): url = 'https://{host}{{ method.http_opt['url'] }}'.format( host=self._host, {%- for field in method.path_params %} - {{ field.field_pb.name }}=request.{{ field.name }}, + {{ field }}=request.{{ method.input.get_field(field).name }}, {%- endfor %} ) diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index 25ad182746..f3250656bb 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -1125,12 +1125,12 @@ def test_{{ method.name|snake_case }}_rest_flattened(): # Call the method with a truthy value for each flattened field, # using the keyword arguments to the method. - {%- for field in method.flattened_fields.values() if field.mock_value is obj %} + {%- for field in method.flattened_fields.values() if field.mock_value is call %} {{ field.name }} = {{ field.mock_value }} {% endfor %} client.{{ method.name|snake_case }}( {%- for field in method.flattened_fields.values() %} - {% if field.mock_value is obj %}{{ field.name }}={{ field.name }},{% else %}{{ field.name }}={{ field.mock_value }},{% endif %} + {% if field.mock_value is call %}{{ field.name }}={{ field.name }},{% else %}{{ field.name }}={{ field.mock_value }},{% endif %} {%- endfor %} ) @@ -1145,7 +1145,7 @@ def test_{{ method.name|snake_case }}_rest_flattened(): {% elif field.ident|string() == 'duration.Duration' -%} assert DurationRule().to_proto(http_call[0].{{ key }}) == {{ field.mock_value }} {% else -%} - assert {% if field.mock_value is obj %}{{ field.ident }}.to_json({{ field.name }}, including_default_value_fields=False) + assert {% if field.mock_value is call %}{{ field.ident }}.to_json({{ field.name }}, including_default_value_fields=False) {%- elif field.mock_value is str %}{{ field.mock_value }} {%- else %}str({{ field.mock_value }}) {%- endif %} in http_call[1] + str(body) diff --git a/gapic/utils/__init__.py b/gapic/utils/__init__.py index 9729591c3c..59cfb4a87e 100644 --- a/gapic/utils/__init__.py +++ b/gapic/utils/__init__.py @@ -15,6 +15,9 @@ 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.checks import is_int +from gapic.utils.checks import is_call +from gapic.utils.checks import is_str from gapic.utils.code import empty from gapic.utils.code import nth from gapic.utils.code import partition @@ -32,6 +35,9 @@ 'cached_property', 'doc', 'empty', + 'is_int', + 'is_call', + 'is_str', 'nth', 'Options', 'partition', diff --git a/gapic/utils/checks.py b/gapic/utils/checks.py new file mode 100644 index 0000000000..8d3f11703a --- /dev/null +++ b/gapic/utils/checks.py @@ -0,0 +1,42 @@ +# Copyright 2018 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import re + + +def is_str(expr: str) -> bool: + """Determine if the expression stored in expr is a string literal. + + Args: + expr (str): An expression of any type stored in a string. + """ + return re.fullmatch(r'\'.*\'|\".*\"', expr) + + +def is_call(expr: str) -> bool: + """Determine if the expression stored in expr is a constructor or method call. + + Args: + expr (str): An expression of any type stored in a string. + """ + return re.fullmatch(r'\w+(\.\w+)*\(.*\)', expr) + + +def is_int(expr: str) -> bool: + """Determine if the expression stored in expr is an int. + + Args: + expr (str): An expression of any type stored in a string. + """ + return re.fullmatch(r'[0-9]+', expr) diff --git a/tests/unit/utils/test_checks.py b/tests/unit/utils/test_checks.py new file mode 100644 index 0000000000..c5b33ca556 --- /dev/null +++ b/tests/unit/utils/test_checks.py @@ -0,0 +1,30 @@ +# Copyright 2018 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from gapic.utils import checks + + +def test_is_str(): + assert checks.is_str("'some string'") + assert not checks.is_str("234") + + +def test_is_int(): + assert checks.is_int("234") + assert not checks.is_str("23.4") + + +def test_is_call(): + assert checks.is_call("module.foo('bar')") + assert not checks.is_call("{'some':'dict'}") From 06bfd2ee11fc56a0789f243c6f22cb1d01f40a2c Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 23 Dec 2020 02:24:19 +0000 Subject: [PATCH 29/32] fix: return type mismatch --- gapic/utils/checks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/utils/checks.py b/gapic/utils/checks.py index 8d3f11703a..b37a55a3bb 100644 --- a/gapic/utils/checks.py +++ b/gapic/utils/checks.py @@ -21,7 +21,7 @@ def is_str(expr: str) -> bool: Args: expr (str): An expression of any type stored in a string. """ - return re.fullmatch(r'\'.*\'|\".*\"', expr) + return bool(re.fullmatch(r'\'.*\'|\".*\"', expr)) def is_call(expr: str) -> bool: @@ -30,7 +30,7 @@ def is_call(expr: str) -> bool: Args: expr (str): An expression of any type stored in a string. """ - return re.fullmatch(r'\w+(\.\w+)*\(.*\)', expr) + return bool(re.fullmatch(r'\w+(\.\w+)*\(.*\)', expr)) def is_int(expr: str) -> bool: @@ -39,4 +39,4 @@ def is_int(expr: str) -> bool: Args: expr (str): An expression of any type stored in a string. """ - return re.fullmatch(r'[0-9]+', expr) + return bool(re.fullmatch(r'[0-9]+', expr)) From 53771e99a91b331bb0987bb7c6d130d88b46d600 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Wed, 23 Dec 2020 02:57:20 +0000 Subject: [PATCH 30/32] fix: reserved keyword issue in --- gapic/schema/wrappers.py | 1 + .../%name_%version/%sub/services/%service/client.py.j2 | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 2b321c8872..0f2e1fde48 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -807,6 +807,7 @@ def filter_fields(sig: str) -> Iterable[Tuple[str, Field]]: continue name = f.strip() field = self.input.get_field(*name.split('.')) + name += '_' if field.field_pb.name in utils.RESERVED_NAMES else '' if cross_pkg_request and not field.is_primitive: # This is not a proto-plus wrapped message type, # and setting a non-primitive field directly is verboten. 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 1977f3cfb7..6d703fb2de 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 @@ -389,18 +389,18 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): {% endif -%} {%- for key, field in method.flattened_fields.items() if not field.repeated and method.input.ident.package == method.ident.package %} if {{ field.name }} is not None: - request.{{ field.name }} = {{ field.name }} + request.{{ key }} = {{ field.name }} {%- endfor %} {# Map-y fields can be _updated_, however #} {%- for key, field in method.flattened_fields.items() if field.map and method.input.ident.package == method.ident.package %} if {{ field.name }}: - request.{{ field.name }}.update({{ field.name }}) + request.{{ key }}.update({{ field.name }}) {%- endfor %} {# And list-y fields can be _extended_ -#} {%- for key, field in method.flattened_fields.items() if field.repeated and not field.map and method.input.ident.package == method.ident.package %} if {{ field.name }}: - request.{{ field.name }}.extend({{ field.name }}) + request.{{ key }}.extend({{ field.name }}) {%- endfor %} {%- endif %} From 85a983db573129bdcfa7fd6adcc2712298c15f35 Mon Sep 17 00:00:00 2001 From: Yonatan Getahun Date: Mon, 28 Dec 2020 18:27:23 +0000 Subject: [PATCH 31/32] fix: replace bad regex checks with checks against field_pb type --- gapic/generator/generator.py | 5 ++-- .../%name_%version/%sub/test_%service.py.j2 | 8 ++--- gapic/utils/__init__.py | 10 +++---- gapic/utils/checks.py | 29 +++++++------------ tests/unit/utils/test_checks.py | 28 ++++++++++-------- 5 files changed, 36 insertions(+), 44 deletions(-) diff --git a/gapic/generator/generator.py b/gapic/generator/generator.py index c101bc57f8..d6eb3aca9d 100644 --- a/gapic/generator/generator.py +++ b/gapic/generator/generator.py @@ -60,9 +60,8 @@ def __init__(self, opts: Options) -> None: self._env.filters["coerce_response_name"] = coerce_response_name # Add tests to determine type of expressions stored in strings - self._env.tests["int"] = utils.is_int - self._env.tests["call"] = utils.is_call - self._env.tests["str"] = utils.is_str + self._env.tests["str_field_pb"] = utils.is_str_field_pb + self._env.tests["msg_field_pb"] = utils.is_msg_field_pb self._sample_configs = opts.sample_configs diff --git a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 index f3250656bb..f912c479a3 100644 --- a/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 +++ b/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2 @@ -1125,12 +1125,12 @@ def test_{{ method.name|snake_case }}_rest_flattened(): # Call the method with a truthy value for each flattened field, # using the keyword arguments to the method. - {%- for field in method.flattened_fields.values() if field.mock_value is call %} + {%- for field in method.flattened_fields.values() if field.field_pb is msg_field_pb %} {{ field.name }} = {{ field.mock_value }} {% endfor %} client.{{ method.name|snake_case }}( {%- for field in method.flattened_fields.values() %} - {% if field.mock_value is call %}{{ field.name }}={{ field.name }},{% else %}{{ field.name }}={{ field.mock_value }},{% endif %} + {% if field.field_pb is msg_field_pb %}{{ field.name }}={{ field.name }},{% else %}{{ field.name }}={{ field.mock_value }},{% endif %} {%- endfor %} ) @@ -1145,8 +1145,8 @@ def test_{{ method.name|snake_case }}_rest_flattened(): {% elif field.ident|string() == 'duration.Duration' -%} assert DurationRule().to_proto(http_call[0].{{ key }}) == {{ field.mock_value }} {% else -%} - assert {% if field.mock_value is call %}{{ field.ident }}.to_json({{ field.name }}, including_default_value_fields=False) - {%- elif field.mock_value is str %}{{ field.mock_value }} + assert {% if field.field_pb is msg_field_pb %}{{ field.ident }}.to_json({{ field.name }}, including_default_value_fields=False) + {%- elif field.field_pb is str_field_pb %}{{ field.mock_value }} {%- else %}str({{ field.mock_value }}) {%- endif %} in http_call[1] + str(body) {% endif %} diff --git a/gapic/utils/__init__.py b/gapic/utils/__init__.py index 59cfb4a87e..98d31c283f 100644 --- a/gapic/utils/__init__.py +++ b/gapic/utils/__init__.py @@ -15,9 +15,8 @@ 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.checks import is_int -from gapic.utils.checks import is_call -from gapic.utils.checks import is_str +from gapic.utils.checks import is_msg_field_pb +from gapic.utils.checks import is_str_field_pb from gapic.utils.code import empty from gapic.utils.code import nth from gapic.utils.code import partition @@ -35,9 +34,8 @@ 'cached_property', 'doc', 'empty', - 'is_int', - 'is_call', - 'is_str', + 'is_msg_field_pb', + 'is_str_field_pb', 'nth', 'Options', 'partition', diff --git a/gapic/utils/checks.py b/gapic/utils/checks.py index b37a55a3bb..a4f7ec7445 100644 --- a/gapic/utils/checks.py +++ b/gapic/utils/checks.py @@ -1,4 +1,4 @@ -# Copyright 2018 Google LLC +# Copyright 2020 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -12,31 +12,22 @@ # See the License for the specific language governing permissions and # limitations under the License. -import re +from google.protobuf.descriptor_pb2 import FieldDescriptorProto -def is_str(expr: str) -> bool: - """Determine if the expression stored in expr is a string literal. +def is_str_field_pb(field_pb: FieldDescriptorProto) -> bool: + """Determine if field_pb is of type string. Args: - expr (str): An expression of any type stored in a string. + field (Field): The input field as a FieldDescriptorProto """ - return bool(re.fullmatch(r'\'.*\'|\".*\"', expr)) + return field_pb.type == FieldDescriptorProto.TYPE_STRING -def is_call(expr: str) -> bool: - """Determine if the expression stored in expr is a constructor or method call. +def is_msg_field_pb(field_pb: FieldDescriptorProto) -> bool: + """Determine if field_pb is of type Message. Args: - expr (str): An expression of any type stored in a string. + field (Field): The input field as a FieldDescriptorProto. """ - return bool(re.fullmatch(r'\w+(\.\w+)*\(.*\)', expr)) - - -def is_int(expr: str) -> bool: - """Determine if the expression stored in expr is an int. - - Args: - expr (str): An expression of any type stored in a string. - """ - return bool(re.fullmatch(r'[0-9]+', expr)) + return field_pb.type == FieldDescriptorProto.TYPE_MESSAGE diff --git a/tests/unit/utils/test_checks.py b/tests/unit/utils/test_checks.py index c5b33ca556..32d5b33b49 100644 --- a/tests/unit/utils/test_checks.py +++ b/tests/unit/utils/test_checks.py @@ -1,4 +1,4 @@ -# Copyright 2018 Google LLC +# Copyright 2020 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -13,18 +13,22 @@ # limitations under the License. from gapic.utils import checks +from test_utils.test_utils import make_field, make_message -def test_is_str(): - assert checks.is_str("'some string'") - assert not checks.is_str("234") +def test_is_str_field_pb(): + msg_field = make_field('msg_field', message=make_message('test_msg')) + str_field = make_field('str_field', type=9) + int_field = make_field('int_field', type=5) + assert not checks.is_str_field_pb(msg_field.field_pb) + assert checks.is_str_field_pb(str_field.field_pb) + assert not checks.is_str_field_pb(int_field.field_pb) -def test_is_int(): - assert checks.is_int("234") - assert not checks.is_str("23.4") - - -def test_is_call(): - assert checks.is_call("module.foo('bar')") - assert not checks.is_call("{'some':'dict'}") +def test_is_msg_field_pb(): + msg_field = make_field('msg_field', message=make_message('test_msg')) + str_field = make_field('str_field', type=9) + int_field = make_field('int_field', type=5) + assert checks.is_msg_field_pb(msg_field.field_pb) + assert not checks.is_msg_field_pb(str_field.field_pb) + assert not checks.is_msg_field_pb(int_field.field_pb) From 638a6395811e7223221e780c00214d828ce152a3 Mon Sep 17 00:00:00 2001 From: yon-mg <71726126+yon-mg@users.noreply.github.com> Date: Wed, 30 Dec 2020 15:02:16 -0600 Subject: [PATCH 32/32] Update gapic/templates/noxfile.py.j2 Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com> --- gapic/templates/noxfile.py.j2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/templates/noxfile.py.j2 b/gapic/templates/noxfile.py.j2 index 4a301477ba..6d39ced1ad 100644 --- a/gapic/templates/noxfile.py.j2 +++ b/gapic/templates/noxfile.py.j2 @@ -6,7 +6,7 @@ import os import nox # type: ignore -@nox.session(python=['3.6', '3.7', '3.8']) +@nox.session(python=['3.6', '3.7', '3.8', '3.9']) def unit(session): """Run the unit test suite."""