OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
447 stars 157 forks source link

Adding Odata in Versioning breaks the ability to perform select, filter etc. and Removing that breaks the swagger versioning #1096

Open akshaybheda opened 8 months ago

akshaybheda commented 8 months ago

Assemblies affected

        <PackageReference Include="Asp.Versioning.OData" Version="6.4.0" />
        <PackageReference Include="Asp.Versioning.OData.ApiExplorer" Version="6.4.1" />
        <PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.21.0" />
        <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="6.0.25" />
        <PackageReference Include="Microsoft.AspNetCore.OData" Version="8.2.3" />
        <PackageReference Include="Microsoft.AspNetCore.OData.NewtonsoftJson" Version="8.2.0" />

Describe the bug I have setup following

 services.AddControllers()
      .AddOData((opt) =>
      {
          opt.EnableQueryFeatures()
          .AddRouteComponents("api/V1", GetEdmModelV1())
          .AddRouteComponents("api/V2", GetEdmModelV2());
      });

and then

services.AddApiVersioning()     // Core API Versioning services with support for Minimal APIs
        .AddMvc()               // API versioning extensions for MVC Core
        .AddApiExplorer()       // API version-aware API Explorer extensions
        .AddOData()             // API versioning extensions for OData
        .AddODataApiExplorer(); 

Adding Odata in Versioning breaks the ability to perform select, filter etc. and Removing that breaks the swagger versioning. Any clue how to resolve this?

Also, If i Add AddOData() to the Versioning Options, It removes all the endpoints from the OData Endpoint Mappings and The response is changed for my api.

julealgon commented 8 months ago

Shouldn't this be moved to the versioning repository? @commonsensesoftware FYI

commonsensesoftware commented 8 months ago

@akshaybheda Your configuration is not correct. Unfortunately, OData doesn't have extensibility points in its configuration so the required setup is split. The correct configuration would be:

// add OData services and query options which are shared across all OData routes
services.AddControllers().AddOData(options => options.EnableQueryFeatures());

// add API Versioning with OData and versioned OData routes
services.AddApiVersioning()
        .AddOData(options => options.AddRouteComponents("api/v{version:apiVersion}"))
        .AddODataApiExplorer(); 

This variance and setup is outlined in the wiki. You can use the ModelBuilder on the options to configure EDMs and it is even possible to explicitly set the EDM model on the route through the container setup callback, but they are typically configured via one or more IModelConfiguration implementations which are automatically picked up via dependency injection (DI). The OData + API Versioning + OpenAPI example demonstrates how to put all of the pieces together.

ElizabethOkerio commented 8 months ago

@commonsensesoftware Thanks for the follow up. Can you suggest how we might add extensibility to the OData configuration to make this simpler.

commonsensesoftware commented 8 months ago

@ElizabethOkerio Thanks for asking. I did talk to Sam about these issues circa Nov'20 before 8.0 was released. Here are a few of the most problematic areas:

  1. ODataOptions.RouteComponents as a dictionary of tuples is pretty icky. That aside, it's very hard to get in front of. To achieve that, I had to replace IOptions<ODataOptions> as seen here. This is the crux of many of the challenges.
  2. ODataRoutingApplicationModelProvider is: a. internal which makes it difficult to replace or extend b. Eagerly evaluates IOptions<ODataOptions> in its constructor, which is not safe (the DI cycle might not be complete) and makes the #1 workaround harder. c. Doesn't consider inheritance and only considers typeof(AttributeRoutingConvention), which can be extended. This convention needs to be replaced by VersionedAttributeRoutingConvention to work properly. You can see exactly how nasty this becomes in ODataMultiModelApplicationModelProvider which decorates over the default implementation in the most contrived of ways.
  3. Batching has always been tricky in OData + ASP.NET Core. Officially, it is not supported by ASP.NET Core and the ASP.NET team has stated that it shouldn't be needed in a post HTTP/2 world. Unfortunately, we can't abandon what the OData protocol defines. The primary issue here is that Features cannot be safely copied or cloned into a new HttpContext. By design, they have request affinity. This is problematic for the Scatter-Gather approach that OData batching uses. OData makes a guess at how to deal with Features without providing any way to override or redefine what should happen. This leads to painful decoration hacks like this one. This area seems largely unchanged since Rob worked on it. We discussed these challenges, but he left the project before something like hooks were discussed.
  4. There any number of other cases where default services are internal and are not easy to replace without relying on type names or Reflection, such as DefaultODataTemplateTranslator remapped as VersionedODataTemplateTranslator, which has to be done like this.

I'm happy to setup another issue or discussion some time. I met with Sam and Mike several years ago with the hope of the projects having better synergy. There are a number of ways things could be smoother. In particular, there is considerable work in the API Explorer space that could be lifted and shifted to work with OData generically instead only with API Versioning. I have stated in other issues that I'm willing to contribute and help drive such initiatives, but that cannot start without agreement and commitment of such features. The amount of work involved is non-trivial.

ElizabethOkerio commented 8 months ago

@mikepizzo @xuzhg

commonsensesoftware commented 8 months ago

@akshaybheda It looks like you might have deleted your post, but to answer your question:

The URL is /api/Orders/{key}/lineItems , I previously using ODataRoutePrefix in the controller and [ODataRoute("({key})/lineItems")] was able to get /api/Orders({key})/lineItems

I am looking for paranthesis

Now if i define the same using HttpGet("RoutePrefix+Route") it adds it to non odata endpoint mappings.

Route options are also shared so the setup looks like:

services.AddControllers()
        .AddOData(
            options =>
            {
                options.EnableQueryFeatures();
                options.RouteOptions.EnableKeyAsSegment = false;
            });

Many people now prefer key as segment versus key in parenthesis. The default configuration in OData supports both styles and both will be reported in OpenAPI. EnableKeyAsSegment = false is enable only the parenthesis style.

akshaybheda commented 8 months ago

@commonsensesoftware Yeah, i saw that and removed my question. Thanks for the update and all your help. Now i understand better. Only thing that's bugging me is template routes don't show v1 v2 etc image

commonsensesoftware commented 8 months ago

@akshaybheda, that looks like the OData route debugging page. When you version by URL segment, API Versioning does not know where API version will appear. It could be v{ver:apiVersion}, api/v{ver:apiVersion}, api/{ver:apiVersion} (no v), and so on. A custom route constraint is used to represent the placeholder where the API version will appear as you've defined it. There is no magic string parsing or regex happening. Furthermore, if an API interleaves multiple versions what should be shown? API versions 1.0 and 2.0 might both be applied to and match api/v{version:apiVersion}/Orders, but there is only one template.

Route parameters are part of a template. Showing them seems perfectly normal. It's no different that if you had order/{id}/items or customer/{id}. If you are referring to documentation in OpenAPI, then the API Explorer options can help you there. Setting options.SubstituteApiVersionInUrl = true will replace the API version route parameter with the corresponding value and remove the parameter. api/v{version:apiVersion}/Orders becomes api/v1/Orders.