Extend google.protobuf.EnumOptions for Schema #4931
Conversation
Signed-off-by: Lukas Hoehl <[email protected]>
| // | ||
| // All IDs are the same, as assigned. It is okay that they are the same, as they extend | ||
| // different descriptor messages. | ||
| Schema openapiv2_enum = 1042; |
There was a problem hiding this comment.
I would like to discuss if we really need to embed the whole Schema message here since most of the fields are not applicable to Enums. When applying the parsed schema I currently only respect a subset of fields.
Maybe something like EnumSchema that is a subset of Schema? Would need more implementation effort though.
There was a problem hiding this comment.
I'd definitely prefer it if we used a smaller more accurate message definition, since I don't see enums ever supporting the same features as the other options 👍🏻
There was a problem hiding this comment.
Where would the new message reside?
https://github.com/grpc-ecosystem/grpc-gateway/blob/442bd3ab9497e4f9b3bbcbf4e53854300a68dd5a/protoc-gen-openapiv2/options/openapiv2.proto currently only contains messages that represent the OpenAPI specification. Having a message that does not represent a OpenAPI spec is somewhat misleading.
https://github.com/grpc-ecosystem/grpc-gateway/blob/442bd3ab9497e4f9b3bbcbf4e53854300a68dd5a/protoc-gen-openapiv2/options/annotations.proto holds the extensions that register to the descriptors, so I guess that file is not an option either.
What's your opinion here?
There was a problem hiding this comment.
My initial thought was openapiv2.proto. How do you mean that it doesn't represent the OpenAPI specification? We're talking about defining a subset of message Schema as it would apply to enums, right? I was under the impression that using Schema wouldn't make sense since many options in there will never be possible for enums. In that sense, I guess we're defining our own subset of the OpenAPI spec?
There was a problem hiding this comment.
Yes, it would be a subset of the Schema message.
What I wanted to say it that currently each message in openapi.proto follows closely to the spec of OpenAPI 2.
For example the Schema message maps to the Message object of the OpenAPI spec.
The new EnumSchema would not map to an object of the spec, but I guess that's negligible
There was a problem hiding this comment.
I appreciate the sentiment - I think we have to break with the convention for the sake of user experience.
References to other Issues or PRs
Fixes #2671
Brief description of what is fixed or changed
Allows to set options on proto enums with OpenAPI Schema.
Only a subset of the Schema fields are respected since most of them are not applicable on enums.
Other comments
PR is based mostly on #2855 with a few adjustments