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.

Sign up for GitHub

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

Jump to bottom

Look up enum value maps by their proto name #315

Merged
merged 3 commits into from
Feb 24, 2017
Merged

Look up enum value maps by their proto name #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdce4a7
runtime: Fix typo in Error call
nilium Feb 23, 2017
e98d5db
runtime: Make enum type distinct for query test
nilium Feb 23, 2017
479142a
runtime: Use protobuf enum name instead of (reflect.Type).String
nilium Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions runtime/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []
if m.Kind() != reflect.Ptr {
return fmt.Errorf("unexpected type %T: %v", msg, msg)
}
var props *proto.Properties
m = m.Elem()
for i, fieldName := range fieldPath {
isLast := i == len(fieldPath)-1
if !isLast && m.Kind() != reflect.Struct {
return fmt.Errorf("non-aggregate type in the mid of path: %s", strings.Join(fieldPath, "."))
}
f := fieldByProtoName(m, fieldName)
var f reflect.Value
f, props = fieldByProtoName(m, fieldName)
if !f.IsValid() {
grpclog.Printf("field not found in %T: %s", msg, strings.Join(fieldPath, "."))
return nil
Expand All @@ -60,7 +62,7 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []
if !isLast {
return fmt.Errorf("unexpected repeated field in %s", strings.Join(fieldPath, "."))
}
return populateRepeatedField(f, values)
return populateRepeatedField(f, values, props)
case reflect.Ptr:
if f.IsNil() {
m = reflect.New(f.Type().Elem())
Expand All @@ -82,26 +84,26 @@ func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []
default:
grpclog.Printf("too many field values: %s", strings.Join(fieldPath, "."))
}
return populateField(m, values[0])
return populateField(m, values[0], props)
}

// 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 {
func fieldByProtoName(m reflect.Value, name string) (reflect.Value, *proto.Properties) {
props := proto.GetProperties(m.Type())
for _, p := range props.Prop {
if p.OrigName == name {
return m.FieldByName(p.Name)
return m.FieldByName(p.Name), p
}
}
return reflect.Value{}
return reflect.Value{}, nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this nil propagate to calls to populateRepeatedField or populateField?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't seen it do so in tests. I could add a nil check around its uses, but an enumeration with no properties seems like bad code gen more than anything else. I'll poke around a bit and see what I can find.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the only case where it's nil would be if the reflect.Value is invalid. If the reflect.Value is invalid, then the caller returns an error (and in this case, there's only one place that fieldByProtoName is called). Could also add a prop == nil check to where it's being called, though? That'd make doubly sure we've got a proto field, I just don't know if it's paranoid vs. practical.

}

func populateRepeatedField(f reflect.Value, values []string) error {
func populateRepeatedField(f reflect.Value, values []string, props *proto.Properties) error {
elemType := f.Type().Elem()

// is the destination field a slice of an enumeration type?
if enumValMap := proto.EnumValueMap(elemType.String()); enumValMap != nil {
if enumValMap := proto.EnumValueMap(props.Enum); enumValMap != nil {
return populateFieldEnumRepeated(f, values, enumValMap)
}

Expand All @@ -120,7 +122,7 @@ func populateRepeatedField(f reflect.Value, values []string) error {
return nil
}

func populateField(f reflect.Value, value string) error {
func populateField(f reflect.Value, value string, props *proto.Properties) error {
// Handle well known type
type wkt interface {
XXX_WellKnownType() string
Expand All @@ -145,7 +147,7 @@ func populateField(f reflect.Value, value string) error {
}

// is the destination field an enumeration type?
if enumValMap := proto.EnumValueMap(f.Type().String()); enumValMap != nil {
if enumValMap := proto.EnumValueMap(props.Enum); enumValMap != nil {
return populateFieldEnum(f, value, enumValMap)
}

Expand Down
12 changes: 6 additions & 6 deletions runtime/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func TestPopulateParameters(t *testing.T) {
msg.Reset()
err := runtime.PopulateQueryParameters(msg, spec.values, spec.filter)
if err != nil {
t.Errorf("runtime.PoplateQueryParameters(msg, %v, %v) failed with %v; want success", spec.values, spec.filter, err)
t.Errorf("runtime.PopulateQueryParameters(msg, %v, %v) failed with %v; want success", spec.values, spec.filter, err)
continue
}
if got, want := msg, spec.want; !proto.Equal(got, want) {
Expand Down Expand Up @@ -241,8 +241,8 @@ type proto3Message struct {
BoolValue bool `protobuf:"varint,8,opt,name=bool_value" json:"bool_value,omitempty"`
StringValue string `protobuf:"bytes,9,opt,name=string_value" json:"string_value,omitempty"`
RepeatedValue []string `protobuf:"bytes,10,rep,name=repeated_value" json:"repeated_value,omitempty"`
EnumValue EnumValue `protobuf:"varint,11,opt,name=enum_value,json=enumValue,enum=api.EnumValue" json:"enum_value,omitempty"`
RepeatedEnum []EnumValue `protobuf:"varint,12,rep,packed,name=repeated_enum,json=repeated_enum,enum=api.EnumValue" json:"repeated_enum,omitempty"`
EnumValue EnumValue `protobuf:"varint,11,opt,name=enum_value,json=enumValue,enum=runtime_test_api.EnumValue" json:"enum_value,omitempty"`
RepeatedEnum []EnumValue `protobuf:"varint,12,rep,packed,name=repeated_enum,json=repeated_enum,enum=runtime_test_api.EnumValue" json:"repeated_enum,omitempty"`
TimestampValue *timestamp.Timestamp `protobuf:"bytes,11,opt,name=timestamp_value" json:"timestamp_value,omitempty"`
}

Expand All @@ -268,8 +268,8 @@ type proto2Message struct {
BoolValue *bool `protobuf:"varint,8,opt,name=bool_value" json:"bool_value,omitempty"`
StringValue *string `protobuf:"bytes,9,opt,name=string_value" json:"string_value,omitempty"`
RepeatedValue []string `protobuf:"bytes,10,rep,name=repeated_value" json:"repeated_value,omitempty"`
EnumValue EnumValue `protobuf:"varint,11,opt,name=enum_value,json=enumValue,enum=api.EnumValue" json:"enum_value,omitempty"`
RepeatedEnum []EnumValue `protobuf:"varint,12,rep,packed,name=repeated_enum,json=repeated_enum,enum=api.EnumValue" json:"repeated_enum,omitempty"`
EnumValue EnumValue `protobuf:"varint,11,opt,name=enum_value,json=enumValue,enum=runtime_test_api.EnumValue" json:"enum_value,omitempty"`
RepeatedEnum []EnumValue `protobuf:"varint,12,rep,packed,name=repeated_enum,json=repeated_enum,enum=runtime_test_api.EnumValue" json:"repeated_enum,omitempty"`
XXX_unrecognized []byte `json:"-"`
}

Expand Down Expand Up @@ -367,5 +367,5 @@ var EnumValue_value = map[string]int32{
}

func init() {
proto.RegisterEnum("runtime_test.EnumValue", EnumValue_name, EnumValue_value)
proto.RegisterEnum("runtime_test_api.EnumValue", EnumValue_name, EnumValue_value)
}