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

Regression for issue #423 in version 4.2.0 (unsupported API version on route constraint failure)

Open pwdst opened this issue 4 years ago • 5 comments
trafficstars

It appears that version 4.2.0 has reintroduced the previously resolved issue #423 as a regression, possibly in commit de9bc84e587cbd3914c923833d7d580747261c81 - although I haven't looked into this in great detail.

When a route constraint on a path using endpoint routing does not match, instead of returning a 404 status the error -

"error": { "code": "UnsupportedApiVersion", "message": "The HTTP resource that matches the request URI [URL of the endpoint] does not support the API version '[supported version]'.", "innerError": null }

Is returned with a Bad Request HTTP 400 response status code.

I have attached a reproduction project based on the ASP.NET Core 3.1 template API project.

Affected package Microsoft.AspNetCore.Mvc.Versioning - version 4.2.0 .NET Core version 3.1 SDK Microsoft.NET.Sdk.Web

In the attached solution change the URL from an arbitrary GUID like https://localhost:44350/weatherforecast/v1/location/3f43133b-9f36-492e-a32a-720d5bd6f033/ to a non-GUID string like https://localhost:44350/weatherforecast/v1/location/Test/ to replicate the problem.

Expected status code 404 Actual status code 400

VersionedRouteConstraintTest.zip

pwdst avatar Jan 11 '21 22:01 pwdst

I'll have to go back and research this. I found at least one edge case where the logic was wrong and fixed it. That might have caused some expected behavioral regression - maybe. Thanks for the repo, I'll take look. It's possible this is fixed, but is in the 5.0 set.

Things can get hairy when the route template changes between versions. For example, consider:

  • 1.0 = weatherforecast/v{version:apiVersion}/location/{id:guid}
  • 2.0 = weatherforecast/v{version:apiVersion}/location/{id}

If you request weatherforecast/v1/location/Test, this will produce 400. API Versioning doesn't change routing, it just helps the routing system disambiguate duplicates. When at least one template could match, you'll get 400 over 404. The only other exception is 405, when reason they don't match is due to the HTTP method.

I'm not saying that is the issue, but I just wanted to highlight a scenario where it can happen. This isn't really something that API Versioning can solve. Ensuring the route templates don't introduce this behavior between versions will help.

commonsensesoftware avatar Feb 08 '21 21:02 commonsensesoftware

Circling back around to this issue. I can confirm that this is indeed a regression in 4.2.0. It appears to have been fixed in 5.0.0. I updated your repro to use 5.0.0 and the behavior seems correct and as expected.

Is there any specific reason you cannot update to 5.0.0, which contains the fix? The 5.0.0 release still multi-targets to .NET Core 3.1. I do occasionally patch older packages when necessary, but honestly it's a lot of extra work for me as a one-man show. I'm also a little worried about the time it will take to track down the difference or change that caused the regression. If you really, really need it, then I'll see what I can do; otherwise, you should be good to go in 5.0.0.

Thanks

commonsensesoftware avatar Jun 17 '21 14:06 commonsensesoftware

Hello @commonsensesoftware, unfortunately I seem to still be getting this issue in my current project.

My dependencies are: Microsoft.AspNetCore.Mvc.Versioning" Version="5.0.0" Microsoft.AspNetCore.Mvc.Versioning.ApiExplorer" Version="5.0.0" Microsoft.AspNetCore.OData" Version="7.5.7"

This issue as described by @pwdst and in issue #423 is still present for me, and I am not using any route template changes between versions.

Downgrading to 4.1.1 has resolved this issue, but a long term fix would be fantastic.

jocf avatar Nov 09 '21 01:11 jocf

@pwdst , @jocf

6.0 is now available in preview and changes some internal routing mechanics, which I believe will address the routing issues. 404, 405, and 415 should be reported in expected ways. There are still a few scenarios for 400 (ex: API versioned, but not provided), but it's far fewer now. This is a bit of a departure from the past, but is simpler, more performant, and likely more closely aligns to expectations.

You can find all the updated examples here.

@pwdst it appears you want the new Asp.Versioning.Mvc package @jocf it appears you want the new Asp.Versioning.OData.ApiExplorer package

commonsensesoftware avatar Apr 09 '22 17:04 commonsensesoftware

6.0 is officially released. If that resolves the issue for you, then I'd like to close this out. I'm still fighting publishing issues with code signing 5.1, which also contains the fix you're looking for. If you really need 5.1, then I'll circle back around once I'm finally able to publish it.

commonsensesoftware avatar Aug 24 '22 01:08 commonsensesoftware

This issue has been fixed in 6.0 so I'm going to close this out. I'm still waiting on MS to unblock me on publishing the 5.1 packages and, at this point, it may never happen 😞. I should also mention that .NET Core 3.1 support ends in December so I do not plan to support it in the 7.0 release (to coincide with the .NET 7.0 release). I'm close to having 6.1 available. It's unfortunate that you'd have to go through the migration process, but any of the 6.x releases will support .NET Core 3.1.

If there's more guidance or questions I can answer, I'm happy to do so. This includes, but is not limited to, reopening the issue. Thanks.

commonsensesoftware avatar Sep 29 '22 14:09 commonsensesoftware