Added middlewares support #315
Conversation
|
this seems reasonable, @jhump what do you think? |
| "github.com/fullstorydev/grpcurl" | ||
|
|
||
| "github.com/fullstorydev/grpcui/internal" |
There was a problem hiding this comment.
Is this separation because grpcui is this module whereas grpcurl is a remote module?
If so, we should probably just move grpcurl up into the previous block with other imports
| return mw(ctx, req, c2) | ||
| } | ||
| } | ||
| results, err := call(r.Context(), req) |
There was a problem hiding this comment.
There's something slightly off for me about the local variable names here, but I can't put my finger on it. Lemme play with this a minute.
| call := func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | ||
| return invokeRPC(ctx, method, ch, descSource, r.Header, r.Body, &options) | ||
| } | ||
| for i := len(options.Middlewares) - 1; i > 0; i-- { | ||
| mw := options.Middlewares[i] | ||
| c2 := call | ||
| call = func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | ||
| return mw(ctx, req, c2) | ||
| } | ||
| } | ||
| results, err := call(r.Context(), req) |
There was a problem hiding this comment.
How about this?
| call := func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | |
| return invokeRPC(ctx, method, ch, descSource, r.Header, r.Body, &options) | |
| } | |
| for i := len(options.Middlewares) - 1; i > 0; i-- { | |
| mw := options.Middlewares[i] | |
| c2 := call | |
| call = func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | |
| return mw(ctx, req, c2) | |
| } | |
| } | |
| results, err := call(r.Context(), req) | |
| handler := func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | |
| return invokeRPC(ctx, method, ch, descSource, r.Header, r.Body, &options) | |
| } | |
| for i := len(options.Middlewares) - 1; i > 0; i-- { | |
| previousMiddleware := options.Middlewares[i] | |
| nextHandler := handler | |
| handler = func(ctx context.Context, req *RPCRequest) (*RPCResult, error) { | |
| return previousMiddleware(ctx, req, nextHandler) | |
| } | |
| } | |
| results, err := handler(r.Context(), req) |
There was a problem hiding this comment.
This seems a little odd to invent a new way to do this and introduce new exported types for this. Why can't you use interceptors on the grpc.ClientConnInterface instead?
If you want to decorate an existing such interface (i.e. adding more middleware to an existing client, instead of configuring the client with your middleware), you could use grpchan.InterceptClientConn.
It's unclear why there should be a way to do this that is so specific to the UI's invocation method.
Hi!
I found that I can not specify any dynamic callbacks like middlewares/interceptors if I use standalone server.
I think that possibility to do that is a good option so I added a support for that.