connectrpc / vanguard-go

Support REST, gRPC, gRPC-Web, and Connect clients with one server.
https://pkg.go.dev/connectrpc.com/vanguard
Apache License 2.0
197 stars 13 forks source link

Add default HTTP routes for non-annotated methods #123

Open anhnmt opened 5 months ago

anhnmt commented 5 months ago

I have the following program: https://github.com/anhnmt/gprc-dynamic-proto/blob/main/cmd/service/main.go

I have successfully connected grpc and restapi, can I call the api like the following?

curl --header "Content-Type: application/json" \
    --data '{}' \
    http://localhost:8080/user.v1.UserService/List

I tried and the result was: Not Found

emcfarlane commented 5 months ago

Hey @anhnmt, default bindings aren't provided yet but it's something we would be interested in adding. On registration of an endpoint like:

service UserService {
  rpc List(ListRequest) returns (ListResponse) {
    option (google.api.http) = {get: "/v1/users/{page=*}"};
  }
}

We should create an additional endpoint with the effective syntax:

    option (google.api.http) = {
      post: "/user.v1.UserService/List"
      body: "*"
    };

Edit: currently to call your API you would need to use the REST syntax:

curl http://localhost:8080/v1/users/1?page_size=10
anhnmt commented 5 months ago

@emcfarlane thank u

I tried your method and it worked, but I would have to fix a lot of other methods to get the same result. I wonder if there is any way to optimize this workload, maybe without having to interfere with the .proto file?

rpc List(ListRequest) returns (ListResponse) {
    option (google.api.http) = {
      get: "/v1/users/{page=*}"
      additional_bindings {
        post: "/user.v1.UserService/List"
        body: "*"
      }
    };
  }
emcfarlane commented 5 months ago

Yes, we plan to add bindings for the default service method similar to https://cloud.google.com/endpoints/docs/grpc/transcoding#where_to_configure_transcoding

anhnmt commented 5 months ago

@emcfarlane Hopefully this feature will be released soon

jhump commented 5 months ago

@anhnmt, how are you actually using Vanguard? This default mapping doesn't seem to provide any value over just using the Connect protocol.

So with no annotations in the proto files you could do this:

curl --header "Content-Type: application/json" \
    --header "Connect-Protocol-Version: 1" \
    --data '{}' \
    http://localhost:8080/user.v1.UserService/List

The only difference between that, and explicitly configuring the service with the annotation (and not including the connect-protocol-version header) is the way that errors are represented. The error body will be JSON in both cases. With the annotations in place, the "RESTful" error format is the JSON format of the google.rpc.Status proto. With Connect, it will be slightly different (link to spec). In particular, Connect formats the code as a string instead of a number, so the response is more human-readable. Also, Connect uses a format for error details that is more robust. The "RESTful" format can fail to be rendered if the transcoding server doesn't have the Protobuf schemas for all detail messages, but the Connect format is not brittle in this way.

So do you actually need the "RESTful" format? Or would simply using the Connect protocol suffice?

anhnmt commented 5 months ago

Hi @jhump, I'm developing an api gateway that uses vanguard to bridge to my back-end services.

My client will call this API like: http://localhost:8000/user.v1.UserService/List and it will automatically forward this request to my service: http://localhost:8080/user.v1.UserService/List.

In addition to the RESTful api, it also supports connecting to services using gPRC as well as the path on /user.v1.UserService/List.

Until recently, I wanted to upload files through this gateway and encountered the Not Found problem above. I tried your method in this case and it still doesn't work.

curl --location 'localhost:8080/user.v1.UserService/Upload' \
--header 'Connect-Protocol-Version: 1' \
--header 'Content-Type: application/json' \
--form 'file=@"/C:/Users/anhnmt/Downloads/build.zip"'

My current way is to use an option as follows

  rpc Upload(UploadRequest) returns (google.protobuf.Empty) {
    options (google.api.http) = {
      post: "/user.v1.UserService/Upload"
      body: "file"
    };
  }

  message UploadRequest {
  // The file contents to upload.
  google.api.HttpBody file = 1;
}

I wonder if you have a better way?

jhump commented 5 months ago

Since your annotation uses body: "file", it must be explicitly defined, vs. relying on some default route (which @emcfarlane mentioned adding). Also, The Connect protocol does not handle google.api.HttpBody as that is only a feature of the REST transcoding.

Prior to your latest comment, the example cURL commands and discussion appear to just be ad-hoc usage of some of the API endpoints that were unary JSON, in which case the Connect protocol would work just as well.

Your prior example showed having to add a route for /user.v1.UserService/List even though it already had an annotation for /v1/users/{page=*}. So why would the client be using the former? Why not use the URLs in the existing HTTP annotations?

anhnmt commented 5 months ago

@jhump Sorry for causing misunderstanding. We use the main route /user.v1.UserService/List and /v1/users/{page=*} for testing purposes only.

My model looks like: Client -> Gateway -> Services. The port will automatically connect to services using host information such as: host:port. The gateway will discover and connect them on its own, where /user.v1.UserService/List will be how we forward to the correct service.

However, my old system doesn't work well and is unstable. Recently I wanted to add some more features including file uploads to the system, we rebuilt it and tried to change as little as possible.