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
17.77k stars 2.19k forks source link

An RPC proto oneof field documents the field not used in the path as an optional request parameter #2494

Open ryandpardey opened 2 years ago

ryandpardey commented 2 years ago

Steps you follow to reproduce the error:

rpc GetArticle (GetArticleRequest) returns (GetArticleResponse) {
    option (google.api.http) = {
        get: "/v2/articles/{slug}"
        additional_bindings: {
            get: "/v2/articles/{id.hex}"
        }
     };
}

message GetArticleRequest {
    oneof article_id {
        insider.protobuf.ObjectID id = 1;
        string slug = 2;
    }
}

With the annotations above, two "separate" endpoints are defined in the output:

/v2/articles/{slug}
/v2/articles/{id.hex}

With the non-path parameter of the oneof listed as an optional query parameter, which is incorrect:

{
    "name": "id.hex",
    "description": "xyz",
    "in": "query",
    "required": false,
    "type": "string"
 }

What did you expect to happen instead:

I expect a single GET endpoint defined with 2 path parameters listed with oneof as required (not both, and nothing listed as a request parameter).

"parameters": [
  {
    "name": "slug",
    "description": "Article slug.",
    "in": "path",
    "required": true,
    "type": "oneof"
  },
  {
    "name": "id.hex",
    "description": "Article ID",
    "in": "path",
    "required": true,
    "type": "oneof"
  }
]

Your answer here.

What's your theory on why it isn't working:

I suspect there is simply no code to deal with the protobuf oneof type.

johanbrandhorst commented 2 years ago

Thanks for the bug report! We've discussed this a bit on other channels and I think there's definitely something we can do here. The case we're interested in is when one of the fields of a oneof appears in a path parameter. In that case, we should be able to deduce that none of the other fields of that oneof can be specified, and so remove them from the parameter list. I don't think we need logic clever enough to produce an OpenAPI oneof, in this case simply two GETs with different parameters should be OK, right?

bhavyay commented 1 year ago

@johanbrandhorst I am interested to contribute to this issue. Let me know if I can pick this up.

johanbrandhorst commented 1 year ago

Hi, yes, please feel free to start work, I don't know anyone else that is working on this.

momom-i commented 1 year ago

@johanbrandhorst Hi, it doesn't seem it's finished yet. Can I work on this? So making separate endpoints with parameters that are included in each endpoint's path will resolve this, right?

johanbrandhorst commented 1 year ago

Hi @momom-i, yes, feel free to start work on this :). I think the ask is to ensure that if one of fields in a oneof is specified in the path, none of the other fields should be in the parameters. The specific case of two RPCs as in this example may be something we need to consider separately, afterwards.