fix(openapiv2): exclude oneof fields from required with proto3 field semantics#6335
Conversation
9cf733d to
f736b46
Compare
johanbrandhorst
left a comment
There was a problem hiding this comment.
Thanks for your contribution, could you please submit an example proto file that reproduces this issue to internal/example/proto? Either add it to an existing one with the flag set, or add a new protobuf file and buf.gen.yaml file with the option set. It's a secondary type of testing we use in addition to template_test.go. Thanks!
…semantics When `use_proto3_field_semantics=true` is set, all non-optional proto3 fields are marked as required. However, fields in a `oneof` group should not be required since at most one of them can be set at a time. This adds a check for `field.OneofIndex == nil` so that oneof members are excluded from the required array. Explicit `REQUIRED` field behavior annotations still override this and mark the field as required. Fixes grpc-ecosystem#6334 Co-authored-by: Cursor <cursoragent@cursor.com>
f736b46 to
abb05f9
Compare
|
@johanbrandhorst, hope you had a good weekend -- thank you for the feedback (and this tool). Let me know if I you need anything else from me. |
johanbrandhorst
left a comment
There was a problem hiding this comment.
Sorry for the delay, seems like the linter found a slight error in the new file.
Add the new proto file to the buf lint ignore_only sections for: - DIRECTORY_SAME_PACKAGE - PACKAGE_DIRECTORY_MATCH - PACKAGE_SAME_GO_PACKAGE - PACKAGE_VERSION_SUFFIX This follows the same pattern as other proto files in the examplepb directory. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Added it to the ignore sections like the other example use cases, tested locally, |
Busted by the formatter this time 😂. Sorry! |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
My fault. I think that should do it. Thank you! |
|
@johanbrandhorst -- what's the releasee process like for this? Thank you! |
|
I'll usually put one out every few months or if there is a significant bug fix, but I think we can put one out for this and other fixes, so I did: https://github.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.27.8. |
|
Hi @johanbrandhorst what steps are required to publish |
|
The buf.build plugin version is maintained by buf: https://github.com/bufbuild/plugins/tree/main/plugins/grpc-ecosystem/openapiv2. I'm not sure why it hasn't been updated yet, maybe a problem with their automation? You could try reaching out on their slack or opening an issue on the repo. |
Summary
oneoffields being incorrectly marked asrequiredwhenuse_proto3_field_semantics=trueis setfield.OneofIndex == nilcheck inupdateSwaggerObjectFromFieldBehaviorso that oneof members are excluded from the required arrayREQUIREDfield behavior annotations still override this correctlyProblem
When
use_proto3_field_semantics=true, all non-optional proto3 fields are added to the OpenAPIrequiredarray. This includesoneofmembers, which is semantically wrong -- aoneofmeans at most one field can be set, so they should never all be required.For example, this proto:
Generates both
listandruleas required, when neither should be.Fix
One-line change in
updateSwaggerObjectFromFieldBehavior:Proto3
optionalfields use synthetic oneofs (OneofIndex != nil), butGetProto3Optional()already handles that case. TheOneofIndex == nilcheck catches realoneofgroups.Test plan
Test_updateSwaggerObjectFromFieldBehavior:REQUIREDFixes #6334
Made with Cursor