Bug 300: When an operationId has already been used; append a postfix...

[sjmelia]

…to get a unique one. This provides a resolution for issue #300 and at least ensures that a valid Swagger spec is produced.

Many thanks for your fantastic work on Swashbuckle; i've used it many times and would love to make a contribution - let me know if any further work is required!

[moander]

Hi,

From swagger 2.0 spec:

• Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions.

I have not looked into the PR but I guess the operationId for a spesific method may change if methods are re-ordered or new ones are placed in between?

[sjmelia]

Well, the order is actually not guaranteed but yes will usually depend on the order of declaration. So changing that order may change the designated id given.

What makes a method unique in C# is of course the signature as a whole; so perhaps a better approach would be to concatenate method name and the names of parameter types? This would be pretty ugly so you would only want to do this for overloaded methods.

[sjmelia]

Had a quick look - it would be pretty trivial to change the FriendlyId to include the parameter types for all methods; slightly less trivial to do this for overloaded methods only - as you'd need to collect duplicates first ahead of creating friendly names. I'm happy to look into either but will probably wait for feedback from the maintainer first. 😄

[moander]

+1 for using argument names. I think the meta used to generate the path should be enough to create a predictable operationId because Swashbuckle already throws if a non-distinct method name is found within a single path :)

// operationId: Foo_Get
string Get() {}

// operationId: Foo_Get_id
string Get(string id) { }

Adding Get(string id, string foobar) to the controller above will throw:

Not supported by Swagger 2.0: Multiple operations with path 'api/Foo/{id}' and method 'GET'.

[heldersepu]

Great addition...
When can we expect this to be merged in?

[heldersepu]

Would love to see this merged...

Can you please fix the conflicts:
Conflicting files
Swashbuckle.Core/Swagger/SwaggerGenerator.cs
Swashbuckle.Dummy.Core/Swashbuckle.Dummy.Core.csproj

[sjmelia]

No problem! I've force pushed a rebase for the original pr; appending an incrementing integer to the end of the name.

There is an unrelated failing test SwaggerUITests.It_returns_correct_asset_mime_type.

[heldersepu]

Awesome!, I hope this makes it soon...
Did you noticed that they are looking for Maintainers: #1053 ?

[heldersepu]

... and I just tested your changes, all tests are passing

[heldersepu]

Can we make this a bit more generic ?!?!

instead of the (var firstGetOperation, secondGetOperation & thirdGetOperation) we should create a list of all operationId, and then make sure they are unique...

See code below:

            var opIds = new List<string>();
            foreach (var path in swagger["paths"])
                foreach (var action in path)
                    foreach (var op in action)
                        opIds.Add(op.First()["operationId"].ToString());            
            Assert.AreEqual(opIds.Count(), opIds.GroupBy(x => x).Count());

[sjmelia]

Thought about it and I agree; a less specific test is more useful here.

[domaindrivendev]

@sjmelia - I know it's been a while and I apologize for not providing feedback sooner.

The operationId is most commonly used as a method/function name in client libraries that are generated from the Swagger description. Although your solution does result in a valid spec. (a good thing for sure) it would result in some confusing method names in those client libraries.

In the ASP.NET Core version of Swashbuckle, I'm generating operationId from a combination of the URI (including path and query params) and HTTP verb instead of the C# method name. This prevents the conflict and hides implementation detail. The code is relatively straightforward:

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/src/Swashbuckle.AspNetCore.SwaggerGen/Generator/ApiDescriptionExtensions.cs#L35

If we're going to solve the conflicting operationId in this version of Swashbuckle, then I'd like to follow a consistent approach.

In addition, I'd like to make it optional and defaulted to the old approach, to maintain backwards compatibility in auto-generated clients unless the service provider explicitly applies this option. This should be highlighted in the docs.

Any chance you'd be interested in submitting a new PR with this approach. I'll try my best to be more responsive this time around :)