Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensions #14

Open
hayesgm opened this issue Oct 25, 2017 · 8 comments
Open

Extensions #14

hayesgm opened this issue Oct 25, 2017 · 8 comments
Labels
Effort:Large Kind:Feature A new feature that's not currently in the library.

Comments

@hayesgm
Copy link

hayesgm commented Oct 25, 2017

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.

@tony612
Copy link
Collaborator

tony612 commented Jan 28, 2018

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

@tony612
Copy link
Collaborator

tony612 commented Feb 13, 2018

As I said in #15 (comment). 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.🙂

@hayesgm
Copy link
Author

hayesgm commented Feb 13, 2018

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.

@qinix
Copy link
Contributor

qinix commented Feb 21, 2018

+1 for this. Looking forward to being merged!

@hayesgm
Copy link
Author

hayesgm commented Aug 1, 2018

@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?

@hassox
Copy link
Contributor

hassox commented Sep 6, 2018

@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?

@mzabka
Copy link

mzabka commented Aug 6, 2019

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

@tony612
Copy link
Collaborator

tony612 commented Jan 9, 2020

Since extension is implemented in #83 I think we can move forward to think about this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort:Large Kind:Feature A new feature that's not currently in the library.
Projects
None yet
Development

No branches or pull requests

6 participants