Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$batch returns 404 or 405 when using Asp.Versioning.OData.ApiExplorer 6.0.0-preview.3, OData 8 and .Net 6 #847

Closed
joevanheerden opened this issue Jul 15, 2022 · 13 comments · Fixed by #879

Comments

@joevanheerden
Copy link

joevanheerden commented Jul 15, 2022

I'm cannot figure out how to make Odata Batching to work when using API Versioing. I cannot tell if I'm missing configuration, or whether the issue is with Odata or API Versioning.

Using URL Segement versioning and the following configuration results in a 404 when making a GET request to api/v1/$batch, and 405 when making a POST request.

builder.Services.AddControllers().AddOData();
    builder.Services.AddApiVersioning(
            options =>
            {
                options.ApiVersionReader = new UrlSegmentApiVersionReader();
            })
        .AddOData(options =>
                  {
                      var defaultBatchHandler = new DefaultODataBatchHandler
                      {
                          MessageQuotas =
                          {
                              MaxNestingDepth = 2,
                              MaxOperationsPerChangeset = 10,
                              MaxReceivedMessageSize = 100
                          }
                      };

                      options.AddRouteComponents("api/v{version:apiVersion}", defaultBatchHandler);
                  });

...

app.UseHttpsRedirection();
app.UseAuthorization();
app.UseODataBatching();
app.MapControllers();
app.Run();

Expected behaviour is to get the expected OData response for POST, and 'The batch request must have a "Content-Type" header.' for GET.

Any help would be much appreciated.

Thanks to the author of this repo for all the hard work in maintaining this - the effort is much appreciated.

@commonsensesoftware
Copy link
Collaborator

GET will never work. The $batch endpoint only accepts POST. The message body is always a MIME-encoded multipart document where each part is a nested HTTP message. The nested parts can be a GET. The response will always be 202. You have to inspect the individual parts in the response, which are themselves nested HTTP responses, to determine which succeeded or failed.

The $batch endpoint does not need to be versioned and should probably be version-neutral. This might be an uncovered edge case (currently), but I'll have to investigate. The template api/v{version:apiVersion}/$batch doesn't appear to be matching up. Off the top of my head, I can see how this would happen since $batch is special middleware and not an endpoint. The batch route behaves the same way, regardless of version, so you only ever need api/$batch. This would be the preferred approach anyway so that you could have api/v1/order/123 and api/v2/order/456 in the same request.

I have a feeling that this may only happen because you're versioning by URL segment. This would be one more entry in a long list of issues with that approach, aside from it not being RESTful. If it's an option, you might consider another approach. Another possible solution would be to register the $batch handler outside of API Versioning using the standard OData configuration. It's been a while since I've looked at the implementation, but in theory, that can work. The batch handler doesn't really do much with the incoming message. It essentially does a Scatter Gather of the nested HTTP messages from the batch request. It shouldn't need the EDM. The routing system should provide everything needed to forward the nested requests.

This should work, so I'll look into it. You might consider whether you actually need OData batching. When it was first created, it was a very novel idea and tackled some tough limitations in HTTP/1.1. These issues have mostly be eliminated in HTTP/2 and HTTP/3. ASP.NET Web API had generic batching functionality, but ASP.NET Core doesn't. In talking with the team, their position is that it is simply no longer needed and porting it is not worth the complexity it introduces. OData chose to try and keep batching for feature parity, but it's really best effort under the hood. The primary issue is creating a nested HttpContext from the current context and its features. Features have pipeline affinity, don't come from DI, and have no formal cloning mechanism. This makes implementing a request within a request difficult.

@knightian
Copy link

Is it expected that swagger doesn't see the versions we define and just shows all the mapped routes on the page and doesn't split them into versions?

@commonsensesoftware
Copy link
Collaborator

@knightian I'm not sure I fully follow the question. API Versioning doesn't specific care anything about OpenAPI (or Swagger). The API Explorer extensions will bucketize all endpoints by API version and represent that via the GroupName. The only exception to that is if you explicitly define a group. There is only one pivot point. If you define one, then it is not overwritten (anymore). If you want both, then you'd need your own method of grouping or merge the text (ex: Orders - V1). If you're not using the API Explorer extensions, then that would be the issue.

Is this relating to batching? This seems to be a completely different question.

@knightian
Copy link

knightian commented Jul 21, 2022

@knightian I'm not sure I fully follow the question. API Versioning doesn't specific care anything about OpenAPI (or Swagger). The API Explorer extensions will bucketize all endpoints by API version and represent that via the GroupName. The only exception to that is if you explicitly define a group. There is only one pivot point. If you define one, then it is not overwritten (anymore). If you want both, then you'd need your own method of grouping or merge the text (ex: Orders - V1). If you're not using the API Explorer extensions, then that would be the issue.

Is this relating to batching? This seems to be a completely different question.

What I mean is, if I click on /v1.0/swagger.json I only want to see my /v1.0/mappedroute returned I don't want to see all the versions in the v1.0 json that makes no sense, why would I see v2.0 etc in the v1.0 json?

@commonsensesoftware
Copy link
Collaborator

I don't see how is related to batching (e.g. this issue) or OData; however, this happening for one or both of the following reasons:

  1. You aren't using the required API Explorer extensions
  2. The OpenAPI generator library is not configured correctly (e.g. Swashbuckle, NSwag, etc)

The examples demonstrate how to properly setup OpenAPI support for standard and Minimal APIs. That includes grouping v1.0 and v2.0 into different swagger.json documents.

@commonsensesoftware
Copy link
Collaborator

@joevanheerden were you ever able to get things working?

@knightian even though your question was a segue, did you get things working as well?

@knightian
Copy link

@joevanheerden were you ever able to get things working?

@knightian even though your question was a segue, did you get things working as well?

Hey yes I did thank you, working well.

@joevanheerden
Copy link
Author

Apologies for the late response.

Thanks for your detailed explanation. I could not get $batch to work - not even when registering the default batch handler for route prefixes without versioning.

I've since abandonded $batch, and started using ODataQueryRequest since it is better suited for my use case, which is to be able to use large OData queries with HTTP POST. It took me only a few minutes to get that to work opposed to hours of fiddling to try and get $batch to work.

Side note: It seems that prior to .Net 6 with OData 8 ODataQueryRequest was enabled by default, but now it is opt-in and requires app.UseODataQueryRequest() to enable it.

Once again @knightian: Thanks for all the effort in producing a great library.

@commonsensesoftware
Copy link
Collaborator

@joevanheerden Good to know. OData is always seems to be a moving target. I've had reports of breaking changes or regressions between even patch versions. 😬 Glad you are unblocked.

I'll leave this issue open for now. I have a feeling there are some edge cases around batching that still need to be ironed out. I've been advising people away from it, but if the really want to use it, then it should work. Thanks for the update.

@fannydengdeng
Copy link

I'm also getting 404 errors from $batch endpoints. I'm using Asp.Versioning.OData 6.0.0, Microsoft.AspNetCore.OData 8.0.11 and .NET 6.

My setup is something like

public void ConfigureServices(IServiceCollection services) {
    services.AddControllers()
        .AddOData(options => options
            .AddRouteComponents("xxx", GetOtherEdmModel()));

    services.AddApiVersioning(
        options =>
        {
            options.ReportApiVersions = true;
            options.AssumeDefaultVersionWhenUnspecified = true;
            options.ApiVersionReader = ApiVersionReader.Combine(
                new QueryStringApiVersionReader(),
                new HeaderApiVersionReader("api-version", "x-ms-version"));
        })
        .AddOData(options => options
            .AddRouteComponents("yyy", new CustomODataBatchHandler())
             .AddRouteComponents("zzz"));
        ...
}

I did some debugging and saw that when the ODataBatchMiddleware is instantiated, there are zero RouteCompoents in the ODataOptions. See https://github.com/OData/AspNetCoreOData/blob/eb3ca8e0b02058c33212f124c810c67f453f74dc/src/Microsoft.AspNetCore.OData/Batch/ODataBatchMiddleware.cs#L83. The resolved IOptions<ODataOptions> in the middleware is the replaced VersionedODataOptions. So when the Value getter is invoked, I'm assuming we are returning a default/empty ODataOptions, due to not being in the scope of a request. Is that correct?

@pil0t
Copy link

pil0t commented Sep 15, 2022

@fannydengdeng I dig a bit into ODataBatchMiddleware and found hack for making it work with versioning:


            foreach (var kvp in options.Mapping)
            {
                var routeComponents = kvp.Value.RouteComponents;
                foreach (var model in routeComponents)

@commonsensesoftware
Copy link
Collaborator

@fannydengdeng This behavior is, unfortunately, by design. I tried to influence the OData 8.0 design early on to no avail. Its design assumes there can be only one ODataOptions and EdmModel per route configuration, which simply will not work with in a versioning scenario. Using the built-in OData AddRouteComponents method and registrations are completely ignored by API Versioning. This is why it's empty.

I'm looking into this issue more deeply, but there are a myriad of problems. One significant issue is that OData now allows an ODataBatchHandler per route configuration. That's simple wrong IMHO. I understand why someone might think it's a good idea, but it's unnecessary. There only needs to be a single $batch endpoint because, by definition, the handler routes to subrequests. Disallowed requests should fail with 403 or some other appropriate response. The new design doesn't prevent that, it just makes it return 404. This behavior is challenging for API Versioning because the $batch endpoint is itself version-neutral. Specifying $batch?api-version=1.0 shouldn't be required. If multiple handlers implicitly match $batch, then which one should be selected?

@pil0t I'd be curious as to the hack or what you got working. I presume you replaced or extended the built-in middleware? It seems I'm going to have to create version-ware variants for batching. ☹️ If you have something working that you can share, it may be useful.

@commonsensesoftware
Copy link
Collaborator

Just a quick update. It appears I finally have something working. The native OData implementation isn't doing us any favors. I tried making things working over the existing implementation, but there is no way to make it work - unfortunately. There are also a litany of bugs and other internal OData issues that made this difficult to troubleshoot and support.

Batching is coming in 6.1 and will add the following:

  • UseVersionedODataBatching (sadly can't get away from custom middleware 😢)
  • VersionedODataBatchHandler (addresses IHttpContextAccessor bug that breaks versioning otherwise)
  • VersionedUnbufferredODataBatchHandler (for parity; probably not used)

Unlike the built-in OData libraries, you don't have to register your own batch handler to get batching to work. VersionedODataBatchHandler will be registered by default, but can be replaced with your own configuration or alternate handler.

The new, minimum setup will be:

var services = builder.Services;

services.AddControllers().AddOData();
services
    .AddApiVersioning(options => options.ApiVersionReader = new UrlSegmentApiVersionReader())
    .AddOData(options => options.AddRouteComponents("api/v{version:apiVersion}"));

app.UseHttpsRedirection();
app.UseAuthorization();
app.UseVersionedODataBatching(); // ← new
app.MapControllers();
app.Run();

It should be noted that it doesn't really matter which batch handler (e.g. $batch) route you hit. The execution process is a Scatter-Gather pattern where each child request re-enters the pipeline. The batch-per-prefix is just plain wrong IMHO and violates REST constraints. You need one and only one $batch endpoint. Attempting to filter or partition off child requests doesn't buy much of anything. Any disallowed child request will return 403, 404, and so on. I don't know why the design was changed this way.

That being said, you can specify an API version to route to a specific ODataBatchHandler implementation (ex: $batch?api-version=2.0). This is the version of the $batch endpoint itself. Each and every child request can/should specify their own API version. The $batch endpoint should be version-neutral. This allows for a client to request different versions of resources in the same request. Since the $batch endpoint is matched via middleware, it's a little trickery to handle being version-neutral.
Each ODataBatchHandler is paired to an EDM, which as an annotated API version. When no API version is specified, all registered API versions are aggregated and a version is resolved via IApiVersionSelector. The SelectVersion method accepts the current HttpRequest so you can make special considerations for just ~/$batch if you want to. The default configuration will likely match some ODataBatchHandler (any should work). If your DefaultApiVersion doesn't correspond to any of your API versions, you'll need to update it. If IApiVersionSelector.SelectVersion returns null in middleware, it will be treated as if it doesn't exist (e.g. parity with the native OData functionality) rather than randomly chose a candidate.

The default ODataBatchHandler can be replaced or reconfigured to suite your needs.

var services = builder.Services;

services.AddControllers().AddOData();
services
    .AddApiVersioning(options => options.ApiVersionReader = new UrlSegmentApiVersionReader())
    .AddOData(
      options => options.AddRouteComponents(
        "api/v{version:apiVersion}",
        new VersionedODataBatchHandler()
        {
          MessageQuotas =
          {
              MaxNestingDepth = 2,
              MaxOperationsPerChangeset = 10,
              MaxReceivedMessageSize = 100
          },
        } ) );

I'm reviewing what else needs to go into 6.1 now, but this could land within the week. Look for the release or track commits/PRs if you want to see or get your hands on stuff early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants