New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflection library is tightly coupled with golang/protobuf #1873

Open
johanbrandhorst opened this Issue Feb 17, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@johanbrandhorst
Contributor

johanbrandhorst commented Feb 17, 2018

What version of gRPC are you using?

gRPC 1.10

What version of Go are you using (go version)?

go version go1.10 linux/amd64

What operating system (Linux, Windows, …) and version?

$ uname -a
Linux johan-x1 4.15.3-2-ARCH #1 SMP PREEMPT Thu Feb 15 00:13:49 UTC 2018 x86_64 GNU/Linux

What did you do?

I tried using grpc/reflection library with gogo/protobuf.

What did you expect to see?

I expected to be able to use an alternative to golang/protobuf with gRPC reflection.

What did you see instead?

I found that the gRPC reflection library imports github.com/golang/protobuf. I also found it is tightly coupled to this implementation. As I understand, gRPC is meant to be codec agnostic, what with official support for flatbuffers, and the nice Codec interface existing in grpc-go. Given this, it is surprising that it is so tighly coupled to golang/protobuf, and perhaps by extension so coupled to protobuf itself.

I propose some sort of interface is defined in the reflection library so that a user can choose which backend to make use of for resolving protobuf types. As I see it, the reflection library makes use of the following functions (illustrated as an interface) from golang/protobuf:

type Protoer interface {
    MessageType(string) reflect.Type
    FileDescriptor(string) []byte
    Unmarshal([]byte, proto.Message) error
    Marshal(proto.Message) ([]byte, error)
    RegisteredExtensions(proto.Message) map[int32]*proto.ExtensionDesc
}

Ideally, I'd like to be able to use gogo/protobuf with gRPC reflection, but the current implementation would force me to reimplement the reflection server in gogo/protobuf manually.

Any thoughts on this greatly appreciated.

@menghanl

This comment has been minimized.

Contributor

menghanl commented Feb 21, 2018

This proposal SGTM.

Assuming this interface will be defined with golang/protobuf. The gogo/protobuf implementation would also need to take golang/protobuf.Message as input, and also return golang/proto.ExtensionDesc.
I'm not familiar with those. Will it be easy to convert between golang/protobuf types and gogo/protobuf types?


Some other thoughts:
Another possible abstraction is to separate the reflection service handler with the layer that provides the data. The reflection protocol is designed for protobuf, for example extension, but it's still potentially useful for other codecs.

This would also require the underlying message interchange format to support similar features as protobuf's file descriptor.
We can hold this and implement the proto only change you suggested first.

@johanbrandhorst

This comment has been minimized.

Contributor

johanbrandhorst commented Feb 21, 2018

@menghanl Thanks for your reply. The nested nature of the proto.Message definition is a problem, as gogo/protobuf has managed to avoid importing anything defined in golang/protobuf thus far. This could still be solved with a client compatibility layer wrapping a Protoer implementation in gogo/protobuf since any gogo/protobuf.Message and golang/protobuf.Message will be functionally equivalent, if technically separate types.

Inviting @awalterschulze (maintainer of gogo/protobuf) to chime in.

@awalterschulze

This comment has been minimized.

awalterschulze commented Feb 22, 2018

Totally, gogo/protobuf/proto would like to avoid importing golang/protobuf/proto, because currently gogo/protobuf has zero dependencies and would like to keep it that way.
Also some users, including (past) me would prefer to have one implementation of the proto library as a dependency. This also becomes really hard with grpc-go.

A few years ago, when I was a user, I used to take a git subtree of grpc-go and change all the import paths from github.com/golang/protobuf/proto to github.com/gogo/protobuf/proto.
That way goimports can automatically resolve to the correct import.

So maybe I would ask, is it possible to create an interface that avoids importing any serialization library, including flatbuffers?

For two reasons:

  1. The reason above
  2. golang interfaces don't really nest well, which means that you must import interface A if you want to implement interface B which has a method that uses interface A, which goes against the whole implicit interface goal in my opinion, but thats a separate issue.
@prasek

This comment has been minimized.

prasek commented Apr 23, 2018

@awalterschulze, I'm porting github.com/jhump/protoreflect to gogo and have an experimental branch with a working protoer interface along the lines of what @johanbrandhorst suggested with zero dependencies: https://github.com/prasek/protoreflect/blob/protoer/proto/protoer.go

The gogo and golang wrappers implement UntypedProtoer
https://github.com/prasek/protoreflect/blob/protoer/proto/gogo/protoer.go
https://github.com/prasek/protoreflect/blob/protoer/proto/golang/protoer.go

type Protoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb Message) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb Message) error
	Marshal(pb Message) ([]byte, error)
	GetExtension(pb Message, field int32) (extval interface{}, err error)
	EnsureNativeMessage(pb Message) (pbout Message, err error)
}

type Message interface {
	Reset()
	String() string
	ProtoMessage()
}

type UntypedProtoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb interface{}) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb interface{}) error
	Marshal(pb interface{}) ([]byte, error)
	GetExtension(pb interface{}, field int32) (extval interface{}, err error)
	EnsureNativeMessage(pb interface{}) (pbout interface{}, err error)
}
@johanbrandhorst

This comment has been minimized.

Contributor

johanbrandhorst commented Apr 24, 2018

@prasek fantastic work! What would be required to integrate this with gogo/golang/gRPC? I'm guessing we'd need these wrappers added to each implementation? I'm sure @dsnet will have opinions on this as well.

@awalterschulze

This comment has been minimized.

awalterschulze commented Apr 24, 2018

Cool :)

May I ask what is the use case for EnsureNativeMessage?

@prasek

This comment has been minimized.

prasek commented Apr 24, 2018

@awalterschulze, EnsureNativeMessage should really be called NativeDescriptor. There are two use cases for NativeDescriptor, one internal to the wrappers to ensure GetExtension() and RegisteredExtensions() get types they recognize, and the other allows libraries to accept either gogo or golang descriptor types by converting them with NativeDescriptor of a separate protoer instance that matches the package used by the library.

For example https://github.com/prasek/protoreflect/blob/protoer/desc/builder.go#L19 (experimental branch) accepts gogo and golang descriptors for some methods but internally has to pick one to use. NativeDescriptor() checks the descriptor type (gogo or golang) and converts it if not the desired type so it will work with descriptor types (gogo or golang) used by the library. The downside to this approach is descriptors are exported as Message and have to be type asserted by the caller, so I'd be fine removing NativeDescriptor from the Protoer interface and just using it internally in the wrappers.

@prasek

This comment has been minimized.

prasek commented Apr 24, 2018

@johanbrandhorst, ideally the packages would get split into grpc, golang/protobuf, and gogo/protobuf respectively. There are no dependencies across the packages.

The only difference between the gogo and golang protobuf protoer are the imports and default aliases.

@prasek

This comment has been minimized.

prasek commented Apr 25, 2018

Here are the updated interfaces with RegisteredExtensions added and NativeDescriptor removed.
https://github.com/prasek/protoreflect/blob/protoer/proto/protoer.go

type Protoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb Message) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb Message) error
	Marshal(pb Message) ([]byte, error)
	RegisteredExtensions(pb Message, desiredType interface{}) (extensions interface{}, err error)
	GetExtension(pb Message, field int32) (extval interface{}, err error)
}

type Message interface {
	Reset()
	String() string
	ProtoMessage()
}

type UntypedProtoer interface {
	MessageType(name string) reflect.Type
	MessageName(pb interface{}) string
	FileDescriptor(file string) []byte
	Unmarshal(b []byte, pb interface{}) error
	Marshal(pb interface{}) ([]byte, error)
	RegisteredExtensions(pb interface{}, desiredType interface{}) (interface{}, error)
	GetExtension(pb interface{}, field int32) (extval interface{}, err error)
}

Note for RegisteredExtensions() the desiredType is needed to check and convert if needed.

extensions, err := proto.RegisteredExtensions(pb, (map[int32]*gogo.ExtensionDesc)(nil))
@dsnet

This comment has been minimized.

dsnet commented Apr 25, 2018

A protobuf reflection API that has Unmarshal and Marshal in its interface is probably too high level. Just as Go reflection is abstraction around the Go language itself, protobuf reflection should be an abstraction around the protobuf language itself. As such, it needs to go even lower-level than Unmarshal and Marshal and provide behavior at the field level in order to be reflexive over all aspect of the proto language.

Adding reflection functionality into the mainline golang/protobuf repo has been a project I've been working on for some time now and turns out to be a difficult task as there are many devils in the details. A reflection API needs to thought in the context of the totality of how Go protobufs operates. As I've been working on a reflection API, I've had to scrap my design and start over many times.

I recently finished writing up the design and published some documents regarding the design: golang/protobuf#364 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment