Skip to content
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

types/dynamicpb: add support for dynamic messages #199

Closed
mwitkow opened this issue Jul 1, 2016 · 12 comments
Closed

types/dynamicpb: add support for dynamic messages #199

mwitkow opened this issue Jul 1, 2016 · 12 comments

Comments

@mwitkow
Copy link

mwitkow commented Jul 1, 2016

Since gRPC now has support for server-side reflection (https://github.com/grpc/grpc-go/tree/master/reflection, basically stubby ls) it would be able to process dynamic messages in Go.

I'd be interested in adding this support either in protobuf or in an outside, but would probably need some restructuring in protobuf itself: e.g. allowing to use the same encoding/decoding methods but with the schema driven from a FileDescriptorProto instead of Golang tags.

@awalterschulze
Copy link

@abe-winter
Copy link

I have a proof-of-concept of this working in a fork
https://github.com/abe-winter/dynproto

@Merovius
Copy link

@abe-winter Awesome :) IIt would be great if it wouldn't depend on cgo, though. Is there a reason you didn't use descriptors in proto-encoding?

@abe-winter
Copy link

@Merovius the output of ParseSchema is a FileDescriptorProto, and all the downstream code uses the proto. My use case requires schema parsing. An alternative is to shell out to the protoc program, but then protoc needs to be installed everywhere.

Would it make sense to set a // +build cgo on the parsing code? I'm not super-familiar with golang, but I think that makes cgo a soft dependency.

@abe-winter
Copy link

☝️ I was wrong above -- I'm already relying on libprotobuf being installed, might as well rely on protoc.

I'll move schema parsing to its own package so it's truly optional.

@Merovius
Copy link

Great, I think it's a good idea to have the schema-parsing in a separate package.

Another change I would suggest, is to add a DynamicMessage (or similar) struct which implements proto.Message statically and embeds/contains a *reflect.Type. DynStruct could then return a []*DynamicMessage, instead of a []*reflect.Type. The advantage would be, that a) Marshal/Unmarshal could remain type-safe and take a proto.Message and b) packages which wrap proto in a message-type agnostic way (grpc reflection comes to mind), could also use the dynamic message without breaking their type-safety.

@abe-winter
Copy link

Hmm. You have two points here:

  • grpc and other codebases use proto.Message internally and so DynStruct should support the Message interface.
    • This sounds right but I'm worried about making this change without a testing plan. I don't know what grpc would do with dynamic messages and what the requirements would be in practice.
  • Buffer.Marshal/Unmarshal gain a safety advantage from taking Message instead of empty interface{}.
    • This sounds wrong to me. Nothing inside those functions calls the Message methods. We can implement Message and Marshaler for your DynamicMessage struct, but this would just be copying around parts of Marshal/Unmarshal. Not sure there's any advantage.

@jhump
Copy link
Contributor

jhump commented Feb 17, 2017

I've been working on something related to this, to be able to use the service reflection to build clients. I haven't gotten to a point of implementing the equivalent of DynamicMessage, but I do have better descriptors (like protobuf provides in Java and C++ runtimes) as well as a better client interface for the reflection service. I think the descriptors would be a suitable base for pure-Go implementation of a dynamic message.
https://github.com/jhump/protoreflect

@jhump
Copy link
Contributor

jhump commented Apr 3, 2017

FWIW, I have an all-Go implementation of dynamic messages:
https://godoc.org/github.com/jhump/protoreflect/dynamic
And there's also a dynamic RPC stub, that invokes RPCs using method descriptors and dynamic messages:
https://godoc.org/github.com/jhump/protoreflect/dynamic/grpcdynamic

It's not 100% yet -- mainly missing useful support for proto3 well-known-types and knobs for better controlling JSON output (for example). I'd say it's currently beta-quality right now -- not a lot of miles on it other than the tests, and there are several TODOs for more tests (though a lot of the code does get exercised by the tests that are present).

The *dynamic.Message type does implement proto.Message and it also has a Descriptor method that is compatible with that of generated messages. It implements proto.Marshaler, proto.Unmarshaler, encoding.TextMarshaler, and encoding.TextUnmarshaler so it can also be used with functions in the proto package for serializing and de-serializing.

It can't yet be used with the jsonpb package. I've opened a pull request that, if merged, would remedy that (#325).

Of course, APIs that accept a proto.Message which in turn use proto.GetProperties, proto.MessageName, or any of the extension-related messages in the proto package will not work with this new type. Those methods all assume the object not only implements the interface, but that it also is a registered generated type.

@dsnet dsnet changed the title Add support for DynamicMessages proposal: add support for DynamicMessages Feb 14, 2018
@dsnet dsnet added the proposal label Feb 14, 2018
@dsnet
Copy link
Member

dsnet commented Mar 9, 2018

Placing on hold until #364 is complete.

@dsnet dsnet added the api-v2 label Aug 2, 2018
@dsnet dsnet changed the title proposal: add support for DynamicMessages APIv2: types/dynamicpb: add support for dynamic messages Aug 18, 2018
@dsnet
Copy link
Member

dsnet commented Aug 18, 2018

Work on #364 is in full swing and support for dynamic messages is planned.

@dsnet
Copy link
Member

dsnet commented Mar 3, 2020

This is fixed in the v1.20.0 release. See the dynamicpb package.

@dsnet dsnet closed this as completed Mar 3, 2020
@dsnet dsnet changed the title APIv2: types/dynamicpb: add support for dynamic messages types/dynamicpb: add support for dynamic messages Mar 4, 2020
@golang golang locked as resolved and limited conversation to collaborators Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants