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

protoc-gen-swagger: add the package name to the tags field of each endpoint if the package name exists in the proto file #860

Merged
merged 6 commits into from
Jan 28, 2019

Conversation

zwcn
Copy link
Contributor

@zwcn zwcn commented Jan 24, 2019

Currently, given a path and a http method, there is no way to identify which package it is generated from. For example, since the following swagger file generated by protoc-gen-swagger contains only the service name dataService and the rpc name GetData, we can't derive the package name in the proto file that defines dataService and GetData.

{
  ...
  "paths": {
    "/api/v1/data": {
      "get": {
          "operationId": "GetData", // <--- the rpc name
          "tags": [
            "dataService" // <--- the service name
          ]
    }
  }
  ...
}

This pull request prepends the package name to the service name in the tags field. For example, if the proto file includes a line package data;, the tags field will be ["data.dataService"]. Doing this allows us to trace back the origin of the GET /api/v1/data http request.

@johanbrandhorst
Copy link
Collaborator

Hi again @zwcn. It looks like you need to regenerate the files after adding this fix. Please see the contribution guide for more information.

@zwcn zwcn changed the title protoc-gen-swagger: add the package name to the tags field of each endpoint protoc-gen-swagger: add the package name to the tags field of each endpoint if the package name exists in the proto file Jan 24, 2019
@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Jan 24, 2019

Looks like we need to update some of the tests after this change:

examples/integration/client_test.go

https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/browser/echo_service.spec.js

@zwcn
Copy link
Contributor Author

zwcn commented Jan 24, 2019

@johanbrandhorst , yes. As shown at https://github.com/grpc-ecosystem/grpc-gateway/pull/860/files#diff-eab1d67e52af78e98dd23e2b08fcc22aR25 , swagger-codegen generates go files based on the tags field of the input swagger file.

How do you feel about these long-name changes?

@johanbrandhorst
Copy link
Collaborator

Haha yeah I noticed. I suppose it's not wrong but they're not pretty names. I wonder if we can remove the tag from the generated names with a generator option? Please take a look.

@zwcn
Copy link
Contributor Author

zwcn commented Jan 25, 2019

Hi @johanbrandhorst ,

Yes, the names aren't wrong. As shown at https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-do-tags-affect-the-generated-code, the tags filed is required for swagger-codegen to group APIs. So swagger-codegen did the right thing.

i've browsed the swagger-codegen docs. Unfortunately, I couldn't find any argument for swagger-codegen to ignore (or override) tags during code generation.

One possible solution would be to add a new boolean flag called allow_package_in_tags to protoc-gen-swagger. The package name is prepended only when it's true. This gives users more flexibility to have pretty names when they don't care about an API's origin at all.

How do you think?

@johanbrandhorst
Copy link
Collaborator

I think we're gonna have to add a flag as you say as this could be considered backwards compatibility breaking as well. Could you update the PR with a flag please? See if you could add some tests or a new file generated with the new flag as well please.

@zwcn
Copy link
Contributor Author

zwcn commented Jan 25, 2019

@johanbrandhorst : Sounds like a plan. Will do.

@codecov-io
Copy link

codecov-io commented Jan 28, 2019

Codecov Report

Merging #860 into master will decrease coverage by 0.07%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   53.08%   53.01%   -0.08%     
==========================================
  Files          39       39              
  Lines        3888     3901      +13     
==========================================
+ Hits         2064     2068       +4     
- Misses       1630     1637       +7     
- Partials      194      196       +2
Impacted Files Coverage Δ
protoc-gen-grpc-gateway/descriptor/registry.go 60.9% <0%> (-1.13%) ⬇️
protoc-gen-swagger/main.go 27.35% <50%> (+1.35%) ⬆️
protoc-gen-swagger/genswagger/template.go 53.48% <50%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a5640...d423496. Read the comment docs.

@zwcn
Copy link
Contributor Author

zwcn commented Jan 28, 2019

Hi @johanbrandhorst , I've added a new boolean flag allow_package_name_in_tags and some unit tests in main_test.go. Could you please take a look?

@johanbrandhorst johanbrandhorst merged commit 1b220ea into grpc-ecosystem:master Jan 28, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks a lot for your contribution!

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…dpoint if the package name exists in the proto file (grpc-ecosystem#860)

* add the package name to the tags field of each endpoint

* prepend the package name only if it is defined in the proto file

* fix the generate stage of the CI by checking in generated files

* add a boolean flag allow_package_name_in_tags and unit tests

* update bazel files

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

Successfully merging this pull request may close these issues.

4 participants