issuu / ocaml-protoc-plugin

ocaml-protoc-plugin
https://issuu.github.io/ocaml-protoc-plugin/
Other
48 stars 19 forks source link

Consider including the RPC name for gRPC services #27

Closed lepoetemaudit closed 1 year ago

lepoetemaudit commented 3 years ago

Firstly, thank you for this library. Being able to parse services in .proto files is really neat indeed!

I noticed that an RPC endpoint gets generated as a tuple of (for shorthand) (req msg * resp msg), as in the below:

module SayHello = struct
  let sayHello = 
    ( (module Word : Runtime'.Service.Message with type t = Word.t ), 
    (module Word : Runtime'.Service.Message with type t = Word.t ) ) 
end

If instead (or additionally) a module was generated, it could include a 'name' field (like the generated messages do!). That would have some really neat applications. For context I am attempting to wrap this library along with ocaml-h2 and perhaps functorize some sort of executor so that a user could swap out the actual http2 client or switch between async, lwt, etc... as desired. In practice this looks like the below (hardcoded to LWT at the moment)

make_request 
    connection 
    "/test.Greeter/SayHello" 
    Greeter.say_hello
    (SayHelloRequest.make ~message: "Hi there" ()
 >>= result ->
     Lwt.return (print_endline result)

That function takes care of all the encoding, decoding and the actual transmission over HTTP2. It's very basic but usable in this state, and if the name were included in a SayHello module for the RPC endpoint, that would remove the requirement to pass "/test.Greeter/SayHello" separately as it's something that could already be inferred, which would mean that the entire gRPC service could be called in a type safe way.

I'd be happy to contribute if you believe this idea to have value. I wouldn't want to encumber this library, so I'd only seek to add that 'name' field in a generated module per RPC endpoint, and would leave the tuple of modules alone for the sake of compatibility. Alternatively,I could change that tuple to be a record or include the name in that tuple as a third element, and update the functions that generate client / server encoder / decoders accordingly.

andersfugmann commented 3 years ago

I would welcome PR's to make it easier to do gRPC. I don't think records are the way to go, so I'd rather use first class modules.

How about a signature for Rpc (defined in service.ml):

 module type Rpc = sig
  module Request : Message
  module Response : Message
  val name : string
end

Then we can change make_client_functions to look like this:

let make_client_functions (type request) (type response) (module R : Rpc with type Request.t = request and type Response.t = response) =
  R.Request.to_proto, R.esponse.from_proto

(And similar for make_serivce_functions)

The auto generated should then instead create a module, e.g:

(* Example based on examples/echo/echo.proto *)

module Echo = struct
  module Call = struct
    let name = "Echo.Call"
    module Request = Request
    module Response = Response
  end
end

A PR for this would certainly be very welcome to replace the simple tuple with a module. Adding more fields to this module later should then be trivial without breaking backward compatibility.

lepoetemaudit commented 3 years ago

The first class module approach seems ideal to me. I hacked it in just adding the name to the tuple and it worked fine, but I think an extensible first class module is much cleaner. I'll have a look at doing that and prepare a PR.

I might add a let binding to the module to save on having to type (module Echo.Call) when using make_service_functions etc., e.g:

module Echo = struct
  module Call = struct
    let name = "Echo.Call"
    module Request = Request
    module Response = Response
  end

  let call = (module Call : RPC (* extra type stuff omitted *) )
end

Existing code that used those functions would continue to work like that as well.

andersfugmann commented 1 year ago

Solved by #28