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.

Sign up for GitHub

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

Jump to bottom

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

Open
ryandpardey opened this issue Jan 11, 2022 · 5 comments · May be fixed by #3646
Open

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

ryandpardey opened this issue Jan 11, 2022 · 5 comments · May be fixed by #3646
Labels
bug good first issue help wanted openapi

Comments

@ryandpardey
Copy link

ryandpardey commented Jan 11, 2022 •
edited by johanbrandhorst

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
Copy link
Collaborator

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
Copy link

bhavyay commented Sep 2, 2022

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

@johanbrandhorst
Copy link
Collaborator

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

@momom-i
Copy link
Contributor

momom-i commented Mar 1, 2023

@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
Copy link
Collaborator

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.

amanraja pushed a commit to amanraja/grpc-gateway that referenced this issue Oct 6, 2023
@aman-raj-ev
fix: added fix for grpc-ecosystem#2494
b1ee40f
@amanraja amanraja linked a pull request Oct 6, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue help wanted openapi
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix 2494 amanraja/grpc-gateway
4 participants
@ryandpardey @johanbrandhorst @bhavyay @momom-i