-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Comments
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(); |
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:
|
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 👍 |
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 As an example, a developer might want to have: api.MapGet( "/hello-world", (ApiVersion apiVersion) => $"Hello world from v{apiVersion}"); |
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). |
One idea is to simply introduce a factory layer to the existing // 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);
} |
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
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 The same would be true for query string. Any thoughts on this or moving this forward in a future version of minimal apis? |
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. |
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
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. |
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? |
Was there some compelling reason this was bumped? I was really looking forward to this for API Versioning. To @davidfowl earlier comments:
I think I'm missing something. First, when would someone really want a composite binder for any parameter
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).
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
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 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 );
My current thinking, which I don't really like, is to provide a hook to resolve the 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 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 ); |
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 |
@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 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 |
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. |
@DamianEdwards Then why take that approach? Why not implement a holistic solution that covers all of these scenarios?
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 Right now, there are two ways to customize parameter binding logic: Declaring either |
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). |
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.
Sure, I definitely wasn't suggesting that.
All I meant by "holistic" was something that would also account for what |
I don't think you were explicitly suggesting it, but the approaches suggested may end up requiring that happen it's not considered.
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). |
What is the status of custom parameter binding right now? Context: |
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. |
Keyed services make it much easier to manage per endpoint/type options and services. +1 to @captainsafia 's suggestion. |
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 Example: RouteGroupBuilder webhookBuilder = app
.MapGroup("webhooks")
.WithNewtonsoftParameterBinder(); |
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. |
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. |
You should be able to do the equivalent of this with the |
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
<#35489 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFY2LVXEO6VV6B6KYSY64DY2V64BAVCNFSM5CNMYJ62U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBSG4ZTKNJTGA2Q>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Not even MVC supports this (likely because it's not common). It would be a different issue than this one. |
@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. |
@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. |
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:
If we did anything here, I would do 3 things:
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 |
@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. |
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)
andValueTask<object> BindAsync(HttpContext httpContext)
on the target type.These are some features of parameter binding to consider:
TryParse
in .NET 6)BindAsync
in .NET 6)IParameterBinder<T>
in DI([Binder(typeof(CustomerBinder))]Customer customer) => { }
BindAsync
in .NET 6 now)Strawman
Example usage:
Related issues:
The text was updated successfully, but these errors were encountered: