Added interceptors. #86

mogol · 2018-05-03T09:22:01Z

Fixes #79

Draft implementation for interceptors #79 to support analysis of metadata (e.g. authorization)

abstract class Interceptor {
  GrpcError handle(ServiceCall call);
}

Any feedback is appreciated.

sigurdm

Hi German Saprykin
Thanks for the contribution!
I have a bunch of comments and questions but the basic approach is good I think.

sigurdm · 2018-06-28T11:08:18Z

test/server_test.dart

    await harness.fromServer.done;
  });
+
+  test('Server returns error if interceptor blocks request', () async {


I guess it ends up being implicit from the other tests, but perhaps add an explicit test of the interceptor not blocking the request?

sigurdm · 2018-06-28T11:19:34Z

lib/src/server/handler.dart


+  GrpcError _applyInterceptors() {
+    for (final interceptor in _interceptors) {
+      final error = interceptor.handle(this);


Do we want to take into account if the interceptor throws and handle that gracefully somehow?

sigurdm · 2018-06-28T11:34:21Z

lib/src/server/handler.dart

 class ServerHandler extends ServiceCall {
  final ServerTransportStream _stream;
  final Service Function(String service) _serviceLookup;
+  final Iterable<Interceptor> _interceptors;


Perhaps store this as a List.
If you end up storing a custom iterator, it might have confusing behaviour for repeated iterations.

sigurdm · 2018-06-28T11:39:18Z

lib/src/server/handler.dart

  Timer _timeoutTimer;

-  ServerHandler(this._serviceLookup, this._stream);
+  ServerHandler(this._serviceLookup, this._stream, this._interceptors);


This would break the public api. Not neccesarily a bad thing as we are pre 1.0.
Perhaps turn it into a named argument.
Anyway we need an update to the pubspec.yaml version string.

sigurdm · 2018-06-28T12:32:11Z

lib/src/server/interceptor.dart

+import 'call.dart';
+
+abstract class Interceptor {
+  GrpcError handle(ServiceCall call);


Should there be a way to silently intercept without returning an error?

What is use case?
What's better way to implement? It could be

GrpcError.custom, check error code and sink stream without sending the error.

Exception, on any exception silently sink stream

I'm not sure about the use-case - it just seemed more general.
I have not used the interceptors for other GRpc implementations so I'm a bit unsure how they are used...

One way would be to make a response class with an enum. Something like:

enum InterceptorResponseType { error, ok, block} class InterceptorResponse { final InterceptorResponseType type; final GrpcError error; InterceptorResponse.ok() : type = InterceptorResponseType.ok; InterceptorResponse.error(this.error) : type = InterceptorResponseType.error; InterceptorResponce.block() : type = InterceptorResponseType.block; }

sigurdm · 2018-06-28T12:49:49Z

lib/src/server/interceptor.dart

+
+import 'call.dart';
+
+abstract class Interceptor {


https://www.dartlang.org/guides/language/effective-dart/design#avoid-defining-a-one-member-abstract-class-when-a-simple-function-will-do

sigurdm · 2018-06-28T12:52:42Z

lib/src/server/server.dart

  /// Create a server for the given [services].
-  Server(List<Service> services) {
+  Server(List<Service> services,
+      {Iterable<Interceptor> interceptors = const []})


add a type annotation to the list literal:
const <Interceptor>[]

sigurdm · 2018-06-28T12:53:28Z

lib/src/server/server.dart

 /// Listens for incoming RPCs, dispatching them to the right [Service] handler.
 class Server {
  final Map<String, Service> _services = {};
+  final Iterable<Interceptor> _interceptors;


Also here I think a List is preferable.

sigurdm · 2018-06-28T12:54:48Z

test/server_test.dart

+
+  test('Server returns error if interceptor blocks request', () async {
+    GrpcError interceptorHandler(ServiceCall call) {
+      return new GrpcError.unauthenticated('Request is unauthenticated');


To make it more realistic: branch on the method name here.

sigurdm · 2018-06-28T13:06:54Z

lib/src/server/interceptor.dart

+import 'call.dart';
+
+abstract class Interceptor {
+  GrpcError handle(ServiceCall call);


We might want to make this async, such that we can do general async handling in the interceptor

Sounds interesting, looks like it will require a refactoring, old code is fully synchronous.
I tried to return Future and mark functions async. UT started to fail.
@sigurdm Don't you mind to do it in separate PR?

Should Interceptor return always Future<GrpcError> in this case?

Good point that the current handling is synchronous. Yes - that should be pushed to another PR.
Yes in that case it would return Future

thelinuxfoundation · 2018-06-29T13:39:31Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

One or more commits are not associated with a GitHub user. The email used in the Git commit must match a verified email address in a GitHub user profile. For more information, please see https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Regards,
CLA GitHub bot

mogol · 2018-06-29T14:27:38Z

Test failures are on dart:dev, it seems not because of changes in this pr

mit-mit · 2018-07-02T13:24:40Z

Yes, looks like the test failures are also happening in master. Filed #91 to track.

sigurdm

Changes looks good!
Just a few typos

sigurdm · 2018-07-03T08:32:08Z

lib/src/server/interceptor.dart

+
+/// A gRPC Interceptor.
+///
+/// Interceptor is called before correspoding [ServiceMethod] invocation.


Typos:
Interceptor is called before its => An interceptor is called before the corresponding
correspoding => corresponding

If interceptor returns [GrpcError] => If the interceptor returns a [GrpcError]
If interceptor returns => If the interceptor returns
correspoding => the corresponding

sigurdm · 2018-07-03T08:38:06Z

lib/src/server/interceptor.dart

+import 'call.dart';
+
+abstract class Interceptor {
+  GrpcError handle(ServiceCall call);


Good point that the current handling is synchronous. Yes - that should be pushed to another PR.
Yes in that case it would return Future

sigurdm · 2018-07-03T09:19:37Z

lib/src/server/interceptor.dart

+import 'call.dart';
+
+abstract class Interceptor {
+  GrpcError handle(ServiceCall call);


I'm not sure about the use-case - it just seemed more general.
I have not used the interceptors for other GRpc implementations so I'm a bit unsure how they are used...

One way would be to make a response class with an enum. Something like:

enum InterceptorResponseType { error, ok, block} class InterceptorResponse { final InterceptorResponseType type; final GrpcError error; InterceptorResponse.ok() : type = InterceptorResponseType.ok; InterceptorResponse.error(this.error) : type = InterceptorResponseType.error; InterceptorResponce.block() : type = InterceptorResponseType.block; }

mogol · 2018-07-03T16:51:54Z

lib/src/server/interceptor.dart

+/// If the interceptor returns a [GrpcError], the error will be returned as a response and [ServiceMethod] wouldn't be called.
+/// If the interceptor throws [Exception], [GrpcError.internal] with exception.toString() will be returned.
+/// If the interceptor returns null, the corresponding [ServiceMethod] of [Service] will be called.
+typedef Interceptor = GrpcError Function(


@sigurdm I have checked go implementation for interceptors

https://github.com/grpc/grpc-go/blob/a3e99ebee0a498c3093e9741d09c579f0fc34a7f/server.go#L1020

From my understanding it has only two cases - no-error and error,
if there is an error, write a status and stop processing a request.
If there is no error, continue processing.

Imo it doesn't make sense to add this feature (I mean silent blocking) now.
Also I am not sure it conforms to grpc standard or not.

What do you think? Is it required?

No let's stick with error/no-error!

mogol · 2018-07-04T05:06:06Z

Is anything else required to be done in this PR?

sigurdm · 2018-07-04T05:44:01Z

No I don't think so. Thanks!
We need to fix the build issues. Then I'll merge this.

mit-mit · 2018-07-09T14:57:03Z

Merging now that the build is passing. Thanks @mogol for the contribution!

Add draft implementation of interceptors.

9e7b138

googlebot added the cla: yes label May 3, 2018

ghost mentioned this pull request May 9, 2018

interceptor support #79

Closed

sigurdm reviewed Jun 28, 2018

View reviewed changes

Fix review comment.

19c9744

mogol force-pushed the interceptor branch from 8c32b7e to 19c9744 Compare June 29, 2018 13:43

Revert example files.

43fe7af

mogol force-pushed the interceptor branch from 4a90d5c to 43fe7af Compare June 29, 2018 13:48

mogol and others added 4 commits June 29, 2018 21:48

Merge branch 'master' into interceptor

90224b2

Revert interop files.

ccc33f9

Revert interop files.

35ef8bf

Format.

0b7a3ab

sigurdm reviewed Jul 3, 2018

View reviewed changes

Fix typos.

09afa79

mogol commented Jul 3, 2018

View reviewed changes

mogol and others added 2 commits July 5, 2018 22:09

Merge branch 'master' into interceptor

1ec2e75

Merge branch 'master' into interceptor

354ac09

mit-mit merged commit 847a362 into grpc:master Jul 9, 2018

mogol deleted the interceptor branch July 12, 2018 02:22

Added interceptors. #86

Added interceptors. #86

Uh oh!

Conversation

mogol commented May 3, 2018 • edited by mit-mit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sigurdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thelinuxfoundation commented Jun 29, 2018

Uh oh!

mogol commented Jun 29, 2018

Uh oh!

mit-mit commented Jul 2, 2018

Uh oh!

sigurdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogol commented Jul 4, 2018

Uh oh!

sigurdm commented Jul 4, 2018

Uh oh!

mit-mit commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mogol commented May 3, 2018 •

edited by mit-mit

Loading