-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add interceptor-like functionality to REST transport #1142
feat: add interceptor-like functionality to REST transport #1142
Conversation
Interceptors are a gRPC feature that wraps rpcs in continuation-passing-style pre and post method custom functions. These can be used e.g. for logging, local caching, and tweaking metadata. This PR adds interceptor like functionality to the REST transport in generated GAPICs. The REST transport interceptors differ in a few ways: 1) They are not continuations. For each method there is a slot for a "pre"function, and for each method with a non-empty return there is a slot for a "post" function. 2) There is always an interceptor for each method. The default simply does nothing. 3) Existing gRPC interceptors and the new REST interceptors are not composable or interoperable.
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/transports/rest.py.j2
Show resolved
Hide resolved
@@ -233,7 +292,7 @@ class {{service.name}}RestTransport({{service.name}}Transport): | |||
}, | |||
{% endfor %}{# rule in method.http_options #} | |||
] | |||
|
|||
request, metadata = self._interceptor.pre_{{ method.name|snake_case }}(request, metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I imagined we'd be able to pass a list of interceptors that could be chained together, but understand that requires additional architecture here. We can handle the chaining within the body of the pre_
/post_
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that's easily done by something like this:
class InterceptorChainer:
def __init__(self, chain):
assert all(isinstance(i, RestInterceptor) for i in chain)
# Make our own copy to prevent external modification
self.chain = list(chain)
def __getattr__(self, name):
if name.startswith("pre_"):
def pre(request, metadata):
for i in self.chain:
request, metadata = getattr(i, name)(request, metadata)
return request, metadata
return pre
elif name.startswith("post_"):
def post(response):
for i in self.chain:
response = getattr(i, name)(response)
return response
return post
else:
raise AttributeError(f"No such attribute: {name}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Did you have a chance to test the new interceptor logic besides unit tests? I.e. maybe generate a client (compute, for example) and try adding custom interceptors (which would output something to stdout) there and make a call? I.e. some sort of integration end-to-end test?
@@ -39,6 +39,7 @@ from google.api_core import grpc_helpers_async | |||
from google.api_core import path_template | |||
{% if service.has_lro %} | |||
from google.api_core import future | |||
from google.api_core import operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a stupid question, but whasn't this required before? I.e. it is logical to assume that {% if service.has_lro %}
that operation should be imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a different operation module :P
@@ -1515,6 +1516,57 @@ def test_{{ method_name }}_rest_unset_required_fields(): | |||
{% endif %}{# required_fields #} | |||
|
|||
|
|||
{% if not (method.server_streaming or method.client_streaming) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just in case, do bidirectional streaming methods have their own flag or are signified by both server_streaming and client_streaming being true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your second guess is correct: bidirectional streaming methods have method.server_streaming
and method.client_streaming
both True.
Manual verification of the interceptor functionality with TextToSpeech has been done. Both pre and post interceptor calls fire. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
let's not forget to fix the showase CI
Interceptors are a gRPC feature that wraps rpcs in
continuation-passing-style pre and post method custom functions.
These can be used e.g. for logging, local caching, and tweaking
metadata.
This PR adds interceptor like functionality to the REST transport in
generated GAPICs.
The REST transport interceptors differ in a few ways:
"pre" function, and for each method with a non-empty return there is a
slot for a "post" function.
does nothing.
composable or interoperable.