Adding a RequestInfo struct for propagating request data to GetRequestMetadata calls. #3032
Conversation
|
|
* change from Inc. to LLC for Google * Add CODE-OF-CONDUCT.md, GOVERNANCE.md and update CONTRIBUTING.md
…ion, and requested comments.
| // | ||
| // This API is experimental. | ||
| type RequestInfo struct { | ||
| // This is assumed to be of the format some.Service/Method |
There was a problem hiding this comment.
I'm not sure if there are strict rules for this, but if a comment appears on the line above the field, I usually see "// Method is __________". It's fine to move the comment to the same line as the field if it's short, and then omit the field name.
There was a problem hiding this comment.
I just followed the convention in the file. (See line 117.)
There was a problem hiding this comment.
Line 117:
// This API is experimental.
I think you meant a different line? Most of the structs in this package appear to contain the field name in comments above fields. See, e.g. line 59 (ProtocolInfo). grpc-go has some problems following consistent style, but we do try to clean things up as we see them, and follow best practices for all new code.
| // | ||
| // This API is experimental. | ||
| type RequestInfo struct { | ||
| // This is assumed to be of the format some.Service/Method |
There was a problem hiding this comment.
Nit: I would doc this as the method passed to Invoke or NewStream for this RPC. You could also say for proto methods, this has the format "some.Service/Method".
| // requestInfoKey is a struct to be used as the key when attaching a RequestInfo to a context object. | ||
| type requestInfoKey struct{} | ||
|
|
||
| // RequestInfoFromContext extracts the RequestInfo object from the context object. |
| return ri | ||
| } | ||
|
|
||
| // WithRequestInfo adds the supplied RequestInfo object to the context object. |
There was a problem hiding this comment.
Let's unexport this for now until we find someone externally that desperately needs it for testing. To still be able to use it from the transport package, you could do something like:
package internal
var newRequestInfoContext interface{} // (this avoids a circular dependency; we do it for other things like this already)
package credentials
import "internal"
func init() {
internal.newRequestInfoContext = func(ctx context.Context, ri RequestInfo) {
return context.WithValue(...)
}
}
package transport
...
ctx = internal.newRequestInfoContext.(func(context.Context, RequestInfo) context.Context)(ctx, ri)
go.mod
Outdated
| @@ -17,3 +17,5 @@ require ( | |||
| google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8 | |||
| honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc | |||
| ) | |||
|
|
|||
| go 1.13 | |||
There was a problem hiding this comment.
Please revert this diff.
In general, we prefer to rebase instead of merge when syncing your branch with master: git fetch upstream master; git rebase upstream/master; this will put all of your diffs after all changes on upstream/master, which is how they will end up being applied when we merge them.
Maybe start a new git branch updated with upstream/master@HEAD if you're having git issues? You shouldn't need to, though. git rebase -i HEAD~### (where ### is some number greater than how many local commits you have not on master) can be used to delete the existing merge commit (and optionally: squash all your local commits so far into one), then git rebase upstream/master, and you should be OK.
| } | ||
|
|
||
| func (s) TestGRPCMethodAccessibleToCredsViaContextRequestInfo(t *testing.T) { | ||
| var method string |
There was a problem hiding this comment.
No need to validate grpc.Method in this test; TestGRPCMethod already does that. Delete all this and make emptyCall do nothing. Or if you follow the recommendation above, you don't even need an implementation of emptyCall at all since it will never be invoked -- ss := &stubServer{} is enough.
| if ri.Method != m.expectedMethod { | ||
| return nil, fmt.Errorf("want method %s in RequestInfo; got %s", m.expectedMethod, ri.Method) | ||
| } | ||
| return map[string]string{}, nil |
There was a problem hiding this comment.
How about returning the method observed as an error string instead, and validating in the test function:
func (m methodTestCreds) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) {
ri := credentials.RequestInfoFromContext(ctx)
return nil, status.Errorf(codes.Unknown, ri.Method)
}
...
const wantMethod = "/grpc.testing.TestService/EmptyCall"
if _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}); status.Convert(err).Message() != wantMethod {
t.Fatalf("ss.client.EmptyCall(_, _) = _, %v; want _, _.Message()=%q", err, wantMethod)
}
This simplifies things and also confirms that the creds are actually being invoked. (As is, if the creds didn't run at all, the test would still pass.)
…ion, and requested comments.
|
To deal with git frustrations I have created a clean PR: #3057 Closing this one. |
Add a RequestInfo struct which initially is used for passing the full request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data.
This is a fix for #3019