OData / AspNetCoreOData

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

ASP.NET Core OData 8.x does not work with Minimal API #578

Open Lonli-Lokli opened 2 years ago

Lonli-Lokli commented 2 years ago

Assemblies affected ASP.NET Core OData 8.x

Describe the bug OData does not work with Minimal API, ie without Controllers

Reproduce steps

app.MapGet("/WeatherForecast", [EnableQuery] () => Enumerable.Range(1, 5).Select((int index) => new WeatherForecast
    {
        Date = DateTime.Now.AddDays(index),
        TemperatureC = Random.Shared.Next(-20, 55),
        Summary =  new[]  {
            "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
        }
    })
    .ToArray());
app.Run();

Expected behavior I would like to use OData without Controllers at all

ElizabethOkerio commented 2 years ago

@Lonli-Lokli could you please provide us with more information on any problems you are getting with using the Minimal Api with OData

Lonli-Lokli commented 2 years ago

@ElizabethOkerio Basically I would like to use OData with only Minial Api, without enabling Controllers on MVC (.AddControllers) or writing OData Controllers with EnableQuery.

I want to use OData with pure Minimal API endpoint - specify [EnableQuery] and make it works same as with Controller.

Now it does not work

TehWardy commented 2 years ago

So basically this is the problem ...

        public static void AddApi(this IServiceCollection services)
        {
            _ = services.AddControllers()
                .AddOData(opt =>
                {
                    opt.RouteOptions.EnableQualifiedOperationCall = false;
                    opt.EnableAttributeRouting = true;
                    _ = opt.Expand().Count().Filter().Select().OrderBy().SetMaxTop(1000);
                    opt.AddRouteComponents($"/Odata", new MyEdmModel());
                });
        }

... the setup for odata is dependant on this it seems ... services.AddControllers() <-- ...

Random thought though ... Have you tried just injecting an ODataOptions in to the get endpoint you're mapping and then using that directly?

In a controller situation it would look like this ...


 public class FooController
 {
        [HttpGet]
        [EnableQuery]
        public virtual IActionResult Get(ODataQueryOptions<Foo> queryOptions)
            => Ok(queryOptions.ApplyTo(service.GetAll()));
 }

I can't see any reason why that couldn't be done in a minimal API sitation unless the above MUST be made in order for some DI thing to be configured.

So this makes your potential solution something like ...

 app.MapGet("/WeatherForecast", [EnableQuery] (ODataQueryOptions<WetherData> options) => 
     options.ApplyTo(
         Enumerable.Range(1, 5).Select((int index) => new WeatherForecast
          {
              Date = DateTime.Now.AddDays(index),
              TemperatureC = Random.Shared.Next(-20, 55),
              Summary =  new[]  {
                  "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
              }
          })
      ) .ToArray());
app.Run();
Lonli-Lokli commented 2 years ago

I was able to call AddOdata without AddControllers with Reflection

var odataMethod = typeof(ODataMvcCoreBuilderExtensions).GetMethod("AddODataCore", BindingFlags.NonPublic | BindingFlags.Static);
odataMethod?.Invoke(null, null);

But your approach with ODataQueryOptions will not work as ODataQueryOptions are not registered as service.

As an addition, AttributeRouting or ConventionRouting doesnt make much sense with Minimal Api. The best use case is as I described above - non-edm way with EnableQuery

TehWardy commented 2 years ago

Just took a look at the code ...

    public class ODataQueryOptions<TEntity> : ODataQueryOptions
    {
        public ODataQueryOptions(ODataQueryContext context, HttpRequest request)

... so you're gonna need to do this ...

        static IEdmModel GetEdmModel()
        {
            ODataConventionModelBuilder builder = new ODataConventionModelBuilder();
            builder.EntitySet<WeatherForecast>("WeatherForecasts");
            return builder.GetEdmModel();
        }

       var edmModel = GetEdmModel();
       builder.Services.AddScoped<ODataQueryOptions<WeatherForcast>>(ctx => 
           new ODataQueryContext(edmModel, typeof(WeatherForecast));   

... OData is dependent on EDM Models it seems at the moment, there's no way to escape that. this should at least allow you to get the functionality.

Lonli-Lokli commented 2 years ago

Even according to your sample it's clear that ODataQueryOptions cannot be added during application building as they requires HttpRequest. Also

builder.Services.AddScoped<ODataQueryOptions<WeatherForcast>>(ctx => 
         new ODataQueryContext(edmModel, typeof(WeatherForecast));   

will not compile because thi constructor is internal, also ODataQueryContext cannot be casted ODataQueryOptions. More valid (but still producing incorrect API results) is

var Summaries = new[]
{
    "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
};

app.MapGet("/WeatherForecast", (HttpRequest request) =>
    new ODataQueryOptions<WeatherForecast>(new ODataQueryContext(edmModel, typeof(WeatherForecast), null), request)
        .ApplyTo(Enumerable
            .Range(1, 5).Select((int index) => new WeatherForecast
            {
                Date = DateTime.Now.AddDays(index),
                TemperatureC = Random.Shared.Next(-20, 55),
                Summary = Summaries[new Random().Next(Summaries.Length)]
            })
            .AsQueryable()));

It will cycled data instead of similar code with controllers

julealgon commented 2 years ago

Even according to your sample it's clear that ODataQueryOptions cannot be added during application building as they requires HttpRequest.

It can using factory-based registrations with IHttpContextAccessor.

Lonli-Lokli commented 2 years ago

Just to show how it looks like with pure Contollers (and to show that EdmModel is NOT required), here is the sample - https://github.com/Lonli-Lokli/net6-conrtoller-odata I've added only builder.Services.AddControllers().AddOData(options => options.Select().Filter().OrderBy());;' and

[HttpGet(Name = "GetWeatherForecast")]
    [EnableQuery]
    public IEnumerable<WeatherForecast> Get()

And as a result with https://localhost:7081/WeatherForecast?$select=Summary I am getting [{"Summary":"Hot"},{"Summary":"Cool"},{"Summary":"Freezing"},{"Summary":"Scorching"},{"Summary":"Sweltering"}]

Lonli-Lokli commented 2 years ago

@julealgon Thanks, forget about it. With current snippet I am getting System.Text.Json.JsonException: A possible object cycle was detected. This can either be due to a cycle or if the object depth is larger than the maximum allowed depth of 64.

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.

var odataMethod = typeof(ODataMvcCoreBuilderExtensions).GetMethod("AddODataCore", BindingFlags.NonPublic | BindingFlags.Static);
odataMethod?.Invoke(null, null);
var odataBuilder = new ODataConventionModelBuilder();
odataBuilder.EntitySet<WeatherForecast>("Weathers");
var edmModel = odataBuilder.GetEdmModel()!;

builder.Services.Configure<ODataOptions>(options =>
{
    options.RouteOptions.EnableQualifiedOperationCall = false;
    options.EnableAttributeRouting = false;
    options.Select().Filter().OrderBy();
    options.AddRouteComponents($"/Odata", edmModel);

});
builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();
builder.Services.AddHttpContextAccessor();
builder.Services.AddScoped(svc =>
    new ODataQueryOptions<WeatherForecast>(new ODataQueryContext(edmModel, typeof(WeatherForecast), null),
        svc.GetRequiredService<IHttpContextAccessor>().HttpContext.Request)); 

var app = builder.Build();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseSwagger();
    app.UseSwaggerUI();
}

app.UseHttpsRedirection();

var Summaries = new[]
{
    "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
};

app.MapGet("/WeatherForecast", (ODataQueryOptions<WeatherForecast> options) => options
        .ApplyTo(Enumerable
            .Range(1, 5).Select((int index) => new WeatherForecast
            {
                Date = DateTime.Now.AddDays(index),
                TemperatureC = Random.Shared.Next(-20, 55),
                Summary = Summaries[new Random().Next(Summaries.Length)]
            })
            .AsQueryable()));
app.Run();
julealgon commented 2 years ago

Weird.... there are no cyclic relationships in the model. I wonder what it is trying to do.

victormarante commented 2 years ago

Hi, is there any more information about this issue? Looking for a way to integrate this in my current minimap API project.

Gigabyte0x1337 commented 2 years ago

This is not possible yet because we need a different input/output formatter i made a project and got pretty far but the only issue is that you can't parse input Delta in .net 7 route filter handlers will be added that will maybe solve that issue.

Gigabyte0x1337 commented 2 years ago

Program.cs.txt

Lonli-Lokli commented 2 years ago

@Gigabyte0x1337 but it will not work without EDM model as with Controllers, right?

Also I think Batch will not be able to handle this as well

Gigabyte0x1337 commented 2 years ago

@Gigabyte0x1337 but it will not work without EDM model as with Controllers, right?

Also I think Batch will not be able to handle this as well

I haven't tried maybe i can get those working too. I don't like the input parameters wrappers that can maybe be fixed in .NET 7. When I have some time i could try to make it work without edm and with the batch functionality.

julealgon commented 1 year ago

@xuzhg / @habbes do you have a ballpark estimate on when this will be looked into? .NET is pushing minimal API heavily lately, especially now with the launch of .NET 7. The fact that OData just doesn't work with it out of the box is a glaring omission at this point.

Not being able to leverage all the benefits of minimal APIs is pretty significant at the moment, and I'll include myself in that sentiment.

Gigabyte0x1337 commented 1 year ago

I created a proof of concept with new .NET features which helps with some ideas on the possibilities for implementation. There is some extra code that doesn't need to be there, some tiny things that can be fixed/changed. Not everything is implemented of course but I hope I can help to move this issue forward. https://github.com/Gigabyte0x1337/ODataMinimalApiExample

NikoGJ commented 8 months ago

Hello, do you know if any progress have been made on that, now that .NET 8 has come out ?

denisulmer commented 8 months ago

I am also interested in the progress here, any news with .NET8?

cilerler commented 8 months ago

~The merge occurred last week. This tag indicates a release candidate. I suggest waiting for the official announcement before proceeding.~

julealgon commented 8 months ago

The merge occurred last week. This tag indicates a release candidate. I suggest waiting for the official announcement before proceeding.

CC: @robertmclaws

@cilerler what does this have to do with AspNetCore OData and Minimal APIs? Maybe I'm missing something obvious here.

cilerler commented 8 months ago

@julealgon, apologies for the confusion—I was browsing the Restier repository and got sidetracked, leading to a mix-up. Please ignore my previous comment; it was not intended for this issue. Thank you for seeking clarification.

xuzhg commented 7 months ago

@cilerler @cilerler @Lonli-Lokli @TehWardy Any one can download the 'miniApi' branch and give it a try and share your thoughts?

Gigabyte0x1337 commented 7 months ago

@xuzhg Does this mean we lose the model validation that is inside the mvc formatter. In mvc you have model state errors how does that work in this scenario? Also do all odata features still work like navigation parameter binding with @ in post for example?

cilerler commented 7 months ago

Nice work @xuzhg, 👏🏻 I've tested it against common use cases I can think of, and it all works as expected. The one thing that wasn't clear to me was what happened to the $metadata; I couldn't access it.

julealgon commented 7 months ago

The one thing that wasn't clear to me was what happened to the $metadata; I couldn't access it.

I believe the way $metadata was implemented was by hooking a pre-built controller class into the application pipeline... so without controller support added, it makes sense it wouldn't be available.

I assume that controller would also need to be converted to support Minimal API.

xuzhg commented 7 months ago

Nice work @xuzhg, 👏🏻 I've tested it against common use cases I can think of, and it all works as expected. The one thing that wasn't clear to me was what happened to the $metadata; I couldn't access it.

@cilerler Thanks for your feedback. I enabled the metadata in the PR, please take a look and share your comments. Thanks.

Merry Christmas! and Happy New Year!

xuzhg commented 7 months ago

The one thing that wasn't clear to me was what happened to the $metadata; I couldn't access it.

I believe the way $metadata was implemented was by hooking a pre-built controller class into the application pipeline... so without controller support added, it makes sense it wouldn't be available.

I assume that controller would also need to be converted to support Minimal API.

@julealgon You are always RIGHT! :) please take a look the PR and share your comments. Merry Christmas! and Happy New Year!

TehWardy commented 5 months ago

I've been out of the loop for a while is this solved now?

jhancock-taxa commented 1 month ago

What I'd like to see is that we can just do AddOData and the inject whatever the class that has the Filter, Sort and Select in it into the minimal api endpoint, and then use that to apply it ourselves.

I.e. the Filter would be Expression<Func<T, bool>>, the Sort similar, and the Select similar as well along with Top and Skip.

And that would end the dependency on entities entirely, which would be highly desirable to work against DTOs correctly etc.

You could sort of hack this up with controllers, but it wasn't pretty.

julealgon commented 1 month ago

@jhancock-taxa I believe what you are describing would be completely out of scope for a Minimal API support effort. My suggestion would be for you to create a separate issue to propose/discuss that.

jhancock-taxa commented 1 month ago

@julealgon #1267