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

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

Open
9 tasks
DamianEdwards opened this issue Aug 19, 2021 · 35 comments
Open
9 tasks

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

DamianEdwards opened this issue Aug 19, 2021 · 35 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing Needs: Design This issue requires design work before implementating. triage-focus Add this label to flag the issue for focus at triage
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 19, 2021

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:

  • bind via target type (sync from request non-body sources, this is TryParse in .NET 6)
  • bind via target type (async from request inc. body, this is BindAsync in .NET 6)
  • bind via registered type (inc. overwriting built-in binders), e.g. register IParameterBinder<T> in DI
  • bind per parameter, e.g. ([Binder(typeof(CustomerBinder))]Customer customer) => { }
  • register via DI and accept services from DI
  • have access to method/parameter info (i.e. callsite details, access to the parameter is supported by BindAsync in .NET 6 now)
  • compose with other binders (e.g. composite binders)
  • emit/mutate endpoint metadata, e.g. for OpenAPI
  • AOT friendliness

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:

@DamianEdwards DamianEdwards added this to the .NET 7-Candidate milestone Aug 19, 2021
@DamianEdwards DamianEdwards added feature-minimal-actions Controller-like actions for endpoint routing api-suggestion Early API idea and discussion, it is NOT ready for implementation area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 19, 2021
@mumby0168
Copy link

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

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)

@Pilchie Pilchie added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Aug 20, 2021
@mumby0168
Copy link

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 👍

@rafikiassumani-msft rafikiassumani-msft added triage-focus Add this label to flag the issue for focus at triage Priority:0 Work that we can't release without Cost:L labels Jan 11, 2022
@commonsensesoftware
Copy link

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

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

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

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

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

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

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

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 );

@DamianEdwards DamianEdwards self-assigned this Feb 28, 2023
@xamir82
Copy link

xamir82 commented May 20, 2023

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

aradalvand commented May 20, 2023

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

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

aradalvand commented May 21, 2023

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

davidfowl commented May 21, 2023

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

aradalvand commented May 21, 2023

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

davidfowl commented May 21, 2023

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:

  • Is fast
  • Is flexible

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

@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
@captainsafia captainsafia removed the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jun 6, 2023
@adearriba
Copy link

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

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

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

+1 to @captainsafia 's suggestion.

@adearriba
Copy link

adearriba commented Feb 12, 2024

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();

@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
@grahambunce
Copy link

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

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

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

danielgreen commented Mar 29, 2024 via email

@davidfowl
Copy link
Member

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

@grahambunce
Copy link

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

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

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

@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.

@danroth27 danroth27 added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Apr 2, 2024
@halter73 halter73 removed their assignment Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing Needs: Design This issue requires design work before implementating. triage-focus Add this label to flag the issue for focus at triage
Projects
No open projects
Status: Committed
Development

No branches or pull requests