mwitkow / go-proto-validators

Generate message validators from .proto annotations.
Apache License 2.0
1.09k stars 164 forks source link

Protofile structure is unidiomatic #34

Open johanbrandhorst opened 6 years ago

johanbrandhorst commented 6 years ago

The go_package option is not declaring a path relative to the $GOPATH/src directory, as is idiomatic. This leads to packages importing it having to use a relative import. It also leads to the unidiomatic file registration.

In my experiments with gRPC reflection, this makes any protofiles using validators completely impossible to use, as reflection forces proto files to be imported in the same fashion as they are registered, which in this case is import "validator.proto". A proper go_package path might solve this as well, but I'm not sure.

mwitkow commented 6 years ago

Ok, I get the point. But a couple of things:

johanbrandhorst commented 6 years ago

Happy to see you back on GitHub :).

The registered type name in this case means that any gRPC server that uses the validator in any of its proto files, or imports a proto file that uses a validator cannot use reflection at all. So it's not so much about being able to reflect on the validation itself but the incompatibility it creates.

The fix would be to change the go_package option to specify a package relative to the gopath src directory, and regenerate the file, (e.g. github.com/mwitkow/go-proto-validators;validator).

Off the top of my head I don't think it should be a breaking change but I don't have my laptop to hand to test right now.

mwitkow commented 6 years ago

Oh I wasn't aware that the grpc reflection was broken. We don't use it ourselves.

Can you iron out a PR with a fix as you seem to have the environment to test it?

johanbrandhorst commented 6 years ago

I'll see what I can do.

johanbrandhorst commented 6 years ago

Having done some cursory investigating, I think this would be blocked on https://github.com/mwitkow/go-proto-validators/issues/26 anyway, as the generated file registers with gogo/protobuf explicitly, and grpc-go reflection is tightly coupled with golang/protobuf. For anyone else wanting to test this, I created a repo with a simple reflection environment.

nzoschke commented 6 years ago

I am having trouble with this too.

I am adding the import and validator options in my .proto:

syntax = "proto3";

package sources.v0;

import "google/protobuf/timestamp.proto";
import "github.com/mwitkow/go-proto-validators/validator.proto";

option go_package = "github.com/myorg/proto-api/server/sources/v0;sources";

service Sources {
  rpc Get (SourcesGetRequest) returns (SourcesGetResponse) {}
  rpc List (SourcesListRequest) returns (SourcesListResponse) {}
}

message SourcesGetRequest {
    string name = 1 [(validator.field) = {regex: "^[a-z]{2,5}$"}];
}

Things build just fine with:

//go:generate protoc --go_out=plugins=grpc:${GOPATH}/src --govalidators_out=${GOPATH}/src -I ../../../proto/sources/v0 --proto_path=${GOPATH}/src sources.proto

I can see my service:

$ grpcurl -plaintext localhost:3000 list
grpc.reflection.v1alpha.ServerReflection
helloworld.Greeter
sources.v0.Sources

But I can't use it due to errors:

$ grpcurl -plaintext localhost:3000 describe sources.v0.Sources
Failed to resolve symbol "sources.v0.Sources": Symbol not found: sources.v0.Sources
caused by: File not found: github.com/mwitkow/go-proto-validators/validator.proto

$ grpcurl -plaintext -d '{"name": "test"}' localhost:3000 sources.v0.Sources.Get
Error invoking method "sources.v0.Sources.Get": target server does not expose service "sources.v0.Sources"

If I remove the import "github.com/mwitkow/go-proto-validators/validator.proto"; things are fine.

johanbrandhorst commented 6 years ago

This is because go-proto-validators registers with the gogoprotobuf registry. See https://github.com/mwitkow/go-proto-validators/blob/0950a79900071e9f3f5979b78078c599376422fd/validator.pb.go#L16

mennanov commented 6 years ago

Is there any workaround? I can't use grpc_cli tool to test my service because i'm getting similar errors to what @nzoschke is getting.

I0719 17:32:45.627790000 140735610065792 proto_reflection_descriptor_database.cc:84] NOT_FOUND from server for FindFileByName(github.com/mwitkow/go-proto-validators/validator.proto) [libprotobuf ERROR google/protobuf/descriptor.cc:3632] Invalid proto descriptor for file "accounts.proto": [libprotobuf ERROR google/protobuf/descriptor.cc:3635] accounts.proto: Import "github.com/mwitkow/go-proto-validators/validator.proto" was not found or had errors.

johanbrandhorst commented 6 years ago

Unfortunately, gogoproto simply does not work with gRPC reflection. See https://jbrandhorst.com/post/gogoproto/ for more information.