Follow-up for 653 Issue - Duplicate operationIds and underscore symbol - AutoRest has a bug around underscores

[centur]

The issue #653 mentions an article about how to solve the problem with duplicate operationIds generated by Swashbuckle.

Duplicate operationId is not only Swashbuckle fault, Autorest can't parse OperationId with more than one underscore correctly: so if your operationIds are :

"operationId": "Debug_Get",
"operationId": "Debug_GetByid",
"operationId": "Debug_ReceiveCommandByjsonData",
"operationId": "Debug_Get2Bymodel",

Autorest will work fine.

if you have some prefixed operationIds like (they are still unique though):

"operationId": "V1_Debug_Get",
"operationId": "V1_Debug_GetByid",
"operationId": "V1_Debug_ReceiveCommandByjsonData",
"operationId": "V1_Debug_Get2Bymodel",

Autorest fails to interpert these operationIds correctly - you'll get a message

error: [FATAL] Error generating client model: Found operation objects with duplicate operationId 'V1_Debug'. OperationId must be unique among all operations described in the API.

It was mentioned by @VR-Architect in #651 that it was fixed in v.0.14, but I checked valid swagger.json with both v.0.15 and v.0.14 and they are both failing to parse operationid when it has more than one underscore.

As soon as I update swagger definition to have operationId with just one underscore - it works fine.

It seems like an issue in AutoRest operationId parsing.

[amarzavery]

@centur - To give you some context behind this design, we use the Noun_Verb a.k.a OperationGroup_Method format for the operationId. This translates to grouping methods under the provided methodgroup. When you call the method, it would be client.OperationGroup.Method(..).

I am not sure, if the multiple underscore issue was fixed. Ideally, AutoRest should have ignored the first underscore and should have concatenated the string until the last underscore and should have made it the OperationGroup name in the generated code.

If this is an issue, it will be fixed as described above.

From the described format "operationId": "V1_Debug_Get", it seems that you are including the api version of the service in the method name. There is a version filed in swagger for that. As per swagger philosophy, one swagger doc should describe the service api for one version that is described in the version section.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#info-object

[centur]

This is just an example, AutoRest will fail if my action in Debug controller will be named as "Get_ByParameter1" when Swashbuckle will generate "Debug_Get_ByParameter1" as operationId. And it is a valid name, valid operationId valid naming convention and has nothing with swagger or any other 'philosophy' matters , nor any API versioning.

I understand where the root of AutoRest parsing problem is - for whatever technical reason team decided to use one of the valid symbols (valid in method name) as a split character to separate Noun from Verb and create nice OperationGroup objects on the client. But one of the edge-cases is missing - Verb by itself can contain an underscore, as well as Noun can too. Not the greatest practice but in the real world that happens. Why not use something like Tags section first, if it's defined. Swashbuckle creates nice tags section

 "/api/test": {
      "get": {
        "tags": [
          "V1_Debug"
        ],
        "operationId": "V1_Debug_Get_ByParameter1",
        "consumes": [],
        "produces": [
          "application/json",
          "text/json",
          "application/xml",
          "text/xml"
        ],
        "responses": {
          "200": {
            "description": "OK",
            "schema": {
              "$ref": "#/definitions/Object"
            }
          }
        },
        "deprecated": false
      }
    },

which, if present, can be used to clearly separate Noun from Verb.

[devigned]

We should be using Tags first, then OpId.

Though, the way @centur describes the Verb and Noun both containing underscores worries me a bit. It seems like this will end up with some pretty nasty names. How do you decide where the noun and the verb are with something like: Nou_n_V_er_b? If you have undefined number of _, then building nice names from OperationIds is going to be pretty gnarly. Do you then got to where Capitalizations are? I don't really want to play that game, b/c I don't think we will ever make everyone happy.

I think we should do the following: 1) Use Tags for groups, then the full OpId in the language normalized form. 2) If no Tags, then use OpIds where the group is the first substring before the first _, and the operation name is the language normalized form of the substring after the first _.

[amarzavery]

We discussed the design for this in the scrum. We want to keep things super simple.

• If there is exactly one underscore in the operationId, then things will function as they are working today.
• If you have more than 1 underscore, then we will not do any kind of grouping. The underscores will be removed and the entire string will be concatenated. The concatenated string will be a method on the client.

We do not want to get into the business of determining the least common subsequence (for determining the operationGroup) and the rest to be a method name.

[centur]

It's a strange decision imo - you voluntary want to make Client experience even more inconvenient for API consumers while you have pretty simple, convention-based option to allow naming customization.

It's much simpler to mark each controller with the right tag (if we are talking about Swashbuckle nuget as one of the most popular Swagger generators in .NET, recommended by MS devs) rather than marking each action inside the controller to have fixed and correct (exactly one '_') OperationId because of what ? Substring is hard or allowing one extra convention will get you into the the entire merciless big business of determining how to split strings?

Edited: now we are not only having 'Drawing_UpdateDocumentDetailsBydrawingIddataapiverparentProjectId' methods on the clients because method overload is hard, but we are going to have 'V1DrawingUpdateDocumentDetailsBydrawingIddataapiverparentProjectId' because of this 'don't want to get into business' decision

[devigned]

@amarzavery we need to include tags in our grouping logic.

[centur]

Also, some extra details and another example where decision to split by a single underscore will not work: If one is using Swashbuckle built-in attribute to specify OperationId like this

        [HttpGet, Route("errorcodes")]
        [SwaggerOperation("GetAllErrors")]
        public string[] ErrorsList() { return GlobalErrors.GetList().Values.ToArray(); }

on the action - it doesn't generate controller prefix so the OperationId will be just 'GetAllErrors'. And according to the current logic - will be added on the client level, not on the MethodGroup level. Although the Tag for that operation is still pointing to the right MethodGroup. Would be strange to leak this Noun_Verb pattern in each operation.

Tags are very helpful when they are in the definition.

[igotlotsofspace]

@centur you are absolutely correct and can confirm everything you said. It took me endless articles, blogs and videos in a span of 48 hours to finally come across your post. Very glad you wrote this! The only solution is to make sure the controller class and endpoint name combination result in a solitary underscore. The problem is not with Swagger or Swashbuckle, but rather AutoRest inability to parse correctly. Thank you!

[markcowl]

@centur the reason we use OperationId is that it is guaranteed, by the swagger spec to be unique across all the operations in the spec. Other mechanisms for determining operation name are not subject to this restriction, and so can result in the need to rename operations, or throw errors about operation name collisions that are difficult to debug. There are real advantages in having a single, simple, mechanism that can uniquely name the operations in a predictable way. This mechanism can also handle naming of client-level operations by omitting the underscore (and the uniqueness claim remains intact).

We originally considered and rejected Tags as a grouping mechanism for precisely this reason.

Note that the appservice specification in the https://github.com/azure/azure-rest-api-specs repo is generated using SwashBuckle with additional filters, so using this mechanism is at least technically possible using SwashBuckle.

[centur]

Are we discussing the replacement of the mechanism or it's enhancement ? I thought the latter, but your arguments are against the former so they are not really applicable:

Swashbuckle supports custom attribute to set the operation Id, e.g. [SwaggerOperation("GetAllErrors")]. It's what we can use to make our names nicer, but it doesn't guarantee that there will be no other action with the same attribute (and as a result - with the same OpId). Neither Swashbuckle cares about action overloads (another issue, another thread). There is really no difference, in terms of workload, to manually guarantee all [SwaggerOperation(OperationId)] attributes uniqueness or the same uniqueness of method names, I'd say even that renaming methods is easier - they are scoped to a single controller, and attributes are project-wide.

I agree there is a problem in this particular definition generator that it is not 100% swagger-compatible atm. This is a life and such things will always exist. It's fine to make Autorest to work only with 100% swagger-compatible definitions, but it it has nothing with the issue described above.

The fact that it's technically possible because X is in spec and someone done it - adds no real arguments towards the decided Noun_Verb pattern - the decision behind using underscore has nothing with a spec, spec doesn't limit number of certain characters in the operationId, does it ?

And we not even contesting an existing pattern - @devigned proposed solution clearly keeps OperationId and 'language normalized' form as a fallback operation IF there is no Tags or the tag string is not in OperationId string. So the fallback algorithm stays the same - look for first underscore and split by it, if there is more than one - remove all and move method on the client itself.

But there is a clear win for using tags info as a helping mechanism for splitting Noun<=>Verb:

Client, who is using Swashbuckle, doesn't need to do anything (except to guarantee method name uniqueness within the controller, which is unrelated issue) and AutoRest works much better with the Defaults, not requiring any custom (and very laborious for any except hello-world-on-stage-demos) markup of every operation.

Well, the raise and fall history of other projects clearly shows that good, meaningful and helpful defaults are playing pretty vital in role in number of bugs and overall project \ library \ language impression, why, if you made this project open-source, so reluctant to a reasonable defaults handling enhancement ?

[fearthecowboy]

Howdy!

In our planning for driving towards a stable '1.0' release, I'm marking this issue as 'deferred' :zzz: and we're going to review it during the post-1.0 planning cycle.

It's not to say that we're not going to work on it, or that this isn't not important, but at the moment, we're picking and choosing the stuff we must do before 1.0. :horse_racing: :horse_racing: :horse_racing:

We'll make sure we pick this back up at that point. :tada:

[mossyblog]

If you need a case study for why verb_another_verb_whatever etc is going to continue then look no further than EVE Online - https://esi.tech.ccp.is/latest/swagger.json?datasource=tranquility

Point is if you're going to walk into this entire "AutoRest" with the attitude of "if its not within our bubble, its not at all" then you've learned nothing from our days with SOAP.

Deferring for "stable" is also a weak avoidance behaviour. Address the issue and keep this open until you've confined the solution to a reasonable approach. GitHub isn't Microsoft Connect and the days of when we used to triage problematic issues like this with "as per design" or "deferred to x" isn't going to work with OSS space.

This is how you bank hate debt with developers.

[bowman-jm]

I have a Swagger doc generated from a Django based REST API. I am generating a client for my Xamarin app and this issue is very annoying, to say the least. The behavior from the Django REST swagger generator is to add an underscore between URL path segments.. So in order to make this work I need to manually edit the swagger doc to get rid of all the 'extra' underscores to have the client generated as expected.(This has to occur every time the API changes.. which is often now as it is in development).. Is there anyway this issue could be given a higher priority?

[fearthecowboy]

I have never been a fan of the implicit decision in AutoRest to parse the operationId and use that as the basis for choosing "method-group" and "method" names. Seems kinda hacky to me to begin with.

AutoRest is replete with implicit assumptions ... such is the life when you inherit a codebase.

Later this week, we're starting work on an overhauled configuration system that will make it simpler to override behavior in the code generation process (by providing an external file that contains the information to the generator), without having to edit the swagger file itself (which helps not only @bowman-jm 's scenario of an automated swagger generation, but others who don't want to pollute the swagger with information that doesn't really describe the wire-behavior)

It should not be terribly difficult to provide an override for methodgroup and method name on a per-operation basis in there.

[mossyblog]

Who are "others" ... i get suspicious when that appeal to authority is thrown on the table to consume. Others being 10 dudes you asked at a bar, or 100,000 people you polled at a Java conference?

So to clarify we're back to hacking in the weeds outside the tool again? which somewhat defeats the purpose of having the said code-gen tool doesn't it?

[erendrake]

This has been a frustration on my team as well. esp because many other swagger tools use the tags for grouping such as SwaggerUI and swagger codegen.

I started implementing tags based grouping in this PR https://github.com/Azure/autorest/pull/2046 its far from complete but i have held off doing the rest of the work until this next configuration pass is complete.

[TsengSR]

Imho tags should be used for grouping and the prefix of operationId being ignored. Then we can use Controller_ActionName or SomeUniqueOrHashedValue_GetAll and ignore everything before (or after) the underscore.

Currently the underscore for grouping is less then optimal, we can't have an action appear in multiple groups.

Let's say I have an OrdersController controller and an CustomersController. There is an Orders.GetAll() Method and an Orders.GetAllByCustomerNumber(string customerNumber).

Now I'd like the Orders.GetOrdersByCustomerNumber(string customerNumber) appear on both, Customers and in Orders group. To make this happen, I can add "tags": [ "Orders", "Customers"] to the swagger.json and swagger-ui (which is mostly used to visualize the spec/doc in the browser), will correctly put this method in both groups.

When now the client is generated, with GroupingName_ActionName pattern, I can't put them in both groups now, but there are valid reasons to have them in both. First its more convenient, that if you want to fetch orders you do that on the orders group. But when you work on customers you also may want to access it, since the orders belong to the customer. api.customers.getOrdersByCustomerId is more convenient.

Also the GroupingName_ActionName breaks support for other code-generators. In 90% of all cases the consumer of the API is not the API developer/company, but a customer who uses your API. And they may decide (or require) to use i.e. swagger-codegen or something else which doesn't use this pattern. Then they end up with ugly Apis such as: OrdersApi.ordersGetByOrdersByCustomerId for the example from above.

It's okay if GroupingName_ActionName is the default behavior, but we definitely need a way to group by tag while utilizing the uniqueness of the operationId (i.e. ignoring specific parts of the operation Id when generating the actionname, whatever that may mean (ignore everything before/after an _ or use [ ] (if the spec allows) to determine the uniqueness hash or just drop the "tags" that are prefixed to the operationId

[fearthecowboy]

So... there is a way to handle this now without changing AutoRest itself (and at this point, I'm not inclined to revisit decisions made prior to me taking over the project).

If you switch to using a configuration file for running AutoRest (use autorest init to get an example) , you can use a directive to modify the model.

Make a readme.md in your folder something like this:

# MyPetStore
> see https://aka.ms/autorest

This is the AutoRest configuration file for the MyPetstore.

---
## Getting Started 
To build the SDK for MyPetstore  simply [Install AutoRest](https://aka.ms/autorest/install) and in this folder, run:

> `autorest`

To see additional help and options, run:

> `autorest --help`
---

#### Basic Information 
These are the global settings for MyAPI.

``` yaml
# list all the input OpenAPI files (may be YAML, JSON, or Literate- OpenAPI markdown)
input-file:
  - petstore.json

# this allows you to programatically tweak the swagger file before it is modeled.
directive:
  from: swagger-document # do it globally 
  where: $.paths.*.* 

  # if every operation has a tag, you can be simple:
  transform: $.operationId = `${$.tags[0]}_${$.operationId}`

  # otherwise if you have some things that don't have a `tags` use this:
  # transform: $.operationId = `${( $.tags || [] )[0] || "some_prefix_here" }_${$.operationId}`  

```

---
#### Language-specific settings: CSharp

These settings apply only when `--csharp` is specified on the command line.

``` yaml $(csharp) 
csharp:
  namespace: My.PetStore
```

and then run

autorest --csharp readme.md

and it will use the first tag for each operation as the group name.

[TsengSR]

@fearthecowboy Correct me if I'm wrong, but this would still only create one method per "path" entry in the swagger definition file? The thing is, when you have multiple tags there, swagger-ui puts the method in all of the groups? Is it anywhere documented on what kind of operations you can use inside the yaml configuration? Can we perform simple substring operations etc on it?

It's quite tricky/hard to make the operation Id unique and with friendly names, though not impossible if you add some redundancy to the operationId (i.e. getAllCustomers vs Customers_getAll and ignoring the Customers_ part when generating the Api.

[fearthecowboy]

@TsengSR Indeed, that's how it would work.

I could change that so it duplicated the messages per-Tag. Not too tricky.

The transform is essentially a javascript evaluation (using a saveEval method) where can tweak a document model pretty much anyway you'd like. (You can see more examples of transform directives in the sample: https://github.com/Azure/autorest/tree/master/Samples/3b-custom-transformations ) @olydis is working on an inspector for AutoRest, that can show you the state of the model at any step in the generation pipeline.

In the transform directive, you get three javascript variables :

• $ - the reference to the current node
• $doc - the whole document
• $path - the json path to the current node

Currently the underscore for grouping is less then optimal, we can't have an action appear in multiple groups.

I agree completely. It was that way when I got here. There are a number of things that I think are sub-optimal, which is why we're about to write a whole new modeler that will support OpenAPI 3 and make it much simpler to identify why a particular assumption was made, and how you can change that assumption. (ie, like using underscore in operationId to determine class name)

[timmclaurin]

It would be great if this could be resolved. It is the oldest item on the issues log and specifically keeps us from using AutoRest. Is there any word on the new modeler, @fearthecowboy ? Thanks for the hard work.

[fearthecowboy]

The