Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ctxtags TagBasedRequestFieldExtractor does not extract tags from fields in a oneof #326

Closed
nvx opened this issue Aug 28, 2020 · 1 comment · Fixed by #327
Closed

ctxtags TagBasedRequestFieldExtractor does not extract tags from fields in a oneof #326

nvx opened this issue Aug 28, 2020 · 1 comment · Fixed by #327

Comments

@nvx
Copy link
Contributor

nvx commented Aug 28, 2020

With the following message and interceptor configuration:

message FooRequest {
  oneof identifier {
    string Bar = 1 [(gogoproto.moretags) = 'log_field:"bar"'];
    string Baz = 2 [(gogoproto.moretags) = 'log_field:"baz"'];
  }
}
grpc_ctxtags.UnaryServerInterceptor(grpc_ctxtags.WithFieldExtractorForInitialReq(grpc_ctxtags.TagBasedRequestFieldExtractor("log_field")))

On messages that has either Bar or Baz set, they do not appear in the ctxtags. Other fields outside of oneof's works as expected.

@nvx
Copy link
Contributor Author

nvx commented Aug 28, 2020

It looks like there's two parts of this, one is that gogoproto.moretags doesn't support oneof currently either, so outside the scope of this repo.

It looks like what's missing is

if kind == reflect.Ptr && field.CanInterface() {
does not match as kind==reflect.Interface for oneof fields.

Changing it to also check for reflect.Interface seems to do the trick there, for example:

if (kind == reflect.Ptr || kind == reflect.Interface) && field.CanInterface() {

Note that noting that once field.Interface() is called on a field where kind==reflect.Interface, the returned value when passed to reflect.ValueOf().Kind() becomes reflect.Ptr again. So the earlier check here is doesn't need updating:

if v.Kind() != reflect.Ptr || v.Elem().Kind() != reflect.Struct {

While gogoproto.moretags can't support setting tags on fields in a oneof directly, making the above change would at least allow it to work where the oneof field is a message type rather than a string/etc and future proof for when gogoproto.moretags (or another tagging option) adds support.

Edit: Looks like I had an old version of gogoproto, the current version does support setting tags on oneof fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant