Adding support for ModelBinderAttribute in core. #1633

harshgMSFT · 2014-11-25T04:41:01Z

This change introduces ModelBinderAttribute which can be used to specify a specific IModelBinder type or a specific IModelBinderProvider type. The attribute can also be used to provide a ModelNamePrefix. The attribute is allowed on types as well as parameters.
The "new" ModelBinderAttribute behaves in all respects like webapi except one.

There is no SuppressPrefixCheck property on the attribute in the current implementation. The rationale for this is as follows :
SuppressPrefixCheck is Only applicable for the following case ( no binderType specified on parameter but a binder type of ModelBinder on the type.

// Notice no type is provided on the parameter's ModelBinder attribute.
void Action([ModelBinder] Person p);

// SuppressPrefixCheck only works if the binder type specified is of type IModelBinder 
// (and not IModelBinderProvider).
[ModelBinder(typeof(TestModelBinder), SuppresPrefixCheck=true)]
public class Person
{
}

What this results is before calling the TestModelBinder it will skip the check if there is a value provider which can provide a value.
IMHO this is a entirely unnecessary. @yishaigalatzer please share your thoughts.

harshgMSFT · 2014-11-25T04:41:27Z

cc : @rynowak @dougbu @pranavkm

dougbu · 2014-11-25T23:47:15Z

would appreciate a high-level description of the change, preferably in the PR comment above. right now I see some refactoring, a new property or two, and lots of "custom" things. couldn't tell you what's being customized.

harshgMSFT · 2014-11-26T00:23:43Z

@dougbu, I thought the code (with its detailed comments/summaries) was descriptive :) added a few comments in PR description. If this does not help lets sync offline.

rynowak · 2014-11-27T00:22:40Z

src/Microsoft.AspNet.Mvc.Core/ModelBinders/CustomModelBinder.cs

+    /// Represents an <see cref="IModelBinder"/> which understands <see cref="ICustomModelBinderMetadata"/> and uses
+    /// the supplied <see cref="IModelBinder"/> bind the model.
+    /// </summary>
+    public class CustomModelBinder : MetadataAwareBinder<ICustomModelBinderMetadata>


I'm with @dougbu - not liking the naming. However this is all internal to the framework so it can be cleaned up whenever.

rynowak · 2014-11-27T01:36:51Z

⌚

rynowak · 2014-12-04T00:19:13Z

@pranavkm updated

pranavkm · 2014-12-04T00:32:22Z

src/Microsoft.AspNet.Mvc.Core/Resources.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>


Revert changes to this file.

pranavkm · 2014-12-04T01:19:01Z

⌚

harshgMSFT · 2014-12-04T16:55:58Z

src/Microsoft.AspNet.Mvc.ModelBinding/Binders/BinderTypeBasedModelBinder.cs

+
+        public async Task<bool> BindModelAsync(ModelBindingContext bindingContext)
+        {
+            if (bindingContext.ModelMetadata.BinderType == null)


instead of doing this should this model binder derive from MetadataAwareModelBinder and follow the pattern which is already there?

This check would still be necessary though, to support a no op ModelBinderAttribute

instead of doing this should this model binder derive from MetadataAwareModelBinder and follow the pattern which is already there?

This should work one of two ways (convince me that I'm wrong):

The binder type is not part of model metadata and is instead part of some IBinderTypeBinderMetadata interface. This class only looks for a binder metadata and looks at the property on it.

The binder type is part of model metadata. This class only looks at model metadata (my implementation)

The previous code in this PR mixed these two, and it's unnecessary coupling. It also breaks scenarios where a custom MMP sets a binder type.

I chose 2. above because I think it results in the best layering based on the current design.

The only problem that I see in the current code is IBinderTypeProviderMetadata derives from IBinderMetadata which means that we are mixing 1 and 2. In the previous implementation ( my implementation ) there were two separate interfaces one for the type provider and one which inherited for IBinderMetadata.
Not sure I understand the reason for IBinderTypeProviderMetadata implementing IBinderMetadata when we are implementing option2.

~~Ok, will remove IBinderMetadata from it then.~~

Actually, this is needed too. The problem is that without this marker being an IBinderMetadata than webapi shim will do wierd things - like override the behavior you set via the attribute.

This is just a bigger issue of having all of the binding concerns be based on ModelMetadata. It's the design we have for now, but it's not great. We should revisit in the future.

rynowak · 2014-12-05T00:43:08Z

@pranavkm updated please take a look.

Enabled [ModelBinder] on properties (for view models) and added a functional test for the same.

pranavkm · 2014-12-05T19:18:52Z

src/Microsoft.AspNet.Mvc.ModelBinding/BinderMetadata/ModelBinderAttribute.cs

+        AttributeTargets.Parameter |
+
+        // Support properties on model DTOs.
+        AttributeTargets.Property |


pranavkm · 2014-12-05T19:30:41Z

Looks good apart from comments.

dougbu · 2014-12-05T21:53:48Z

src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedDataAnnotationsMetadataAttributes.cs

@@ -36,6 +37,12 @@ public CachedDataAnnotationsMetadataAttributes(IEnumerable<object> attributes)
        }

        /// <summary>
+        /// Gets (or sets in subclasses) <see cref="IEnumerable{IBinderTypeProviderMetadata}"/> found in collection passed


teensy nit; is this a long line?

dougbu · 2014-12-05T22:02:08Z

⌚ needs another PR adding the appropriate tests for new properties etc. in ModelMetadata hierarchy

harshgMSFT force-pushed the MetadataProviderCleanup branch 2 times, most recently from 5ee8068 to 89b319a Compare November 25, 2014 06:12

harshgMSFT force-pushed the ModelBindAttribute branch from 27b92e1 to b01b057 Compare November 25, 2014 06:17

rynowak reviewed Nov 27, 2014
View reviewed changes

rynowak changed the title ~~Adding support for ModelMetadataAttribute in core.~~ Adding support for ModelBinderAttribute in core. Dec 3, 2014

rynowak force-pushed the MetadataProviderCleanup branch from c22f3e7 to 91549e1 Compare December 3, 2014 20:54

rynowak force-pushed the ModelBindAttribute branch 3 times, most recently from 5dd3c34 to a9f0661 Compare December 4, 2014 00:18

pranavkm reviewed Dec 4, 2014
View reviewed changes

harshgMSFT reviewed Dec 4, 2014
View reviewed changes

rynowak force-pushed the ModelBindAttribute branch 2 times, most recently from 9aa9256 to 60ff84e Compare December 4, 2014 22:44

rynowak force-pushed the ModelBindAttribute branch from 60ff84e to acb1710 Compare December 5, 2014 00:31

rynowak force-pushed the ModelBindAttribute branch from acb1710 to 9eb59f8 Compare December 5, 2014 19:12

pranavkm reviewed Dec 5, 2014
View reviewed changes

rynowak force-pushed the ModelBindAttribute branch from 9eb59f8 to b818cc0 Compare December 5, 2014 19:54

Adding support for ModelMetadataAttribute in core.

e9bcc3f

rynowak force-pushed the ModelBindAttribute branch from b818cc0 to e9bcc3f Compare December 5, 2014 20:00

rynowak closed this Dec 5, 2014

rynowak deleted the ModelBindAttribute branch December 5, 2014 20:11

dougbu reviewed Dec 5, 2014
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for ModelBinderAttribute in core. #1633

Adding support for ModelBinderAttribute in core. #1633

harshgMSFT commented Nov 25, 2014

harshgMSFT commented Nov 25, 2014

dougbu commented Nov 25, 2014

harshgMSFT commented Nov 26, 2014

rynowak Nov 27, 2014

rynowak commented Nov 27, 2014

rynowak commented Dec 4, 2014

pranavkm Dec 4, 2014

pranavkm commented Dec 4, 2014

harshgMSFT Dec 4, 2014

harshgMSFT Dec 4, 2014

rynowak Dec 4, 2014

rynowak Dec 4, 2014

harshgMSFT Dec 5, 2014

rynowak Dec 5, 2014

rynowak commented Dec 5, 2014

pranavkm Dec 5, 2014

pranavkm commented Dec 5, 2014

dougbu Dec 5, 2014

dougbu commented Dec 5, 2014

Adding support for ModelBinderAttribute in core. #1633

Adding support for ModelBinderAttribute in core. #1633

Conversation

harshgMSFT commented Nov 25, 2014

harshgMSFT commented Nov 25, 2014

dougbu commented Nov 25, 2014

harshgMSFT commented Nov 26, 2014

Choose a reason for hiding this comment

rynowak commented Nov 27, 2014

rynowak commented Dec 4, 2014

Choose a reason for hiding this comment

pranavkm commented Dec 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Dec 5, 2014

Choose a reason for hiding this comment

pranavkm commented Dec 5, 2014

Choose a reason for hiding this comment

dougbu commented Dec 5, 2014