Quality of life enhancements for RPC modules

[tmcgilchrist]

Quality of life enhancements for RPC modules service, rpc, encode, decode. This cleans up the calling code for ocaml-grpc.

For the Greeter gRPC example of:

syntax = "proto3";
package mypackage;

// The greeting service definition.
service Greeter {
  // Sends a greeting
  rpc SayHello(HelloRequest) returns (HelloReply) {}
}

// The request message containing the user's name.
message HelloRequest { string name = 1; }

// The response message containing the greetings
message HelloReply { string message = 1; }

This change produces this code:

  module Greeter = struct
    module SayHello = struct
      let name = "/mypackage.Greeter/SayHello"
      let service = "mypackage.Greeter"
      let rpc = "SayHello"
      module Request = HelloRequest
      module Response = HelloReply
    end
    let sayHello = 
      (module HelloRequest : Runtime'.Service.Message with type t = HelloRequest.t ), 
      (module HelloReply : Runtime'.Service.Message with type t = HelloReply.t )
    
  end

Calling this code can then be configured with the generated types rather than using strings. For example:

let server =
  let open Greeter.Mypackage in
  let greeter_service =
    Server.Service.(
      v () |> add_rpc ~name:Greeter.SayHello.rpc ~rpc:(Unary say_hello) |> handle_request)
  in
  Server.(
    v () |> add_service ~name:Greeter.SayHello.service ~service:greeter_service)

[andersfugmann]

Suggested change

	Code.emit t `None "let service = \"%s\"" (Scope.get_proto_path scope);
	Code.emit t `None "let service = \"%s\"" service_name;

[andersfugmann]

In general, I'm all in favor of improving developer lives, and I'm ok with merging the PR.

However, I think there is an alternative better and more scalable approach; Extending on the pattern in the service module (service.ml) I think you will be able to avoid boilerplate code and be able to extend functionality on your end.

Consider the following utility functions:

open Runtime
open Service

let create_coders (type t) (module M: Message with type t = t) =
  let encode t = M.to_proto t |> Runtime'.Writer.contents in
  let decode s = Runtime'.Reader.create s |> M.from_proto in
  (encode, decode)

let get_service_attributes (type req) (type rep) (module R : Rpc with type Request.t = req and type Response.t = rep) =
  let (service, rpc) = match String.split_on_char '/' R.name with
    | [_; service; rpc] -> (service, rpc)
    | _ -> failwith ("Unable to parse name: " ^ R.name)
  in
  let request_coders = create_coders (module R.Request) in
  let response_coders = create_coders (module R.Response) in
  (service, rpc, request_coders, response_coders)

Invoking get_service_attributes with an Rpc endpoint you get all the fields this PR added. In addition, the extra code given as an example could be generalized to reduce boilerplate code even further to create an even more declaration way to specify gRPC service endpoints by expaning on this pattern.

Let me know what you think. As I said, I'm ok of merging the PR, but would like your take on this first.

[tmcgilchrist]

I agree the calling code generated could be much better.

I tried out the suggestion (putting the code in service.ml and came up with this example code:

  (* gRPC client code *)
  let do_request = H2_lwt_unix.Client.request connection ~error_handler:ignore in
  (* code generation *)
  let open Ocaml_protoc_plugin in
  let open Greeter.Mypackage in
  (* Code gen this module! *)
  let a =
    let module M = struct
        let name = Greeter.SayHello.name
        module Request = Greeter.SayHello.Request
        module Response = Greeter.SayHello.Response
      end in
    (module M : Service.Rpc with type Request.t = M.Request.t and type Response.t = M.Response.t)
  in
  let (service, rpc, _, (encode, decode)) = Service.get_service_attributes a in

  Client.call ~service ~rpc ~do_request
    ~handler:
      (Client.Rpc.unary (encode req) ~f:(fun response ->
           let+ response = response in
           match response with
           | Some response -> (
             decode response |> function
               | Ok v -> v
               | Error e ->
                  failwith
                    (Printf.sprintf "Could not decode request: %s"
                       (Result.show_error e)))
           | None -> Greeter.SayHello.Response.make () (* Having access to this from module M would be useful. *)))
    ()

I slightly prefer a record type to be returned from Service.get_service_attributes the tuple can get confusing which encode / decode to use. The server side code is a bit confusing whether to use:

  let decode, encode = Service.make_service_functions Greeter.sayHello in
  let encode, decode = Service.create_coders (fst Greeter.sayHello) in

The returned types from the second are better but the argument fst Greeter.sayHello is not intuitive. If it was Service.create_coders (Greeter.Request) with the right module type that would be better.

For defining the service type it does clean up things:

let server =
  let open Ocaml_protoc_plugin in
  let open Greeter.Mypackage in
  (* Code gen this module! *)
  let a =
    let module M = struct
        let name = Greeter.SayHello.name
        module Request = Greeter.SayHello.Request
        module Response = Greeter.SayHello.Response
      end in
    (module M : Service.Rpc with type Request.t = M.Request.t and type Response.t = M.Response.t)
  in
  let (service, rpc, _, _) = Service.get_service_attributes a in
  Server.(
    v ()
    |> add_service ~name:service
         ~service:Server.Service.(v ()
                                  |> add_rpc ~name:rpc ~rpc:(Unary say_hello)
                                  |> handle_request))

This can probably be improved further with more experimentation. I am working on improving the API for gRPC with ocaml-protoc-plugin while still keeping the option for other serialisation libraries or doing things manually. There isn't anything concrete to share at the moment.

This PR came originally from patches @quernd at Dialohq had for the codegen. It might be helpful for them to have this accepted and then have a breaking change to get a redesigned API.

[andersfugmann]

My suggestion was actually that the code would reside on the gRPC side of things and not extend the Service module.
This would allow gRPC to encapsulate boilerplate code in helper functions, and you could change the API a will without the need to patch ocaml-protoc-plugin nor having to upstream changes.

By defining utility functions for creating client and server, you could hide all boilerplate code behind functions which essentially just takes an Ocaml_protoc_plugin.Service.Rpc module as argument:

let create_client (type req) (type rep) (module R : Rpc with type Request.t = req and type Response.t = rep) = ...
let create_server (type req) (type rep) (module R : Rpc with type Request.t = req and type Response.t = rep) = ...

The point is that if all the needed information is available in the auto generated modules, its trivial to create generic functions to extract this information.

[quernd]

Thanks @tmcgilchrist for opening this PR and bringing me into the discussion!

My suggestion was actually that the code would reside on the gRPC side of things and not extend the Service module. This would allow gRPC to encapsulate boilerplate code in helper functions, and you could change the API a will without the need to patch ocaml-protoc-plugin nor having to upstream changes.

That's a good suggestion and maybe that's the most flexible way to avoid boilerplate code. The only thing that irks me a bit is this:

  let (service, rpc) = match String.split_on_char '/' R.name with

It's a minor inconvenience but it goes against the spirit of generating typesafe code to concatenate strings only to split them again. So service and rpc would be great to have while the coders are optional.

I slightly prefer a record type to be returned from Service.get_service_attributes the tuple can get confusing which encode / decode to use. The server side code is a bit confusing whether to use:

  let decode, encode = Service.make_service_functions Greeter.sayHello in
  let encode, decode = Service.create_coders (fst Greeter.sayHello) in

The returned types from the second are better but the argument fst Greeter.sayHello is not intuitive. If it was Service.create_coders (Greeter.Request) with the right module type that would be better.

The confusing bit here might be that you always need both Request and Response. The client needs an encoder for the request and a decoder for the response, and the server vice versa.

That said, I'd also prefer a record, or even a module. But like @andersfugmann says: if all the information is there we can handle it on our side as we please.

This can probably be improved further with more experimentation. I am working on improving the API for gRPC with ocaml-protoc-plugin while still keeping the option for other serialisation libraries or doing things manually. There isn't anything concrete to share at the moment.

This PR came originally from patches @quernd at Dialohq had for the codegen. It might be helpful for them to have this accepted and then have a breaking change to get a redesigned API.

It would be helpful, yes. But I'm starting to think that it's better to use this pattern instead, either in ocaml-grpc or (for the time being) in our own code:

let create_client (type req) (type rep) (module R : Rpc with type Request.t = req and type Response.t = rep) = ...
let create_server (type req) (type rep) (module R : Rpc with type Request.t = req and type Response.t = rep) = ...

And then for alternative serialization libraries we can leverage the same pattern and define a common record type or module signature that the client and server APIs expect. What do you think?

[andersfugmann]

It's a minor inconvenience but it goes against the spirit of generating typesafe code to concatenate strings only to split them again. So service and rpc would be great to have while the coders are optional.

I fully agree. I am however a bit confused with the names rpc and service.
If service the endpoint name and rpc the method name? If so, maybe we could use more descriptive names?

[quernd]

I fully agree. I am however a bit confused with the names rpc and service. If service the endpoint name and rpc the method name? If so, maybe we could use more descriptive names?

It's the terminology used in protobuf files:

ocaml-protoc-plugin/examples/echo/echo.proto

Lines 23 to 25 in d57a66b

	service Echo {
	rpc Call (Request) returns (Reply);
	}

I think anything else wouldn't be more descriptive, but rather more confusing.

[andersfugmann]

Fair point, but if IIRC the service is denoting the fully scoped name, and not only the service name (e.g. mypackage.Greeter. Also rpc just means Remote Procedure Call. Maybe rename to rpc_name or rpc_endpoint or similar.

What terms are using for gRPC? Maybe we could use the same terminology.

[andersfugmann]

Looking at the API for various languages supported for gRPC, it seems that we should decompose the name into three elements: package_name, service_name and method_name, where package_name is an option type, as the service endpoints are not required to be defined in a package (IIRC).

[tmcgilchrist]

My suggestion was actually that the code would reside on the gRPC side of things and not extend the Service module.

My misunderstanding, it would be more useful for ocaml-grpc to define this common interface for different kinds of serialisations. I want to leave the option open to other kinds of serialisation libraries while having a user-friendly API for ocaml-protoc-plugin.

Looking at the API for various languages supported for gRPC, it seems that we should decompose the name into three elements: package_name, service_name and method_name, where package_name is an option type, as the service endpoints are not required to be defined in a package (IIRC).

Happy to provide a PR that does just that, if everyone is agreed it is the right way to go.

[quernd]

Looking at the API for various languages supported for gRPC, it seems that we should decompose the name into three elements: package_name, service_name and method_name, where package_name is an option type, as the service endpoints are not required to be defined in a package (IIRC).

This sounds like the best way forward to me 👍

[andersfugmann]

I've created #50 which added the extra fields + tests as suggested above.

[andersfugmann]

@tmcgilchrist Would the changes made in #50 work for your use case?

[andersfugmann]

I've merged #50. Would you prefer a new opam release with these changes, or should we hold back a bit for you to try out the changes to see if we need to make additional changes to the interfaces?

[andersfugmann]

I've create a new release 4.5.0 (ocaml/opam-repository#23936) containing the changes.