Aspnetcore: Consistency across binding attributes

[rynowak]

Summary

MVC has a few different flavours of binding attributes and they each do different things, largely for legacy reasons - but they also overlap in some ways which leads a lot of confusion. We've tried to keep them orthogonal in the past, but they didn't start that way, so maybe we should stop trying?

Could we think about making them consistent and maybe deprecating some? Time horizon for this is long-term. I want to come up with a plan for the zoo of current stuff.

Existing Stuff

We have the following attributes:

[Modelbinder]
[Bind]
[BindProperty]
[FromBody]/[FromHeader]/[FromServices]/[FromRoute]/[FromQuery]/[FromForm]
[BindRequired]/[BindNever]/[BindingBehavior] // these are hidden away  in the .ModelBinding namespace for some reason

We also have the following policies that these attributes can configure (see BindingInfo):

• Binding Source (where do we look for data) IBindingSourceMetadata
• Model Name (what's the key-space) IModelNameProvider
• Binder Type (specifies a model binder to use by-type) IBinderTypeProviderMetadata
• Property Filters (Include) IPropertyFilterProvider
• Request Predicate (filter by request type) IRequestPredicateProvider
• Binding Behavior (never, required) - this is actually part of model metadata which is wierd - BindingBehaviorAttribute

Here's a handy table:

| | Targets | Binding Source | Model Name | Binder Type | Property Filter | Request Filter | Binding Behavior |
|---------------- |--------------------- |---------------- |------------ |------------- |----------------- |---------------- |------------------ |
| [ModelBinder] | All | X | X | X | | | |
| [Bind] | Type, Parameter | | X | | X | | |
| [BindProperty] | Property | X | X | X | | X | |
| [FromRoute] | Parameter, Property | X | X | | | | |
| [BindRequired] | All | | | | | | X |

Some random thoughts

[ModelBinder], [Bind] and the [BindRequired] family all have their root in our legacy systems, but only [ModelBinder] is truly compatible with MVC5. We cut exclude support from [Bind] and the [BindRequired] attribute used to be called [HttpBindRequired]. This makes me sad because it represents a missed opportunity 👎

The [FromXyz] is new as of ASP.NET Core and is mostly intended to for APIs.

[BindProperty] is a new feature that was added for Razor Pages. It's special trick is that it doesn't bind on GET requests by default IRequestPredicateProvider

[BindRequired] and friends don't support any real customization of the model binding system. They are kinda separate.

[rynowak]

Given all of this data, here are some proposals.

Short Term

[BindProperty] should add support for IPropertyFilterProvider via an string[] Include { get; set; } property and potentially a constructor.

[Bind] should add support for IBindingSourceMetadata, IBinderTypeProviderMetadata and IRequestPredicateProvider. The implementation of IRequestPredicateProvider should be the same as it is for [BindProperty] - but the default should be SupportsGet=true.

We should consider obsoleting [ModelBinder] or using analyzers to suggest that you use [Bind] instead.

We should leave the [FromXyz] attributes alone.

We should leave [BindRequired] alone.

These changes just require updating the attributes themselves. The underlying systems are not tied to the attributes, they use the metadata interfaces.

Rationale

We identified a gap this week where [Bind] doesn't apply to properties, but [BindProperty] doesn't have IPropertyFilterProvider. We should close this gap because the ViewModel pattern works for everything, but the Include=... pattern doesn't work for [BindProperty].

The fundamental difference between [Bind] and [BindProperty] is that properties are always opt-in. If you use [BindProperty] that means that GET requests require a second opt-in. I think this is good - it's meant to avoid pain by not trying to validate models when the form is shown the first time.

For this reason I like [Bind] not applying to properties - we don't have to explain why it applies to both properties and parameters, and we don't have to explain why it also has a different default behavior for GET requests. I think trying to make the two truly interchangeable is a fools errand.

I'd like to leave the [FromXyz] attributes alone because they are being de-emphasized for APIs, you should only need them sparingly. The features that they lack would be very very rarely needed in an API.

I'd like to take the [BindRequired] kinds of things out of this discussion. They are orthogonal with everything else, and In another world they would have better names.

Long Term

As part of Project Houdini we're looking at building a separate binding system outside of the context of MVC. If we're willing to keep the [Bind] design we can ship something that drop in pretty well as a compatible replacement. I would consider making [Bind] in the new world always require an opt-in to bind on GET when applied to a property.

I would first consider making an obvious and clear separation between attributes that apply to types and attributes that apply to properties or parameters. MVC has all of this with the same set of attributes, and it's very ambiguous what they do. This isn't a place to be frugal and try to reuse things.

In this new system, I would nuke [ModelBinder] from orbit.

I would keep the [FromXyz] attributes, but I would make them subclasses of [Bind]. If you ignore the extra features, they cost you nothing.

[Rick-Anderson]

We cut exclude support from [Bind] and the [BindRequired]

IIRC we cut blacklisting (Exclude ) because it's less secure than whitelisting (Include)

As the schema evolves, a new secret prop may be added and not protected by the blacklist. Is that right @blowdart

[blowdart]

Yup. I wanted a strict binder where you had to specify everything, but apparently this was too mean. Of course really everything should be view model based.

@rynowak How would you feel about throwing if someone bound to a class used in a dbcontext? 👿

[rynowak]

I'm open to discussions like these if we reinvent this stuff in the far future. Please note that anything which adds requirements needs to also be easier to use correctly. If we do features that make view models required, we have to do additional features that make it trivial to use.

[Rick-Anderson]

Don't bring back (Exclude ), it's less secure and white-listing works.

+1 In strict mode, throw if "if someone bound to a class used in a dbcontext'

cc @JonPSmith @tdykstra

[RehanSaeed]

There is a nice blog post on how you can force the [Required] attribute to also behave like [BindRequired], thus removing the need to use [BindRequired] completely. Also, this has the added benefit of removing the dependency on MVC, since [Required] is in the System namespace.

[mkArtakMSFT]

Pushing the near-term fix out by a release. For the long term fix we'll discuss separately later.

[dougbu]

// these are hidden away in the .ModelBinding namespace for some reason

Think the decision in #7351 was to leave this alone.

The [FromXyz] is new as of ASP.NET Core and is mostly intended to for APIs.

Not exactly new since these many (most?) of these attributes exist in Web API.

[BindProperty] should add support for IPropertyFilterProvider via an string[] Include { get; set; } property and potentially a constructor.

Why not place [BindProperty] on the property or properties you'd list in Include? Not sure why we'd add an additional way to include properties.

[Bind] should add support for IBindingSourceMetadata, IBinderTypeProviderMetadata and IRequestPredicateProvider.

This part of the proposal also seems to worsen the original "binding attributes … overlap in some ways which leads a lot of confusion" objection. What's the short term mitigation for that additional confusion?

[pranavkm]

To add on to this, BindProperty is allowed on types, but it only works on page handlers. Would be yet another thing we need to add to make consistent.