Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

wanghong230
Copy link

No description provided.

@googlebot
Copy link

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@wanghong230
Copy link
Author

I signed it!

@achew22
Copy link
Collaborator

achew22 commented May 24, 2018

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

@wanghong230
Copy link
Author

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.

@ivucica
Copy link
Collaborator

ivucica commented May 25, 2018

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.

  1. Please write/update a test to demonstrate old, incorrect behavior.
  2. Please run make test locally to regenerate the changed .json files.
  3. Please fix broken tests which use real HTTP requests to test various components of grpc-gateway.

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 grpc-gateway integration tests (see TravisCI build).

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 bIdx.) This will address the immediate issue.

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.

@achew22
Copy link
Collaborator

achew22 commented May 27, 2018

@ivucica, thanks for the thoughts! Just a few things to add:

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 bIdx.) This will address the immediate issue.

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 a.proto file that has a method Get. I use this for a long time. Later I add a Get method to a different service and now, all the code I wrote before needs to be updated to reflect the surprise changing of the original Get method's name to packageGet.

To me, it seems like explicitly renaming their method packageGet is more obvious about what is happening and has most (if not all) of the benefits.

What do you think?

@ivucica
Copy link
Collaborator

ivucica commented May 27, 2018

Later I add a Get method to a different service and now, all the code I wrote before needs to be updated to reflect the surprise changing of the original Get method's name to packageGet.

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.

@johanbrandhorst
Copy link
Collaborator

It sounds like this would have to be part of a v2 release if it goes in at all.

@dadgar
Copy link
Contributor

dadgar commented Mar 31, 2020

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.

@johanbrandhorst
Copy link
Collaborator

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?

@dadgar
Copy link
Contributor

dadgar commented Mar 31, 2020

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

@johanbrandhorst
Copy link
Collaborator

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

@dadgar
Copy link
Contributor

dadgar commented Mar 31, 2020

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

@johanbrandhorst
Copy link
Collaborator

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?

@dadgar
Copy link
Contributor

dadgar commented Mar 31, 2020

@johanbrandhorst I thought about only adding the prefix if a conflict was detected but decided against it for two reasons:

  1. I'd rather have consistency of the operation ID so I can for example customize generators to strip the prefix.
  2. Don't want operation IDs of existing endpoints to change if I add a future endpoint with the same name. That could break people using generated clients.

I will move forward with creating the PR then.

@dadgar
Copy link
Contributor

dadgar commented Mar 31, 2020

@johanbrandhorst #1193

@johanbrandhorst
Copy link
Collaborator

Superceded by #1193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants