Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Remove DefaultValue support from model binding and do it only in the invoker for action parameters #2414

Closed
rynowak opened this issue Apr 20, 2015 · 9 comments

Comments

@rynowak
Copy link
Member

rynowak commented Apr 20, 2015

MutableObjectModelBinder performs reflection to look up properties and their default values.

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs#L298

We should move this into ModelMetadata so it can be cached, and so that the model binding system is more data driven.

@rynowak rynowak changed the title Moved DefaultValue handling into ModelMetadata, remove reflection from MutableObjectModelBinder Move DefaultValue handling into ModelMetadata, remove reflection from MutableObjectModelBinder Apr 20, 2015
@rynowak
Copy link
Member Author

rynowak commented Apr 21, 2015

We should also consider just removing this code.

See: https://msdn.microsoft.com/en-us/library/system.componentmodel.defaultvalueattribute%28v=vs.110%29.aspx

A DefaultValueAttribute will not cause a member to be automatically initialized with the attribute's value.
You must set the initial value in your code.

@rynowak
Copy link
Member Author

rynowak commented Apr 21, 2015

Legacy info:

MVC5 and WebAPI2 respected [DefaultValue(...)] applied to a parameter of an action method.

MVC futures and WebAPI2 respected [DefaultValue(...)] for binding model properties.

@danroth27 danroth27 added this to the 6.0.0-beta5 milestone Apr 23, 2015
@Eilon Eilon changed the title Move DefaultValue handling into ModelMetadata, remove reflection from MutableObjectModelBinder Remove DefaultValue support from model binding and do it only in the invoker for action parameters Apr 24, 2015
@Eilon
Copy link
Member

Eilon commented Apr 24, 2015

Remove DefaultValue support from model binding and do it only in the invoker for action parameters.

rynowak added a commit that referenced this issue May 14, 2015
This part of the change removes default value support from ModelBinding.

Updated some unit tests to verify that it does nothing in that case.
Deleted a functional test as it was pure duplication of another
(supported) case where the property has a pre-initialized value.
@rynowak
Copy link
Member Author

rynowak commented May 14, 2015

Remove DefaultValue support from model binding and do it only in the invoker for action parameters.

Does this include bound-controller-properties? This affects the shape of the implementation.

Is the goal to limit the spread of a feature that we need for compat? Or to be consistent?

@Eilon
Copy link
Member

Eilon commented May 14, 2015

Bound controller properties are an interesting case. They're more akin to action parameters, so that makes it more interesting.

Having said that, I'm fine not supporting DefaultValue on controller properties for now, and we could add that in the future if there's demand.

@rynowak
Copy link
Member Author

rynowak commented May 14, 2015

The interesting thing about doing it on both properties/parameters is that the implementation concerns would probably dictate that it be first-class at that point -- ParameterModel, ParameterDescriptor, customizing via convention, etc.

Seems to me like that's a bit overkill, will just do it at a low-level for now and just for parameters 👍

@Eilon
Copy link
Member

Eilon commented May 14, 2015

Yeah that sounds like the best plan for now. Thanks!

@rynowak
Copy link
Member Author

rynowak commented May 14, 2015

Part 1 checked in: 2fc983b

@rynowak
Copy link
Member Author

rynowak commented May 14, 2015

Part 2 checked in: cc4ee10

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants