Add support for enum path parameters #738
Conversation
maros7
force-pushed
the
enum-path-parameter
branch
from
48c72a7
to
a21f07f
Compare
Related to grpc-ecosystem#322 Updated protoc-gen-swagger to output enum path parameters correctly Updated protoc-gen-grpc-gateway to handle enum path parameters Regenerated examples Added pathenum proto for an externally imported enum example and verification Added enum_helper.go to handle the lack of enum support in the swagger-codegen-cli 2.2.2 for Go, see swagger-api/swagger-codegen#5635 Fixed browser integration test cases Updated bazel config Fixed last, faulty bazel config Updated integration test case to test both index == 0 and index > 0 for enums
maros7
force-pushed
the
enum-path-parameter
branch
from
a21f07f
to
7cf77b0
Compare
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
=========================================
Coverage ? 55.36%
=========================================
Files ? 30
Lines ? 3213
Branches ? 0
=========================================
Hits ? 1779
Misses ? 1259
Partials ? 175
Continue to review full report at Codecov.
|
| option (google.api.http) = { | ||
| post: "/v1/example/a_bit_of_everything/{float_value}/{double_value}/{int64_value}/separator/{uint64_value}/{int32_value}/{fixed64_value}/{fixed32_value}/{bool_value}/{string_value=strprefix/*}/{uint32_value}/{sfixed32_value}/{sfixed64_value}/{sint32_value}/{sint64_value}/{nonConventionalNameValue}" | ||
| post: "/v1/example/a_bit_of_everything/{float_value}/{double_value}/{int64_value}/separator/{uint64_value}/{int32_value}/{fixed64_value}/{fixed32_value}/{bool_value}/{string_value=strprefix/*}/{uint32_value}/{sfixed32_value}/{sfixed64_value}/{sint32_value}/{sint64_value}/{nonConventionalNameValue}/{enum_value}/{path_enum_value}/{nested_path_enum_value}" |
| @@ -594,8 +595,13 @@ func renderServices(services []*descriptor.Service, paths swaggerPathsObject, re | |||
| return fmt.Errorf("only primitive and well-known types are allowed in path parameters") | |||
| } | |||
| case pbdescriptor.FieldDescriptorProto_TYPE_ENUM: | |||
| paramType = fullyQualifiedNameToSwaggerName(parameter.Target.GetTypeName(), reg) | |||
| paramType = "string" | |||
There was a problem hiding this comment.
It seems the handler below handles both the numerical and string representations of an enum, should we really hardcode this to string?
There was a problem hiding this comment.
I agree. I chose to use the same logic as already existed for enums in query params, see https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query.go#L339-L356. Do you know the rationale of supporting numerical, i.e. supporting that you input an enum by its index? From my perspective string support is sufficient and I'd like to update the convert Enum function to only support string. But please share any rationale behind supporting both first.
There was a problem hiding this comment.
I think using the query params precedent is good. I think the reason we want both the numerical representation and the string representation to work is because they're both valid ways of representing the enum value, and the user isn't able to configure which they prefer (unlike for the marshaller), so we have to support both. Anyway, I'm happy to keep this the same as the query params, there isn't really an important distinction in the URL anyway (unlike in JSON).
| // Enum converts the given string into an int32 that should be type casted into the | ||
| // correct enum proto type. | ||
| func Enum(val string, enumValMap map[string]int32) (int32, error) { | ||
| e, ok := enumValMap[val] |
There was a problem hiding this comment.
This lookup means we define the correct enum values as the strings generated by the go Protobuf implementation. Is this appropriate? It'll be things like MyEnum_FIRST_VALUE etc. Is there some way we can use the values from the protofile directly (i.e. FIRST_VALUE). I like having the protofile content as the source of truth.
There was a problem hiding this comment.
Not sure I follow. The convert Enum function will use the name->index map that is generated by the proto compiler, see e.g. grpc-gateway/examples/proto/examplepb/a_bit_of_everything.pb.go. That map is provided to the convert Enum function when conversion from path parameters to protobuf is done, see e.g. grpc-gateway/examples/proto/examplepb/a_bit_of_everything.pb.gw.go. So not sure exactly what you mean.
There was a problem hiding this comment.
Ah, my bad, the values aren't prefixed. Carry on!
|
Thanks for your contribution! |
This is related to #322. I've taken a stab at implementing enum type path parameter support.
I have updated protoc-gen-swagger to render enum path parameters correctly. However, since swagger-codegen-cli 2.2.2 doesn't support generating go enums (it was added in a later version, see swagger-api/swagger-codegen#5635) I also had to add some helper functions to make the tests pass correctly.
protoc-gen-grpc-gateway was of course updated to handle enum path parameters. The runtime enum convert function will return an int32 that will be type casted to an enum, which is generated via the update to the template.
Any feedback on my take on this is appreciated.