AssumeDefaultVersionWhenUnspecified does not work correctly if ApiVersionNeutral is used in the controller

jorenjamar commented 6 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the bug

In the config, AssumeDefaultVersionWhenUnspecified is set to true. If a controller now uses an ApiVersionNeutral attribute for one of its endpoints with a variable in the URL, for example, some/{id}, the API always uses this endpoint if there is no version specified. Even if the default API version can use a more specific one like the some/task endpoint, it will always use the some/{id} endpoint instead.

Expected Behavior

If the default API version has a better match as the one specified with ApiVersionNeutral, it should use this better match instead of the ApiVersionNeutral one.

Steps To Reproduce

Example repo

I have created an example repo to showcase the issue.. If you run the application, the problem is introduced in the Broken endpoint. The working endpoint is provided as an illustration is does work without the ApiVersionNeutral attribute in the controller. A GET and POST is used as an illustration. The bug happens with every CRUD operation used. There is no real functionality to the POST, it just returns a value. A swagger endpoint is provided which can be used for testing.

Bug explanation in the repo

In the config of the Program.cs file, the AssumeDefaultVersionWhenUnspecified is set to true. This will make sure API version 1.0 is used as a default when no API version is provided(since no version is specified to be used).

builder.Services.AddApiVersioning(o =>
{
    o.AssumeDefaultVersionWhenUnspecified = true;
    o.ReportApiVersions = true;
    o.ApiVersionReader = new HeaderApiVersionReader("api-version");
})
            .AddMvc()
            .AddApiExplorer(
                options =>
                {
                    options.GroupNameFormat = "'v'VVV";
                    options.SubstituteApiVersionInUrl = true;
                });

There are 2 identical controllers in the project, Broken and Working. They both have 2 endpoints GET /{id} and POST /task. The endpoints allow version 1.0 and 1.1 of the API. The only difference is the GET /{id} of the Broken endpoint uses the [ApiVersionNeutral] attribute.

/Broken endpoint:

namespace ApiVersionNeutralBugExample.Controllers
{
    [ApiVersion("1.0")]
    [ApiVersion("1.1")]
    [ApiController]
    [Route("[controller]")]
    public class BrokenController : ControllerBase
    {
        private readonly ILogger<BrokenController> _logger;

        public BrokenController(ILogger<BrokenController> logger)
        {
            _logger = logger;
        }

        [HttpPost("task")]
        public string DoTask()
        {
            return "task";
        }

        [ApiVersionNeutral]
        [HttpGet("{id}")]
        public string DoId(string id)
        {
            return "The id is " + id;
        }
    }
}

If you send with version 1.0 or 1.1 specified, all 4 endpoints work. No issue here. The issue exists when you do not specify a version. In this case, following the config and the AssumeDefaultVersionWhenUnspecified setting, the API should use version 1.0. If you use endpoint /Working without specifying the version, you indeed notice this is the case. When you use /Broken on the other hand, the /task does not work anymore if you do not specify the version.

In the error you clearly see only a GET is allowed for this endpoint. In other words, it defaults to using the /{id} endpoint when you try to call the /task endpoint.

Error: response status is 405

Response headers
 allow: GET 
 content-length: 0 
 date: Wed,24 Apr 2024 20:08:03 GMT 
 server: Kestrel

Exceptions (if any)

No response

.NET Version

6.0.311

Anything else?

No response

commonsensesoftware commented 6 months ago

Hmm... I'm not able to repro your condition. I verified from the current examples and it worked. Then I took your repro verbatim and it also works. I tried the following URLs:

https://localhost:7088/Broken/42
https://localhost:7088/Broken/42?api-version=1.0
https://localhost:7088/Broken/42?api-version=1.1

It works in the Swagger UI as well:

It's also worth noting that the API version parameter is not included for version-neutral APIs by default. If you want to enable that - say to hide the fact the API is version-neutral, you can set the API Explorer option AddApiVersionParametersWhenVersionNeutral = true.

commonsensesoftware commented 6 months ago

Hmmm... the explanation threw me off a bit, but I see the issue now. It seems like it is related specifically to the combination of HeaderApiVersionReader and AssumeDefaultVersionWhenUnspecified.

I'm not entirely sure it's specifically related to [ApiVersionNeutral], but it is throwing a wrench into the mix. I'm not sure why - yet, but it seems that the implicitly versioned /task endpoint is being included in the version-neutral bucket of matches, when it shouldn't be.

https://github.com/dotnet/aspnet-api-versioning/blob/db4031872105fd882f8391b35acd156c94cb65ed/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Routing/ApiVersionPolicyJumpTable.cs#L66

I need to look a bit deeper. This probably related to collation somehow. I'm not sure why this is an edge case. I'll investigate. Thanks for the repro.

dotnet / aspnet-api-versioning