Add optional_nullable option (default: off) to mark Proto3 Optional fields as x-nullable #2215
Conversation
…oto3 Optional fields as x-nullable
protoc-gen-openapiv2/defs.bzl
Outdated
| @@ -278,6 +283,11 @@ protoc_gen_openapiv2 = rule( | |||
| doc = "whether to remove the service prefix in the operationID" + | |||
| " generation. Can introduce duplicate operationIDs, use with caution.", | |||
| ), | |||
| "optional_nullable": attr.bool( | |||
internal/descriptor/registry.go
Outdated
| // optionalNullable specifies whether Proto3 Optional fields should be marked as x-nullable. | ||
| optionalNullable bool |
There was a problem hiding this comment.
Can we name this field and related helpers proto3OptionalNullable to make it more explicit?
There was a problem hiding this comment.
Yep, done. I was debating this myself.
| Type string `json:"type,omitempty"` | ||
| Format string `json:"format,omitempty"` | ||
| Ref string `json:"$ref,omitempty"` | ||
| Nullable bool `json:"x-nullable,omitempty"` |
There was a problem hiding this comment.
x-nullable isn't defined in the referenced docs (http://swagger.io/specification/#itemsObject), could you please add a reference here?
There was a problem hiding this comment.
I updated that URL to point to the v2 specification, and added a reference to the v3 spec that calls out nullable. x-nullable is a "back-ported" extension to emulate this v3 behavior in v2, but it's more of a widely used convention than part of any formal spec. I've added what I think is helpful; let me know if it's too much or missing something you'd want to see/know.
| @@ -581,6 +582,10 @@ func schemaOfField(f *descriptor.Field, reg *descriptor.Registry, refs refMap) o | |||
| updateSwaggerObjectFromFieldBehavior(&ret, j, f) | |||
| } | |||
|
|
|||
| if reg.GetOptionalNullable() && f.Proto3Optional != nil && *f.Proto3Optional { | |||
There was a problem hiding this comment.
Can we just use the getter?
| if reg.GetOptionalNullable() && f.Proto3Optional != nil && *f.Proto3Optional { | |
| if reg.GetOptionalNullable() && f.GetProto3Optional() { |
There was a problem hiding this comment.
Yep, I was using completion and missed the presence of the getters. Updated.
|
Looks like there's a syntax error in the bazel definitions? |
Fixed, I had a missing comma. I didn't have access to the CircleCI to see the details of why the test failed. |
|
Thanks so much for the time, @johanbrandhorst ! |
See #669 for more background.
This change adds an optional (default: off)
proto3_optional_nullableflag toprotoc-gen-openapiv2that will mark Proto3 fields marked with theoptionallabel asx-nullable. The OAS specification calls this out asnullable, but this behavior has been "back-ported" to OAS2 using the System Extensionx-nullable.The effect of the Proto3 Optional flag on the GRPC is exactly this—it sets primitive types to pointers. It's certainly open to debate why we have Proto2
optional, theoptionalfield descriptor, and Proto3optional, but fundamentally Proto3 Optional is designed to solve the "true presence" issue as discussed in #669, at least IMHO. Whereas the other options never felt specific to this nullable behavior. Proto3 optional field presence discussion is here.It could be a valid approach to make this behavior the default, but for now it felt best to me to retain backwards compatibility until the semantics are well understood.
Potentially:
Fixes #669
Adds:
proto3_optional_nullablecommand line flag.proto3_optional_nullableis set, and the proto field hasproto3optionalset, the schema XNullable state is set to true, which emits the OAPIx-nullabletag.Comments and feedback welcome! @johanbrandhorst