add support for resource name in swagger plugin (#702)

[ch3rub1m]

Fix this issue: The swagger plugin couldn’t distinguish two rpcs if we use the resource name design style. (#702)

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[ch3rub1m]

I signed it!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[johanbrandhorst]

Reopening on the request of @ch3rub1m

[johanbrandhorst]

@ch3rub1m could you push your changes again please?

[ch3rub1m]

@johanbrandhorst @achew22 I already pushed. But it built failed again.

[achew22]

Yeah, the problem is that some library we depend on for serializing changed their default for serializing enums from the numeric representation to the string representation. A reasonable change to make, but we need to explicitly set that in whatever library that is. Sorry I just haven't had time to dig into it

[ch3rub1m]

@achew22 Ok, I got it, thanks.

[achew22]

Could you rebase off of master? We have since fixed the CI issue

[achew22]

Also, I was thinking about this. Would you be willing to (feel free to push back) add some new usage in the everything example? I would love it if we could do an e2e too

[ch3rub1m]

@achew22 It still not work with GO: 1.10.x. Could you help to check it please?

[ch3rub1m]

@achew22 Hi, I would like to add more usage, but it seems like it's not so suitable to add them directly to the everything example. May I create a new example? By the way, please take a look at the CI issue when you are not that busy.

[achew22]

Yes, please. Feel free to add as many examples as you think are necessary

[jon-whit]

What's the status on this PR? I think I have the same need? Correct me if I am wrong please :)

I have a field in a protobuf message called "parent". It looks like this:

  message ListRolesRequest {
    // The resource name of the parent resource in one of the following formats:
    // `` (empty string) -- this refers to curated roles.
    // `organizations/{organization_id}`
    // `tenants/{tenant_id}`
    string parent = 1;
    ...
  }

  rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
      //Maps to HTTP GET.
      option (google.api.http) = {
        get: "/auth/iam/roles"
        additional_bindings {
          get: "/auth/iam/{parent=organizations/*}/roles"
        }
        additional_bindings {
          get: "/auth/iam/{parent=tenants/*}/roles"
        }
      };
  }

The parent can take the form "organizations/<org_id>" or "tenants/<tenant_id>" or it can be empty. I'd like the swagger to match all three. For example:

GET /v1/organizations/<org_id>/roles
GET /v1/tenants/<tenant_id>/roles
GET /v1/roles

How can I achieve this?

[ch3rub1m]

@jon-whit I am writing the examples for this PR. I think it's different with your need. But you could do this to reach your demand:

GET /v1/{parent=*/*}/roles

[jon-whit]

@ch3rub1m Are you saying I can already achieve what I'm looking for? If so, what would the protobuf HTTP options look like for that? That's what I'm struggling with.

If not, are you saying that once this PR is merged the example above will be supported? Sorry for the confusion.

[ch3rub1m]

@jon-whit Yes, you could already achieve what you are looking for in current version! But there are some bug in generated swagger doc. And I am trying to fix it. But the bug just affect the doc so the service is still work. You should write as follow to achieve what you want:

message ListRolesRequest {
    // The resource name of the parent resource in one of the following formats:
    // `` (empty string) -- this refers to curated roles.
    // `organizations/{organization_id}`
    // `tenants/{tenant_id}`
    string parent = 1;
    ...
  }

  rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
      //Maps to HTTP GET.
      option (google.api.http) = {
          get: "/auth/iam/{parent=*/*}/roles"
      };
  }

[jon-whit]

@ch3rub1m That doesn't match the three mentioned above though.. It will only match

GET /auth/iam/organizations/<org_id>/roles
GET /auth/iam/tenants/<tenant_id>/roles

I'd like a match that matches all three of the following:

GET /auth/iam/roles
GET /auth/iam/organizations/<org_id>/roles
GET /auth/iam/tenants/<tenant_id>/roles

Is this a scenario where the additional bindings are meant to be used?

[codecov-io]

Codecov Report

Merging #704 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   56.16%   56.29%   +0.12%     
==========================================
  Files          30       30              
  Lines        3112     3121       +9     
==========================================
+ Hits         1748     1757       +9     
  Misses       1193     1193              
  Partials      171      171

Impacted Files	Coverage Δ
protoc-gen-swagger/genswagger/template.go	39.39% <100%> (+0.63%)	⬆️

---

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 bb916ca...291e7c1. Read the comment docs.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[ch3rub1m]

Hi, @achew22. I tried to write some usage in everything example but it failed in travis CI. The bazel really confused me and I don't know how to generate *.pb.go and swagger file by bazel. So I roll back the example commit and it works again. Could you approve this PR first please? I have done many tests in my own project and it seems the PR is ready to be merged. If you wants more usage in example I will open another PR.

[johanbrandhorst]

This LGTM, thanks for your contribution!

[tlyng]

I'm unable to generate valid swagger output after this merge. I've tried two validators / parsers and they both complain about the same thing.

Semantic error at paths./api/v1/{name=shelves/*/books/*}
Declared path parameter "name=shelves/*/books/*" needs to be defined as a path parameter at either the path or operation level

Take this example protobuf specification:

syntax = "proto3";

import "google/api/annotations.proto";
import "google/protobuf/empty.proto";

service LibraryService {
    
    // Shelf management
    rpc GetShelf(GetShelfRequest) returns (Shelf) {
        option (google.api.http) = {
            get: "/pai/v1/{name=shelves/*}"
        };
    };

    rpc CreateShelf(CreateShelfRequest) returns (Shelf) {
        option (google.api.http) = {
            post: "/api/v1/shelves"
            body: "shelf"
        };
    };

    rpc DeleteShelf(DeleteShelfRequest) returns (google.protobuf.Empty) {
        option (google.api.http) = {
            delete: "/api/v1/{name=shelves/*}"
        };
    };

    rpc ListShelfs(ListShelfRequest) returns (ListShelfResponse) {
        option (google.api.http) = {
            get: "/api/v1/shelves"
        };
    };

    // Book management
    rpc GetBook(GetBookRequest) returns (Book) {
        option (google.api.http) = {
            get: "/api/v1/{name=shelves/*/books/*}"
        };
    };

    rpc CreateBook(CreateBookRequest) returns (Book) {
        option (google.api.http) = {
            post: "/api/v1/{parent=shelves/*}/books"
            body: "book"
        };
    };

    rpc DeleteBook(DeleteBookRequest) returns (google.protobuf.Empty) {
        option (google.api.http) = {
            delete: "/api/v1/{name=shelves/*/books/*}"
        };
    };

    rpc ListBooks(ListBookRequest) returns (ListBookResponse) {
        option (google.api.http) = {
            get: "/api/v1/{parent=shelves/*}/books"
        };
    };
}

message Shelf {
    // Resource name of the shelf. It must have the format of "shelves/*".
    // For example: "shelves/shelf1"
    string name = 1;
}

message Book {
    // Resource name of the book. It must have the format of "shelves/*/books/*".
    // For example: "shelves/shelf1/books/book2"
    string name = 1;
    string isbn = 2;
    string author = 3;
}

message GetShelfRequest {
    // Resource name of the shelf.
    // For example: "shelves/shelf1"
    string name = 1;
}

message CreateShelfRequest {
    // Resource name of the shelf.
    // For example: "shelves/shelf1"
    Shelf shelf = 1;
}

message ListShelfRequest {
}

message ListShelfResponse {
    repeated Shelf shelves = 1;
}

message DeleteShelfRequest {
    // Resource name of the shelf.
    // For example: "shelves/shelf1".
    string name = 1;
}

message GetBookRequest {
    // Resource name of the book.
    // For example: "shelves/shelf1/books/book1"
    string name = 1;
}

message CreateBookRequest {
    // Resource name of the parent resource where to create the book.
    // For example: "shelves/shelf1".
    string parent = 1;
    Book book = 2;
}

message DeleteBookRequest {
    // Resource name of the book to be deleted.
    // For example: "shelves/shelf1/books/book1"
    string name = 1;
}

message ListBookRequest {
    // Resource name of the parent resource.
    // For example: "shelves/shelf1"
    string parent = 1;
}

message ListBookResponse {
    repeated Book books = 1;
}

when generating the swagger output for this the result contain the following problematic statements:

...
 "/api/v1/{name=shelves/*/books/*}": {
      "get": {
        "summary": "Book management",
        "operationId": "GetBook",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/libraryserviceBook"
            }
          }
        },
...
"/api/v1/{name=shelves/*}": {
      "delete": {
        "operationId": "DeleteShelf",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "properties": {}
            }
          }
        },
...

{name=shelves/*}and similar is not defined in parameters, therefor the validators fail to validate it. Previously this got rendered to /api/v1/{name}, which was defined.

[johanbrandhorst]

@ch3rub1m Any ideas for this error? I was looking through our examples but couldn't find one with a name definition like this. I'm tempted to revert this MR as it has broken some users.

[johanbrandhorst]

I'll give @ch3rub1m a week to respond, there might be a workaround.

[ch3rub1m]

@johanbrandhorst I just knew this problem. I'll try to find a trade-off to solve it.

[johanbrandhorst]

Great, thanks for your response. Since this breaks existing users I will roll back this change and reopen the original issue.

[ch3rub1m]

@tlyng Hi, I ran your examples in my environment, and I found that there are some errors was complained by validator but it works well for doc generation (by https://github.com/Redocly/redoc).

Could you tell me your real use case? Actually in your use case, if this PR was reverted, you couldn't both get apis /api/v1/{name=shelves/*/books/*}" and /api/v1/{name=shelves/*} in your swagger file. Because this PR is for fixing that the multiple apis using resource name style being translated to same swagger json key, in your example, which one is /api/v1/name, so just one of them will be kept.

@johanbrandhorst For the above reasons, I think this PR shouldn't be reverted, because it might fix the warnings of validator and generate a valid swagger but more logical errors back. We should consider another workaround to mitigate the problems of validator instead of reverting this PR.

[johanbrandhorst]

As I understand it, it's not just a matter of "warnings in the validator", but not being able to use generators or web UIs that rely on valid swagger files. I think that is a bigger problem right now, although of course I would like to see both issues fixed. I am proposing reverting this PR until we can get a new solution to the duplicate resource generation that still produces valid swagger files.

[ch3rub1m]

@johanbrandhorst Understood, how about using encoded slash %2F instead of / and use whole string name=users%2F* as the name of parameter, like below:

    "/v1/{name=users%2F*}": {
        "parameters": [
          {
            "name": "name=users%2F*",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
...

It's a valid swagger and solve the duplicate resource generation even if it doesn't look good.

[johanbrandhorst]

I'm happy with that solution, if you would like to draft a competing PR to #1000 I'd be happy to merge that if it fixes @tlyng's problem.

[ch3rub1m]

I am working on it. Thank you.

[tlyng]

Sounds good enough for me! I'm not personally using swagger documentation for consuming API's, it's a request I've gotten from other developers. Personally I use protobuf , grpc and grpc-web. Providing swagger/openapi reduces the brainwork required by people already used to operate in such environments.