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

[glerchundi]

Let protoc know that these generators support optional keywords. This
is by no means an implementation in grpc-gateway of the optional aware
generated code, it's just shielding of being considered as blockers in
code generation workflows.

Closes #1278

[johanbrandhorst]

Thanks for this Gorka. I'm a little worried about just adding "support" for this without thinking about what that should mean. The grpc-gateway isn't responsible for generating any types, so we don't need to worry about generating pointers to primitive fields, but I think we should at least have a think about what this could mean for the generator. Do you have any optional fields you can test this PR on? It's not clear to me exactly what we would need to change, if anything, so it'd be good to have some practical experience.

CC @achew22 @tmc @ivucica, thoughts?

[achew22]

I couldn't agree more. I think next steps here are to create a test case that falls without this patch applied and then we can discuss what is the best course forward.

Thanks so much for contributing, this is a really important emerging feature in the proto3 ecosystem and I'm sorry I don't know enough about the interactions of everything to be able to definitively say much about it.

[glerchundi]

Thanks for your time @johanbrandhorst & @achew22.

I have been thinking on how to prepare the current code to achieve testing over the final generator/executable and my impression is that the quantity of work it requires it's not worth it. Even so, if you consider it is, sadly I won't be able to work on this as my time currently is very limited.

I tried to find examples of other generators to see how they approach the very same problem we've here. Fortunately I found the grpc-go generator quite interesting and similar to our generators. It's also a generator where the optional keyword is not needed (as it's not used at the service level), but should be flagged like it's supporting it. Another interesting thing about their support for optionals is that they didn't add tests for this case. Although I don't know if the missing test is on purpose (due to its value vs cost) or not: grpc/grpc-go#3914

I don't know if it's enough for you, but I empirically tested and it worked. This is what the generator says if we have a message with a field which has an "optional" and without applying these changes:

$ protoc --experimental_allow_proto3_optional --grpc-gateway_out . test.proto 
test.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-grpc-gateway hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional.--grpc-gateway_out: 

WDYT?

[achew22]

@glerchundi, looks very promising to me. Could I have you add a testcase into our repo that uses optional so that we are sure we don't regress. The real test of this case will be, if the lines you have in the current PR are commented out, will this test case fail. @johanbrandhorst am I way off base here? Is there more that you think we would need?

[glerchundi]

That would work and it's trivial, at least for the optional part. I'm not quite sure how difficult would be to include the experimental and global parameter when executing the protoc. I never used Bazel and i'm not sure if it's what we needed but seems to be related: bazelbuild/rules_proto#71 (comment)

[johanbrandhorst]

That would work and it's trivial, at least for the optional part. I'm not quite sure how difficult would be to include the experimental and global parameter when executing the protoc. I never used Bazel and I'm not sure if it's what we needed but seems to be related: bazelbuild/rules_proto#71 (comment)

For this sort of test, we'll probably add a new file with some optionals set at various levels and just add the flags to the generation step in the Makefile. It should hopefully be straightforward enough to copy one of the existing targets and apply the flags. We can figure out the bazel thing separately if you can get a new Makefile target in place. Let me know if you need any help.

@glerchundi, looks very promising to me. Could I have you add a testcase into our repo that uses optional so that we are sure we don't regress. The real test of this case will be, if the lines you have in the current PR are commented out, will this test case fail. @johanbrandhorst am I way off base here? Is there more that you think we would need

The only thing I can think of is that we could potentially use these annotations in the openapiv2 generator, but not in the gateway generator.

[glerchundi]

I must admit that I'm having a really hard time trying to figure out how to solve bazel issues. I tested that the absence of the protoc flag (--experimental_allow_proto3_optional) makes the bazel and examples/generate targets fail:

bazel:

examples:

But now it's failing due to this:

[0 / 4] [Prepa] BazelWorkspaceStatusAction stable-status.txt
ERROR: /root/.cache/_grpc_gateway_bazel/external/com_google_protobuf/BUILD:284:15: Generating Descriptor Set proto_library @com_google_protobuf//:any_proto failed (Exit 1) protoc failed: error executing command bazel-out/host/bin/external/com_google_protobuf/protoc '--proto_path=bazel-out/host/bin/external/com_google_protobuf/_virtual_imports/any_proto' ... (remaining 7 argument(s) skipped)

It seems like protoc command is not including

I don't know how to proceed, would you mind guiding me in the right direction?

[johanbrandhorst]

I can try and take a look at the bazel errors tomorrow, or maybe Andrew will find some time later today ;). Thanks for your efforts so far.

[glerchundi]

Thanks for the review @johanbrandhorst

I'm not quite sure, but as we're using buildifier which is built using Go and uses go_proto_library, I think we're stuck until they decide what to do with this issue: bazel-contrib/rules_go#2673

Just a supposition as I don't have the needed knowledge around Bazel.

[johanbrandhorst]

Yeah I suspected we might be at the mercy of rules_go as is often the case 😬.

[johanbrandhorst]

I'm going to close this for now, sorry for the hassle @glerchundi. I'd be happy to reopen this once there is support in the Go Bazel toolchain.

[glerchundi]

No worries, I'll keep an eye in those two issues.