dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

Extend the ability to customize parameter binding for Minimal APIs #35489

Open DamianEdwards opened 3 years ago

DamianEdwards commented 3 years ago

This issue is to discuss future updates to extend the ability to customize parameter binding for Minimal APIs, beyond what's available in .NET 6, i.e. static methods bool TryParse(string input, out T value) and ValueTask<object> BindAsync(HttpContext httpContext) on the target type.

These are some features of parameter binding to consider:

Strawman

public interface IParameterBinderFactory<T>
{
    IParameterBinder<T> Create(IServiceProvider provider, ParameterInfo parameter, MethodInfo method);
}

public interface IParameterBinder<T>
{
    ValueTask<T> BindAsync(HttpContext httpContext);
}

Example usage:

var builder = WebApplication.CreateBuilder();

builder.Services.AddSingleton<IParameterBinderFactory<Customer>, CustomerBinder>();

var app = builder.Build();

app.MapPost("/customers", (Cusomter customer) =>
{
   return Results.Created(customer);
});

public class CustomerBinder : IParameterBinderFactory<Customer>, IParameterBinder<Customer>
{
    public CustomerBinder Create(IServiceProvider provider, ParameterInfo parameter, MethodInfo method)
    {
        // Called at application startup, access to parameter and method here to change behavior based on
        // naming, attributes, etc.
        return new CustomerBinder();
    }

    public async ValueTask<Customer> BindAsync(HttpContext httpContext)
    {
        // Called per invocation of routing delegate, i.e. per request
        // Do whatever custom binding logic desired, including reading from request body, etc.
        return await httpContext.Request.ReadAsJsonAsync<Customer>();
    }
}

Related issues:

mumby0168 commented 3 years ago

This looks cool, assuming this would allow for a generic binder for say route/query parameters that takes an object looks at it's properties and tries to match them based on the route/query parameters?

Then it would be a case doing something like this:

var builder = WebApplication.CreateBuilder();

builder.Services.AddRoutingDelegateParameterBinder<RouteParameterBinder<Foo>>();
builder.Services.AddRoutingDelegateParameterBinder<RouteParameterBinder<Bar1>>();
builder.Services.AddRoutingDelegateParameterBinder<QueryParameterBinder<Bar>>();

var app = builder.Build();
davidfowl commented 3 years ago

This looks cool, assuming this would allow for a generic binder for say route/query parameters that takes an object looks at it's properties and tries to match them based on the route/query parameters?

You get to look at anything, we give you the HttpContext. If you want to use the double generic then that's up to you.

Some thoughts:

mumby0168 commented 3 years ago

This looks cool, assuming this would allow for a generic binder for say route/query parameters that takes an object looks at it's properties and tries to match them based on the route/query parameters?

You get to look at anything, we give you the HttpContext. If you want to use the double generic then that's up to you.

Some thoughts:

  • We need to make sure this can be AOT friendly. Right now, the generic design forces the framework code to also be generic to avoid reflection. This might be fine if we end up using generics everywhere, otherwise we'll need to de-generic this API.

    • If we make the API non-generic then we need to deal with the performance costs of boxing everything.
  • Another downside of making the API generic is that it forces users into using reflection to build composite binders (maybe that's OK?)
  • We need to decide if we're going to allow replacing the built-in builders with these.
  • We need to figure out how to expose metadata from custom binders to api explorer. MVC has a similar problem today (your number 8)
  • Can you fallback to default behavior if you configure one of these binders but the parameter doesn't match the criteria? (e.g missing an attribute)

Yeah, that certainly makes sense, I suppose the thought on generic is worth considering since this is aimed at people new to the language trying to get into C#. This could be a step too far or maybe a really good way to introduce generics as they are in my opinion required fairly early on now with many packages/libraries. Personally always like a generic approach.

On your last point, I would say that if something has been registered then if it fails to match to that method of parsing the incoming request for it to fall back to a default method could add confusion. That is from the viewpoint of when registering a different binder you intended for it to be handled differently.

This is really cool though, would love to get involved when this is ready to be worked on 👍

commonsensesoftware commented 2 years ago

Parameter binding often involves values that come from the request. Are there any thoughts as to how IFeatureCollection might be supported here? Specifically, API Versioning has the IApiVersioningFeature, which contains the resolved ApiVersion by the time it reaches an action. There is no parsing to do at that point. It's current ModelBinder implementation merely calls HttpContext.Features.Get<IApiVersioningFeature>().ApiVersion to provide the value. This is analogous to how CancellationToken works.

As an example, a developer might want to have:

api.MapGet( "/hello-world", (ApiVersion apiVersion) => $"Hello world from v{apiVersion}");
davidfowl commented 2 years ago

Supporting features is interesting, we'd need an attribute. I'm afraid doing it implicitly is impossible because the feature collection isn't available at startup (when the bindings are determined).

DamianEdwards commented 2 years ago

One idea is to simply introduce a factory layer to the existing BindAsync support, so that implementers don't have to manage caching of the reflection types themselves, e.g.:

// Factory per parameter info
interface IBinderFactory<TValue>
{
    public static abstract IBindFromHttp<T> CreateBinder(ParameterInfo parameter, ISerivceProvider serviceProvider);
}

interface IBindFromHttpInstance<TValue>
{
    public abstract ValueTask<TValue?> BindAsync(HttpContext httpContext);
}
waynebrantley commented 2 years ago

The route parameters not being able to bind do a complex object is very impactful. At this point we just leave those in regular controllers to avoid the pain as this is a big step backwards for our needs.

In reading the other ticket, you indicated

we don't want to end up writing a de-serializer the way that MVC does today as it is infinitely complex and fragile.

And that makes sense. Deserializing from json in the body into a complex object is already supported - and this situation is very similar.

As an example - a route like this "/api/someroute/{id:int}/{guid:guid}" can be expressed as JSON like { id: 3, guid: 'someguid' } - and then that can be mapped to a complex object in the same way the body is. No need to write any custom deserialization, etc. Seems like the hard part of pulling the values out of the routes is already coded and could be easily mapped onto a complex object.

The same would be true for query string.

Any thoughts on this or moving this forward in a future version of minimal apis?

davidfowl commented 2 years ago

We added [AsParameters] for this scenario, see https://devblogs.microsoft.com/dotnet/asp-net-core-updates-in-dotnet-7-preview-5/#minimal-api-parameter-binding-for-argument-lists.

davidfowl commented 2 years ago

Specifically, for what you describe above it would look like:

app.MapGet("/api/someroute/{id:int}/{guid:guid}", ([AsParameters]RouteInfo r) =>
{
   ...
});

record struct RouteInfo(int Id, Guid Guid);

It'll treat id and guid like top level parameters and bind them from the route. Or you can be explicit and say [FromRoute], it's up to you.

As an example - a route like this "/api/someroute/{id:int}/{guid:guid}" can be expressed as JSON like { id: 3, guid: 'someguid' } - and then that can be mapped to a complex object in the same way the body is. No need to write any custom deserialization,

Should you also be able to describe an object hierarchy as routes? or query strings? or headers? We kept it simple by allow you to treat an object like top level parameters. This lets you refactor things into classes or structs (less overhead) for cleanliness but doesn't enable complex object serialization from those sources.

waynebrantley commented 2 years ago

I think that is perfect. Nothing complex needed for route binding for us - just simple pocos. Question - was this major work under the hood is it something that could easily be added into our local solution for net 6?

commonsensesoftware commented 2 years ago

Was there some compelling reason this was bumped? I was really looking forward to this for API Versioning.

To @davidfowl earlier comments:

Another downside of making the API generic is that it forces users into using reflection to build composite binders (maybe that's OK?)

I think I'm missing something. First, when would someone really want a composite binder for any parameter T? I'll assume there is some valid scenario that hasn't be articulated. Assuming there are composite binders, in what scenario are all the binders not covariant with a given parameter of T?

We need to decide if we're going to allow replacing the built-in builders with these.

I agree it's little strange to think someone would want to change the built-in binders, but why limit it? The right thing happens out-of-the-box. If you go off the rails, change something, and it breaks things, then caveat emptor (IMHO).

We need to figure out how to expose metadata from custom binders to api explorer.

Do we? I completely support this idea, but there are two ways of looking at. First would simply be another method on the binder implementation. This might be annoying to someone that wants a custom binder, but doesn't care about API Explorer (because they aren't using it). Another approach would be to use a separate interface. A binder implementation can chose to implement both interfaces if they want/need both sets of functionality.

I'm not 100% sure what the input needs to look like - yet, but I can say from experience, the output needs to allow multiple parameters from a single model. For example:

void Explore(ApiDescription apiDescription);

This would be expected to append parameters to ApiDescription.ParameterDescriptions. The ApiDescription itself should provide all of the necessary context to achieve the required result. If a single interface is used, the default implementation (now that it's an option) could simply do nothing.

Can you fallback to default behavior if you configure one of these binders but the parameter doesn't match the criteria? (e.g missing an attribute)

I would say - no. "Do what I say, not what I mean." By changing any default behavior, you've basically stated "I don't want to do it the way you do it. Do it my way." A clear way to make sure fallback behaviors can still be used and/or composed with custom behavior need only ensure that the default binder types are public and can be initiated or extended (e.g. inherited) by 3rd parties.

A Minimal API user expects to be able to declare a method signature in a similar way to how it worked before. Something like:

var orders = app.NewApiVersionSet( "Orders" ).Build();

app.MapGet(
      "/api/orders/{id:int}",
      ( int id, ApiVersion version ) => new Order() { Id = id, Customer = $"John Doe (v{version})" } )
   .Produces<Order>()
   .Produces( 404 )
   .WithApiVersionSet( orders )
   .HasApiVersion( 1.0 );

ApiVersion no longer has a TryParse method of its own because it defers to IApiVersionParser which does. This allows developers to replace the default behavior with their own method of parsing. This is particularly important if someone wants to create a custom ApiVersion (which was a long-time ask). Similarly, ApiVersion no longer cares anything about ASP.NET Core directly. This allows a single implementation across old Web API and the Core implementations. It also opens the option for it to be used by clients (e.g. HttpClient), which has also been a long-time ask. This means that the BindAsync method, as supported, will not work either. I did come up with a way to do a bit of a 🐶 and 🐴 show with BindAsync, but then I realized it falls down with a custom ApiVersion implementation.

My current thinking, which I don't really like, is to provide a hook to resolve the ApiVersion via DI. That will work with the existing mechanics, but it's yucky. Essentially:

public static IApiVersioningBuilder AddRequestedApiVersion( this IApiVersioningBuilder builder )
{
  var services = builder.Services;

  // HACK: yucky, but gives us access to the HttpContext
  services.AddHttpContextAccessor();

  // this is a lie because the current requested API version can be null; however, it seems to be reasonable
  // compromise since the API version will only ever be null in the following scenarios:
  //     
  // 1. No current HttpContext, which is unexpected
  // 2. The requested API version is invalid (e.g. didn't parse)
  // 3. There is no incoming API version
  //     a. This might be allowed
  //     b. The endpoint could be version-neutral
  // 
  // In the context of the RequestDelegate for a Minimal API, the current API version will ONLY ever resolve to null
  // if the API is version-neutral and no version was specified. In that case, why would you declare the API version
  // as a parameter to the API? If the absence of an incoming API version is allowed (say - for legacy reasons), then
  // the server will select an appropriate version. The selected version must be non-null and match an endpoint in
  // order for the RequestDelegate to be called back
  services.TryAddTransient(sp => sp.GetRequiredService<IHttpContextAccessor>().HttpContext?.GetRequestedApiVersion()!);

  return builder;
}

A developer can then choose to opt into this behavior with:

builder.Services.AddApiVersioning().AddRequestedApiVersion();

If they don't want to do that, they can still use the special HttpContext binding support to get the API version via:

var orders = app.NewApiVersionSet( "Orders" ).Build();

app.MapGet(
      "/api/orders/{id:int}",
      ( int id, HttpContext context ) =>
            new Order() { Id = id, Customer = $"John Doe (v{context.GetRequestedApiVersion()})" } )
   .Produces<Order>()
   .Produces( 404 )
   .WithApiVersionSet( orders )
   .HasApiVersion( 1.0 );
captainsafia commented 1 year ago

Triage: We discussed a possible approach to this that would allow users to opt a parameter out of the default binding logic that exists in the RequestDelegateFactory so that they could implement their own binding logic in an endpoint filter.

davidfowl commented 1 year ago

Triage: We discussed a possible approach to this that would allow users to opt a parameter out of the default binding logic that exists in the RequestDelegateFactory so that they could implement their own binding logic in an endpoint filter.

This does need more design, like a strawman approach even. I'm not sure what that would look like for the scenario @commonsensesoftware describes.

DamianEdwards commented 1 year ago

Yep, hence the "Needs: Design" label. I can work with @halter73 to come up with a proposal here, but in a nutshell the idea is to enable endpoint filters to perform custom parameter binding by doing something like the following:

commonsensesoftware commented 1 year ago

@DamianEdwards I'm glad to see the API Explorer is being considered too. I've already had to do some yucky hacks for API Versioning with Minimal APIs because AddEndpointsApiExplorer doesn't completely do the right thing (see #41773). Specifically, the design should provide some level of parity with the behavior of BindingSource. API Versioning uses Custom for the ApiVersion type (just like CancellationToken) because it not only resolves from a HTTP feature, but from an exploration standpoint, it can come from multiple places. This means there is not a 1:1 mapping. The ApiVersion parameter in a delegate could produce multiple ApiParameterDescriptor instances (ex: one from the query string and one from the header). The API Versioning extensions for API Explorer understand how to deal with that. There effectively needs to be an escape hatch for the built-in logic that understands exploration should explicitly not be inferred and will come from some other source.

I'd be curious to put eyes on any type of rough design as soon as you have something to provide feedback on. Whether the process involves adding the filter, or whatever the final mechanism is, API Versioning will want to wire that up automatically for a developer, if possible, to keep things as terse as possible. A developer shouldn't necessarily have to remember to add the filter in every place they declare an ApiVersion parameter in their delegate or method.

Unless you're of the opinion that developer will explicitly have to opt into these filters for specific endpoints, that highlights a couple of additional things:

  1. If a filter for a parameter binding will never be applied (because it's not in the signature), should it be ignored when the Endpoint is finally built? a. Why retain the memory?
  2. If such filtering can be done, will it be done automatically or must it be done by extenders? a. I suspect this would effect the public API surface area
xamir82 commented 1 year ago

This is arguably among the most important features that's currently lacking in Minimal APIs.

It's all too common to run into situations where you don't have access to the actual type of a parameter and therefore you have no way of declaring static TryParse/BindAsync methods on said type, which makes this feature pretty essential. Hopefully it's given priority.

aradalvand commented 1 year ago

@DamianEdwards Would the endpoint filter approach you have in mind allow opting out of the default "parsing logic", if you will, without also opting out of the binding source? Basically the equivalent of TryParse(string input, out T? result)? Because it doesn't seem like it would, judging by your explanation in this comment, but this is a crucial thing.

For example, currently if you have an enum as a query string or route parameter, the incoming values are compared against the enum names case-sensitively — an issue described in #45590 and #48346. In the approach you're proposing, would it be possible to customize that specific logic (make the enum parsing case-insensitive) without having to manually look inside HttpContext.Request.Query or HttpContext.Request.RouteValues? Again, essentially the equivalent of what now TryParse does.

DamianEdwards commented 1 year ago

No, it would only allow a wholesale replacement of the parameter binding logic. Changing specific details of how the built-in binding works would require separate hooks into that logic. I'd suggest logging separate issues to cover those kinds of customizations.

aradalvand commented 1 year ago

@DamianEdwards Then why take that approach? Why not implement a holistic solution that covers all of these scenarios? Correct me if I'm wrong, but this approach is clearly unideal if it fails to account for this use case, which is probably even a more common use case than wanting to re-implement a parameter's entire binding logic from scratch.

I'd suggest logging separate issues to cover those kinds of customizations.

I thought these customizations were tracked by this issue? Are they not? I'm not really talking about anything fundamentally different, just the ability to customize an individual parameter's TryParse (in addition to its BindAsync, which is all your proposal would allow). Isn't that within the scope of what this issue is about?

Right now, there are two ways to customize parameter binding logic: Declaring either BindAsync or TryParse on the parameter's type. If the goal here is to "extend" this — which is quite literally what the title of this issue says — then the solution you're currently suggesting is incomplete as it only "extends" BindAsync and not TryParse.

davidfowl commented 1 year ago

There's a single case that's not accounted for that is already solvable today but it requires wrapping the type or declaring your own type. That's the only case left to be solved. We also don't want to do anything that makes the cost for every parameter expensive just in case somebody wants to change how an existing type is bound. That's why it has to be opt-in, and high performance. The other consideration is that it needs to work with the source generated version of minimal APIs shipping in .NET 8.

Wholistic logic is how we end up with MVC's hard to optimize model binding system (which we really want to avoid recreating).

aradalvand commented 1 year ago

There's a single case that's not accounted for that is already solvable today but it requires wrapping the type or declaring your own type.

Yes, but that is obviously an awkward workaround. Technically all the use cases here (including the ones that will be covered by the endpoint filter approach) are already solvable today by creating wrapper types. The whole point of this proposal, as far as I understand, is to make this more convenient and streamlined.

We also don't want to do anything that makes the cost for every parameter expensive just in case somebody wants to change how an existing type is bound.

Sure, I definitely wasn't suggesting that.

Wholistic logic is how we end up with MVC's hard to optimize model binding system (which we really want to avoid recreating).

All I meant by "holistic" was something that would also account for what TryParse now does on a type basis, on a parameter basis. I really don't think that's an outrageous suggestion.

davidfowl commented 1 year ago

Sure, I definitely wasn't suggesting that.

I don't think you were explicitly suggesting it, but the approaches suggested may end up requiring that happen it's not considered.

All I meant by "holistic" was something that would also account for what TryParse now does on a type basis, on a per-parameter basis. I really don't think that's an outrageous or unreasonable suggestion.

It's not, but we've built at least 4 of these systems in each of the ASP.NET Core frameworks (e.g. SignalR, MVC, Blazor, minimal) and understand the tradeoffs. We're trying to come up with something that:

Those are usually at odds 😄. We understand the scenarios and there are currently workarounds (as "not nice" as they are).

adearriba commented 7 months ago

What is the status of custom parameter binding right now?

Context: I currently depend on a library that has their model exposed using Newtonsoft and just having System.Text.JSON serializer available is a pain. It would be nice to be able to declare which serializer to use per endpoint or endpoint group. That would be a great flexibility to have.

captainsafia commented 7 months ago

What is the status of custom parameter binding right now?

We don't have custom parameter binding support as is outlined in this issue.

@adearriba Do you mind filing a new issue with a description of what you're trying to do? I'm curious about some of the details and wonder if existing APIs might be able to help you out.

davidfowl commented 7 months ago

Keyed services make it much easier to manage per endpoint/type options and services.

+1 to @captainsafia 's suggestion.

adearriba commented 7 months ago

I didn't explain myself correctly. Let me re-phrase:

My service receives webhooks and it depends on a library that has their model exposed using Newtonsoft. This means that when I receive a webhook, it doesn't deserialize the content correctly. Just having System.Text.JSON deserializer available is a pain in these scenarios. It would be nice to be able to declare which deserializer to use per endpoint or endpoint group. That would be a great flexibility to have.

Example:

RouteGroupBuilder webhookBuilder = app
    .MapGroup("webhooks")
    .WithNewtonsoftParameterBinder();
grahambunce commented 5 months ago

Since . NET 8 has been released and we are now onto .NET 9, can I ask about the state of improving the custom binding experience?

We are looking to implement the minimal APIs in our organisation for our migration from .NET 6 to . NET 8 and the current model binding experience in .NET 8 is ..... interesting.

We do a lot of "mixed model" binding. This is where we have a single view model request that uses attributes to control where the model gets populated from, i.e. we can have property X with a [FromHeader] attribute, properly Y with a [FromRoute] attribute and the rest from the body. We then use Data Annotations to validate the whole model, e.g. [Required] a custom [NotDefault] for UUIDs etc.

This was easy to implement in MVC style controllers. We could create a [MixedModelBinding] attribute and annotate our controller methods with this. The attribute simply forced both the BodyModelBinderProvder and ComplexModelBinderProvider to execute. We could enable this though MvcOptions via the ModelBinderProviders.

i.e. with one attribute prefixing the request model on the method, we had an object that would pull in values from wherever it found them, without any need for "proper" custom binders. This was very clean.

Now, in .NET 8, none of this appears to work. The minimal APIs will not accept any binding that isn't from a known list, so even custom binders are not supported.

We are left with having to update all of our simple DTOs with custom BindAsync code. As this code invariably needs to deserialise JSON, we need a static JsonSerializerOptions (to handle casing differences automatically) and because BindAsync must be a static method itself, we can't use inheritance to make this easier.

We now need to write custom binding logic for every DTO, making every DTO significantly more complicated, I have not found a way to make this generic or add this to the pipeline. We cannot use Custom Binders to isolate our binding logic from our DTOs (I accept there is an argument that the binding logic should be with the DTO, but in this case I just disagree with that).

This is a bit of a mess frankly and makes the porting effort far more significant, with essentially boiler plate code in every DTO that really shouldn't be there.

Is there a plan to address this, as this ticket has been open since 2021.

davidfowl commented 5 months ago

We then use Data Annotations to validate the whole model, e.g. [Required] a custom [NotDefault] for UUIDs etc.

There's no support for data annotations validation (yet) in minimal APIs, we hope to bring that in .NET 9. You can use https://github.com/DamianEdwards/MiniValidation in the meantime.

There's no plan to re-build the MVC model binding system but you have some options as you are looking to migrate over:

Bridge the MVC model binding system into minimal APIs

BTW this package is on nuget https://www.nuget.org/packages/MinimalApis.Extensions/.

You are looking at using inheritance but you can also use generics as an alternative. Essentially your model binder is no longer an attribute but a wrapper type that does the binding. That way your DTOs don't need the complex logic.

martincostello commented 5 months ago

We do a lot of "mixed model" binding. This is where we have a single view model request that uses attributes to control where the model gets populated from, i.e. we can have property X with a [FromHeader] attribute, properly Y with a [FromRoute] attribute and the rest from the body.

You should be able to do the equivalent of this with the [AsParameters] attribute, where you can add [FromHeader], [FromRoute] etc. onto your DTOs directly.

danielgreen commented 5 months ago

Is there support (current or planned) for multipart form binding?

For example the first part containing JSON to be automatically deserialized, and subsequent parts containing IFormFiles to be uploaded.

AFAIK at the moment we need to grab the JSON string and manually deserialize in the above scenario.

On Fri, 29 Mar 2024, 15:01 David Fowler, @.***> wrote:

We then use Data Annotations to validate the whole model, e.g. [Required] a custom [NotDefault] for UUIDs etc.

There's no support for data annotations validation (yet) in minimal APIs, we hope to bring that in .NET 9. You can use https://github.com/DamianEdwards/MiniValidation in the meantime.

There's no plan to re-build the MVC model binding system but you have some options as you are looking to migrate over:

Bridge the MVC model binding system into minimal APIs https://github.com/DamianEdwards/MinimalApis.Extensions/blob/main/src/MinimalApis.Extensions/Binding/ModelBinderOfT.cs

BTW this package is on nuget https://www.nuget.org/packages/MinimalApis.Extensions/.

You are looking at using inheritance but you can also use generics as an alternative. Essentially your model binder is no longer an attribute but a wrapper type that does the binding. That way your DTOs don't need the complex logic.

— Reply to this email directly, view it on GitHub https://github.com/dotnet/aspnetcore/issues/35489#issuecomment-2027355305, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFY2LVXEO6VV6B6KYSY64DY2V64BAVCNFSM5CNMYJ62U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBSG4ZTKNJTGA2Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>

davidfowl commented 5 months ago

Not even MVC supports this (likely because it's not common). It would be a different issue than this one.

grahambunce commented 5 months ago

@martincostello Thanks - unfortunately in our case [AsParameters] will not work. It seems to pick up [FromHeader] etc but in our POST/PUTs we have body parameters too. We can only have one [FromBody] but using the current technique we have a flattened structure so that we do not need to map to a specific Body property that could itself be annotated with [FromBody].

We'd need to redesign all our model classes. However you are correct that if we did then it would bind correctly, but I think this exposed a weakness in Model Validation (I have a vague memory of this being the case back in .NET 6 which is why we looked for an alternative) in that the Data Annotations model validation does not pick up body failures on the Body property class, it only picks up failures at the higher level "request" class.

grahambunce commented 5 months ago

@davidfowl Ok. I don't have a problem with re-introducing Data Annotations validation via a empty RouteGroupBuilder, although adding cross-cutting concerns back into the pipeline where the developer decides they necessary should be easier and still aligns with your goal to strip minimal APIs right back ( I think I saw a conversation about this on another ticket).

Having a better way for a letting the developer have the final say on how to bind a model such as re-introducing the Custom Model Binder should be on the roadmap though - the current approach I don't feel works at all well IMO. This would ease the transition for developers with years of learned experiences into the new API pattern, but I know you are aware of this.

I don't know why it wasn't added in the first place - to add the others but not this seems a bit weird.... some AOT reason perhaps?

I will look at that package though, so thanks, and see if we can introduce it to our organisation but, as I'm sure you are aware, relying on open source functionality for something that previously existed isn't always accepted by the engineering departments.

davidfowl commented 5 months ago

I think there are a couple of concrete things here that we can do to improve things. This issue is about making so that there's no wrapper type with a BindAsync but that's just the mechanism for you tell us that you want to do model binding. With keyed services we can improve this mechanism.

Once you are able to decide that you want to "hook" model binding, there's no way to invoke the default logic to do anything complex (like the complex model binder). We're not going to rebuild that because:

  1. It already exists in MVC. You can always use if directly if you choose to build models for that sort of system.
  2. It will not work with AOT (you guess it!). The model binding system is a full de-serializer and making it AOT friendly is a similar level of effort as making the JSON serializer AOT friendly.
  3. The model binding system is recursive; you can invoke registered binders from other binders. This is incredible flexible but also adds lots of complexity and overhead for simple cases.

If we did anything here, I would do 3 things:

  1. Make it so that you can add a "binder" surrogate via a keyed service. This is just another way of declaring you want to take over.
  2. Make it so you can invoke the default binder logic we have in some way (composition).
  3. Let you bridge MVC's model binding system into minimal APIs (with all the caveats mentioned above).

Most applications don't need to model their DTOs this way, MVC was powerful in that it allowed developers to build this (we call it a framework for building frameworks). Minimal APIs has a different philosophy and doesn't want to have lots of extensibility points as it prevents us from optimizing what we consider the 90% case.

Take a look at https://www.nuget.org/packages/MinimalApis.Extensions/, and see if you can use the ModelBinder<T> for your scenario.

commonsensesoftware commented 5 months ago

@davidfowl any rough idea of when or if the required effort will get any consideration for .NET 9 or even .NET 10? If it might be considered, it would be nice to line up feature and release plans. 😉

You may or may not have reviewed the new high-level proposal I put forth in #50672. I know you're busy looking at a gazillion other proposals so I'm not going to pester you about it. It's a reimaging of an earlier proposal in #45525 that we decided didn't fit the bill and closed out. I had forgotten about the first proposal when I created the second one. After studying the source of how binding works in detail again, the new proposal should be much closer to the same approach that the built-in binding mechanisms use. It should also be amenable to source generation and AOT, albeit probably with some additional revisions.