Skip to content

Commit

Permalink
runtime: Add oneof lookup for fields
Browse files Browse the repository at this point in the history
This modifies fieldByProtoName to do two things:

1. It may now return an error. It only does this for the oneof case
   right now, as the not-found case only causes the field to be
   skipped.

2. It will first look for the field name in the message's OneofTypes
   map. If the field matches a OneOf type, it will check first that the
   field is nil -- if it isn't, the field is already set and it returns
   an error. If nil, it allocates a new oneof type for the field and
   returns the first field of the oneof type.

The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler but involves allocating and sorting an array of field names
before walking them (only to get predictable field selection).

This change amends the error result tested for in
TestPopulateParameters to include the name of oneof_value.

Related to issue grpc-ecosystem#82.

Change-Id: I7a5ecc9ce397bd71156cd4ac8690bae782ecc55e
  • Loading branch information
nilium committed Mar 3, 2017
1 parent 22832c8 commit e9752d4
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 6 deletions.
25 changes: 20 additions & 5 deletions runtime/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,11 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []
return fmt.Errorf("non-aggregate type in the mid of path: %s", strings.Join(fieldPath, "."))
}
var f reflect.Value
f, props = fieldByProtoName(m, fieldName)
if !f.IsValid() {
var err error
f, props, err = fieldByProtoName(m, fieldName)
if err != nil {
return err
} else if !f.IsValid() {
grpclog.Printf("field not found in %T: %s", msg, strings.Join(fieldPath, "."))
return nil
}
Expand Down Expand Up @@ -89,14 +92,26 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []

// fieldByProtoName looks up a field whose corresponding protobuf field name is "name".
// "m" must be a struct value. It returns zero reflect.Value if no such field found.
func fieldByProtoName(m reflect.Value, name string) (reflect.Value, *proto.Properties) {
func fieldByProtoName(m reflect.Value, name string) (reflect.Value, *proto.Properties, error) {
props := proto.GetProperties(m.Type())

// look up field name in oneof map
if op, ok := props.OneofTypes[name]; ok {
v := reflect.New(op.Type.Elem())
field := m.Field(op.Field)
if !field.IsNil() {
return reflect.Value{}, nil, fmt.Errorf("field already set for %s oneof", props.Prop[op.Field].OrigName)
}
field.Set(v)
return v.Elem().Field(0), op.Prop, nil
}

for _, p := range props.Prop {
if p.OrigName == name {
return m.FieldByName(p.Name), p
return m.FieldByName(p.Name), p, nil
}
}
return reflect.Value{}, nil
return reflect.Value{}, nil, nil
}

func populateRepeatedField(f reflect.Value, values []string, props *proto.Properties) error {
Expand Down
2 changes: 1 addition & 1 deletion runtime/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func TestPopulateParameters(t *testing.T) {
},
filter: utilities.NewDoubleArray(nil),
want: &proto3Message{},
wanterr: errors.New("field already set for oneof"),
wanterr: errors.New("field already set for oneof_value oneof"),
},
} {
msg := proto.Clone(spec.want)
Expand Down

0 comments on commit e9752d4

Please sign in to comment.