Add interceptors #276
Add interceptors #276
Conversation
| @@ -28,7 +28,7 @@ test_python_client: generate build/clientcompat build/pycompat | |||
|
|
|||
| setup: | |||
| ./install_proto.bash | |||
| GOPATH=$(CURDIR)/_tools go install github.com/twitchtv/retool/... | |||
| GO111MODULE=off GOPATH=$(CURDIR)/_tools GOBIN=$(CURDIR)/_tools/bin go get github.com/twitchtv/retool | |||
There was a problem hiding this comment.
This is needed for any development system that has GO111MODULE and GOBIN set, and has no effect on other systems for Twirp's Makefile logic.
middleware.go
Outdated
|
|
||
| // ChainInterceptors chains the Interceptors. | ||
| // | ||
| // Returns nil if interceptors is empty. |
There was a problem hiding this comment.
Let's remember to add more comments explaining Method, Interceptor and ChainInterceptors. The extended comments should contain some basic example. We should also add a new page for interceptors in the docs, and make sure it is linked from docs for Hooks, and from other pages that mention hooks.
There was a problem hiding this comment.
I added a bit more Godoc, and a skeleton for the docs folder - could you propose some more language you'd like to add here? Would be helpful.
| @@ -574,6 +575,48 @@ func TestErroringHooks(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestInterceptor(t *testing.T) { | |||
| interceptor := func(next twirp.Method) twirp.Method { | |||
| return func(ctx context.Context, request interface{}) (interface{}, error) { | |||
There was a problem hiding this comment.
verify that the context contains info about the request:
methodName, _ := twirp.MethodName(ctx)
if methodName != "MakeHat" {
return nil, fmt.Errorf("unexpected methodName: %q", methodName)
}
serviceName, _ := twirp.ServiceName(ctx)
if serviceName != "Haberdasher" {
return nil, fmt.Errorf("unexpected serviceName: %q", serviceName)
}
packageName, _ := twirp.PackageName(ctx)
if packageName != "twirp.internal.twirptest" {
return nil, fmt.Errorf("unexpected packageName: %q", packageName)
}later, after calling the service, we should also verify that the context was annotated with the status code:
response, err := next(ctx, request)
status, ok := twirp.StatusCode(ctx)
if status != "200" || !ok {
return nil, fmt.Errorf("unexpected status in context: %q", status)
}There was a problem hiding this comment.
Added the first part, but the second part (status on context) isn't how contexts work - contexts are immutable, and we'd have to return a new context to do this (which isn't standard). I'd expect to get the status code off of the error, not the context, and all other middleware of this variety in the Golang ecosystem does the same.
There was a problem hiding this comment.
good idea, thanks. Getting statusCode from context - doesn't work.
This is how I did:
twerr, ok := err.(twirp.Error)
if ok {
statusCode = twirp.ServerHTTPStatusFromErrorCode(twerr.Code())
}
|
Addressed comments, will start on client interceptors now. |
|
Update: the client interceptors are now added, including testing. |
|
Will merge in master with the release v7.1 soon. Great job! 👍 |
Implements #265