grpc-gateway icon indicating copy to clipboard operation
grpc-gateway copied to clipboard

Error when using oneof name in response body selector

Open jefferai opened this issue 5 years ago • 8 comments

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'

jefferai avatar May 01 '20 14:05 jefferai

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?

johanbrandhorst avatar May 01 '20 14:05 johanbrandhorst

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

jefferai avatar May 01 '20 16:05 jefferai

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!

jefferai avatar May 01 '20 21:05 jefferai

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.

johanbrandhorst avatar May 02 '20 09:05 johanbrandhorst

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.

jlewi avatar Oct 12 '21 13:10 jlewi

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

johanbrandhorst avatar Oct 13 '21 01:10 johanbrandhorst

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] avatar Jan 09 '22 03:01 stale[bot]

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] avatar Jul 10 '22 09:07 stale[bot]