FileDescriptor uses filename as a reference, which breaks reflection when protofile is renamed, but Go code is not recompiled

[utrack]

(started the write-up as an issue for golang/protobuf and then realized that the error text comes from protoreflect, please let me know if I should refile it to golang/protobuf!)

Short description Protofile FileDescriptor registry uses an assumed path to proto as a key, breaking the reflection when Go-compiled FD is imported from an 'external' package, and imported proto of an external package has a different file name

Long description Protofiles and Go libraries are mostly disjointed - meaning that I can move the protofile somewhere else and refer it to an original Go package via the go_package directive.

External Go packages in relation to the current one are imported instead of being built by default - meaning that emitted Go code whose source proto refers to the standard google/type/datetime.proto will import the Go package google.golang.org/genproto/googleapis/type/datetime instead of compiling its own version somewhere. It happens because datetime.proto has option go_package = "google.golang.org/genproto/googleapis/type/datetime;datetime"; set.

The thing is, compiled Go code has raw FileDescriptor built in (desc for datetime.proto), which has the name of the file built-in in the first few bytes. Here's the (mangled) print-out of datetime.pb.go description:

google/type/datetime.protogoogle.typegoogle/protobuf/duration.proto"
DateTime
year
 
(Ryear
month 
(Rmonth
...

---

Let's say I decide to organize protofiles according to their FQDNs. I copy datetime.proto to ./github.com/googleapis/googleapis!/google/type/datetime.proto to resolve any possible conflicts, and I import it as github.com/googleapis/googleapis!/google/type/datetime.proto in my own proto.

The Go code compiles successfully, and everything works except for the reflection. I suspect datetime's FD registers itself as google/type/datetime.proto, but my protofile looks it up by github.com/googleapis/googleapis!/google/type/datetime.proto - and the resolution fails.

It probably makes sense to register a file using its resulting Go package+filename.

---

What version of protobuf and what language are you using? libprotoc 3.21.12 protoc-gen-go v1.28.1

What did you do? I've copied a standard google/type/datetime.proto to another location and left everything else as-is. I've imported this file using a new path.

What did you expect to see? Reflection should work via grpcurl -plaintext -emit-defaults -vv localhost:8081 list pet.v1.PetStore

What did you see instead?

Failed to list methods for service "pet.v1.PetStore": file "path/to/own/proto/test-service.proto" included an unresolvable reference to ".google.type.DateTime"

Steps to reproduce

cp datetime.proto some/other/path/datetime.proto

own proto

syntax = "proto3";

package pet.v1;

import "some/other/path/datetime.proto";// original: google/type/datetime.proto

option go_package = "gitlab.com/foo/bar/pkg/testapppb.v1";

// Pet represents a pet in the pet store.
message Pet {
  PetType pet_type = 1;
  string pet_id = 2;
  string name = 3;
  google.type.DateTime created_at = 4;
}

[utrack]

Oh, just saw #170 - this one can be closed as a duplicate

[jhump]

@utrack, that issue is for linking descriptors that are downloaded via service reflection.

For descriptors that are actually compiled into your Go program (registered from generated code), there is already API for this: desc.ImportResolver. It requires you to manually map your custom/non-standard relative import path to the file's "canonical" import path.

However, I am very surprised that what you are doing even works. I thought as of v1.4.0 of github.com/golang/protobuf, it would no longer let you register files that had mismatching imports this way (thus mostly eliminating the need for ImportResolver). What version of the protobuf runtime and protoc-gen-go plugin are you using?

FWIW, a proto source file's import path is really meant to be singular -- meaning all files import it via the same import path. So you're deciding to import it as "some/other/path/datetime.proto" is not really supposed to work. I know this isn't documented well in the protobuf.dev site (maybe not at all?), but this is an assumption baked into several runtimes. I think the Java runtime has ways of linking file descriptors that does not rely on the import path this way, but Go and C++ runtimes (and probably others) don't work if you try to import the file using a different import path than how that file was actually compiled. Its relative import path is really part of the file's identity for runtime reflection support.

[utrack]

Hey, thanks for clarifying! Those packages are in my go.mod:

    google.golang.org/protobuf v1.28.1
    github.com/golang/protobuf v1.5.2 // indirect
    github.com/jhump/protoreflect v1.12.0 // indirect

I've generated my protos with:

//  protoc-gen-go v1.28.1
//  protoc        v3.21.12

So it seems that it actually quietly 'loses' data (well not loses, but debugging it was pretty hard...)

That's a bummer. For context - I've written some go get-like tool for the protobufs which actually resolves all those relative imports to full paths like github.com/foo/bar/baz/qux.proto; however, because of this bug, it seems to be a dead end. I'll check out the ImportResolver stuff, thank you!

[utrack]

Hm, this is a grpcui issue indeed - it downloads the defs via reflection and then returns an error. If I do the ImportResolver thing server side - it won't fix the client, right?

[jhump]

Hm, this is a grpcui issue indeed - it downloads the defs via reflection and then returns an error. If I do the ImportResolver thing server side - it won't fix the client, right?

Ah, bummer. Correct, handling this server-side will not aid clients that use the gRPC server reflection service. I would strongly recommend using canonical import paths instead of re-writing the imports with more qualifiers.

You might take a look at Buf which is meant to make it easier to define dependencies and perform code generation compared to protoc (among other things).

[jhump]

Duplicate of #170