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

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

Open
Lonli-Lokli opened this issue Apr 26, 2022 · 29 comments · May be fixed by #1140
Open

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

Lonli-Lokli opened this issue Apr 26, 2022 · 29 comments · May be fixed by #1140
Assignees

Comments

@Lonli-Lokli
Copy link

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

@Lonli-Lokli Lonli-Lokli added the bug Something isn't working label Apr 26, 2022
@ElizabethOkerio ElizabethOkerio added enhancement New feature or request feature and removed enhancement New feature or request feature bug Something isn't working labels Apr 26, 2022
@ElizabethOkerio
Copy link
Contributor

@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
Copy link
Author

@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
Copy link

TehWardy commented Apr 28, 2022

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
Copy link
Author

Lonli-Lokli commented Apr 28, 2022

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
Copy link

TehWardy commented Apr 28, 2022

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

Lonli-Lokli commented Apr 28, 2022

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
Copy link
Author

@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
Copy link
Contributor

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

@victormarante
Copy link

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

@Gigabyte0x1337
Copy link

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
Copy link

Program.cs.txt

@Lonli-Lokli
Copy link
Author

@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
Copy link

@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
Copy link
Contributor

@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
Copy link

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
Copy link

NikoGJ commented Nov 19, 2023

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

@denisulmer
Copy link

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

@cilerler
Copy link

cilerler commented Nov 21, 2023

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

@julealgon
Copy link
Contributor

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
Copy link

@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 added a commit that referenced this issue Dec 15, 2023
@xuzhg
Copy link
Member

xuzhg commented Dec 15, 2023

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

@xuzhg xuzhg linked a pull request Dec 15, 2023 that will close this issue
@Gigabyte0x1337
Copy link

@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
Copy link

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
Copy link
Contributor

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
Copy link
Member

xuzhg commented Dec 27, 2023

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
Copy link
Member

xuzhg commented Dec 27, 2023

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
Copy link

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

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.

10 participants