Closed ewr23523 closed 9 months ago
For anyone interested workaround with custom codec that uses dynamicpb.Message:
And another workaround with custom codec that uses both dynamicpb.Message and protoreflect.MessageType (depending on what is available):
Usage demonstration (uses connect-demo repo):
@jhump is the author of grpcurl
, and is definitely the right person to chime in here :)
A slightly simpler way to do this with the existing APIs is to use a more general codec. That's sort of like what we did in buf curl
, which provides most of the functions of grpcurl
but adds support for using Git repos and Buf Schema Registry modules as the schema source and also adds support for gRPC-Web and Connect protocols. The codec there uses a deferredMessage
for received messages. The codec simply stores the bytes in the deferredMessage
, and the application can then umarshal those later, after instantiating a *dynamicpb.Message
of the right type.
But I agree that it would be ideal to do this without needing custom codecs. As it is, if you want to use a custom codec that provides a different serialization format, it must be wrapped to handle these deferred/dynamic message cases.
I think the thing we are missing is a way to provide a factory or constructor. Right now, the framework assumes it can construct a new instance of a message simply by taking the address of a zero value. But that does not work with dynamic messages. We could add new functions that accept these as args, that supplement the existing NewClient
and New*Handler
functions.
-func NewClient[Req, Res any](
+func NewClientWithResponseFactory[Req, Res any](
httpClient HTTPClient,
url string,
+ factory func() *Res,
options ...ClientOption
) *Client[Req, Res]
-func NewUnaryHandler[Req, Res any](
+func NewUnaryHandlerWithRequestFactory[Req, Res any](
procedure string,
unary func(context.Context, *Request[Req]) (*Response[Res], error),
+ factory func() *Req,
options ...HandlerOption,
) *Handler
// similar changes for new functions for the three kinds of streaming methods
Sadly, we can't use ClientOption
or HandlerOption
to supply the factories because (1) they are used in the generated code across all methods of a service, but message types in that scenario are usually heterogenous; and (2) there wouldn't be type-safety in that approach, to ensure that the factory's response type matches the actual generic type of the client or handler.
@akshayjshah, what do you think of something like the above? It's extra surface area in the exported API, but the nice property is that it can potentially enable other usages, since it is very general and not bespoke to the dynamicpb
package.
😭 Early versions of Connect looked exactly like this, and one of my favorite parts of introducing generics was dropping these function pointers. Wish I'd considered this use case more carefully back then! Let's do what we can now to make this easier for users like @ewr23523 and buf curl
.
I can't shake the feeling that we should be able to make this just work in a more ergonomic way. Dynamic messages aren't that exotic (especially in a world where protobuf schema registries are common!), so it feels a little kludgy to lean on user-visible function pointers to make them work properly. I'm willing to do it if there's no better option, but I'd like to explore a more specific solution first.
For unrelated reasons, I was hoping to add func WithSchema(schema any) Option
to Connect. I'd imagined it as a small convenience: generated code would use it to pass method descriptors to clients and handlers, which could then expose them to interceptors via Spec
. But if clients and handlers had access to a MethodDescriptor
, they'd be able to initialize dynamic messages automatically. I sketched out a partial implementation in the ajs/schema
branch. With those changes, clients would look like this.
@jhump, what do you think? I don't love that we're introducing protobuf-specific logic outside codecs and error details, but it seems better for users.
I didn't suggest anything like that since I figured something more general and less protobuf-specific would be preferred. While the link you provided looks great, I have a couple of concerns:
dynamicpb.Message
but no schema has been provided? Where and how does it fail? Since connect.NewClient
doesn't return an error, it can't provide immediate feedback but would instead have to defer any errors to actual RPCs which is quite unfortunate to be unable to validate client config while the program is initializing.Another thing I thought of is the case where the user does not know the message type at compile time. For example, if they are using a protoregistry.Types
to resolve the request and response types. This gives them only a proto.Message
. While concrete implementations will surely always be pointers, the actual pointed-to type is not knowable. That means the resulting message types in the client or handler is *proto.Message
, which doesn't play nice with the default codecs. (There's also the extra pointer indirection, but I don't see any way to change that.)
To work-around this, I've done tricks in the codec, similar to handling dynamic types in received messages. But this could be accommodate in the runtime by having the NewClient
or New*Handler
functions check if the type parameter is an interface type and, if so, handling the extra indirection -- for example, de-referencing the pointer to send a plain proto.Message
(not *proto.Message
) to the codec for marshalling. This only works for marshalling/sending messages, of course. For unmarshalling/receiving messages, a factory function would have to be provided (that would call MessageType.New()
, for example).
I came across a similar issue with using dynamic types for the server. Inspired by @jhump:s code snippet in this issue I created a small patch to extend the API with support for request factories.
I understand that you have some consideration to do before deciding on how this functionality could be best implemented but I thought I could share what worked for me anyway. Let me know if you would be interested in a contribution. https://github.com/connectrpc/connect-go/compare/main...williamleven:connect-go:feat/dynamic-message-factories
Have verified the above path to work with dynamicpb
for Unary requests.
Would love to see a canonical solution to this.
Disclaimer: It may be possible that connect does support this and my lack of understanding makes me unable to make it work. However I couldn't find anything in GitHub issues or in docs (please note, that there is a similar issue #312, but author decided to go with other approach and there is no answer about dynamic types).
Is your feature request related to a problem? Please describe. How one can perform connect request with dynamic types, like when a request is created using google.golang.org/protobuf/types/dynamicpb package and there is no concrete Go type available (the service, method and type information is inferred using reflection or read from a file containing the file descriptor set)?
As an example we can talk about tools like grpcurl - how to perform a task similar to one done by such a tool, but using connect protocol?
Describe the solution you'd like I imagine something like UntypedClient (in contrast to currently available Client that uses generic) or a special handling of dynamic messages inside of the current implementation.
It feels like it should be possible to call a service using connect starting with just a "google.golang.org/protobuf/reflect/protoreflect".MethodDescriptor and without reimplementing of large parts of connect-go package.
Describe alternatives you've considered I have found something about (and experimented with) creating custom codec, but it feels like it should be possible to just use connect with dynamic message, without special hacks, especially that dynamicpb is part of protobuf in Go.
I have also attached in the following messages results of my experimentation - a workaround if someone needs to achieve this with connect.
Additional context If I'm wrong and there is other issue or it is mentioned in the documentation, then I'm sorry that I have bothered you. If there is already a support for such things in connect, then please provide explanation in this issue, to allow others (like me ;) ) to find it.