HttpPut and HttpDelete template parameter names issue

[davidl63]

This is very similar to #72, but is not the same or it might have been the core cause of #72.

If the route templates for HttpPut and HttpDelete only have a single parameter, there is a toxic relationship between the parameter names that will cause a version error to be thrown in certain situations.

The server reports:
Microsoft.AspNetCore.Mvc.Versioning.ApiVersionActionSelector: Information: Multiple candidate actions were found, but none matched the requested service API version '1.0'. Candidate actions:

The client reports:

  "Error": {
    "Code": "UnsupportedApiVersion",
    "Message": "The HTTP resource that matches the request URI 'http://localhost:27162/api/v1.0/values/5' does not support the API version '1.0'."
}

Good

[HttpPut("{id}")]
[HttpDelete("{id}")]

[HttpPut("{boots}")]
[HttpDelete("{id}")]

Bad

[HttpPut("{middle}")]
[HttpDelete("{id}")]

[HttpPut("{boots}")]
[HttpDelete("{boo}")]

There seems to be some kind of super/sub string issue between the names. I did all my testing on the Put call but it turns out that a relationship between the parameter names exists that will prevent the Delete route from being matched also.

Here is an example project. I started with a new .NET Framework 4.6.1 ASP.NET Core Web Application (.NET Framework) project. I pruned out unnecessary methods and includes and updated all of the Microsoft.AspNetCore packages.

https://www.dropbox.com/s/vo80iolv08fms5l/RoutingSample.zip?dl=0

I have been using a combination of Postman and the PowerShell Invoke-RestMethod to call my app.
Here is my powershell script

https://www.dropbox.com/s/eshnz68kx17bwm7/test.ps1?dl=0

[commonsensesoftware]

There isn't anything particularly special about HTTP methods in routing; however, they are used to whittle down the candidates for dispatching. Based on your test script, I think the issue may be a mismatch in the media type (like #72). Your example uses only primitive strings, but you are specifying that the media type is application/json. Using the literal value "Eric" is not valid JSON. Try switching to the world's simplest complex type or make your payload {"value":"Eric"}. That should do the trick. If not, I'll investigate further.

[davidl63]

OK. I think you are getting caught up in how I implemented my test rather than the actual issue. The name of the template parameter in the HttpPut route can prevent an HttpDelete from getting routed correctly.

I took your suggestion and updated my code to use a class as a parameter and I modified my powershell script to pass a JSON string. I still get VersionActionSelector errors.

Checkout TwoController.cs

irm 'http://localhost:27162/api/v1.0/two/6' -Method Delete
fails with a 400
the server reports
Microsoft.AspNetCore.Mvc.Versioning.ApiVersionActionSelector: Information: Multiple candidate actions were found, but none matched the requested service API version '1.0'. Candidate actions:
If I make the call with Postman it reports

{
  "Error": {
    "Code": "UnsupportedApiVersion",
    "Message": "The HTTP resource that matches the request URI 'http://localhost:27162/api/v1.0/two/5' does not support the API version '1.0'."
  }
}

Here is my new project
https://www.dropbox.com/s/mcfrijb16gksl1f/RoutingSample.zip?dl=0

Dave

[commonsensesoftware]

Hmmm... ok. Thanks for the follow up. I'll try to repro and diagnose this over the weekend. The action selector doesn't do anything specific with parameter names. Route constraint filtering should be the same as it always was. Out of curiosity, what happens if you remove API versioning? I'm assuming you may have already tried or started with that, but I want to make sure there isn't something already flawed with the default ASP.NET Core action selector. I had the fork the code for that class. It's possible there was an update that I don't have.

[davidl63]

I don't have the issue without API versioning.

[commonsensesoftware]

I just wanted to let you know what I've found. There is seemingly some part of the ASP.NET Core routing infrastructure that I don't understand.

For PUT api/v1/one/42, here's what I see happen:

• IActionSelector.SelectBestCandidate is called containing only the action description for DELETE
  • The method returns null
• IActionSelector.SelectBestCandidate is called again, but this time only with PUT
  • The action is selected and the operation succeeds

The reason it isn't working with API versioning is that the ApiVersionActionSelector assumes it will not be called again and if there are no matches, but there could be one, it returns an appropriate response (400 or 405). If there is no possible match, then it returns null, which propagates a 404. It appears that there is some logic that causes the IActionSelector to be called again. I was able to confirm that if I let the ApiVersionActionSelector return null, it will be called again.

No closer to a solution just yet, but that's what I've found. It sounds like I may have something fundamentally incorrect about when a result should be returned in the pipeline. Odd that this only occurs in your scenario. The solution to unblock yourself is obviously make the route parameter names be the same. Regardless, I'll continue looking into this issue. It appears to be a bug on my side, I'm just not sure why yet.

[davidl63]

Thanks.

[thegyorf]

Would this problem be relevant on a controller marked as ApiVersionNeutral?

My problematic method looks like so:

        [Route("edit/{id:int?}")]
        [HttpGet]
        public async Task<IActionResult> Edit(int? id) {
            if (id == null) {
                return NotFound();
            }

            var product = await _context.Products.SingleOrDefaultAsync(m => m.ID == id);
            if (product == null) {
                return NotFound();
            }
            return View(product);
        }

        [Route("edit/{id:int}")]
        [HttpPost]
        [ValidateAntiForgeryToken]
        public async Task<IActionResult> Edit(int id, [Bind("ID,Name")] Product product) {
            if (id != product.ID) {
                return NotFound();
            }

            if (ModelState.IsValid) {
                try {
                    _context.Update(product);
                    await _context.SaveChangesAsync();
                }
                catch (DbUpdateConcurrencyException) {
                    if (!ProductExists(product.ID)) {
                        return NotFound();
                    }
                    else {
                        throw;
                    }
                }
                return RedirectToAction("Index");
            }
            return View(product);
        }

With ApiVersioning enabled the HttpPost method produces:

A 400 - An API version is required, but was not specified if I remove AssumeDefaultVersionWhenUnspecified from the ApiVersioning configuration in startup.cs.

A 405 - The HTTP resource that matches the request URI 'http://localhost:3712/products/edit/1' does not support the API version '1.0' if I set AssumeDefaultVersionWhenUnspecified to true.

If I change the HttpPost method's name/route & update the view to reflect that it works fine.

The controller & views were generated using the "Add Controller > MVC Controller with views, using EntityFramework" scaffolding option in VS2017, with the only change being I added the annotations for Attribute Routing.

[commonsensesoftware]

I expect the issue to remain. Regardless of whether you use explicit or implicit versioning, the ApiVersionActionSelector expects to only be called once. The selector doesn't find a match on the first pass and then short-circuits the pipeline causing the incorrect behavior. I've confirmed that if the selector just returns null on the first pass, it will be called again and will perform the expected action. I'm not going to arbitrary change this without a better understanding of things because that would break a now established behavior. I'm meeting with the ASP.NET team so I should have an answer and fix to this problem soon.

One of the key culprits I have found that cause this behavior is when you have two overlapping routes that only differ by route parameter constraints. In your scenario, you have such a case. You have two route definitions that only differ by {id:int} and {id:int?}. As long as these are the same, this problem won't occur. This is by no means a solution, but it may unblock you for now.

[commonsensesoftware]

The 1.1 RC has now been published that resolves this issue. Note that there is breaking, behavioral change for ASP.NET Core to support it. :( You'll now need to call app.UseMvc().UseApiVersioning(). This is called out in the release notes.

Let me know if you run into anything, but I think 1.1 is ready.

[ikwilkoffie]

Hello!
I'm not really sure whether I should open up a new issue or comment here so I guess I''ll leave a comment here.

With the new version these two routes aren't clashing anymore, which is good! Thanks!
//api/v1.0/persons/idstring
[HttpGet("{ids}")]

//api/v1.0/persons/idstring
[HttpDelete("{id}")]

But now I have the problem that it doesn't differentiate between these two routes:
//api/v1.0/persons/search/?query=name
[HttpGet("search")]

//api/v1.0/persons/idstring
[HttpGet("{ids}")]

Am I doing something wrong? Or could this possible be a new bug?

Thanks in advance!

[commonsensesoftware]

I locally updated the HelloWorldController in the "BasicSample" to match your scenario. The following routes work as expected:

• http://localhost:1237/api/v1/helloworld/
• http://localhost:1237/api/v1/helloworld/42
• http://localhost:1237/api/v1/helloworld/search?query=name

Can you share more about your controller and/or configuration? Perhaps compare them to the BasicExample? I'm not able to reproduce the behavior you're seeing.

[ikwilkoffie]

Hey Chris!

Thanks for the quick reply, sorry for the late answer!

I downloaded the BasicExample project and changed the names and endpoints to reflect my project and I found something interesting.

These are the routes I have:

• http://localhost:5000/api/v1.0/persons/search?query=test [HttpGet("search")] (Search)
• http://localhost:5000/api/v1.0/persons [HttpGet] (Get)
• http://localhost:5000/api/v1.0/persons/{ids} [HttpGet("{ids:required}")] (GetMultiple)
• http://localhost:5000/api/v1.0/persons [HttpPost] (Post)
• http://localhost:5000/api/v1.0/persons [HttpDelete("{id:required}")] (Delete)

When the example runs and I call the search endpoint first, I get the following 500 internal server error:

info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 GET http://localhost:5000/api/v1.0/persons/search?query=asdasd application/json
fail: Microsoft.AspNetCore.Mvc.Routing.DefaultApiVersionRoutePolicy[1]
      Request matched multiple actions resulting in ambiguity. Matching actions: Microsoft.Examples.Controllers.PersonsController.Search (BasicSample)
      Microsoft.Examples.Controllers.PersonsController.GetMultiple (BasicSample)
fail: Microsoft.AspNetCore.Server.Kestrel[13]
      Connection id "0HL50RM9M3GSD": An unhandled exception was thrown by the application.
Microsoft.AspNetCore.Mvc.Internal.AmbiguousActionException: Multiple actions matched. The following actions matched route data and had all constraints satisfied:

Microsoft.Examples.Controllers.PersonsController.Search (BasicSample)
Microsoft.Examples.Controllers.PersonsController.GetMultiple (BasicSample)
   at Microsoft.AspNetCore.Mvc.Routing.DefaultApiVersionRoutePolicy.OnMultipleMatches(RouteContext context, ActionSelectionResult selectionResult) in D:\xampp\htdocs\apitest\src\Microsoft.AspNetCore.Mvc.Versioning\Routing\DefaultApiVersionRoutePoli
cy.cs:line 166
   at Microsoft.AspNetCore.Mvc.Routing.DefaultApiVersionRoutePolicy.RouteAsync(RouteContext context) in D:\xampp\htdocs\apitest\src\Microsoft.AspNetCore.Mvc.Versioning\Routing\DefaultApiVersionRoutePolicy.cs:line 111
   at Microsoft.AspNetCore.Routing.RouteCollection.<RouteAsync>d__9.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Hosting.Internal.RequestServicesContainerMiddleware.<Invoke>d__3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Server.Kestrel.Internal.Http.Frame`1.<RequestProcessingAsync>d__2.MoveNext()
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 4.6099ms 200

But, if I call the GetMultiple endpoint first and after that the Search endpoint, I get a 200 but it connects to the GetMultiple endpoint:

info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 171.3243ms 200 application/json; charset=utf-8
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 GET http://localhost:5000/api/v1.0/persons/search?query=asdasd application/json
info: Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker[1]
      Executing action method Microsoft.Examples.Controllers.PersonsController.GetMultiple (BasicSample) with arguments (search) - ModelState is Valid
info: Microsoft.AspNetCore.Mvc.Internal.ObjectResultExecutor[1]
      Executing ObjectResult, writing value Microsoft.AspNetCore.Mvc.ControllerContext.
info: Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker[2]
      Executed action Microsoft.Examples.Controllers.PersonsController.GetMultiple (BasicSample) in 4.6482ms
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 8.4701ms 200 text/plain; charset=utf-8

I hope I put enough information in my post, I can add the code of the controller to if you'd like!

[commonsensesoftware]

Hmm... this is an interesting edge case. On one hand, it's a regression, but on the other the route is ambiguous. Without even seeing the code, I suspect that {ids} is a string. This means that following are ambiguous:

• api/v1/persons/{ids}
• api/v1/persons/search

The router will treat the action search as the string value for ids. This doesn't happen with the default action selector because it always stops at the first match in the route tree (e.g. search). API versioning requires that all the candidates be considered in order to select the best candidate. If API versioning stops too early candidates are missed. If API versioning goes to long (as is the case now), then invalid candidates might be discovered. This is also why you are seeing the behavior where the wrong action is selected. Once a full pass has been made on a route, there is a optimization to short-circuit the pipeline so that the whole chain doesn't need to be evaluated for every request.

I suspected the problem might be caused these symptoms. I'm not sure exactly how to resolve the ambiguity just yet, but I'll start looking into it. I'll create a new issue to track this and link it back. It's related to this issue, but it's really not the same thing.

[commonsensesoftware]

I'm now tracking this as part of #133