@silas, thanks for noting this. I think it's unfortunate that this violates the OpenAPI spec because it seems consistent with the meaning of additional_bindings to treat additional bindings as the same operation. Did something specifically break for you as a result of this?

@timburks I had additional_bindings in my current API, so when I updated to the latest version it generated a schema which causes validation errors, but at least still renders in Swagger UI.

I haven't really tested it in other tools yet. I'm using OpenAPI as a fallback for users that can't use supported clients, so to support the widest variety of tools I want it to follow the spec.

I'll probably maintain a fork either way for my customizations, so I don't necessarily need this fixed upstream, just wanted to report it in case you considered it a bug/regression.

It would certainly break code generators that use the operation ID and are not prepared to handle duplications.

The easiest solution would probably be slapping a counter after the generated name for each additional binding.

In addition to the auto-generated operationId we'll need to figure out how to resolve it
when manually applied:

Personally, I wouldn't worry about that much. Manually supplied operationId makes it a user responsibility.

Personally, I wouldn't worry about that much. Manually supplied operationId makes it a user responsibility.

@sagikazarmark How the protobuf definitions are currently defined there is no way for the user to fix it. It only allows you to define the operation_id at the method level:

service TestService {
  rpc Test(TestRequest) returns (TestResponse) {
    option (google.api.http) = {
      get: "/test"
      additional_bindings {
        get: "/test2"
      }
    };
    option (openapi.v3.operation) = {
      operation_id: "test"
    };
  }
}

The above will create two operations with the same operationId.

But it's still because of the additional bindings which is a problem even without the custom operation id, isn't it?

But it's still because of the additional bindings which is a problem even without the custom operation id, isn't it?

Correct.

I'd treat it as a separate problem then (ie. see my earlier proposal for adding a counter at the end of the operation id).

I guess you are aiming at allowing the operation id to be set for each additional binding which is probably doable, but can't be the only solution as not setting it can still yield duplications.

The format of operationId can be changed like this, the first one is set as operationId, and the rest are set as operationId_method_path

@Adol1111 The operationId is commonly used as a method name when generating clients from an OpenAPI spec. This is going to break that use-case.

operationId: Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is RECOMMENDED to follow common programming naming conventions.

This seems to be an unsolvable problem, unless we don't use additional_bindings, or there could be another field to be used as method name

@silas @Adol1111 couldn't we allow users to set a comment with a custom operationId on top of the additional_bindings line? It could be either a full override value or just a suffix to be added - the latter would not only solve the duplication but could possibly ensure the recommended naming convention previously described.

@davikawasaki Lots of ways it could be solved, it's really whatever seems appropriate to the maintainers. I forked protoc-gen-openapi a while ago and I'm just maintaining my own version so I don't really have a lot of skin in the game anymore.

@davikawasaki Lots of ways it could be solved, it's really whatever seems appropriate to the maintainers. I forked protoc-gen-openapi a while ago and I'm just maintaining my own version so I don't really have a lot of skin in the game anymore.

@silas is it a public fork that fix this issue?

@silas is it a public fork that fix this issue?

Sorry, no. In the end I just removed my usage of additional_bindings anyway.