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-openapiv2 generates additional paths when API includes several services with resource name based paths #2489

Closed
oyvindwe opened this issue Jan 7, 2022 · 7 comments · Fixed by #2496

Comments

@oyvindwe
Copy link
Contributor

oyvindwe commented Jan 7, 2022

🐛 Bug Report

When specifying a gRPC API with multiple services with resource name based paths, and these services have multiple methods for these paths, extra paths are generated in the OpenAPI file; each service in the proto file gets one extra set of paths per method per number of services below with the same path.

To Reproduce

  1. Make a proto definition of your API, e.g. test/foo_api.proto:
syntax = "proto3";

package test;

option go_package = "example.com/test";

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

service FooService {
  rpc GetFoo(GetFooRequest) returns (Foo) {
    option (google.api.http) = {
      get: "/v1/{name=foos/*}",
    };
  }
  rpc DeleteFoo(DeleteFooRequest) returns (google.protobuf.Empty) {
    option (google.api.http) = {
      delete: "/v1/{name=foos/*}"
    };
  }
}

message GetFooRequest {
  string name = 1;
}

message DeleteFooRequest {
  string name = 1;
}

message Foo { }

service BarService {
  rpc GetBar(GetBarRequest) returns (Bar) {
    option (google.api.http) = {
      get: "/v1/{name=bars/*}"
    };
  }
  rpc DeleteBar(DeleteBarRequest) returns (google.protobuf.Empty) {
    option (google.api.http) = {
      delete: "/v1/{name=bars/*}"
    };
  }
}

message GetBarRequest {
  string name = 1;
}

message DeleteBarRequest {
  string name = 1;
}

message Bar { }
  1. Make sure you have google/api/annotations.proto and google/api/http.proto available in the include path.

  2. Generate the OpenAPIv2 file: protoc -I . --go_out=. --go_opt=module=example.com/test --go-grpc_out=. --go-grpc_opt=module=example.com/test --openapiv2_out . test/foo_api.proto

(I originally encountered this issue using allowMerge=true, but it occurs with a single file containing multiple services as well.)

Expected behavior

The file should only contain paths /v1/{name} and /v1/{name_1}. Each path should only apply to one service.

Actual Behavior

The resulting file contains three paths with get and delete operations, mixed between the two services (responses removed for readability):

  "paths": {
    "/v1/{name_1}": {
      "get": {
        "operationId": "BarService_GetBar",
        "parameters": [
          {
            "name": "name_1",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "bars/[^/]+"
          }
        ],
        "tags": [
          "BarService"
        ]
      },
      "delete": {
        "operationId": "FooService_DeleteFoo",
        "parameters": [
          {
            "name": "name",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "foos/[^/]+"
          }
        ],
        "tags": [
          "FooService"
        ]
      }
    },
    "/v1/{name_2}": {
      "get": {
        "operationId": "FooService_GetFoo",
        "parameters": [
          {
            "name": "name",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "foos/[^/]+"
          }
        ],
        "tags": [
          "FooService"
        ]
      },
      "delete": {
        "operationId": "BarService_DeleteBar",
        "parameters": [
          {
            "name": "name_2",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "bars/[^/]+"
          }
        ],
        "tags": [
          "BarService"
        ]
      }
    },
    "/v1/{name}": {
      "get": {
        "operationId": "FooService_GetFoo",
        "parameters": [
          {
            "name": "name",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "foos/[^/]+"
          }
        ],
        "tags": [
          "FooService"
        ]
      },
      "delete": {
        "operationId": "FooService_DeleteFoo",
        "parameters": [
          {
            "name": "name",
            "in": "path",
            "required": true,
            "type": "string",
            "pattern": "foos/[^/]+"
          }
        ],
        "tags": [
          "FooService"
        ]
      }
    }
  }

Your Environment

macOS: 12.0.1
protoc: libprotoc 3.17.3
go: go version go1.17.2 darwin/amd64
grpc-gateway: 2.7.2

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Jan 9, 2022

Thanks for the issue! Do you think you could try and figure out when this was introduced? Is it a regression or something that has always been present?

@oyvindwe
Copy link
Contributor Author

With 2.7.1, I get paths /v1/{name=bars/*} and /v1/{name=foos/*}, so I guess this is a bug introduced in PR #2461. Unfortunately, I don't know go, so it's a bit hard for me to propose a fix.

@johanbrandhorst
Copy link
Collaborator

Thanks for confirming, that will make it easier to fix!

@aethanol
Copy link
Contributor

@johanbrandhorst @oyvindwe I have raised a PR that should fix this: #2496

@oyvindwe
Copy link
Contributor Author

@aethanol Thank you very much! I'll try to get a dev environment up and see if I can verify the fix locally as well.

While you're at it, how connected is issue #1670?

@oyvindwe
Copy link
Contributor Author

@aethanol I have tested your fix locally, and the additional paths are gone. Thank you very much for your effort!

johanbrandhorst pushed a commit that referenced this issue Jan 18, 2022
…urce path (#2496)

* Fixes additional paths generated when many methods have the same http paths. Fixes #2489

* udpate the pathItemObject from the new path when we have a duplicate path

* add comment for new change

Co-authored-by: Ethan Anderson <eanderson@atlassian.com>
@aethanol
Copy link
Contributor

@aethanol Thank you very much! I'll try to get a dev environment up and see if I can verify the fix locally as well.

While you're at it, how connected is issue #1670?

Hmm that I am not familiar with, can perhaps take a look in the future.

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