feat: add grpc transcoding, clean up unimplemented rest method/test generation #776
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #776 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 27 +1
Lines 1608 1642 +34
Branches 328 333 +5
=========================================
+ Hits 1608 1642 +34
Continue to review full report at Codecov.
|
|
|
||
| for http_rule in http_options: | ||
| try: | ||
| method, uri = next((method, uri) for method,uri in [ |
There was a problem hiding this comment.
Formatting nit: for method, uri
| method, uri = next((method, uri) for method,uri in [ | ||
| ('get',http_rule.get), | ||
| ('put',http_rule.put), | ||
| ('post',http_rule.post), | ||
| ('delete',http_rule.delete), | ||
| ('patch',http_rule.patch), | ||
| ('custom.path',http_rule.custom.path), | ||
| ] if uri | ||
| ) | ||
| except StopIteration: | ||
| continue |
There was a problem hiding this comment.
I think it's better not to catch a StopIteration and instead use explicit control flow.
This is a fairly common pattern when using next:
thing = next((generator_expression_or_iterable), None) # Or (None, None) if detupling
if not thing:
continue # Or do some other action| @property | ||
| def {{ method.name|snake_case }}(self) -> Callable[ | ||
| [{{ method.input.ident }}], | ||
| {{ method.output.ident }}]: | ||
| return self._{{ method.name|snake_case }} |
There was a problem hiding this comment.
Why the extra indirection? Is it removable?
| # Jsonify the query params | ||
| query_params = json.loads({{ method.input.ident }}.to_json( | ||
| {{ method.input.ident }}(transcoded_request['query_params']), | ||
| including_default_value_fields=False, | ||
| use_integers_for_enums=False | ||
| )) |
There was a problem hiding this comment.
Unless I'm misunderstanding something, this is turning the transcoded request back into a Request message object, turning it into a JSON string, and then turning it back into a python dict. Is that correct? Can we use to_dict instead of the indirection?
If the above is correct, can you add some comments describing the dataflow? If it's not correct, then could you describe what the dataflow is?
There was a problem hiding this comment.
for the record: this is actually not redundant, provided we want to rely on proto+ to do the proper case conversion. to_dict doesn't do the conversion from snake_case to camelCase, only to_json does that. But the result of to_json is a string that can't be used as-is to construct the query string.
| @@ -208,9 +199,38 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): | |||
|
|
|||
| # Return the response | |||
| return {{ method.output.ident }}.from_json(response.content) | |||
| {%- else %} | |||
|
|
|||
| # Returh the response | |||
There was a problem hiding this comment.
Nit: Return the response
|
|
||
| <<<<<<< HEAD |
| {%- endif %} in http_call[1] + str(body) + str(query_params.values()) | ||
| {% endif %} | ||
| {% endif %}{% endfor %} | ||
| ======= |
| @@ -1144,11 +1178,13 @@ def test_{{ method.name|snake_case }}_rest_flattened(): | |||
| {%- endif %} in http_call[1] + str(body) | |||
| {% endif %} | |||
| {% endif %}{% endfor %} | |||
| >>>>>>> upstream/master | |||
|
Hi @yon-mg , I'm going to close this PR due to inactivity but please feel free to re-open it. |
|
This is still needed for REGAPIC Python. Please do not close it. |
This pr introduces usage of grpc transcoding and is reliant on changes made to to,
googleapis/python-api-core#146.
With this implementation of grpc transcoding, there are a few more changes that need to be made to ensure Showcase tests can run correctly. LRO rpcs, streaming rpcs and rpcs without
google.api.httpannotations need to also be handled. In this pr they are not implemented fully and instead rather raise errors if used in a generated client. The intention is to implement them at a later time and remove them as temporary blockers to push the grpc transcoding changes.