tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.85k stars 500 forks source link

Ability to provide default implementations on service traits #844

Closed NickLarsenNZ closed 1 year ago

NickLarsenNZ commented 1 year ago

I'm using a crate (https://github.com/dapr/rust-sdk) uses prost to generate code for developers to build out their dapr service.

One thing I am finding frustrating is having to implement all trait methods (from the proto service) when I only need one or two at a time.

I was wondering if it is possible to generate default trait methods so the developer only has to override the ones they are interested in?

Dapr SDK Proto: https://github.com/dapr/rust-sdk/blob/b69e1f84b04858f72de4c70793cc353fcbf08529/dapr/proto/runtime/v1/appcallback.proto#L30-L48

Example implementation (which only need to implement one method): https://github.com/dapr/rust-sdk/blob/b69e1f84b04858f72de4c70793cc353fcbf08529/examples/invoke/grpc/server.rs#L60-L90

It appears to me that all generated return types from services and message fields impl Default, so this should be doable in some form (and I can imagine not always desirable, so perhaps an opt-in thing).

Expand to see a snippet of generated code showing impl Default for message types ```rs // impl Default for rpc return type // proto-src: https://github.com/dapr/rust-sdk/blob/ecc0e4e813af0c87b749ad14f57320cf3f8f9368/dapr/proto/runtime/v1/appcallback.proto#L245-L249 impl ::core::default::Default for ListTopicSubscriptionsResponse { fn default() -> Self { ListTopicSubscriptionsResponse { subscriptions: ::core::default::Default::default(), } } } // impl Default for message used in the above message // proto-src: https://github.com/dapr/rust-sdk/blob/ecc0e4e813af0c87b749ad14f57320cf3f8f9368/dapr/proto/runtime/v1/appcallback.proto#L251-L271 impl ::core::default::Default for TopicSubscription { fn default() -> Self { TopicSubscription { pubsub_name: ::prost::alloc::string::String::new(), topic: ::prost::alloc::string::String::new(), metadata: ::core::default::Default::default(), routes: ::core::default::Default::default(), dead_letter_topic: ::prost::alloc::string::String::new(), bulk_subscribe: ::core::default::Default::default(), } } } ```
NickLarsenNZ commented 1 year ago

There won't always be a clear default to return, but I can see it coming down to two:

In my particular case, I would want the defaults for listing subscriptions and bindings to be:

Ok(Response::new(T::default())) // where type T = ListInputBindingsResponse, for example

and for handlers that haven't been overridden to return the "unimplemented" Status:

Err(Status::unimplemented("x not implemented")) // where x could be on_invoke, for example
NickLarsenNZ commented 1 year ago

I'm unsure if it is frowned upon to encode this in the proto files, but perhaps something like:

service AppCallback {
  // Invokes service method with InvokeRequest.
  // @default(unimplemented)
  rpc OnInvoke (common.v1.InvokeRequest) returns (common.v1.InvokeResponse) {}

  // Lists all topics subscribed by this app.
  // @default(default)
  rpc ListTopicSubscriptions(google.protobuf.Empty) returns (ListTopicSubscriptionsResponse) {}

  // Subscribes events from Pubsub
  // @default(unimplemented)
  rpc OnTopicEvent(TopicEventRequest) returns (TopicEventResponse) {}

  // Lists all input bindings subscribed by this app.
  // @default(default)
  rpc ListInputBindings(google.protobuf.Empty) returns (ListInputBindingsResponse) {}

  // Listens events from the input bindings
  //
  // User application can save the states or send the events to the output
  // bindings optionally by returning BindingEventResponse.
  // @default(unimplemented)
  rpc OnBindingEvent(BindingEventRequest) returns (BindingEventResponse) {}
}

Otherwise, perhaps it could be configured in the prost Builder, eg:

tonic_build::configure()
    .build_server(true)
    .override_returns!(
        AppCallback.on_invoke => Err(Status::unimplemented("on_invoke not implemented")),
        AppCallback.list_topic_subscriptions => Ok(Response::new(T::default())),
        // any others not defined here will require explicit implementation by the developer
    )
    .compile(
        &[...],
        &["."],
    )?;
NickLarsenNZ commented 1 year ago

Perhaps this is the purpose of the ServiceGenerator trait, and that we need to make our own one for the dapr/rust-sdk?:

https://github.com/tokio-rs/prost/blob/6b5516aff83a331628f1e040f71fad6dfce23e4b/prost-build/src/lib.rs#L161-L175

NickLarsenNZ commented 1 year ago

... and it looks like I'm needing to be over in https://github.com/hyperium/tonic.

Their implementation of ServiceGenerator: https://github.com/hyperium/tonic/blob/65d909e85050e0cbbf50455e0b3531a0d08eff8d/tonic-build/src/prost.rs#L165-L220

I'll close this issue and open one over there after a little more digging.


Update: I did find this issue: https://github.com/hyperium/tonic/issues/1347 which is relevant.