protoc-gen-openapiv2: `go_package` option is required since version 2.0.1

plaflamme commented 3 years ago

🐛 Bug Report

Since version 2.0.1, the protobuf option go_package is required to generate the gateway or the OpenAPI schema.

To Reproduce

Using this .proto file:

syntax = "proto3";
package your.service.v1;

message StringMessage {
  string value = 1;
}

service YourService {
  rpc Echo(StringMessage) returns (StringMessage) {}
}

Running protoc like so:

protoc -I . --grpc-gateway_out ./output my_service.proto

The output is:

protoc-gen-grpc-gateway: unable to determine Go import path for "my_service.proto"

Please specify either:
    • a "go_package" option in the .proto source file, or
    • a "M" argument on the command line.

See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information.

(The problem is the same with protoc-gen-openapiv2)

Expected behavior

The go_package option shouldn't be require, at least to produce the OpenAPI output since this value has no meaning for this generator.

Ideally, it wouldn't be required for generating the gateway as well since the gateway is meant to be a runnable binary and not linked against as code.

Actual Behavior

protoc-gen-grpc-gateway: unable to determine Go import path for "my_service.proto"

Please specify either:
    • a "go_package" option in the .proto source file, or
    • a "M" argument on the command line.

See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information.

Your Environment

MacOS Catalina, gen-gateway and openapiv2 from 2.0.1 to 2.4.0 all exhibit the same behaviour

protoc --version
libprotoc 3.14.0

johanbrandhorst commented 3 years ago

Hm, yes, the openapiv2 behaviour is definitely incorrect, that shouldn't require any go specific settings at all. The protoc-gen-grpc-gateway is correct though and aligns with the behaviour of protoc-gen-go and protoc-gen-go-grpc, like it or not. I think we should try to fix it for the openapiv2 generator though.

plaflamme commented 3 years ago

@johanbrandhorst I understand that a lot of protobuf users are on the Go stack, but requiring the go_package option is pretty crippling for other stacks.

A workaround is to use the M option, e.g.: --grpc-gateway_opt Msomefile.proto=example.com/some/package but this becomes quickly unmanageable when there are lots of files without the declaration.

More importantly though, the package that is declared using this workaround has absolutely no impact on the resulting gateway binary. One can specify any (valid go package) value for the package and the resulting binary would work the same regardless. As such, it's questionable for it to be required, i.e.: as a user, I shouldn't be required to specify something that has no visible impact. IMO this only introduces friction for no user benefit.

If grpc-gateway is meant to be used for non-Go use cases (which I would assume it is), then I would suggest the following:

define a mechanism to derive a Go package name from the protobuf package name, e.g. my.package -> fake.com/my/package (presumably, this exists somewhere since go_package wasn't required)

Then do one of:

enable this behaviour by default (my suggestion)
enable this behaviour by default, warn the user when it is used
enable this behaviour by default, add a flag to disable it and fail when missing
enable this behaviour through a flag, perhaps taking the "root" as an argument, e.g.: go_package_root=fake.com

johanbrandhorst commented 3 years ago

I don't agree with the premise of your argument - the grpc-gateway generator generates a library, it is absolutely tied to the Go ecosystem. I don't know how you're using it which makes you think it generates a binary, because the gateway output is not buildable on its own.

More importantly though, the package that is declared using this workaround has absolutely no impact on the resulting gateway binary. One can specify any (valid go package) value for the package and the resulting binary would work the same regardless. As such, it's questionable for it to be required, i.e.: as a user, I shouldn't be required to specify something that has no visible impact. IMO this only introduces friction for no user benefit.

This is specific to your own use case and is not generally true. The go_package option informs how other package should import the generated library.

It's definitely correct to say that it's supposed to be accessible to users from other ecosystems, as a simple proxy in front of your gRPC server, but it's wrong to assume it's not tied to the Go dependency ecosystem.

Again, for protoc-gen-openapiv2, none of the above is true and we shouldn't require this option to be set.

Sidenote: I work at Buf and we're working on making the go_package managed for you in our compiler, so you don't have to set it in your files.

plaflamme commented 3 years ago

@johanbrandhorst I have to admit that I was assuming a few things here, namely, that the output is Go code that compiles to a binary. For some reason, I had a vague memory of this being the case. The documentation clearly states that you need to roll your own main.go, so I'm not sure why I assumed this. My bad.

Indeed, considering that the output is a library, it does make sense that go_package is required for the grpc-gateway plugin.

Presumably, the generator could be renamed to something like go-grpc-gateway to avoid making wrong assumptions like I did. But it's probably not worth the confusion to all other existing users...

In any case, the actual issue I have is with the openapiv2 plugin, so I'm happy to hear this can get fixed for that use case 👍

Sidenote: we're (partial) users of buf already (though we only use the linting plugin for now), so I'm happy to hear we'll get additional benefits by embracing more of it.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

amitz commented 3 years ago

Leaving protoc-gen-grpc-gateway, it sounds like the warning shouldn't be there for protoc-gen-openapiv2? is it just a matter of removing this parameter from the protoc call? I'm creating multiple OpenAPI json files, and getting bombarded by this warning.

johanbrandhorst commented 3 years ago

Agreed, it should not be there for protoc-gen-openapiv2. I haven't looked into exactly what the cause is for the warning, but I would be happy to review a proposed fix.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mmellin commented 3 years ago

Hi, we have run into this error when building with Bazel now many times since upgrading to grpc-gateway v2.7.1. We have autogenerated .proto files created by the github.com/openconfig/ygot tool which apparently doesn't yet support an option to include a go_package = in the file, so when running these proto files through protoc_gen_openapiv2() rules it breaks our builds.

Is there any workaround here?

 > Please specify either:
 >      • a "go_package" option in the .proto source file, or
 >      • a "M" argument on the command line.

johanbrandhorst commented 3 years ago

Not sure why stalebot closed this, I marked it as a bug 🤔. I think we use the protoc-gen-openapiv2 rules in this repository in our own tests but I'm actually not much of a bazel expert. Maybe @achew22 can weigh in on the particulars of our rules.

amitz commented 2 years ago

Sorry! I did some research about it, and completely forgot to update :man_facepalming:

It's been a while, so I might be wrong here, but from what I gathered - protoc-gen-openapiv2 uses the Go Protobuf plugin to parse the Protobuf files. The error you're seeing, coming from the Go plugin, and not from the OpenAPI plugin. There's some information Here about this change.

What I did back at the day to "quickly hack it", was to use an older Go Protobuf version. In the past, this has been a warning, and not an error. I didn't mind using an older version, but if you do - the only possible solution I can think of, is to use go_package.

@achew22 should have a 2nd look, but I think this is not something grpc-gateway can solve...

ddeath commented 1 month ago

Hi just my 2 cents..... if you are using buf to generate the code, then you can "bypass" this by setting:

    - file_option: go_package
      value: special/value/package/v1

in buf.gen.yaml so it will look like:

version: v2
managed:
  enabled: true
  override:
    - file_option: java_package_prefix
      value: ""
    - file_option: go_package
      value: special/value/package/v1
plugins:
....

it will get rid of the error

leungster commented 1 month ago

We're importing an external proto into our own where we don't control the proto's go_package. Is there a way to supply an override with M{PROTO_FILE}={PATH} to get around this?

rules_go does this to always supply a go_package via the M flag.

johanbrandhorst commented 4 weeks ago

Unsure if it's possible to override this with an option, sorry.

leungster commented 4 weeks ago

I ended up writing another bazel rule to pass in M{PROTO_FILE}={PATH} as openapiv2_opt flags. It doesn't autodetect the go package names but we can at least supply a static mapping like what we already do with gazelle.

grpc-ecosystem / grpc-gateway