API Proposal - Client Interceptors #558

rogchap · 2019-05-11T01:03:07Z

No description provided.

[PLT-53] Sync upstream changes

rogchap · 2019-05-11T01:18:11Z

proposals/client-interceptors.md

+
+*Do not modify the listener passed in. Either pass it along unmodified or call methods on it to short-circuit the interceptor stack.*
+
+The `listener` methods are:


So, I'm not 100% convinced on the listener API for inbound operations.
This has been ported from the NodeJS API; however I believe that this is over-complicated for the web implementation.
The way I see it, there only a few places where we need to intercept:

https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpcwebclientbase.js#L66

GrpcWebClientBase.prototype.rpcCall = function( method, request, metadata, methodInfo, callback) { // Intercept HERE onStart() // ... stream.on('data'); // Intercept HERE onData() stream.on('status'); // Intercept HERE onStatus() stream.on('error'); // Intercept HERE onError() // ... // Intercept HERE onSend() xhr.send(method, 'POST', payload); return stream; };

The serverStreaming is similar.

This would simplify the requester object to:

var requester = { onStart: function(metadata, next){}, onSend: function(request, next){}, onData: function(response, next){}, onStatus: function(status, next){}, onError: function(error, next){}, };

A case can be made to keep the API design consistent with other platforms. That way developers don't need to re-learn concepts. For instance, I find it convenient that the Java API[1] is the same for Android and server. A ClientCall in Java roughly translates to InterceptingCall as defined in this proposal. So defining a Listener means the JS API looks similar to the Java API.

OTOH, the ObjC Interceptor API proposal does not define the concept of Listener and instead puts all methods on GRPCInterceptorInterface[2] instead.

[1]https://grpc.github.io/grpc-java/javadoc/io/grpc/ClientCall.html#start-io.grpc.ClientCall.Listener-io.grpc.Metadata-
[2]https://github.com/grpc/proposal/blob/9bcd06aae8e6f9122885711a602ecdc9af75c560/L50-objc-interceptor.md#interceptor-chain

dunxen · 2019-05-11T05:03:44Z

proposals/client-interceptors.md

+ * @param {string} options.hostname The server host to call
+ * @param {Object} options.credentials Credentials used to authenticate with the server
+ * @param {MethodDescriptor} options.methodDiscriptor A container of properties describing the call (see below)
+ * @param {Function} nexCall Constructs the next interceptor in the chain


nexCall -> nextCall 🙂

dunxen · 2019-05-11T05:29:19Z

Thanks for this! It looks great. 😄

jatraman · 2019-05-13T18:01:35Z

proposals/client-interceptors.md

+ * @param {MethodType} methodType One of two types: MethodType.UNARY or MethodType.SERVER_STREAMING
+ * @param {grpc.web.AbstractClientBase.MethodInfo} methodInfo Properties containing the serialize and deserialize functions
+ */
+MethodDescriptor(name, serviceName, path, methodType, methodInfo)


wdyt about moving the existing class out of AbstractClientBase and creating grpc.web.MethodInfo that works for this and other use cases?

Couple of reasons to consider this:

If grpc.web.MethodInfo also contains the path it should simplify the proto compiler as well (js_grpc_web_library).

It'll be confusing to have both MethodInfo and MethodDescriptor

Yup, I think this would be a good idea.
My initial thoughts on the implementation was to make minimal changes to the current API and CodeGen and provide backward compatibility (hence keeping methodInfo as-is). The reality may be that the refactor may not be compatible... we would need to make a call on backward compatibility support; if no then this would push this project to v2.0.0

fyi, #567 introduced grpc.web.MethodDescriptor and MethodType.

jatraman · 2019-05-13T18:30:19Z

proposals/client-interceptors.md

+```javascript
+/**
+ * An interception method called before an outbound call has started.
+ * @param {grpc.web.Metadata} metadata The call's outbound metadata (request headers).


Is grpc.web.Metadata a new class?

No, I stole this from the TypeScript definition:
https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/index.d.ts#L3
I probably should have left it as Object<string, string>

jatraman · 2019-05-13T18:37:12Z

proposals/client-interceptors.md

+
+#### At call invocation
+
+If an array of interceptors is provided at call invocation via `options.interceptors`, the call will ignore all interceptors provided via the client constructor.


Why ignore vS add to end of list? Do you have some concerns about interceptor ordering?

Interceptors like authorization, logging, metrics collection are likely to be defined and configured centrally. It would be unfortunate if a call site that just wants to add a custom interceptor needs to redefine this entire list again.

I would expect that most interceptor authors to have there interceptor chains easily available to add at call invocation.
Yes, the main reason is the ordering, or possibly you want a different authorization mechanism for that call etc.
I still think that this API has to override those at the client level, however maybe there is a second API that allows you to add interceptors to the chain? This would be useful for tools to inject interceptors; for example the grpc-web chrome DevTools.

clehene · 2019-11-05T20:30:47Z

Hey @stanley-cheung what's holding this PR? It would be useful to have a clean way to refresh client side tokens. Perhaps even provide a JWT interceptor with the distribution. Something like https://github.com/avast/grpc-java-jwt

kahirokunn · 2020-01-24T13:14:24Z

I need grpc web client intercepter.
What’s holding this PR?

kahirokunn · 2020-01-24T13:25:47Z

@duncandean how about this now ?

kahirokunn · 2020-02-04T11:56:11Z

@duncandean hello...?

dunxen · 2020-02-04T12:08:33Z

@kahirokunn I'm not a reviewer authorised with write access. This proposal probably also needs more thoughts and comments.

kahirokunn · 2020-02-06T06:53:09Z

I see... 😢

oreno4649 · 2020-02-06T13:44:55Z

I think this PR is necessary

maguro · 2020-03-11T04:40:43Z

How are things going on this?

stanley-cheung · 2020-03-11T05:02:06Z

We will be picking this up soon, sorry for the delay.

stanley-cheung · 2020-04-01T22:43:32Z

Hi @rogchap, and everyone else who is interested in interceptors:

First we are sorry it took us so long to get our act on this - resources, priorities, blablabla :) But we had not been idle on this - over the last many months, we have done some investigation internally to see how we can have a unified implementations of interceptors that will satisfy both our internal use case and the open source use case here. We have some designs and discussions internally to see how we will approach this. Also we identified some issues and requirements with the existing APIs that kind of get in our way for some of the features we plan internally.

So over the last few months we had done some background work to introduce a few concepts like grpc.web.Request, grpc.web.MethodDescriptor that requires some delicate migration work, and a few other classes like GenericClient and UnaryResponse. Some of those come from the need to fulfill some requirements to integrate with some other internal frameworks. And at the same time we want to use those concepts for the Interceptors design too.

We are at a stage where we are close to be able to put a PR forward in the coming days to show how we plan to implement interceptors here in this repo. I have started a thread over at #766 to talk about how the interceptors are designed and provided some examples there. This is still work-in-progress - we are still working through some issues. Please check these out and let us know how you think!

Thanks.

stanley-cheung · 2020-04-28T00:40:19Z

Closing this. The new interceptors proposal have been detailed in #766 and have the code merged in #785. We will be working on a 1.1.0 release soon.

* fix test warning * A flag for grpcWebProxy to allow to propagate some request headers The commit also fix grpcweb.WrappedGrpcServer header propagation to gRPC in case of using websockets Closes grpc#548

rogchap added 4 commits January 29, 2019 10:50

Merge pull request #1 from SafetyCulture/PLT-53

649c749

[PLT-53] Sync upstream changes

Merge remote-tracking branch 'upstream/master'

6c9d967

Merge remote-tracking branch 'upstream/master'

89f1001

Initial draft of the api proposal for client interceptors

6705102

rogchap commented May 11, 2019

View reviewed changes

rogchap mentioned this pull request May 11, 2019

API Proposal - Client Interceptors #555

Closed

dunxen reviewed May 11, 2019

View reviewed changes

jatraman reviewed May 13, 2019

View reviewed changes

Merge branch 'master' into client-interceptors-proposal

7e01c0c

stanley-cheung mentioned this pull request May 24, 2019

Introducing MethodDescriptor #567

Merged

Merge branch 'master' into client-interceptors-proposal

ae71dfa

stanley-cheung mentioned this pull request Mar 28, 2020

Interceptors implementation #766

Closed

stanley-cheung closed this Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Proposal - Client Interceptors #558

API Proposal - Client Interceptors #558

rogchap commented May 11, 2019

rogchap May 11, 2019

jatraman May 13, 2019

dunxen May 11, 2019

dunxen commented May 11, 2019

jatraman May 13, 2019

rogchap May 15, 2019

jatraman Jun 10, 2019

jatraman May 13, 2019

rogchap May 15, 2019

jatraman May 13, 2019

rogchap May 15, 2019

clehene commented Nov 5, 2019

kahirokunn commented Jan 24, 2020

kahirokunn commented Jan 24, 2020

kahirokunn commented Feb 4, 2020

dunxen commented Feb 4, 2020

kahirokunn commented Feb 6, 2020

oreno4649 commented Feb 6, 2020

maguro commented Mar 11, 2020

stanley-cheung commented Mar 11, 2020

stanley-cheung commented Apr 1, 2020

stanley-cheung commented Apr 28, 2020


		Do not modify the listener passed in. Either pass it along unmodified or call methods on it to short-circuit the interceptor stack.

		The `listener` methods are:


		#### At call invocation

		If an array of interceptors is provided at call invocation via `options.interceptors`, the call will ignore all interceptors provided via the client constructor.

API Proposal - Client Interceptors #558

API Proposal - Client Interceptors #558

Conversation

rogchap commented May 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented May 11, 2019

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

clehene commented Nov 5, 2019

kahirokunn commented Jan 24, 2020

kahirokunn commented Jan 24, 2020

kahirokunn commented Feb 4, 2020

dunxen commented Feb 4, 2020

kahirokunn commented Feb 6, 2020

oreno4649 commented Feb 6, 2020

maguro commented Mar 11, 2020

stanley-cheung commented Mar 11, 2020

stanley-cheung commented Apr 1, 2020

stanley-cheung commented Apr 28, 2020