Look up enum value maps by their proto name #315
Conversation
This also means that the reflect-based name tests will now fail. This is intentional, we want to use the protobuf enum's name to look it up via proto.EnumValueMap, not the reflect-based type name, since that can vary.
This adds pass-through of the *proto.Properties for fields so that the fields being populated can be addressed by their enum name when the type is an enum (i.e., (*proto.Properties).Enum is set). As a result of this, the tests now pass, using the protobuf enum name as registered (i.e., fully qualified package name) instead of the Go package name derived by calling (reflect.Type).String. This should have no real impact on existing use, but it may be justified to re-add a fallback case for that reflect.Type name for hand-written protobuf types. Even then, however, it seems unlikely that it should be expected for enums-by-name to work when registering the enum map under a name other than the one it's referred to in protobuf.
| } | ||
| } | ||
| return reflect.Value{} | ||
| return reflect.Value{}, nil |
There was a problem hiding this comment.
Can this nil propagate to calls to populateRepeatedField or populateField?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@nilium thanks for catching this -- have you signed the CLA? |
|
@tmc Yes, I have. |
|
Thank you. |
Look up enum value maps by their proto name
This fixes parsing of the string form of protobuf enum values in query
parameters and fixes the enum value map lookup done by query.go,
originally introduced in #314.
Previously, the runtime looked up the the enum value map via the Go
type name of the enum, derived from its reflect.Type. The Go type name
may not always map 1:1 to the protobuf name, though. For example, if
the proto file declares a package such as
google.protobuf, it won'tmatch since the enum's name is fully qualified when registered for the
proto package. To make sure tests reflected the disparity, the query
test was amended to use a different package name from the Go package
when registering the enum.
To support this, it's necessary to pass the *proto.Properties for
fields forward when populating them, in order to allow the
populateField functions to determine if the field is an enum and look
up its value map by the props.Enum field.
As a result of this, the tests now pass, using the protobuf enum name
as registered (i.e., fully qualified package name) instead of the Go
package name derived by calling (reflect.Type).String. This should have
no real impact on existing use, but it may be justified to re-add
a fallback case for that reflect.Type name for hand-written protobuf
types. Even then, however, it seems unlikely that it should be expected
for enums-by-name to work when registering the enum map under a name
other than the one it's referred to in protobuf.