Epic: Improve API version support

[RicoSuter]

Issues:

• NSwag.MSBuild does not support API versioning or multiple routes #1354
• When using inheritance api versions are not correctly versioned #1288
• Accept multiple Route Url in the same controller #1160
• multiple ApiVersion get skipped in WebApiToSwagger for ASPNetCore #1241
• Failed to generate documentation for multiple API versions using ApiVersioning and ApiExplorer #1393
• Improve ApiVersioning support by automatically generate routes using Controllers definitions #1524

[commonsensesoftware]

I just stumbled across a number of posts and issues that all seem to lead back to here.

The implementation of ApiVersionProcessor.cs is not correct and will miss many scenarios. API Versioning does not actually care about attributes and that is only one way that API versions can be provided. API versions can also be provided by conventions or through custom extensions.

The API Explorer extensions provided by API Versioning already does all of the difficult work of collating API descriptions by API version so they can be integrated with other libraries such as Swagger generators like NSwag. Things already work well with Swashbuckle without any cross-dependencies or understanding of what each library does. I would be curious and interested in understanding why the same model doesn't work for NSwag. I'm open to discussing how the integration can be made better.

[RicoSuter]

@commonsensesoftware you are right, we should remove the ApiVersionProcessor completely and only rely on the grouping in the versioned API Explorer... this way it should just work. I never used this in a project this is why it's not really correctly implemented.

[commonsensesoftware]

No problem. It was less of a critique and more of "Hey, is something not working right?".

In working with Swashbuckle, we uncovered a design flaw/omission in the ASP.NET Core API Explorer design. You'll want to take a peek at: aspnet/Mvc#7563.

The ASP.NET team has made the necessary adjustments, but it won't be available until 2.2. This may affect your backward compatibility story. At a minimum, it should help explain what can or cannot be mapped. The documentation is pretty thin in this area and I was left scratching my head for quite a while.

There's a pretty long thread in Swashbuckle on this topic too. Effectively, we agreed that ASP.NET Core has this gap (which is how we got them to change it) and we shouldn't make any special workarounds. Once the change is available, API Versioning will populate the correct explored values and consumers like NSwag, Swashbuckle, etc can consume it.

Should you run into any issues. Feel free to reach out.

[RicoSuter]

@commonsensesoftware just checked this, if you use AddVersionedApiExplorer:

https://github.com/RSuter/NSwag/blob/master/src/NSwag.SwaggerGeneration.AspNetCore.Tests.Web/Startup.cs#L27

And specify the included versions as shown here (i.e. a api group name filter):

https://github.com/RSuter/NSwag/blob/master/src/NSwag.SwaggerGeneration.AspNetCore.Tests/ApiVersionProcessorWithAspNetCoreTests.cs#L16

It just filters by api group name and removes {version} from the route (if available):

https://github.com/RSuter/NSwag/blob/master/src/NSwag.SwaggerGeneration/Processors/ApiVersionProcessor.cs#L33

No other reflection hacks (as i see it).

But maybe it would be still good to add a ApiGroup setting to AspNetCoreToSwaggerSettings so that we can filter by api group directly (but then the {version} removal is missing if the processor is not added).

[commonsensesoftware]

If a service author is versioning by URL segment and they don't want the route parameter or API parameter desciption included, they just need to specify that in the API explorer options a la:

AddVersionedApiExplorer( options => options.SubstituteApiVersionInUrl = true );

This removes all API version parameters and substitutes the route parameter with a value formatted according to the options.SubstitutionFormat (defaults to "VVV", which is the most common). If the URL segment method is not used, these options have no effect.

It's also worth noting that the apiVersion route constraint key is configurable starting in API Versioning 3.0+ via options.RouteConstraintName, even though it's not likely to be changed. The name of route parameter is completely user-defined. While it's common and used in many examples, there is no requirement that the route parameter name needs to be version. The route parameter name is only meaningful to the service author. ASP.NET Core does the work of parsing the route template, matching the route constraint, and passing the name and value of the parameter to the constraint for processing. In other words, a route parameter defined as {v:apiVersion} or {ver:apiVersion} is perfectly valid. The ASP.NET API Explorer strips out the route constraint definition, so the generated path is left with {v} and {ver} respectively. You should be able to make this work if you can determine which API parameter description matches the API version parameter and use it's name to replace the token. While it's possible, it seems like it's something difficult and brittle for NSWag to determine and would be better served by making the API Versioning API Explorer support do the work instead (which it does).

[RicoSuter]

Ok so my proposal

• Remove api version processor
• Add setting ApiGroups so that you can filter the api groups (default: empty = all)

This way its fully controlled by the api explorer. What do you think?

[commonsensesoftware]

That sounds right to me. I don't mind if you do double-work, but I figure you don't want to support external idiosyncrasies if you don't have to. ;)

Honestly, I'm not super familiar with the particulars of NSwag. I know things are similar to Swashbuckle. Obviously, I know the API Explorer very well. If you haven't seen the sample project I provide, it might be worth giving it a once over for ideas that might influence the integration points of your design.

You won't see much of anything related directly to the API Explorer. The AddVersionedApiExplorer registers the required services, specifically the API version-aware IApiDescriptionProvider implementation. When you request or depend on the IApiDescriptionGroupCollectionProvider, the built-in ASP.NET Core default implementation runs all the IApiDescriptionProvider implementations and collates the results. By the time you access the collection, all of the grouping and other work is done. It should just be a matter of generating Swagger documents. If you wanted to produce a list of the groups, you can make a first pass which computes a distinct set of all defined group names. If all of the groups names are null or empty, then they all fall into a single group for your default or all case.

I also provide a custom IApiVersionDescriptionProvider. You see this service used in the example. The purpose of this service is to distinctly enumerate all possible API versions with their group name and indicate whether they are deprecated. API descriptions and so on are not provided by this collection. There are many possible use cases for this service, but a common scenario is enumerating the results to produce a Swagger document for each API version. The provided group name can be used to match up against the group name defined in API descriptions. For service authors that want to omit or otherwise call out deprecated API versions, that information is provided so they don't have to figure it out.

Naturally, you don't want any dependencies on any of that. However, hopefully that puts some context around how things are meant to work and they have worked together with other libraries such as Swashbuckle thus far. The integration from your standpoint is supposed to be easy so you can focus on what you do best - generating Swagger. :)

[RicoSuter]

I completely agree with you - and I want to reuse as much as possible of this logic and not implement it in NSwag. This processor comes from the times when everything was reflection based and I think the api versioning package didnt have api explorer support back then... The goal is to remove this feature in a way that breaking users get a good notification how to fix it/change it for the new way...

[RicoSuter]

@commonsensesoftware I created a PR based on our discussion: #1701

[commonsensesoftware]

Other than the broken tests, it looks pretty good.

I just realized that you support Web API too. I have similar support for grouping in Web API, but due its limited API Explorer design, it doesn't surface in a library-sharable manner. You'll have to pardon my ignorance because I don't know how NSwag bridges this gap. In Swashbuckle, they used a callback function. The API Versioning API Explorer for Web API provides an extension method to get the corresponding group name, which is expected to match the format of the Swagger document version.

This has the callback form Func<ApiDescription, string, bool> that is realized as:

( apiDescription, version ) => apiDescription.GetGroupName() == version

You can see the full Web API example here.

Beyond something like a callback function, I'm not sure how you can marry the two sides without some type of dependency. If you were content with using Reflection or dynamic dispatching, this is probably the one and only place were it might make sense. I had to do a bunch of my own hackery to make the Web API IApiExplorer work because a significant part of the default implementation relies on internal types and methods.

If you were going to support such a concept, it might look something like the implementation of the GetGroupName extension method. The only difference in your case would be testing for the presence of a property named GroupName on the ApiDescription. This would bridge the gap for API Versioning and any other IApiExplorer implementation that happens to return ApiDescription instances with a GroupName property.

I doubt we'll ever see things changed in Web API. These are the best ideas I can think of. I leave it up to you as to which route you think is appropriate for NSwag consumers.

[RicoSuter]

The web api generator is deprecated and i dont have the capacity to work on this at the moment...

[RicoSuter]

Will fix the tests soon

[commonsensesoftware]

Ah … gotcha. I thought I saw changes related to Web API. It caught my attention so I thought I'd mention it. If you ever decide to revisit it at a later point, I'm happy to discuss the integration points.