gen-grpc-gateway, gen-openapiv2: add support for proto3 optional #1951

adambabik · 2021-02-05T15:50:29Z

Even though, the main piece of code was added in google.golang.org/protobuf/compiler/protogen package, each plugin needs to explicitly enable support for proto3 optional fields. This change does exactly that. Note that gen-openapiv2 still does not fully benefit from protogen, hence, there are two helper methods enabling the feature in two different ways.

Also, this change does not treat optional fields in any different way in neither gen-grpc-gateway, nor gen-openapiv2 which actually might be required to deliver a valid experience.

adambabik · 2021-02-05T16:35:16Z

@johanbrandhorst I will need some help with failing bazel job. It seems that --experimental_allow_proto3_optional needs to be provided somehow in examples/internal/proto/examplepb/BUILD.bazel. This might be related.

achew22 · 2021-02-05T19:43:41Z

@adambabik I think you might be blocked by bazelbuild/rules_go#2788 on adding protocopts to rules_go

johanbrandhorst

This LGTM, but I'm still slightly worried that we should do something special here. Do you think you could add an integration test that uses this new optional string? The test would go here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/internal/integration/integration_test.go.

adambabik · 2021-02-11T18:06:37Z

@johanbrandhorst thanks for the suggestion. Adding a case to the integration tests revealed one more missing piece, that is optional fields require different converters. I used converters from proto2 because they return pointers. It would be nice to move them to more generic place I think. It's for another PR :) I didn't add so through testing for optional fields as it is for repeated. If it's not enough, let me know.

johanbrandhorst

Thanks for the new test Adam, I think it exposes an important question - we should probably disallow optional fields in path parameters, since they're required by definition.

johanbrandhorst · 2021-02-12T09:34:02Z

examples/internal/proto/examplepb/a_bit_of_everything.proto

@@ -458,7 +460,7 @@ service ABitOfEverythingService {
 	// This API creates a new ABitOfEverything
 	rpc Create(ABitOfEverything) returns (ABitOfEverything) {
 		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}/{enum_value}/{path_enum_value}/{nested_path_enum_value}/{enum_value_annotation}"
+			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}/{enum_value_annotation}/{optional_string_value}"


This is an interesting case, because if optional_string_value is truly optional, should it be possible to have it be a path parameter? Will our parser handle the case where this is unset? We may need to trigger a generation-time error when a user does this?

That's a good idea! Is there a place where I could document how optional fields behave?

johanbrandhorst · 2021-02-12T09:35:12Z

examples/internal/proto/examplepb/a_bit_of_everything.swagger.json

+          {
+            "name": "optionalStringValue",
+            "in": "path",
+            "required": true,


This might break the promise of optional values

johanbrandhorst · 2021-02-12T09:37:12Z

internal/descriptor/types.go

+		if c.Target.GetProto3Optional() {
+			// Continue as a proto3 optional field does not need a special treatment.
+			// This condition is required as otherwise it is matched by `c.Target.OneofIndex != nil`.
+		} else if c.Target.OneofIndex != nil { // check if it is a oneOf field


Can we invert this to avoid an else?

Suggested change

if c.Target.GetProto3Optional() {

// Continue as a proto3 optional field does not need a special treatment.

// This condition is required as otherwise it is matched by `c.Target.OneofIndex != nil`.

} else if c.Target.OneofIndex != nil { // check if it is a oneOf field

if !c.Target.GetProto3Optional() && c.Target.OneofIndex != nil { // check if it is a oneOf field

I wanted to separate proto3 optional and oneof cases, hence, separate blocks but I can also join them. I will keep the explanation.

Actually I read a bit more about it. It does use oneof under the hood.

johanbrandhorst

LGTM

johanbrandhorst · 2021-02-13T12:38:22Z

Bazel failure seems genuine - we need to configure protoc to use the new flag:

examples/internal/proto/examplepb/a_bit_of_everything.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

johanbrandhorst · 2021-02-18T09:06:22Z

Please rebase on master 😄

mvrhov · 2021-02-19T11:39:44Z

protoc 3.15 was released a few hours ago....

Optional fields for proto3 are enabled by default, and no longer require the --experimental_allow_proto3_optional flag.

achew22 · 2021-02-20T01:42:23Z

Well... I don't think you're blocked by rules_go any more then 😉

glerchundi · 2021-02-20T16:58:51Z

Whoop I didn't notice that this PR was created before creating a new one #1988 after we decided to close the attempt I made months ago #1834.

My approach in the new PR is to just make protoc-gen-grpc-gateway & protoc-gen-openapiv2 compatible with the latest protoc >3.15 releases without changing the current generators behaviour.

Feel free to do whatever you want with this new PR.

google-cla bot added the cla: yes label Feb 5, 2021

adambabik force-pushed the add-support-for-proto3-optional branch from 1a5b78d to 50ee7ec Compare February 5, 2021 15:56

adambabik mentioned this pull request Feb 5, 2021

Support optional annotation in proto3 files in generators #1278

Closed

adambabik force-pushed the add-support-for-proto3-optional branch from 50ee7ec to 756c77b Compare February 5, 2021 16:20

johanbrandhorst requested changes Feb 6, 2021

View reviewed changes

adambabik force-pushed the add-support-for-proto3-optional branch 5 times, most recently from cca9e5d to 4700507 Compare February 11, 2021 18:04

johanbrandhorst requested changes Feb 12, 2021

View reviewed changes

adambabik force-pushed the add-support-for-proto3-optional branch 2 times, most recently from 3c64f2c to a938514 Compare February 12, 2021 21:54

johanbrandhorst approved these changes Feb 13, 2021

View reviewed changes

adambabik force-pushed the add-support-for-proto3-optional branch from a938514 to d6767e9 Compare February 18, 2021 17:42

johanbrandhorst mentioned this pull request Feb 20, 2021

protoc-gen-{grpc-gateway,openapiv2}: add optional support #1988

Closed

Adam Babik and others added 3 commits February 22, 2021 17:33

gen-grpc-gateway, gen-openapiv2: add support for proto3 optional

1cd9c20

internal/descriptor: add support for proto3 optional

f0a50d7

internal/descriptor: forbid proto3 optional fields from URL path

bbd3444

adambabik force-pushed the add-support-for-proto3-optional branch from d6767e9 to bbd3444 Compare February 22, 2021 16:33

adambabik merged commit 0bbdda9 into grpc-ecosystem:master Feb 24, 2021

adambabik deleted the add-support-for-proto3-optional branch February 24, 2021 08:53

huan mentioned this pull request Mar 17, 2021

Proto 3.15 supports optional again! wechaty/grpc#109

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gen-grpc-gateway, gen-openapiv2: add support for proto3 optional #1951

gen-grpc-gateway, gen-openapiv2: add support for proto3 optional #1951

adambabik commented Feb 5, 2021

adambabik commented Feb 5, 2021

achew22 commented Feb 5, 2021

johanbrandhorst left a comment

adambabik commented Feb 11, 2021

johanbrandhorst left a comment

johanbrandhorst Feb 12, 2021

adambabik Feb 12, 2021

johanbrandhorst Feb 12, 2021

johanbrandhorst Feb 12, 2021

adambabik Feb 12, 2021

adambabik Feb 12, 2021

johanbrandhorst left a comment

johanbrandhorst commented Feb 13, 2021

johanbrandhorst commented Feb 18, 2021

mvrhov commented Feb 19, 2021 •

edited

Loading

achew22 commented Feb 20, 2021

glerchundi commented Feb 20, 2021

gen-grpc-gateway, gen-openapiv2: add support for proto3 optional #1951

gen-grpc-gateway, gen-openapiv2: add support for proto3 optional #1951

Conversation

adambabik commented Feb 5, 2021

adambabik commented Feb 5, 2021

achew22 commented Feb 5, 2021

johanbrandhorst left a comment

Choose a reason for hiding this comment

adambabik commented Feb 11, 2021

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Feb 12, 2021

Choose a reason for hiding this comment

adambabik Feb 12, 2021

Choose a reason for hiding this comment

johanbrandhorst Feb 12, 2021

Choose a reason for hiding this comment

johanbrandhorst Feb 12, 2021

Choose a reason for hiding this comment

adambabik Feb 12, 2021

Choose a reason for hiding this comment

adambabik Feb 12, 2021

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Feb 13, 2021

johanbrandhorst commented Feb 18, 2021

mvrhov commented Feb 19, 2021 • edited Loading

achew22 commented Feb 20, 2021

glerchundi commented Feb 20, 2021

mvrhov commented Feb 19, 2021 •

edited

Loading