dotnet / aspnet-api-versioning

Provides a set of libraries which add service API versioning to ASP.NET Web API, OData with ASP.NET Web API, and ASP.NET Core.
MIT License
3.05k stars 703 forks source link

Multiple versions on the same controller #821

Closed Mhirji closed 2 years ago

Mhirji commented 2 years ago

Hi,

I'm not sure if you are seeing the same thing (and I don't suspect it is normal behavior) but when using the ODataOpenApiExample project, calling version v0.9 ~/api/Orders/1 returns a 404 and v1 ~/api/Orders/1 returns the order as expected. v2 and v3 both work as expected, but they each have their own controller.

On my own project, when using multiple versions on the same controller, one version will throw a 404 where the other returns the data - the same behavior. I should note in my case neither version is deprecated - thought that might be a clue but nope. I thought it may be due to the order, but changing it does not seem to make a difference either on the ODataOpenApiExample project or mine - which to be honest I found strange :) - it appears to always take the lowest whole number. So in my case I had v3 and v4, v3 would return data and v4 would return a 404.

Perhaps I am missing something or misunderstanding something somewhere?

commonsensesoftware commented 2 years ago

This is very strange indeed. I can confirm that I can reproduce the behavior.

Order does not, nor ever, matters. The API version information is purely metadata and you can annotate it in any way you like. The collated lists are often sorted, but that is purely for sanity and optimizations when they are accessed later (ex: reporting API versions). Deprecated also does not matter. Deprecated or not, it's a matchable candidate. Deprecated just means the API version will go away.

The stranger thing is that a similar configuration is used for People and both api/people/42?api-version=0.9 and api/people/42?api-version=1.0 work. The ODataBasicExample has a similar setup and it also works.

Debugging, I see the route is mapped and resolves to the correct endpoint. It's not clear how we get to 404? 😕 Continuing to research. There's a chance it's a bug, but I haven't been able to prove it - yet. Thanks for reporting the issue.

Mhirji commented 2 years ago

Hmmmm, a mystery 🤔. Have you tried with preview three? If not, let me know and I will to see if some of the changes fixed the underlying issue. It would be good to get to the root cause, but if it's working with the next release...

commonsensesoftware commented 2 years ago

I'm working directly from the Preview 3 build and it's still there - unfortunately. I can reproduce the problem without any OpenAPI stuff. It's very strange that it only happens sometimes. Stranger still, I've noticed that adding the explicit route template (ex: [HttpGet("api/[controller]/{key}")]) fixes the problem in many cases. That should not be required. That might be a short-term stopgap for you. The issue seems to be somewhere down were OData is not recognizing or otherwise building out the implicit conventions. There is likely a mistake - somewhere. I haven't found it, but that's my hunch.

commonsensesoftware commented 2 years ago

Ultimately, this was a bug, which has now been fixed. It was the last hurdle I wanted to iron out before publishing Preview 3 with all the other changes and fixes. I've also gone through each route in the example to make sure it actually works.

TL;DR

The cause comes down to the combination of a few things. OData adds multiple SelectorModel instances to ActionModel. ASP.NET Core will create an ActionDescriptor for each ActionModel.Selectors it finds. The problem is that API Versioning has already run and applied ApiVersionMetadata to the ActionModel.Selectors[0] (in most scenarios there is only one). By default, there is no way that OData could know this exists and there is no way for API Versioning (core) to know this will happen. This is also only problem when there multiple API versions, and hence EDMs, pointing to the same action (e.g. ActionModel == ActionDescriptor). That is why some routes work just fine, while others don't.

The fix was simple that the API Versioning extensions for OData need to realize that if there is more than one EDM created and there are more than one selectors per action (e.g. ActionModel.Selectors.Count > 1), then any ApiVersionMetadata needs to be copied from ActionModel.Selectors[0].EndpointMetadata to ActionModel.Selectors[i].EndpointMetadata, where i > 1.

I had actually forgotten and vaguely remembered that I had previously run into this issue. There was some code in place to handle it, but it was doing it wrong. There's some internal shuffling to put things in the right spot and optimize whether processing is even required. Fortunately, any work performed is fast and cheap. ApiVersionMetadata is immutable and doesn't need to be recalculated or cloned; it just needs a shallow copy to be added to other selectors.

commonsensesoftware commented 2 years ago

Preview 3 is now available and contains the fix for this issue.

Mhirji commented 2 years ago

Thanks @commonsensesoftware!!