cmd/protoc-gen-openapi: additional_bindings duplicate operationId about gnostic HOT 10 OPEN

@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.

from gnostic.

@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?

from gnostic.

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.

from gnostic.

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.

from gnostic.

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

from gnostic.

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

Correct.

from gnostic.

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.

from gnostic.

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

from gnostic.

@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.

from gnostic.

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

from gnostic.