gnostic icon indicating copy to clipboard operation
gnostic copied to clipboard

cmd/protoc-gen-openapi: additional_bindings duplicate operationId

Open silas opened this issue 2 years ago • 14 comments

It looks like the changes merged in #367 generate duplicate operationId's.

You can see this in the test example:

  • https://github.com/google/gnostic/blob/3751138311725a0e9d353b805b2e121c94155d18/cmd/protoc-gen-openapi/examples/tests/additional_bindings/openapi.yaml#L13
  • https://github.com/google/gnostic/blob/3751138311725a0e9d353b805b2e121c94155d18/cmd/protoc-gen-openapi/examples/tests/additional_bindings/openapi.yaml#L37

According to the spec these must be unique:

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.

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

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

silas avatar Dec 04 '22 20:12 silas

@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 avatar Dec 09 '22 20:12 timburks

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

silas avatar Dec 09 '22 21:12 silas

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.

sagikazarmark avatar Jan 08 '23 14:01 sagikazarmark

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.

silas avatar Jan 08 '23 15:01 silas

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

sagikazarmark avatar Jan 08 '23 15:01 sagikazarmark

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

Correct.

silas avatar Jan 08 '23 16:01 silas

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.

sagikazarmark avatar Jan 09 '23 08:01 sagikazarmark

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 avatar Jul 12 '23 02:07 Adol1111

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

silas avatar Jul 12 '23 14:07 silas

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

Adol1111 avatar Jul 13 '23 09:07 Adol1111

@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 avatar Nov 24 '23 23:11 davikawasaki

@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 avatar Nov 24 '23 23:11 silas

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

davikawasaki avatar Nov 24 '23 23:11 davikawasaki

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

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

silas avatar Nov 25 '23 00:11 silas