Raw Requested API Versions Causes Stack Overflow

[commonsensesoftware]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

When calling the HttpContext.ApiVersioningFeature() extension method and accessing IApiVerisoningFeature.RawRequestedApiVersions, it is possible for infinite recursion to occur which causes StackOverflowException.

Expected Behavior

No exception should occur.

Steps To Reproduce

var builder = WebApplication.CreateBuilder( args );
var app = builder.Build();
var values = app.NewVersionedApi();
var v1 = values.MapGroup("/values").HasApiVersion(1.0);
v1.MapGet("/", (HttpContext context) => {
    // this will go through RawRequestedVersions and result in a stack overflow
    var version = context.ApiVersioningFeature().RawRequestedVersion;
    return Ok(new { message = $"{version} was requested." });
});

Note that IServiceCollection.AddApiVersioning() was not called

Exceptions (if any)

StackOverflowException

.NET Version

No response

Anything else?

No response

[commonsensesoftware]

This happens because of fallback behavior at:

aspnet-api-versioning/src/AspNetCore/WebApi/src/Asp.Versioning.Http/ApiVersioningFeature.cs

Line 38 in f29831e

?? ApiVersionReader.Combine(

This was primarily intended to support simplifying testing, but it can surface at runtime. The UrlSegmentApiVersionReader is configured by default and is stateful because it can recursively all itself. Since a new instance is created on every called to IApiVerisoningFeature.RawRequestedApiVersions, the state does not carry forward and leads to infinite recursion.

This only happens if API Versioning has not been properly configured via AddApiVersioing(), which should never happen. Although it's an invalid configuration to not register the required services, a StackOverflowException should not happen.

[jcageman]

Ran into this exact same thing while upgrading from 5.0.0 to 5.0.1 All our controller unit tests started failing on it. Seems to be a breaking change in a minor update.

[commonsensesoftware]

@jcageman There is no 5.0.1 release or package. Did you mean 5.1.0?

I reviewed the changes from 5.0.0 to 5.1.0 and I do see how this could technically happen, but it never should. I'm curious how you ran into this situation. I debated whether this should even be fixed as it was discovered through a colleague that failed to register the necessary services through AddApiVersioning (which is wrong). The update ended up being more for consistency and removal of code duplication than the actual fix; however, the nice benefit is that this issue can no longer happen, even if you never register the API Versioning services (which is strange and unexpected).

With a little more context and understanding, I'd consider backporting the fix. Unless you have a very compelling reason, you should be moving to Asp.Versioning.Mvc. The older packages are in maintenance mode for servicing only and do not officially support newer versions of .NET or ASP.NET Core. The Asp.Versioning.Mvc package (and company) supports .NET 6, .NET 7, and will support .NET 8 and beyond.

[jcageman]

Indeed meant 5.1.0

i think the stack overflow due to this construction in our mocking code:

_mockFeatureCollection
    .Setup(f => f.Get<IApiVersioningFeature>())
    .Returns(() => new ApiVersioningFeature(_mockHttpContext.Object));
_mockHttpContext.SetupGet(c => c.Features).Returns(_mockFeatureCollection.Object);

After updating i also get a ArgumentNullException on the following code:

var urlHelper = new Mock<IUrlHelper>();
_urlHelper.Setup(urlHelper => urlHelper.Link(It.IsAny<string>(), It.IsAny<object>())).Returns((string routeName, object _) => routeName);
var actionContext = new ActionContext
{
    HttpContext = new DefaultHttpContext()
};

urlHelper.Setup(urlHelper => urlHelper.ActionContext).Returns(actionContext);

urlHelper.ActionContext.HttpContext.GetRequestedApiVersion();

With callstack:
Message: 
System.ArgumentNullException : Value cannot be null. (Parameter 'provider')

Stack Trace: 

ServiceProviderServiceExtensions.GetService[T](IServiceProvider provider)
ApiVersioningFeature.ReadApiVersions()
ApiVersioningFeature.get_RawRequestedApiVersions()
ApiVersioningFeature.get_RawRequestedApiVersion()
ApiVersioningFeature.get_RequestedApiVersion()
HttpContextExtensions.GetRequestedApiVersion(HttpContext context)

This was all working fine with 5.0.0
I am aware we should update to Asp.Versioning.Mvc, so i will for sure will be doing that, thanks for the headsup!

[commonsensesoftware]

I see. The primary issue is that the required services haven't been setup as noted above. There are several ways to make the necessary setup work.

Option 1: Register the Built-In Services

Set up a provider that registers the necessary services:

var services = new ServiceCollection();

services.AddApiVersioning();

_mockHttpContext.SetupProperty(hc => hc.RequestServices, services.BuildServiceProvider());

Option 2: Build a Mocked Service Provider

If you prefer a more stripped down provider, it's easy to set one up:

var services = new Mock<IServiceProvider>();
var reader = new QueryStringApiVersionReader();

services.Setup(s => s.GetService(typeof(IApiVersionReader))).Returns(reader);
_mockHttpContext.SetupProperty(hc => hc.RequestServices, services.Object);

NullReferenceException

I see how this happens, but I would expect this to happen in previous versions too. The following is the 5.0.x implementation:

aspnet-api-versioning/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/ApiVersioningFeature.cs

Line 34 in 435a17e

var reader = context.RequestServices.GetService<IApiVersionReader>() ?? new QueryStringApiVersionReader();

The feature tries to resolve IServiceProvider from HttpContext.RequestServices. Depending on your setup, it can be null. It is evident that this is happening based on the stack trace. provider is being passed to the GetService<IApiVersionReader>() extension method, but is null. At runtime this will never be null. If this used to work, but now it doesn't, the only other theory I have is that you may have changed versions of ASP.NET Core itself. There have been many improvements, particularly removing allocations, over the years. It is entirely possible that the previous version used an empty IServiceProvider a la the Null Object pattern, but now it defaults to null. That's just a theory. I don't otherwise see how you would not run into the problem in both paths.

Built-In Feature Extension Method

Starting in 5.1.0 and beyond, there is a new extension method for requesting the IApiVersioningFeature. This is a simplification and makes sure the feature is never null. If you need to use it in your testing, it's a nice convenience. The following will work from above:

_mockHttpContext.SetupGet(c => c.Features).Returns(new FeatureCollection());
var feature = _mockHttpContext.Object.Features.ApiVersioningFeature();

It doesn't do anything that fancy. You can see the implementation at:

aspnet-api-versioning/src/Microsoft.AspNetCore.Mvc.Versioning/HttpContextExtensions.cs

Line 18 in 0612bbd

public static IApiVersioningFeature ApiVersioningFeature( this HttpContext context )

Conclusion

Glad to hear you know about the updates and have a plan to transition at some point. My hands have really been tied on that and I find there are still a lot of people using the older versions and/or expecting updates. I'm thinking of marking the old packages as deprecated (which I think I can do) and hopefully that will bring awareness - if not by shock. 😛

The workarounds here are pretty straight forward, but if you really need/want a fix, I can queue something up. As an army of one, it is a lot to maintain multiple versions. Now that .NET has a clear LTS and STS strategy, I already have to content with two live branches. I try not to patch the older versions unless I really have to. A lot has changed between 5.x and 6.x+, so it's a big mental context switch, even for me. Regardless, I want to make sure I do right by the community. Thanks.

[jcageman]

Thanks for the very elaborate response! Agree with it completely! For now we can workaround it with your provided answer and will ensure we also plan the upgrade towards the new nuget.