-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
#661 add service name to OperationID in swagger gen #662
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
This would be an interesting change to merge. Can you help me understand what the migration strategy would be for people who are using the project right now and aren't expecting their service names to change |
It is not breaking the existing logic. People will receive the new swagger spec which has a different opertional id. "operationId is an optional unique string used to identify an operation. If provided, these IDs must be unique among all operations described in your API." So, it shouldn't cause any issues in reality. But existing logic definitly broken, since you can have multiple services in same proto file, but using same set of method names like Get, List, Delete, the swagger spec could be generated but cannot be used without manual fixing duplicated operation ids. |
I think this is useful and addresses a real problem. However, you should address several concerns; this is, unfortunately, not a one-liner change that it seems to be.
As @achew22 mentioned, because you are changing the operation IDs, this is invasive. Let's be more specific: people who use Swagger definitions to generate client library code will suddenly have to update all their client software, as the method names will have changed to include service name. This is what broke There are ways out of this; how about adding a suffix only if the operation ID is determined to be duplicate? (Similar to what's done with But -- as a separate thing -- you could add a flag that allows the developer to specify if the service name should be included in the operation identifier. Over time the old behavior can be deprecated, and the new behavior can be the default. But it should not be an instant change breaking people's code using generated REST clients. |
@ivucica, thanks for the thoughts! Just a few things to add:
We have discussed proposals like this before, but I'm fairly reluctant to have behavior change based on duplication. My primary consideration is this case: I have To me, it seems like explicitly renaming their method What do you think? |
What I meant is 'leave the original alone', but then again, you can't really tell which method is the original. So ignore the suggestion. I agree, renaming the Swagger operation explicitly is the only way to go about it, and right now it's the only way to do it. But renaming the Swagger operation can only be done by renaming the gRPC method. That breaks the gRPC interface. In gRPC, it's a valid expectation that different services have differently named methods. It's simply not right that Swagger's limitations force a change in how the methods must be named. There's the possibility of using the OpenAPI proto-options to rename the operation (I don't know if this can currently be done, but surely it can be hacked in). That, however, seems like a manual workaround, and necessary only because the default behavior is incorrect. So how about hiding the new operation ID proposed in this PR behind a flag, and announcing that the default value of the flag will change? This makes me sad a bit. To state the obvious: Having the power of hindsight on our side, it feels like 'operation ID is global' and 'we'll just use tags to group methods' was a big mistake on Swagger's part. It's not even an issue on grpc-gateway's side that it declares service membership using tags; Swagger's own code generators use tags to generate service-like groupings of methods in client code. |
It sounds like this would have to be part of a v2 release if it goes in at all. |
I feel like this is a pretty important issue to revisit. As it stands, if you have multiple services with basic CRUD methods, you get operationId conflicts. It seems like the default behavior should be updated to append the service name to generate unique operationIds across services. As a compatibility bridge, supporting setting the operationId explicitly via an annotation seems reasonable for those who want to continue using the unprefixed operationId. |
We welcome help in this area, and fixing clear bugs is an acceptable reason to break users. I don't think we necessarily need the annotation since if our users are going in to manually change their proto files to fix this change they can probably just update their generated files. Would you be interested in contributing a fix for this? |
@johanbrandhorst Just to level set on the expectation for this. The functional change of the PR would be prefixing the operationId with the service name (similar to this PR), and then some unit testing? |
Sounds good to me. I know @ivucica was proposing a slightly less damaging change (new behaviour behind flag), but tbh this does seem like a bug to me, and I don't anticipate it's going to have that big of an impact on users. If I'm missing something, please let me know before you start working on this. |
@johanbrandhorst So I implemented this (https://github.com/dadgar/grpc-gateway/compare/master...f-operation-id?expand=1) but it has a knock on effect I didn't consider and I wanted to discuss before opening the PR. The code generators use operationId to generate the method names. So this change causes the generated clients to be quite verbose: dadgar@bdeecff#diff-2f368f91f1b1c6435040ee606a82216eR818 This is clearly not nice. FWIW, this pattern is what Azure does with their swagger (https://github.com/Azure/azure-rest-api-specs/blob/master/specification/compute/resource-manager/Microsoft.Compute/stable/2019-12-01/compute.json#L42) and they fix it on the client generation (https://github.com/Azure/azure-sdk-for-go/blob/master/services/compute/mgmt/2019-12-01/compute/operations.go#L45). Further the spec that is being generated today is not compliant. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-5 says the ID must be unique. Let me know how you want to proceed. |
Thanks for all that, it's making me more convinced that going ahead with this change is the right choice. Obviously we don't control the generators so I imagine the names will be different, but it's probably worthwhile. One thought I didn have though, would it be possible for us to detect if this is necessary at generation time? So we could make a fix that only activates if necessary? |
@johanbrandhorst I thought about only adding the prefix if a conflict was detected but decided against it for two reasons:
I will move forward with creating the PR then. |
Superceded by #1193 |
No description provided.