Skip to content
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

[Question] Valid to call super().intercept for method call handling in async #27

Open
chamini2 opened this issue Dec 13, 2022 · 1 comment

Comments

@chamini2
Copy link

I have the following interceptor

class AuthInterceptor(AsyncServerInterceptor):
    async def intercept(
        self,
        method: Callable,
        request: Any,
        context: async_grpc.ServicerContext,
        method_name: str,
    ):
        try:
            await auth_check(context)

            # Leave to pacakge the method handling
            return await super().intercept(method, request, context, method_name)

        except TypeError as err:
            raise GrpcException('\n'.join(err.args), grpc.StatusCode.UNAUTHENTICATED)
        except RuntimeError as err:
            raise GrpcException('\n'.join(err.args), grpc.StatusCode.UNAUTHENTICATED)

And it seems to work fine for the error and success cases I tested. It made sense to me since AsyncServerInterceptor does the

        response_or_iterator = method(request, context)
        if hasattr(response_or_iterator, "__aiter__"):
            return response_or_iterator
        else:
            return await response_or_iterator

thing where it decides how to handle the next method.

And it also made sense because a not-implemented interceptor should be a no-op

class NothingInterceptor(AsyncServerInterceptor):
  pass

Is this intended use to call the super().intercept and let it handle? Is this a bad idea?

@d5h
Copy link
Collaborator

d5h commented Dec 16, 2022

Well, technically AsyncServerInterceptor.intercept is an abstract method which is not intended to be called. I gave it a default no-op implementation as an example. That said, I don't see any reason to change the example implementation. If you have a test for your interceptor, so you'll know if it ever breaks on update, then you'll be fine.

Now that you bring this up, it might make sense to make the default implementation non-abstract. I need to write some tests before committing to that, but maybe I'll make this the intended usage in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants