protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Extensions

Open hayesgm opened this issue 6 years ago • 8 comments

Cool, thanks @tony612 for this project. I feel this is the right direction for protobufs in Elixir.

Quick question: do you have thoughts on how we'd build extensions? I want to add something akin to:

service ExampleService {
  rpc ping (PingRequest) returns (PongResponse) {
  	option (google.api.http) = { post: "/ping" };
  }

  rpc status (StatusRequest) returns (StatusResponse) {
  	option (google.api.http) = { get: "/status" };
  }
}

This would allow us to use services like grpc gateway. I would expect this to build a service object similar to:

defmodule Defs.ExampleService do
  use Protobuf.Service

  rpc :ping, Defs.PingRequest, Defs.PongResponse, %{"google.api.http": %{post: "/ping"}}
  rpc :status, Defs.StatusRequest, Defs.StatusResponse, %{"google.api.http": %{post: "/status"}}
end

Also, would it be possible to make the service definitions generic and then the grpc library could easily build off of this. E.g., as above, we could just have the service definition be a simple module and the grpc could run something akin to:

in grpc library:

# ... somewhere
grpc.add_services([Defs.ExampleService])

# ... somewhere else
def add_services(services) do
  for service <- services do
    for rpc <- service.rpcs do
      add_route(rpc.name, rpc.request_message, rpc.response_message, rpc.options)
    end
  end
end

I've started looking into how to build extensions, but it's a bit mind-bending to track how to add that. Happy for your thoughts here.

hayesgm avatar Oct 25 '17 21:10 hayesgm

I'm happy with this feature and PR for it :)

tony612 avatar Jan 28 '18 15:01 tony612

As I said in https://github.com/tony612/protobuf-elixir/pull/15#issuecomment-365290883. I want to give some ideas:

First of all, I want to give a summary about extensions and options in Protobuf:

  1. Only protobuf2 supports extensions, protobuf3 only supports using extension for defining options.
  2. Protobuf supports options in different levels(file, message, field, ...).
  3. Developers can use extensions to define custom options(extend google.protobuf.*Options).

For your PR, I think there're some problems:

  1. We need to split the implementations of extension and options because they're different features(in different PRs).
  2. The way you implement extensions is a little complex(indeed, as you said, it's difficult 😄). So, is there a better way to do that?
  3. Maybe we should let other projects handle custom options instead of in this lib.

For the 3rd problem(custom options), I looked into official Go implementation of protobuf and gRPC, I found it doesn't generate custom options for you. You have to use grpc-gateway plugin to generate code by using the custom options. The idea behind it, I think, is the protobuf lib should care about official defined options. For the custom options, it's the business of other plugins. In this way, the generated can be simpler(we only have what we need to use, like rpc definitions or http definitions).

Considering it's they're really advanced features, we can discuss more before implementing them.🙂

tony612 avatar Feb 13 '18 16:02 tony612

As per 1: sorry that a lot has been pushed into this PR-- I've been using it for several projects in production.

As per 2, happy to discuss alternative implementations. Holistically, the issue was that we needed to parse everything once to define extensions and messages, and then parse everything in a second pass to allow extensions. This works well (minus the redefining of modules) and doesn't present real issues since the process is only called on protocol generation, not once the modules are loaded by a project.

For 3, my core question, and the one that led to this pull request is how to handle this protobuf:

extend google.protobuf.MethodOptions {
  // See `HttpRule`.
  HttpRule http = 72295728;
}
message Http {
  // A list of HTTP configuration rules that apply to individual API methods.
  //
  // **NOTE:** All service configuration rules follow "last one wins" order.
  repeated HttpRule rules = 1;
  // ...
}

For me, it's a sine qua non that a library I use has the ability to process and read from these protobufs. These are standard libraries and promoted by Google itself. Are you suggesting that we don't handle options and/or extensions at all? Again, I am happy to discuss how this is best implemented, but I think it's important that we make this library fully-featured with the official protobuf specifications.

hayesgm avatar Feb 13 '18 19:02 hayesgm

+1 for this. Looking forward to being merged!

qinix avatar Feb 21 '18 15:02 qinix

@tony612 Any thoughts on how we should handle addressing/merging this? For my usage, it's been pretty helpful. I wouldn't mind this ending up as a permanent fork (by a different name?) or looking for a way to merge. What do you think is best?

hayesgm avatar Aug 01 '18 05:08 hayesgm

@tony612 was about to start a pr to implement this also. Having options/extensions available in generated code is very useful for lots of different projects that I work on. We've been able to use these in the golang proto package to help us write libs but once I got to elixir I've had to pause until I can get access to the options.

@hayesgm wrt the code snippet at the start of this issue, would you consider the RPC/field signature to out the options map inside a keyword, keyed to :option?

hassox avatar Sep 06 '18 16:09 hassox

@tony612 Do you have any updates on support for options in protobuf-elixir?

mzabka avatar Aug 06 '19 12:08 mzabka

Since extension is implemented in https://github.com/tony612/protobuf-elixir/pull/83 I think we can move forward to think about this again.

tony612 avatar Jan 09 '20 06:01 tony612