Generate Swagger with Unique Operation IDs

[dadgar]

This PR prepends the service name to the method name to generate unique operation IDs. Prior to this change, the swagger generator would produce spec non-compliant swagger when multiple services had shared method names. This is quite problematic for generic CRUD services with method names like Create, Delete, etc.

[johanbrandhorst]

LGTM! Thanks your contribution!

[dadgar]

@johanbrandhorst Awesome thanks for merging! Do you think you will cut a release soon?

[aselus-hub]

Is there any way to disable this? a flag?

This has essentially hamstrung our entire autogeneration pipeline as everything that was using a swagger generated SDK is now broken (if this is applied, to clarify)

[johanbrandhorst]

Hi @aselus-hub, how did it break it exactly? Do you rely on the operationID matching the name of the RPC function? This change was made to fix our generated code being noncompliant with the swagger spec.

@dadgar I plan to make a RC to test the waters with this change first.

[johanbrandhorst]

https://github.com/grpc-ecosystem/grpc-gateway/releases/tag/v1.14.4-rc.1

[dadgar]

@aselus-hub You should also be able to use the old operation name by adding annotations:

option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
  operation_id: "oldName";
};

[aselus-hub]

@johanbrandhorst using openapi-generator for REST SDKs, this changed the names of the functions that are generated for it ( link for reference: https://github.com/OpenAPITools/openapi-generator )

[aselus-hub]

@dadgar is that on a per endpoint basis?

[johanbrandhorst]

@johanbrandhorst using openapi-generator for REST SDKs, this changed the names of the functions that are generated for it ( link for reference: https://github.com/OpenAPITools/openapi-generator )

Okay, I'm sorry to hear this breaks your setup, but I still don't understand how? Do you mean that you have existing code calling these functions that are now not calling them correctly?

@dadgar is that on a per endpoint basis?

Yes, this will be a per-endpoint annotation.

Another "solution" is to pass the generated swagger file through some sed to remove the prefix:

sed -i 's/yourServicePrefix_//g' swagger.json

Let me know if that works for you.

If we keep getting feedback about this breaking users we may consider reversing it, but for now I think we'll keep it.

[aselus-hub]

@johanbrandhorst the open-apigen generates an api for each gRPC gateway service we have. This api is used by client code. The names of the functions in the api are the same as the definitions in the swagger file. For example what before was api_name.create is now api.api_name_create, triggers_service.create_trigger becomes triggers_service.trigger_service_create_trigger in python autogenerated code. This also makes the names stutter, which I know upsets some people, me not so much. This should affect anyone using openapi-gen on top of grpc-gateway, i'm not sure how common the usage practice is in all honesty. Reasons for using this is if one is stuck behind load balancers(e.g.:ALB) or having client software that can't/won't use gRPC directly for other reasons yet still want to provide an SDK.

[johanbrandhorst]

Thanks for the background! I do expect a lot of the clients using the gateway to also use swagger. It is good that we have a workaround in form of the operationID override, though it is quite ugly. Maybe a flag to enable the old behaviour is suitable for users who do not wish to subscribe to the bugfix?

[aselus-hub]

Just read through the docs, and according to the documentation operationID is specifically used for code generation, so this means the use surface for this change is 100% code/api generation, meaning all code would be generated with this stutter sequence. Is generating a separate swagger definition per service out of the question? e.g.: protofile.service.swagger.json ?

[johanbrandhorst]

That is interesting, but would probably break more users, since it's harder to include two files than one. Also would be quite a bit more work to get it done with the generator.

[mmourafiq]

Any solution to this breaking change?

[johanbrandhorst]

Any solution to this breaking change?

#1193 (comment)

How exactly is this affecting you? We're still gathering feedback on this change.

[mmourafiq]

The change broke all generated sdks(python/ts/js/go/java) that are used by several services and clients. I am not sure if the change makes sense because of the redundant name of the class in functions, e.g.:

# Previous
AgentsV1Api(self.api_client).get_agent_state 
# New
AgentsV1Api(self.api_client).agents_v1_get_agent_state

The functions now are nested inside a class and have a prefix with the same class name.

[mmourafiq]

Any solution to this breaking change?

#1193 (comment)

How exactly is this affecting you? We're still gathering feedback on this change.

IMHO #1193 (comment) is not really a viable solution, especially if the project has several services. There should be a global flag to fallback to the previous behaviour.

[johanbrandhorst]

Any solution to this breaking change?

#1193 (comment)
How exactly is this affecting you? We're still gathering feedback on this change.

IMHO #1193 (comment) is not really a viable solution, especially if the project has several services. There should be a global flag to fallback to the previous behaviour.

This has been proposed before, and I think it's a reasonable thing to provide since this will probably affect a lof of our users, giving them the power to choose between the old (buggy) behaviour, or adopting the new behaviour. @mouradmourafiq @dadgar would either of you be interested in implementing this? I'd be happy to review it.

[mmourafiq]

I am currently using v1.14.3 to fix the packages that have been published, it was not expected. I will see later if I can open a PR.

[aselus-hub]

I imagine this would affect lots of people that won't even find this, this change is essentially a contract break since the ID is used exclusively for generation as per spec. Another possible suggestion is making the prepend happen only when there's a collision of IDs?

[johanbrandhorst]

Another possible suggestion is making the prepend happen only when there's a collision of IDs?

This was something we discussed but I think @dadgar has it right: #662 (comment).

We'll add a flag to enable the old buggy behaviour, then I'll make a new release and add it to the release notes. All we need is a volunteer to add the flag 😄.

[wibobm]

The operationIds generated are not unique because they do not take into account the package version. The names generated for component schemas do take this concern into account.

[johanbrandhorst]

The operationIds generated are not unique because they do not take into account the package version. The names generated for component schemas do take this concern into account.

Wouldn't that require generating a single swagger file from several different proto packages?

[wibobm]

The operationIds generated are not unique because they do not take into account the package version. The names generated for component schemas do take this concern into account.

Wouldn't that require generating a single swagger file from several different proto packages?

@johanbrandhorst I don't think that would be a requirement or necessity because the swagger plugin has all the package, service and method metadata available when generating the operationIds. Is @dadgar willing to take look at this?

[johanbrandhorst]

What do you mean package version though? Do you mean proto2and proto3? Can you give an example of where this new model does not generate unique operationIds?

[wibobm]

What do you mean package version though? Do you mean proto2and proto3? Can you give an example of where this new model does not generate unique operationIds?

Yeah, I'll try a brief example 1st. Consider this snippet:
...
package my.pacakge.v2;
service MyAPI {
rpc MyRanker (MyMessage) returns (MyMessageResponse);
}
message MyMessage { ... }
message MyMessageResponse { ... }
...

The plugin will generate a swagger schema component called v2MyMessage and v2MyMessageResponse but the operationId will be MyAPI_MyRanker without the "v2" included.

Imagine if you had v1, v3, vX, ... of this interface defined in a different proto. The messages would be generated properly but the service calls would not be represented properly because they would all share the same operationId.

[johanbrandhorst]

I agree with that, but my original point stands about how this would still require you to map several different proto packages to a single swagger file. We don't attempt to guarantee uniqueness across all proto packages. Is that something we should be doing? I'm not currently convinced this is a concern for us.

[johanbrandhorst]

I've merged #1216, which adds an option to reintroduce the old behaviour. With that in place, I'll cut a new release with this functionality and the workaround documented.