grpc-ecosystem / grpc-gateway

gRPC to JSON proxy generator following the gRPC HTTP spec
https://grpc-ecosystem.github.io/grpc-gateway/
BSD 3-Clause "New" or "Revised" License
18.26k stars 2.24k forks source link

Error when using oneof name in response body selector #1264

Open jefferai opened 4 years ago

jefferai commented 4 years ago

As asked from my question in #585 I'm filing a new bug here.

I'm trying to have the response_body in the HTTP options use the oneof field, so that we can have alternate schemas in the response. However, this causes an error. Here is a minimal proto:

syntax = "proto3";

package services;

option go_package = "services;services";

import "google/api/annotations.proto";

service HostService {
  // This retrieves the host specified in the request using the basic view.
  rpc GetHost(GetHostRequest) returns (GetHostResponse) {
    option (google.api.http) = {
      get: "/hosts/{id}"
      response_body: "item"
    };
  }
}

message HostA {
  string foo = 1;
}

message HostB {
  string bar = 1;
}

message GetHostRequest {
  string id = 1;
}

message GetHostResponse {
  oneof item {
    HostA a = 1;
    HostB b = 2;
  }
}

Here is tree output:

.
├── gen
│   └── service
└── service
    ├── service.proto
    └── third_party
        ├── google
        │   └── api
        │       ├── annotations.proto
        │       ├── http.proto
        │       └── httpbody.proto
        └── protoc-gen-swagger
            └── options
                ├── annotations.proto
                └── openapiv2.proto

And here is my invocation: protoc -I. -I service/third_party --go_out=plugins=grpc,paths=source_relative:./gen --grpc-gateway_out=logtostderr=true,paths=source_relative:./gen service/service.proto

When I run the command I get: --grpc-gateway_out: no field "item" found in GetHostResponse

Note that if I change the response body def to select either "a" or "b" -- not actually what I want, but just to try it out -- I get a different error (which I think is already captured in a different ticket): --grpc-gateway_out: 224:9: expected operand, found 'if'

johanbrandhorst commented 4 years ago

Thanks for raising the issue. The reponse_body handling is all in this file: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-grpc-gateway/descriptor/services.go#L146. I expect we may need to add some logic to handle the case where a oneof is used. What can I do to help you bring this fix in?

jefferai commented 4 years ago

Well, having never dealt with either underlying proto handling code or grpc-gateway code, any pointers you may have would be useful :-)

jefferai commented 4 years ago

Closing -- it may be a bug but we're actually going to structure our API differently (for different reasons than this). If you want to keep it open to track, feel free!

johanbrandhorst commented 4 years ago

Okay! Feel free to reopen again if you need it later. If you need any other support for the gateway, I'm usually available in #grpc-gateway on Gophers slack.

jlewi commented 3 years ago

This seems like a useful feature.

I investigated a bit and it looks like we'd want to modify the request handler template https://github.com/grpc-ecosystem/grpc-gateway/blob/9ec62387b4d04e454fcc84ab8f7d0d0c11dddde1/protoc-gen-grpc-gateway/internal/gengateway/template.go#L412

To test the oneof and return the value that is actually set. When I hacked my generated code the following seemed to do the correct thing.

    msg, err := client.Get(ctx, &protoReq, grpc.Header(&metadata.HeaderMD), grpc.Trailer(&metadata.TrailerMD))

    var result proto.Message
    switch x := msg.GetResult().(type) {
    case *GetResponse_Document:
        result = x.Document
        break
    case *GetResponse_Status:
        result = x.Status
    case nil:
    default:
        result = nil;
    }

where my proto looked like

message GetResponse {  
  oneof result {
    Document document = 1;
    ResponseStatus status = 2;
  }
}

No immediate plans to submit a fix for this. We are currently evaluating the use of gRPC and gRPC-gateway. If we end up adopting it we may try to tackle this.

johanbrandhorst commented 3 years ago

Thanks for your input, I'll reopen this bug report then :).

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.