ApiVersionMatcherPolicy Invalidates valid candidates

[bergmania]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

I'm experiencing an issue where ApiVersionMatcherPolicy invalidates a valid candidate, from what looks to be an attempt to optimize performance.

The setup is quite cumbersome, but I managed to setup a minimal solution to verify it (See below).
The use case is when I have a plain old WebAPI without any versioning and I have two endpoints with the same route. Furthermore I need to have a dynamic route, to even hit this code in the ApiVersionMatcherPolicy.

When I try to call ny POST endpoint, it returns 404, because it have been invalidated by ApiVersionMatcherPolicy that finds the GET endpoint as the bestMatch. This is a problem because the ApiVersionMatcherPolicy.Order is hardcoded to be lower than HttpMethodMatcherPolicy.Order.

The issue is originally reported in Umbraco here: umbraco/Umbraco-CMS#16434

Expected Behavior

My GET endpoint is hit, like if ApiVersionMatcherPolicy is removed from the service collection.

Steps To Reproduce

I have setup a minimal setup, that shows the issue.

https://github.com/bergmania/DemoIssueWithAPIVersioning

On a new MVC project, I added a dependency

<PackageReference Include="Asp.Versioning.Mvc.ApiExplorer" Version="8.1.0" />

I added the following in program.cs

builder.Services.AddApiVersioning(x=>x.AssumeDefaultVersionWhenUnspecified = true).AddApiExplorer();
builder.Services.AddSingleton<MyDynamicControllerRoute>();

Before var app = builder.Build(); and the following afterwards

app.MapDynamicControllerRoute<MyDynamicControllerRoute>("/{**slug}");

Furtheremore I have a single file with two classes

using System.Net.Mime;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Routing;

namespace WebApplication5;

public class MyDynamicControllerRoute : DynamicRouteValueTransformer
{
    public override ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpContext, RouteValueDictionary values)
    {
        return ValueTask.FromResult(values);
    }
}

[ApiController]
[Route("api/")]
public class ShowBugController : Controller
{
    [HttpPost("test")]
    [ProducesResponseType(StatusCodes.Status200OK)]
    [Consumes(MediaTypeNames.Application.Json)] // This is important because it leads to a lower (better) score when the request enter the ApiVersionMatcherPolicy, even when requested with http GET
    public IActionResult MyPost()
    {
        return Ok("OK POST");
    }

    [HttpGet("test")]
    [ProducesResponseType(StatusCodes.Status200OK)]
    public IActionResult MyGet()
    {

        return Ok("OK GET");
    }
}

Exceptions (if any)

No response

.NET Version

8.0.303

Anything else?

No response

[bergmania]

Just to verify, I tried to downgrade to 6.0 before this PR 3857a33

And it works as expected in that version

[commonsensesoftware]

OOOOFF! Another issue where I haven't even replied. 😞 Sorry about that.

Starting with invalidating all candidates has always been the behavior since Endpoint Routing was introduced. You have to spelunk the /ms branch to see that. I basically had to start over when MS had me rename all the packages and namespaces.

The order of matchers is important. The ApiVersionMatcherPolicy.Order does have a fixed offset, but it is intentionally not hardcoded. It's based on its nearest known matcher - HttpMethodMatcherPolicy. It was intentionally done this way so that it would be possible for someone to fit additional matchers before or after the ApiVersionMatcherPolicy. This is how many of the built-in matchers work. You have to spelunk the ASP.NET Core codebase to really see all of that. The API version is part of the routing decisions so it needs to happen this way over things like 406 and 415 will not work correctly when versioning by media type.

In looking at the setup, what it looks you are trying to do is have a mix of versioned and unversioned APIs side-by-side. Setting AssumeDefaultVersionWhenUnspecified = true is one of the most widely abused features to achieve such a thing. This can be a similar case with version-neutral APIs. That is not what these features were intended to address.

• Enabling AssumeDefaultVersionWhenUnspecified is intended to support backward compatibility with existing clients when you introduce versioning
  • If you don't have an existing API, enabling this is the wrong use case
• A version-neutral API allows an API to match any version, including none at all
  • This is the closest you get to an unversioned API or opting out, but that's not wholly accurate
  • When version is specified, the version must be one of the versions collated from all known versions
    • Things fail if you have all version-neutral APIs, which also begs the question why API versioning is required in such a case

It's important to remember that once you opt into API versioning, anything that is considered an API will be versioned, even if it's version-neutral. There is no built-in mechanism to explicitly opt out, but I have been considering for this type of edge case - assuming I'm understanding correctly.

The ApiVersionMatcherPolicy will ignore any endpoint that is not versioned. It decides this by the presence of the corresponding metadata, which would have been discovered and applied at startup exactly once. A controller is a controller. If it didn't behave this way, that would mean that UI-based controllers for fully MVC or Razor would fail. In fact, this used to be a real problem in the way, way back. So how does API versioning know or decide what's an API and what's not? This achieved by the following services:

• IApiControllerFilter - applies a filter to all discovered controllers
• IApiControllerSpecification - determines whether a discovered controller is an API

There are two specifications provided out of the box:

• ApiBehaviorSpecification - identifies a controller is an API if API Behaviors are applied a la [ApiController]
• ODataControllerSpecification - identifies a controller as an API if [ODataRouting] is applied

The DefaultApiControllerFilter simply accepts a sequence of specifications and filters out any controllers that do not meet any of the specifications. Any of these services can be changed or replaced to fit your needs. As a possible example, you might remove the ApiBehaviorSpecification because you want to keep the normal [ApiController] behavior, but you don't want versioning semantics. You'll need some other discriminator to identify an API, which might be an expected base class (ex: UmbracoController?) or a custom attribute; for example, [Unversioned] or the absence of any versioning attributes. I can provide some additional guidance with some more information about the scenario.

The other scenario that I might be seeing here is that you are trying to dynamically register one or more endpoints on an existing controller. This is a much, much trickier situation and, honestly, I'm not sure if or how it would work. If these are supposed to be versioned, then I expect they'd need the necessary metadata applied. APIs are collated by the logic name. There's also a chance that things are being grouped together by the routing system when that's not intended. The API version is part of the criteria for routing to a versioned endpoint.

[commonsensesoftware]

I believe I can confirm that this is a bug. It took some time to go back and remember why this change happened. It was actually fixing a different bug. After a lot of spelunking, I was able to figure out where ASP.NET calculates the initial score. It indeed bumps the score for matched endpoints that are not equal here:

https://github.com/dotnet/aspnetcore/blob/5f1f2ce6ece7ec84f68c848dc1a6f56209c1c0da/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs#L725

The comparison is complex and nuanced. Ultimately, I was mistaken and Score is not required to be evaluated as part of the candidate filtering. I have something working that would address the issue; however, it appears the provided example would not have matched anyway. The example (also above) is referenced here:

https://github.com/bergmania/DemoIssueWithAPIVersioning/blob/74725f680c761b1e9bf9d17d752de15db2cc354d/WebApplication5/Controllers/ShowBugController.cs#L22

Even with the pending fix, I cannot get POST to match correctly unless it is defined as:

[ApiController]
[Route("api/[controller]")]
public class TestController : ControllerBase
{
    [HttpPost]
    [ProducesResponseType(StatusCodes.Status200OK)]
    [Consumes(MediaTypeNames.Application.Json)]
    public IActionResult MyPost([FromBody] string body) => Ok("OK POST");

    [HttpGet]
    [ProducesResponseType(StatusCodes.Status200OK)]
    public IActionResult MyGet() => Ok("OK GET");
}

Note: Simplified the example to make it more succinct.

This makes sense to me. The original way expects application/json, but without a body most tools (ex: curl or the Swagger UI) will generate:

POST /api/test?api=version=1.0 HTTP/2
accept: */*

With or without accept, it has the same meaning by spec. Since the action says it requires JSON and it's not provided, it doesn't match. After I change it, it will send:

POST /api/test?api=version=1.0 HTTP/2
content-type: application/json
content-length: 6

"test"

This correctly matches the constraints and routes the request as expected. Does this still match your expectations? I've added a new test case for this scenario and the fix continues to pass for the entire test suite. If this is amenable to you, then I will proceed with the creating a PR and getting it in. This is one of the last issues before I think I'm ready to publish the next patch version.

[commonsensesoftware]

If you're curious at the change, PR #1156 is published. PR #1155 might also need to be merged for the test suite to pass. I tested them together, but I'm keeping the PRs separate.

[commonsensesoftware]

Based on further analysis, I believe the above assertion is true that the original POST example provided will not work. This was verified by removing API Versioning from the repro and creating a test.http file with the following:

# For more info on HTTP files go to https://aka.ms/vs/httpfile

// succeeds

GET https://localhost:7193/api/test

###

// fails with or without the 'body' parameter defined

POST https://localhost:7193/api/test

###

// succeeds with the 'body' parameter defined

POST https://localhost:7193/api/test
content-type: application/json
content-length: 6

"test"

###

This is sufficient evidence for me to believe this is the correct and expected behavior so that the PR can move forward. If there are additional thoughts or concerns, feel free to raise them.

[lommijuho]

Hi, thanks for the follow-up.

I can confirm that the example I provided in the original bug thread was incorrect. The controller where we encountered this issue does have the [FromBody] attribute defined as it should, but for some reason, it was missing from my example.

My apologies for the confusion and thanks a lot for investigating this issue!