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

The swagger plugin couldn’t distinguish two rpcs if we use the resource name design style. #702

Closed
ch3rub1m opened this issue Jul 15, 2018 · 13 comments

Comments

@ch3rub1m
Copy link
Contributor

The swagger plugin couldn’t distinguish two rpcs designed according to the Google API Design Guide Resource Names as below:

  rpc GetService(GetServiceRequest) returns (Service) {
    option (google.api.http) = {
      get: "/v1/{name=service_categories/*/services/*}"
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      description: "Get a service.The **name** must have the format of `service_categories/*/services/*`.";
      tags: "Service";
    };
  }
  rpc GetUser(GetUserRequest) returns (User) {
    option (google.api.http) = {
      get: "/v1/{name=users/*}"
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      description: "Get a user.The **name** must have the format of `users/*`.";
      tags: "User";
    };
  }

The two generated results from the swagger plugin both use the /v1/{name} as the key.
There is no problem if we use ID as the user input, but according to the Google API Design Guide the name is a better choice.
Is there any solution about this issue?

@achew22
Copy link
Collaborator

achew22 commented Jul 15, 2018

@ch3rub1m, thanks for the detailed and interesting bug report. You have definitely run into an issue with our Swagger generation.

Reading through the linked documentation, I think you're interpreting things in a valid way, but I'm not sure how we would map it in Swagger/OpenAPI land. The concept works in the gateway because it can parse the whole string and determine the match, but in Swagger it wouldn't be able do to that since name is just a string to the type system. I would be very interested at hearing alternative ways to approach this, but as of right now the only solution I see is to call the field something other than name for the 2ndary match. You could also deviate from their naming scheme and decompose the URL into the constituent tuples with something like:

  rpc GetService(GetServiceRequest) returns (Service) {
    option (google.api.http) = {
      get: "/v1/service_categories/{category}/services/{id}"
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      description: "Get a service.The **name** must have the format of `service_categories/*/services/*`.";
      tags: "Service";
    };
  }
  rpc GetUser(GetUserRequest) returns (User) {
    option (google.api.http) = {
      get: "/v1/users/{name}"
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      description: "Get a user.The **name** must have the format of `users/*`.";
      tags: "User";
    };
  }

@ch3rub1m
Copy link
Contributor Author

ch3rub1m commented Jul 18, 2018

@achew22, thanks for the response. I considered that the key in the Swagger generation could contain the format info, use

  "paths": {
    "/v1/{name=users/*}": {
    }
  }

instead

  "paths": {
    "/v1/{name}": {
    }
  }

it seems could fix this issue.

@achew22
Copy link
Collaborator

achew22 commented Jul 18, 2018

What happens if you manually edit the Swagger to say that?

@ch3rub1m
Copy link
Contributor Author

It works if I manually edit the Swagger, but the changes will be overwritten when I generate the file next time.

@achew22
Copy link
Collaborator

achew22 commented Jul 18, 2018

I guess that means it would be possible to edit templateToSwaggerPath and the associated tests to render that correctly. The function is very encapsulated and doesn't manipulate or depend on any external state. All you would have to do is add some test cases that match your use case. That's a very welcome change if you would be interested in taking it on.

@ch3rub1m
Copy link
Contributor Author

ch3rub1m commented Jul 20, 2018

@achew22 I created a pull request (#704).
Fixed the issue and added some test cases.

However it built failed weirdly in Travis CI.
Could you check about it please?

@achew22
Copy link
Collaborator

achew22 commented Jul 20, 2018

I'm confused. It looks like that PR is empty. What am I missing?

@ch3rub1m
Copy link
Contributor Author

ch3rub1m commented Jul 20, 2018

@achew22 I'm sorry, I want to trigger the Travis CI again, so I used push -f on my branch. It closed the PR automatically. Could you reopen this PR? Or maybe I should open a new PR?

@ivucica
Copy link
Collaborator

ivucica commented Jul 24, 2018

Sidenote on style of your proto definition, unrelated to this bug:

  • you don't need to specify a tag if the service name matches the tag value
  • you don't need to specify the description; while first paragraph will be the summary, remaining paragraphs should end up in the description
    • if you use comments in place of openapi_v2 options, the documentation will appear in generated Go (and other) code, which is nice and useful

I'm viewing the use of openapi_v2 as a last resort.

@frolickingferret445
Copy link

frolickingferret445 commented Aug 14, 2018

What is the current state of this merge request? And are we sure that it won't fix this issue?

#720

Thanks!

@ch3rub1m
Copy link
Contributor Author

@frolickingferret445 I am still waiting for being merged.
Unfortunately it cannot fix #720.

@frolickingferret445
Copy link

hmmm thanks!

@ch3rub1m
Copy link
Contributor Author

Issue was fixed.

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

No branches or pull requests

4 participants