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.

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

Wrong Path Parameter Naming When Using ':' in Path #3517

Open
arthaali opened this issue Aug 21, 2023 · 5 comments · May be fixed by #3532
Open

Wrong Path Parameter Naming When Using ':' in Path #3517

arthaali opened this issue Aug 21, 2023 · 5 comments · May be fixed by #3532

Comments

@arthaali
Copy link

🐛 Bug Report

Plugin version

The parameters are not correctly named when the path contains :

To Reproduce

Given a proto

syntax = "proto3";

  rpc SearchItems(item.SearchItemsRequest) returns (item.SearchItemsResponse) {
    option (google.api.http) = {get: "/item/search:item/{item_no_query}"};
  }
  1. buf generate openapiv2.swagger file api.swagger.json
{
 "swagger": "2.0",
 "info": {
   "title": "api-rest-gateway/proto/api.proto",
   "version": "version not set"
 },
"paths": {
 "/item/search:items/{item_no_query}": {
     ....
   },
  }
}

Expected behavior

Expect to convert the parameter item_no_query to camel case itemNoQuery

{
 "swagger": "2.0",
 "info": {
   "title": "api-rest-gateway/proto/api.proto",
   "version": "version not set"
 },
"paths": {
 "/item/search:items/{itemNoQuery}": {
     ....
   },
  }
}

Actual Behavior

The path parameter remains in snake case instead of being converted to the camel case.

{
  "swagger": "2.0",
  "info": {
    "title": "api-rest-gateway/proto/api.proto",
    "version": "version not set"
  },
  "paths": {
    "/item/search:items/{item_no_query}": {
      ...
    }
  }
}

Your Environment

-Ubuntu
-Buf
-Plugin version: [email protected]

@johanbrandhorst
Copy link
Collaborator

Thanks for your issue. Do you mean to say that other query parameters are correctly converted? It only happens when there's a : in the path?

@arthaali
Copy link
Author

arthaali commented Aug 22, 2023

@johanbrandhorst Yes exactly, Query parameters are correctly converted in other requests.

example:

syntax = "proto3";

  rpc SearchItems(item.SearchItemsRequest) returns (item.SearchItemsResponse) {
    option (google.api.http) = {get: "/item/search-item/{item_no_query}"};
  }

is converted to

{
  "swagger": "2.0",
  "info": {
    "title": "api-rest-gateway/proto/api.proto",
    "version": "version not set"
  },
  "paths": {
    "/item/search-items/{itemNoQuery}": {
      ...
    }
  }
}

Please Note, this is only happening when the ":" is in the middle of the path, if I add the ":" at the end then it works.
example:

syntax = "proto3";

  rpc SearchItems(item.SearchItemsRequest) returns (item.SearchItemsResponse) {
    option (google.api.http) = {get: "/item/search/{item_no_query}:item"};
  }

is converted correctly to
"/item/search/{itemNoQuery}:item"

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Aug 22, 2023

Thanks for the clarification. This will make it easier to fix. I think if someone wants to pick this up the first step would be to recreate this scenario in a test in https://github.com/grpc-ecosystem/grpc-gateway/blob/main/protoc-gen-openapiv2/internal/genopenapi/template_test.go. Then the fix will probably be somewhere in https://github.com/grpc-ecosystem/grpc-gateway/blob/main/protoc-gen-openapiv2/internal/genopenapi/template.go (

func partsToOpenAPIPath(parts []string, overrides map[string]string) string {
for index, part := range parts {
part = canRegexp.ReplaceAllString(part, "{$1}")
if override, ok := overrides[part]; ok {
part = override
}
parts[index] = part
}
if last := len(parts) - 1; strings.HasPrefix(parts[last], ":") {
// Last item is a verb (":" LITERAL).
return strings.Join(parts[:last], "/") + parts[last]
}
return strings.Join(parts, "/")
}
or
case ':':
if depth == 0 {
// As soon as we find a ":" outside a variable,
// everything following is a verb
parts = append(parts, buffer)
buffer = path[i:]
break pathLoop
}
buffer += string(char)
jsonBuffer += string(char)
look particularly suspicious).

Ragnaroek pushed a commit to Ragnaroek/grpc-gateway that referenced this issue Aug 23, 2023
@Ragnaroek Ragnaroek linked a pull request Aug 23, 2023 that will close this issue
@mibo-fdc
Copy link

Digged a bit into it. I think the code assumes that a "resource:action" part only occurs on the end. This PR fixes the generation for our use-case, however I'm not sure about the general impact: #3532

@mibo-fdc
Copy link

mibo-fdc commented Aug 23, 2023

updated the PR with a test that should reproduce the issue (and a fix proposal)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants