Null Reference Exception on Batch with Versioning

[jtTorres]

I pulled the sample aspne-api-versioning and tried adding $batch to the ODataBasicSample project and it did not work.
I followed the instructions in the wiki but still nothing. It seems like it's trying to obtain the IApiVersioningFeature but that's returning a null so it's not able to access anything else.

Here's my setup:

public void ConfigureServices( IServiceCollection services )
        {
            services.AddControllers();
            services.AddApiVersioning(
                options =>
                {
                    options.ReportApiVersions = true;
                    options.RegisterMiddleware = false;
                } );
            services.AddOData().EnableApiVersioning();
        }

public void Configure( IApplicationBuilder app, VersionedODataModelBuilder modelBuilder )
        {
            app.UseODataBatching();
            app.UseApiVersioning();
            app.UseRouting();
            app.UseEndpoints(
                endpoints =>
                {
                    var models = modelBuilder.GetEdmModels( "api" );
                    var batchHandler = new DefaultODataBatchHandler();
                    endpoints.MapControllers();
                    endpoints.MapVersionedODataRoute( "odata", "api", models, batchHandler );
                } );
        }

Here's the error message I get:
System.NullReferenceException
HResult=0x80004003
Message=Object reference not set to an instance of an object.
Source=Microsoft.AspNetCore.Mvc.Versioning
StackTrace:
at Microsoft.AspNetCore.Mvc.Routing.ApiVersionUrlHelper.get_RouteParameter() in ...\Microsoft.AspNetCore.Mvc.Versioning\Routing\ApiVersionUrlHelper.cs:line 37

[koshdim]

I also stumbled upon this. looks like integration with batch functionality does not work.

I tried to debug, created by own ODataBatchHandler and found that when context is passed into
public override async Task ProcessBatchAsync(HttpContext context, RequestDelegate nextHandler)
context.GetEndpoint() - returns null and
context.GetRouteData().Routers - empty collection.
this feels wrong. is it? my setup is the same as @jtTorres 's

[commonsensesoftware]

This is very strange. I've not seen this happen and I can't figure out how to create a repro. Does anyone have a repro? Does this happen for all requests with this setup or just batch requests specifically?

It's also possible that I've already addressed this by way of some other fix. Which version is this happening against? In particular, the forthcoming patch will contain an extension method that will create an instance of IApiVersioningFeature on demand rather than rely solely on middleware.

[jtTorres]

I just pulled the latest from this repo and added the configuration mentioned in my first post. The issue still exists. It only happens for $batch requests

I'm using POSTMAN for the request. Please see my request below:

[commonsensesoftware]

The change/fix hasn't been pushed/merged - just yet. I'm burning down the full list. I can push the branch if you're interested in trying out the changes. I see it definitely happens with a batch request. Does it still happen in a non-batch request? Just curious.

As a potential temporary workaround, you could try adding the world's simplest middleware to ensure the feature is setup. You might have to play with the order. Ultimately, there isn't anything magical about the middleware. It just adds the feature and that was primarily meant to be an optimization.

Something like this:

app.Use((context, next) =>
    {
        IApiVersioningFeature feature = new ApiVersioningFeature(context);
        context.Features.Set(feature);
        return next(context);
    });
app.UseODataBatching();
app.UseApiVersioning();

HttpContext is a monad in the request pipeline. The properties in the ApiVersioningFeature are lazy-evaluated. It shouldn't really matter where it is set as log as it is set. This should address the NRE issue.

[jtTorres]

sure, I can pull your your branch and try that first to see if that works. Just send me the name.
Everything works as expected with non-batch requests.

[commonsensesoftware]

@jtTorres the branch is dev/css/201606-fixes and has now been pushed up. Take a peek at your leisure. It's not entirely clear to me why this would only happen during a batch request. Hopefully this will be resolved in the next patch. Thanks.

[jtTorres]

@commonsensesoftware pulled your branch and made the updates for $batch described in the initial post and everything worked as expected. I'm able to do both batch and non-batch transactions.

[commonsensesoftware]

@jtTorres Great news! Thanks for confirming. This fix will be included with the PR and other changes going in. The "PM" (e.g. me) is trying everything offline as they burn down. There's just a few more issues left to address and then I'll work on getting a release out.

[jtTorres]

@commonsensesoftware thanks for looking into this. We're definitely going to be using this feature a lot in my company so we're grateful and anxiously awaiting the release. :)

[CSharpFiasco]

@commonsensesoftware I had this exact issue, thanks for working on it. Is there a timeline for getting this bug fix into a release?

[spaasis]

No cigar on the minimal middleware (dotnet 5, api-versioning 5.0.0). Any middleware order results in NRE

app.Use((context, next) => {
                    var feature = new ApiVersioningFeature(context);
                    context.Features.Set(feature);
                    return next(); 
                })

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.AspNetCore.Mvc.Routing.ApiVersionUrlHelper.get_RouteParameter()
   at Microsoft.AspNetCore.Mvc.Routing.ApiVersionUrlHelper.AddApiVersionRouteValueIfNecessary(Object current)
   at Microsoft.AspNetCore.Mvc.Routing.ApiVersionUrlHelper.Link(String routeName, Object values)
   at Microsoft.AspNet.OData.Batch.ODataBatchHttpRequestExtensions.GetODataBatchBaseUri(HttpRequest request, String oDataRouteName, IRouter route)
   at Microsoft.AspNet.OData.Batch.ODataBatchHandler.GetBaseUri(HttpRequest request)
   at Microsoft.AspNet.OData.Batch.DefaultODataBatchHandler.ParseBatchRequestsAsync(HttpContext context)
   at Microsoft.AspNet.OData.Batch.DefaultODataBatchHandler.ProcessBatchAsync(HttpContext context, RequestDelegate nextHandler)
   at Microsoft.AspNet.OData.Batch.ODataBatchMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

[commonsensesoftware]

@spaasis Do you have a repro you can share? Something is amiss. It's pretty clear that if you make it to DefaultODataBatchHandler.ParseBatchRequestsAsync and get NRE at ODataBatchHandler.GetBaseUri, then the feature hasn't been set upstream (e.g. yet).

It should be noted that the concept of batching the way that it historically worked in Web API is not supported in ASP.NET Core. I've run into this problem in the past and confirmed it with the ASP.NET Core team. They have no plans to ever support it (the way it was). Their position is that the improvements starting in HTTP/2 negate the need to support it. The problem with a parallel design to classic Web API is that ASP.NET Core has a 1:1 relationship between HttpContext and HttpRequest. The OData protocol does define/allow batching which is fine, but the Scatter Gather implementation in ASP.NET Core has issues. The primary issue is Features. Each HttpContext has it's own features, they don't come from DI, they aren't (necessarily) cloneable, and there's no clear way to create/recreate them. You can see OData's futile attempt to get around this in ODataBatchReaderExtensions.CreateHttpContext.

For clarity on those trying to combine batching and API Versioning, you probably want to explicitly set the API Versioning feature before and after the OData batch middleware. It doesn't specifically have to be in the middleware, but you need it in both places.

Consider the following:

POST $batch HTTP/2
Host: localhost
Content-Type: multipart/mixed; boundary=cbe401ac-6ffb-4604-be1e-4f2d7d92df5c

--cbe401ac-6ffb-4604-be1e-4f2d7d92df5c
Content-Type: application/http; msgtype=request

GET orders/123?api-version=1.0 HTTP/2
Host: localhost

--cbe401ac-6ffb-4604-be1e-4f2d7d92df5c
Content-Type: application/http; msgtype=request

GET customers/456?api-version=2.0 HTTP/2
Host: localhost

--cbe401ac-6ffb-4604-be1e-4f2d7d92df5c--

This is a completely valid and plausible batch request. The problem is in the implementation. OData will shallow copy IApiVersioningFeature if it's set. The implementation reads and parses the API version just once. If the serial OData ordering rules applied to the processing, this means that the second request item will use API version 1.0 instead of 2.0 because the feature would have already been created and the value parsed!

This is exactly how things are supposed to work. The issue is specific to OData. Even when the fix for NRE comes, you'll still have to contend with this problem. A comprehensive solution to the problem would to:

1. Make sure the IApiVersioningFeature is set before the OData batch middleware
2. Create a custom batch handler that extends DefaultODataBatchHandler. After ParseBatchRequestsAsync, every ODataBatchRequestItem.HttpContext.Features should update with a new, separate instance of IApiVersioningFeature

Re-examining this, it could be something added to the OData support. For now, this is your best bet.

[spaasis]

Wow. It's never simple with these things, is it?

For a simple repro, open the latest master, samples/aspnetcore/SwaggerODataSample and attempt to $batch there:

Startup.cs:

        public void Configure( IApplicationBuilder app, VersionedODataModelBuilder modelBuilder, IApiVersionDescriptionProvider provider )
        {
            app.Use( ( context, next ) =>
            {
                var feature = new ApiVersioningFeature( context );
                context.Features.Set( feature );
                return next();
            } ); //place this anywhere in the middleware chain and the $batch still fails with NRE in ApiVersionUrlHelper
            app.UseODataBatching();
            app.UseRouting();
            app.UseEndpoints(
                endpoints =>
                {
                    endpoints.Count();
                    endpoints.MapVersionedODataRoute( "odata", "api", modelBuilder.GetEdmModels("api"), new DefaultODataBatchHandler() );
                } );
            app.UseSwagger();
            app.UseSwaggerUI(
                options =>
                {
                    // build a swagger endpoint for each discovered API version
                    foreach ( var description in provider.ApiVersionDescriptions )
                    {
                        options.SwaggerEndpoint( $"/swagger/{description.GroupName}/swagger.json", description.GroupName.ToUpperInvariant() );
                    }
                } );
        }

I even tried to place the tiny middleware multiple times after any other middleware call, but still..

As an aside, I think it's awesome that you provide such in-depth answers so often in these issues. It really shows your expertise and commitment. Thanks, again!

[commonsensesoftware]

A lot of fixes have been merged into ms (formerly master) to address this issue and many others. I'm a bit curious if anyone has tried this against the latest and great commits? They will be going into the 5.1 release. I'm still waiting to complete code signing onboarding so I can publish the changes.

I'm also curious if the setup works correctly with the 6.0 preview release. I suspect there's a much greater chance that it will work because it has all the 5.1 fixes and more. Note that the new 6.0 packages are Asp.Versioning.OData and Asp.Versioning.OData.ApiExplorer.

Sidebar: I'm interested in preserving OData batching support so much as OData itself supports it, but it is really just for backward compatibility. You may have noticed that the batching support that used to exist in ASP.NET Web API is curiously missing from ASP.NET Core. The ASP.NET team has said that it was purposely excluded because the transport improvements in HTTP/2, and more so in HTTP/3, supplant the need to support it. In HTTP/1.1, there were definitely benefits to this type of batching technique, but no more. That's something you may want to consider going forward. The ASP.NET Core repo has an in-depth discussion and explanation in #6000.

[commonsensesoftware]

Rather than repost, check out this comment. The short, short answer is that official OData batch support is coming in 6.1. 😄

[jtTorres]

Woot! Looking forward to this. Do you have a date in mind yet? 😅

[commonsensesoftware]

My goal is within the week, but it could be even days. I'm going through the open issues and seeing with other low-hanging fruit applies and can be included. 😉