-
Notifications
You must be signed in to change notification settings - Fork 161
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
Support Minimal API #1140
base: release-8.x
Are you sure you want to change the base?
Support Minimal API #1140
Conversation
Happy to see some progress on minimal API support @xuzhg Also can you clarify the extent of OData features we support in this PR and what do not support (and do not plan to support). |
Thanks for your feedback. Actually, the product codes in the PR changed less. OData routingIn my understanding, most of customers only want to get the 'OData Query functionalities" when building the minimal API. They don't want to know "Entity Type, Complex Type", or "Entity set, Singleton, Navigation Source, Navigation Property", concepts like these. Even thought they don't want to know about "Key Segement" and It should come after "Collection Segement". So, there's no "OData routing" at all. However, we can add "OData routing" mechanise, for example:
Where, Does this support model binding?In my understanding, ASP.NET Core doesn't add Delta< T > ? DeltaSet< T >I think we should support to bind those types. I also added the comments in my PR to support them. It could be a little bit same as ODataQueryOption< T > implementation. It's not complicated but I think let's do it step by step. Before implementing it, let's finish our first BrainStorm on the initial design. To Be DiscussionOData payload is JSON payload. I think it's time to consider to use |
Honestly... this would be the biggest improvement to OData in years and would avoid so many problems. At the same time... seems ambitious to consider that alongside the Minimal API changes 😅. As much as I want that, I'd avoid doing them together, or potentially even in the same release. |
It would be ABSOLUTELY AMAZING if OData switched to out-of-the-box System.Text.Json serialization with converters. I think it would improve perf by at least 50%, and I wouldn't be surprised if you saw a 200-400% improvement. Probably needs a separate work branch tho. Hopefully the unit tests are designed to be able to handle this. Happy to help wherever possible. |
I agree we stand to gain a lot from leveraging System.Text.Json. But it's not a trivial change given all the scenarios that OData's current serialization supports as well as adherence to the OData spec. However, this is something we're currently looking into. I'll share results from the progress soon. (see: OData/odata.net#2798) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you add tests?
|
||
namespace ODataMiniApi; | ||
|
||
public class MetadataHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is in the sample project, does this mean this is something the customer will have to write, or does this work out of the box?
SkipValidation = false | ||
}; | ||
|
||
// we can't use response body directly since ODL writes the JSON CSDL using Synchronous operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prioritize making this async in ODL?
app.MapGet("/customized/schools", (AppDb db, ODataQueryOptions<School> options) => { | ||
db.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking; | ||
return options.ApplyTo(db.Schools); | ||
}).UseOData("customized"); // In customized OData, MailAddress and Student are complex types, you can also use 'IODataModelConfiguration' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you use IODataModelConfiguration
?
/// </summary> | ||
/// <param name="context">The HttpContext.</param> | ||
/// <param name="builder">The <see cref="ODataModelBuilder">builder</see> used to apply configurations.</param> | ||
void Apply(HttpContext context, ODataModelBuilder builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we envision that more arguments could be needed in the future, such that we could use an ODataModelConfiguratonOptions
class to encapsulate the options, or is an ODataModelBuilder
sufficient?
@@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.OData.Query.Wrapper | |||
/// <summary> | |||
/// Supports converting <see cref="SelectExpandWrapper{T}"/> types by using a factory pattern. | |||
/// </summary> | |||
internal class SelectExpandWrapperConverter : JsonConverterFactory | |||
public class SelectExpandWrapperConverter : JsonConverterFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this made public?
/// A cache for <see cref="Endpoint"/> and <see cref="IEdmModel"/> mapping. | ||
/// It's typically used for minimal API scenario. | ||
/// </summary> | ||
public interface IODataEndpointModelMapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the likelihood that the customer will want to customize this? Maybe we should make it internal until there's explicit/clear demand from the customer to make it extensible, that way it can give us room to change the design without breaking changes.
Is there any update on this pull-request? |
Can this please be merged?! I would really like to use minimal API with OData! |
One thing I have noticed is that the response from the minimal API endpoints just have the "bare" data. Meaning it doesn't have "@odata.context" field and then the result in a "value" field. If there is a way to have this, please include an example. |
That's likely unintentional. That response body is a critical part of the OData protocol and needs to be kept intact. |
Any updates on this? |
What does it take for this PR to be merged? Can someone take care of this? |
Fixes #578
Minimal APIs are a simplified approach for building fast HTTP APIs with ASP.NET Core. You can build fully functioning REST endpoints with minimal code and configuration. Skip traditional scaffolding and avoid unnecessary controllers by fluently declaring API routes and actions. For example, the following code creates an API at the root of the web app that returns the text, "Hello World!".
Customer want to get the OData query options functionalities. We can provide couple solutions
Provide a HttpRequest-less ODataQueryOptions< T > (Without using IEdmModel, using CLR type directly) and customer can use it in the
Delegate
to generate the Linq Expression and apply to the data.Use the existing ODataQueryOptions< T >. It covers two scenarios: 1) EdmModel based 2) Non-EdmModel (but will create EdmModel on the fly)
This PR is to support the second.
Then, client can send request the "/schools" using OData query options, for example,
$select
, etc.