aspnet-api-versioning icon indicating copy to clipboard operation
aspnet-api-versioning copied to clipboard

Swagger paths include http method names

Open ithielnor opened this issue 3 years ago • 7 comments
trafficstars

I'm not sure if this is a bug in Versioning, in Swagger, or in my routes.

I'm using ASP.NET WebApi and Versioning version 4.1.1.

My controller looks like this (I've cut it down to what's relevant):

namespace MyProj.Api.v1.Controllers
{
    public class BookingsController : BaseApiController
    {
        /// <summary>
        /// Get unavailable information about bookings or blocks.
        /// </summary>
        public IPageableEnumerable<BookingModel> Get(int[] arg1, DateTime? arg2, ...) { ... }

        /// <summary>
        /// Get unavailable information about a specific booking or blocked-off time.
        /// </summary>
        public BookingModel Get(int id, bool? someOtherArgs, ...) { ... }

        /// <summary>
        /// Gets a list of booking availability information entries based on criteria.
        /// </summary>
        [HttpGet]
        public AvailabilityViewModel Availability(int[] arg1, DateTime? arg2, ...) { ... }
    }
}

I would expect Swagger to report paths of

  • /v1/bookings (Description: Get unavailable information about bookings or blocks.)
  • /v1/bookings/{id} (Description: Get unavailable information about a specific booking or blocked-off time.)
  • /v1/bookings/availability (Description: Gets a list of booking availability information entries based on criteria.)

But instead it outputs this: image

Two problems here:

  1. The first path is getting its description from the Availability endpoint.
  2. The /v1/bookings/get path should not include the /get at the end.

Here is my configuration setup:

config.Routes.MapHttpRoute(name: "Api", routeTemplate: "v{apiVersion}/{controller}/{id}",
    defaults: null,
    constraints: new { apiVersion = new ApiVersionRouteConstraint(), id = new RegexOptionalConstraint(@"^(\d+)$") });

config.Routes.MapHttpRoute(name: "ApiAction", routeTemplate: "v{apiVersion}/{controller}/{action}",
    defaults: new { id = (int?)null, action = RouteParameter.Optional },
    constraints: new { apiVersion = new ApiVersionRouteConstraint(), action = new RegexOptionalConstraint(@"^([a-zA-Z]+)$") });

config.AddApiVersioning(options =>
{
    //https://github.com/microsoft/aspnet-api-versioning/wiki/API-Version-Conventions#version-by-namespace-convention
    options.Conventions.Add(new VersionByNamespaceConvention()); 
    options.ReportApiVersions = true;
});

var apiExplorer = config.AddVersionedApiExplorer(options =>
{
    // format the version as "'v'major[.minor][-status]"
    options.GroupNameFormat = "'v'VVV";
    options.SubstituteApiVersionInUrl = true;
    options.SubstitutionFormat = "VVV";
});

var swaggerConfig = config.EnableSwagger(
    "swagger/{apiVersion}",
    swagger =>
    {
        swagger.MultipleApiVersions(
            (apiDescription, version) => apiDescription.GetGroupName() == version,
            info =>
            {
                foreach (var group in apiExplorer.ApiDescriptions)
                {
                    var builder = info.Version(group.Name, $"MyProj API {group.ApiVersion}");

                    if (group.IsDeprecated)
                    {
                        builder.Description("This API version has been deprecated.");
                    }
                }
            });

        // add a custom operation filter which sets default values
        swagger.OperationFilter<SwaggerDefaultValues>();

        // add a documnet filter to format the routes accurately
        swagger.DocumentFilter<SwaggerLowercasePaths>();

        // integrate xml comments
        swagger.IncludeXmlComments(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "bin/MyProj.Api.xml"));

        swagger.UseFullTypeNameInSchemaIds();

        // Without this, I get a "Multiple operations with path 'v1/Bookings/{id}' and method 'GET'" swagger error
        swagger.ResolveConflictingActions(apiDescriptions => apiDescriptions.First());

ithielnor avatar Apr 22 '22 19:04 ithielnor

A few questions:

  1. Does the routing work as expected; regardless of the Swashbuckle stuff?
  2. Do you have a minimal repro (link or attachment is fine)
  3. What's in BaseApiController?

It's been a while since I've looked at, or even seen anyone using, convention-based routing. It's much harder to rationalize IMHO. What I think I see are overlapping route templates. This is why you end up with /get being used for {action}. There is also no {id} in the second template, but there is a default(int?) value for it. It's hard to say without further investigation, but that might be crossing the streams.

Optional parameters in an action should be supported, but I wouldn't expect it to work as written. Despite being optional, you still have to assign the parameter a default value; for example, DateTime? arg2 = default or bool? someOtherArgs = true. As I recall, these parameters will attempt to match route parameters and if they aren't there, then they are expected to come from the query string.

You don't appear to have anything that crazy going on. Your setup looks like something that should be supported. I suspect there's some mismatch in the routing. I would be surprised if the routing works in all scenarios, but still produces incorrect API Explorer results. I won't rule out a bug, but I'll need a repro first.

commonsensesoftware avatar Apr 23 '22 00:04 commonsensesoftware

One other thing that seems suspect (without more testing) is that the route template is all lowercase, unless you've modified it. The {controller} token should have been filled in as Booking and not booking. The API Explorer extensions replace the token verbatim. I don't recall whether Swashbuckle changes the casing. It may all be irrelevant, but it's a little clue that something else may be at play.

commonsensesoftware avatar Apr 23 '22 00:04 commonsensesoftware

Thanks for the feedback.

  1. The routing works as expected in 4.0 before upgrading to 4.1.1. I can't confirm yet in 4.1.1 because I'm running into #703. I can't get swagger working in 4.0.0 because I get #234! So a little "catch 22" here.
  2. I do not, but if I can't work around it, I'll try and produce one.
  3. Nothing much. A few helpers and some unit of work glue. It inherits from ApiController.
  4. I added swagger.DocumentFilter<SwaggerLowercasePaths>(); which just lowercases the paths for swagger output.

I'm not sure why convetion-based vs attribute-based would make a significant difference. I had used convention-based to be a bit DRYer in regards to versioning etc, but I'll try it out with attributes and see how much of a difference it makes.

ithielnor avatar Apr 25 '22 14:04 ithielnor

  1. Strange. I remember #703, but not why the implementation changed between 4.0 and 4.1. As I vaguely recall, there was some other edge case missed, but I can't remember what caused it. a. Maybe going back to 4.0 is a reasonable stop-gap? b. I'm still waiting for code signing 😞 , which is preventing me from publishing the fix for #703. c. #234 was specifically related to OData.
  2. 👍🏽
  3. 👍🏽
  4. 👍🏽

You're not using OData are you? That would change a few things. For starters, I wouldn't not recommend attribute-routing mixed with OData. OData in Web API only works using convention-based routing under the hood. There is a possibility that API Versioning will not work correctly in some edge cases if you mix them (I've tried to address that for years; there's no good solution).

It would be surprising, but possible for #234 that to happen for a non-OData action. I would, however, be surprised if it didn't also break or behave in the same way as the intrinsic API Explorer. The code is forked (because there was no other way) and processing of the return type should be the same. A repro would be very beneficial for that.

A few other possible solutions, even though they probably won't work for you 😉:

  1. Ditch IIS 😆
  2. Don't version by URL segment. No other method should have this problem. It's not RESTful anyway.
  3. Build directly from the ms branch in the repo source, which has the fix for #703

commonsensesoftware avatar Apr 25 '22 15:04 commonsensesoftware

I'd be curious if you ever had a working repro. I've tried multiple combinations and I simply cannot make anything work. I even tried completely removing API Versioning from the picture and I still can't get the basic routing to work. I'm not sure this combination of generic route configurations can work (together).

Using direct routing (aka Attribute Routing), things work exactly as you'd expect:

[RoutePrefix( "v{version:apiVersion}/bookings" )]
public class BookingsController : ApiController
{
    /// <summary>
    /// Get unavailable information about bookings or blocks.
    /// </summary>
    [Route]
    public IEnumerable<BookingModel> Get( int[] arg1, DateTime? arg2 = null ) => new[] { new BookingModel() };

    /// <summary>
    /// Get unavailable information about a specific booking or blocked-off time.
    /// </summary>
    [Route( "{id:int}" )]
    public BookingModel Get( int id, bool? someOtherArgs = false ) => new BookingModel();

    /// <summary>
    /// Gets a list of booking availability information entries based on criteria.
    /// </summary>
    [HttpGet]
    [Route( "availability" )]
    public AvailabilityViewModel Availability( int[] arg1, DateTime? arg2 = null ) => new AvailabilityViewModel();
}
image

commonsensesoftware avatar Jun 17 '22 19:06 commonsensesoftware

I gave up and switched to Attribute routing. Couldn't get around it otherwise.

ithielnor avatar Jun 17 '22 20:06 ithielnor

Gotcha. Glad you got something working, even if it wasn't what you were hoping for.

commonsensesoftware avatar Jun 17 '22 20:06 commonsensesoftware

Looks like there is a solution to this issue.

6.0 has been officially released and contains all of the fixes, including the LockRecursionException problem. The 6.0 release may be a bit of an initial disruption as many things have changed. The 5.1 also contains the fix and will be simple to update, but I'm still fighting getting NuGet to accept my signing certificate for the old packages. I'm sure it will be solved - eventually. If you'd like to update now, switching to 6.0 is the way to go. Ultimately, that will the long-term direction anyway.

Thanks again for the repro. Glad you are unblocked.

commonsensesoftware avatar Aug 24 '22 02:08 commonsensesoftware