feat: add rest transport generation for clients with optional flag #688

yon-mg · 2020-11-02T17:59:52Z

This pr adds traditional rest/json transport to GAPIC clients. As of now, this does not yet handle all cases and there may be bugs in the generated client. As such, there is now a --transport flag set to 'grpc' by default so any new additions won't be reflected in generated clients.

software-dov

First pass, some broad outlines and some nitpicking.

I'm more in favor of naming the new transport and its module "Http" instead of Rest; it is possible to do protobuf over http, and it's more immediately obvious to someone swimming in the code what it means.

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

software-dov · 2020-11-02T19:09:34Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+
+
+    {%- for method in service.methods.values() %}
+    {# TODO(yonmg): consider using enums #}


+1 for using enums.

software-dov · 2020-11-02T19:11:04Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+        {%- 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 %}


I'm leaning towards anything that involves state modification living in the generator datatypes. It's easier to test and easier to change if we need to.

I'm not sure I understand what you mean by generator datatypes. Could you clarify?

The gapic/schema/wrappers.py:{MessageType,Service,Method} classes. The auxiliary datastructures that the generate creates before iterating over the templates.

busunkim96 · 2020-11-02T20:46:35Z

Is the REST transport going to be added in one PR? If not, can we maintain a long-running branch and merge to it instead?

The microgenerator is now relied on by Cloud clients at GA stability so I'd prefer not to to surface this until it is somewhat stable.

yon-mg · 2020-11-02T22:02:54Z

Is the REST transport going to be added in one PR? If not, can we maintain a long-running branch and merge to it instead?

The microgenerator is now relied on by Cloud clients at GA stability so I'd prefer not to to surface this until it is somewhat stable.

This sounds reasonable to me. I'll defer to the other reviewers on what the best practice should be. @software-dov @vam-google @vchudnov-g

software-dov · 2020-11-02T23:14:05Z

If it's not too much work, I think I'd be more in favor of feature toggles and small PRs instead of having a long-running branch. It's easier to do merging. If that seems like too much work, I'm okay with a long-running branch.

yon-mg · 2020-11-06T21:31:27Z

I'm more in favor of naming the new transport and its module "Http" instead of Rest; it is possible to do protobuf over http, and it's more immediately obvious to someone swimming in the code what it means.

I agree with this somewhat but for consistency across other languages, flag arguments and regapic design specs, I think it's more appropriate to name it Rest. In addition, it's because it's possible to do protobuf over http that naming it such may be ambiguous. Rest seems to me more clearly defined.

software-dov

Looks good, just a few things.

software-dov · 2020-11-06T21:36:56Z

gapic/utils/options.py

@@ -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?


+1 for using an enum

gapic/utils/options.py

gapic/generator/generator.py

gapic/schema/wrappers.py

codecov · 2020-11-11T14:38:45Z

Codecov Report

Merging #688 (b5c6d06) into master (b85cbd4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #688   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1560      1578   +18     
  Branches       316       320    +4     
=========================================
+ Hits          1560      1578   +18

Impacted Files	Coverage Δ
gapic/generator/generator.py	`100.00% <100.00%> (ø)`
gapic/schema/wrappers.py	`100.00% <100.00%> (ø)`
gapic/utils/options.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b85cbd4...b5c6d06. Read the comment docs.

software-dov

LGTM, just two minor nits.

software-dov · 2020-11-10T18:02:54Z

tests/unit/generator/test_generator.py

+        ]
+        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(


Formatting nit: putting this on a new line makes it slightly easier to read the entire thing.

software-dov · 2020-11-11T21:40:02Z

gapic/generator/generator.py

+                        'transport' in template_name
+                        and not self._is_desired_transport(template_name, opts)


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

software-dov · 2020-11-11T21:45:04Z

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


Is Set used somewhere?

vam-google

LGTM
All the comments are minor. Please address them and then push the changes (please use squash and merge button). Thanks @software-dov for sharing your expertise in Python review!

vam-google · 2020-11-12T19:01:47Z

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 #}


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

vam-google · 2020-11-12T19:04:35Z

gapic/schema/wrappers.py

@@ -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


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

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

vam-google · 2020-11-12T19:05:58Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2

@@ -220,13 +223,13 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
        client.
        """
        # Sanity check: Only create a new client if we do not already have one.
-        if 'operations_client' not in self.__dict__:
-            self.__dict__['operations_client'] = operations_v1.OperationsClient(
+        if self._operations_client is None:


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

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

vam-google · 2020-11-12T19:07:05Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2

@@ -172,7 +175,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
                       **kwargs) -> grpc.Channel:
        """Create and return a gRPC channel object.
        Args:
-            address (Optionsl[str]): The host for the channel to use.
+            address (Optional[str]): The host for the channel to use.


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

I approve of fixing typos on the fly!

vam-google · 2020-11-12T19:07:39Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2

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


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

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

+1 for consistency

vam-google · 2020-11-12T19:09:26Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+__all__ = (
+    '{{ service.name }}RestTransport',
+)
+{% endblock %}


Please add new line

vchudnov-g

Looks good. Only minor comments. Nice job! Very excited to have this in!

vchudnov-g · 2020-11-12T20:24:56Z

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 #}


vchudnov-g · 2020-11-12T20:25:35Z

gapic/cli/generate.py

@@ -42,7 +42,6 @@ def generate(

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


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

vchudnov-g · 2020-11-12T20:37:29Z

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


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

vchudnov-g · 2020-11-12T21:08:11Z

gapic/schema/wrappers.py

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


For clarity I would suggest

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

vchudnov-g · 2020-11-12T21:19:17Z

gapic/schema/wrappers.py

+
+        http_method = http[0]
+        answer: Dict[str, str] = {
+            'method': http_method[0].name,


Based on our offline discussion, let's s/'method'/'verb'/, since "method" is overloaded (service method, object method, HTTP method)

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

vchudnov-g · 2020-11-12T21:38:49Z

gapic/schema/wrappers.py

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


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

gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2

gapic/utils/options.py

vchudnov-g · 2020-11-12T21:53:56Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2

@@ -172,7 +175,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
                       **kwargs) -> grpc.Channel:
        """Create and return a gRPC channel object.
        Args:
-            address (Optionsl[str]): The host for the channel to use.
+            address (Optional[str]): The host for the channel to use.


I approve of fixing typos on the fly!

gapic/schema/wrappers.py

🤖 I have created a release \*beep\* \*boop\* --- ## [0.36.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.35.11...v0.36.0) (2020-11-14) ### Features * add rest transport generation for clients with optional transport flag ([#688](https://www.github.com/googleapis/gapic-generator-python/issues/688)) ([af59c2c](https://www.github.com/googleapis/gapic-generator-python/commit/af59c2c3c3d6b7e1f626c3fbc2c03f99ca31b4a4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).

feat: add rest transport generation for clients

c1a6f89

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 2, 2020

yon-mg marked this pull request as ready for review November 2, 2020 18:09

yon-mg requested a review from a team as a code owner November 2, 2020 18:09

yon-mg marked this pull request as draft November 2, 2020 18:17

yon-mg added 2 commits November 2, 2020 18:47

feat: add rest transport generation for clients

bd7c32d

Merge branch 'master' of github.com:yon-mg/gapic-generator-python

cd3a7f1

yon-mg requested review from software-dov, vam-google and vchudnov-g November 2, 2020 18:57

software-dov reviewed Nov 2, 2020

View reviewed changes

yon-mg changed the title ~~feat: add rest transport generation for clients~~ feat WIP: add rest transport generation for clients Nov 2, 2020

yon-mg changed the title ~~feat WIP: add rest transport generation for clients~~ feat[WIP]: add rest transport generation for clients Nov 2, 2020

software-dov requested a review from busunkim96 November 3, 2020 18:36

yon-mg added 2 commits November 6, 2020 19:42

feat: add transport flag

b41db31

refactor: moved template logic outside

ce69e7d

yon-mg requested a review from software-dov November 6, 2020 21:17

software-dov reviewed Nov 6, 2020

View reviewed changes

fix: small fixes in transport option logic

89fa08b

software-dov reviewed Nov 9, 2020

View reviewed changes

gapic/generator/generator.py Outdated Show resolved Hide resolved

gapic/schema/wrappers.py Outdated Show resolved Hide resolved

yon-mg added 2 commits November 9, 2020 22:59

test: added unit test for transport flag

86f3a9c

test: add unit test for http option method

234cc09

yon-mg changed the title ~~feat[WIP]: add rest transport generation for clients~~ feat(WIP): add rest transport generation for clients Nov 11, 2020

yon-mg added 3 commits November 11, 2020 14:01

test: add unit test for http option method branch

35ac276

fix: fix import paths

c8155a5

fix: style check issues

55a815c

fix: more style check issues

ac0bdef

yon-mg requested a review from software-dov November 11, 2020 14:42

yon-mg changed the title ~~feat(WIP): add rest transport generation for clients~~ feat: add rest transport generation for clients Nov 11, 2020

yon-mg marked this pull request as ready for review November 11, 2020 19:44

software-dov reviewed Nov 11, 2020

View reviewed changes

software-dov approved these changes Nov 11, 2020

View reviewed changes

vam-google approved these changes Nov 12, 2020

View reviewed changes

vchudnov-g approved these changes Nov 12, 2020

View reviewed changes

yon-mg added 3 commits November 13, 2020 20:04

fix: addressing pr reviews

e79e8c7

fix: typo in test_method

824ad11

fix: style check fixes

2d11e19

yon-mg changed the title ~~feat: add rest transport generation for clients~~ feat: add rest transport generation for clients with optional flag Nov 14, 2020

Merge branch 'master' into master

b5c6d06

yon-mg merged commit af59c2c into googleapis:master Nov 14, 2020

software-dov mentioned this pull request Nov 16, 2020

feat: generator options are parsed into a standalone struct googleapis/gapic-generator-go#479

Merged

busunkim96 linked an issue Jan 6, 2021 that may be closed by this pull request

Re-add HTTP transport and make it the fallback #267

Closed

yon-mg mentioned this pull request Jan 28, 2021

showcase unit tests missing for rest transport #744

Closed

This was referenced Jan 18, 2022

chore(master): release reverted #1133

Merged

chore(master): release 0.39.1 #1146

Closed

This was referenced Jan 26, 2022

chore(main): release 0.39.1 #1160

Closed

chore(main): release 0.39.1 #1162

Closed

chore(main): release 0.61.0 #1163

Merged



		{%- for method in service.methods.values() %}
		{# TODO(yonmg): consider using enums #}

		'transport' in template_name
		and not self._is_desired_transport(template_name, opts)

		@@ -42,7 +42,6 @@ def generate(

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

feat: add rest transport generation for clients with optional flag #688

feat: add rest transport generation for clients with optional flag #688

Conversation

yon-mg commented Nov 2, 2020 • edited by vam-google Loading

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busunkim96 commented Nov 2, 2020

yon-mg commented Nov 2, 2020

software-dov commented Nov 2, 2020

yon-mg commented Nov 6, 2020

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg commented Nov 2, 2020 •

edited by vam-google

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

yon-mg Nov 12, 2020 •

edited

Loading