Making Pages Binding Consistent

This changeset reckonciles the binding work we did for pages with
controllers.

A quick summary:
- Moves [BindProperty] to the MVC namespace (#6401)
- Makes [FromRoute] and friends behave consistently (#6402)
- Makes [BindProperty] work with controllers (untracked)

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/BindingInfo.cs

@@ -34,6 +34,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             BinderModelName = other.BinderModelName;
             BinderType = other.BinderType;
             PropertyFilterProvider = other.PropertyFilterProvider;
+            RequestPredicate = other.RequestPredicate;
         }
 
         /// <summary>
@@ -56,6 +57,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
         /// </summary>
         public IPropertyFilterProvider PropertyFilterProvider { get; set; }
 
+        /// <summary>
+        /// Gets or sets a predicate which determines whether or not the model should be bound based on state
+        /// from the current request.
+        /// </summary>
+        public Func<ActionContext, bool> RequestPredicate { get; set; }
+
         /// <summary>
         /// Constructs a new instance of <see cref="BindingInfo"/> from the given <paramref name="attributes"/>.
         /// </summary>
@@ -113,6 +120,17 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 bindingInfo.PropertyFilterProvider = new CompositePropertyFilterProvider(propertyFilterProviders);
             }
 
+            // RequestPredicate
+            foreach (var requestPredicateProvider in attributes.OfType<IRequestPredicateProvider>())
+            {
+                isBindingInfoPresent = true;
+                if (requestPredicateProvider.RequestPredicate != null)
+                {
+                    bindingInfo.RequestPredicate = requestPredicateProvider.RequestPredicate;
+                    break;
+                }
+            }
+
             return isBindingInfoPresent ? bindingInfo : null;
         }
 

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/IRequestPredicateProvider.cs

@@ -0,0 +1,20 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding
+{
+    /// <summary>
+    /// An interface that allows a top-level model to be bound or not bound based on state associated
+    /// with the current request.
+    /// </summary>
+    public interface IRequestPredicateProvider
+    {
+        /// <summary>
+        /// Gets a function which determines whether or not the model object should be bound based
+        /// on the current request.
+        /// </summary>
+        Func<ActionContext, bool> RequestPredicate { get; }
+    }
+}

src/Microsoft.AspNetCore.Mvc.Core/BindPropertyAttribute.cs

@@ -4,11 +4,15 @@
 using System;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 
-namespace Microsoft.AspNetCore.Mvc.RazorPages
+namespace Microsoft.AspNetCore.Mvc
 {
     [AttributeUsage(AttributeTargets.Property | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
-    public class BindPropertyAttribute : Attribute, IModelNameProvider, IBinderTypeProviderMetadata
+    public class BindPropertyAttribute : Attribute, IModelNameProvider, IBinderTypeProviderMetadata, IRequestPredicateProvider
     {
+        private static readonly Func<ActionContext, bool> _supportsAllRequests = (c) => true;
+
+        private static readonly Func<ActionContext, bool> _supportsNonGetRequests = IsNonGetRequest;
+
         private BindingSource _bindingSource;
 
         public bool SupportsGet { get; set; }
@@ -35,5 +39,18 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages
 
         /// <inheritdoc />
         public string Name { get; set; }
+
+        Func<ActionContext, bool> IRequestPredicateProvider.RequestPredicate
+        {
+            get
+            {
+                return SupportsGet ? _supportsAllRequests : _supportsNonGetRequests;
+            }
+        }
+
+        private static bool IsNonGetRequest(ActionContext context)
+        {
+            return !string.Equals(context.HttpContext.Request.Method, "GET", StringComparison.OrdinalIgnoreCase);
+        }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs

@@ -152,6 +152,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 throw new ArgumentNullException(nameof(metadata));
             }
 
+            if (parameter.BindingInfo?.RequestPredicate?.Invoke(actionContext) == false)
+            {
+                return ModelBindingResult.Failed();
+            }
+
             var modelBindingContext = DefaultModelBindingContext.CreateBindingContext(
                 actionContext,
                 valueProvider,

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageBoundPropertyDescriptor.cs

@@ -9,7 +9,5 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
     public class PageBoundPropertyDescriptor : ParameterDescriptor
     {
         public PropertyInfo Property { get; set; }
-
-        public bool SupportsGet { get; set; }
     }
 }

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs

@@ -248,26 +248,31 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                 var property = properties[i];
                 var bindingInfo = BindingInfo.GetBindingInfo(property.Property.GetCustomAttributes());
 
+                // If there's no binding info then that means there are no model binding attributes on the
+                // property. So we won't bind this property unless there's a [BindProperty] on the type.
                 if (bindingInfo == null && bindPropertyOnType == null)
                 {
                     continue;
                 }
 
+                // If this property is declared as part of a pages base class, then it's likely infrastructure,
+                // so skip it.
                 if (property.Property.DeclaringType.GetTypeInfo().IsDefined(typeof(PagesBaseClassAttribute)))
                 {
                     continue;
                 }
 
-                var bindPropertyOnProperty = property.Property.GetCustomAttribute<BindPropertyAttribute>();
-                var supportsGet = bindPropertyOnProperty?.SupportsGet ?? bindPropertyOnType?.SupportsGet ?? false;
+                bindingInfo = bindingInfo ?? new BindingInfo();
+
+                // Allow a predicate on the class to cascade if it wasn't set on the property.
+                bindingInfo.RequestPredicate = bindingInfo.RequestPredicate ?? ((IRequestPredicateProvider)bindPropertyOnType)?.RequestPredicate;
 
                 var descriptor = new PageBoundPropertyDescriptor()
                 {
-                    BindingInfo = bindingInfo ?? new BindingInfo(),
+                    BindingInfo = bindingInfo,
                     Name = property.Name,
                     Property = property.Property,
                     ParameterType = property.Property.PropertyType,
-                    SupportsGet = supportsGet,
                 };
 
                 results.Add(descriptor);

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PagePropertyBinderFactory.cs

@@ -7,7 +7,6 @@ using System.Threading.Tasks;
 using Microsoft.AspNetCore.Mvc.Abstractions;
 using Microsoft.AspNetCore.Mvc.Internal;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
-using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure;
 
 namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
 {
@@ -56,16 +55,9 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
             IList<ParameterDescriptor> properties,
             IList<ModelMetadata> metadata)
         {
-            var isGet = string.Equals("GET", pageContext.HttpContext.Request.Method, StringComparison.OrdinalIgnoreCase);
-
             var valueProvider = await CompositeValueProvider.CreateAsync(pageContext, pageContext.ValueProviderFactories);
             for (var i = 0; i < properties.Count; i++)
             {
-                if (isGet && !((PageBoundPropertyDescriptor)properties[i]).SupportsGet)
-                {
-                    continue;
-                }
-
                 var result = await parameterBinder.BindModelAsync(pageContext, valueProvider, properties[i]);
                 if (result.IsModelSet)
                 {

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerBinderDelegateProviderTest.cs

@@ -13,6 +13,7 @@ using Microsoft.AspNetCore.Mvc.Formatters;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
+using Microsoft.AspNetCore.Mvc.RazorPages;
 using Microsoft.AspNetCore.Routing;
 using Moq;
 using Xunit;
@@ -490,6 +491,125 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             Assert.Null(controller.NullableProperty);
         }
 
+        [Fact]
+        public async Task BindActionArgumentsAsync_SupportsRequestPredicate_ForPropertiesAndParameters_NotBound()
+        {
+            // Arrange
+            var actionDescriptor = GetActionDescriptor();
+
+            actionDescriptor.Parameters.Add(new ParameterDescriptor
+            {
+                Name = "test-parameter",
+                BindingInfo = new BindingInfo()
+                {
+                    BindingSource = BindingSource.Custom,
+
+                    // Simulates [BindProperty] on a parameter
+                    RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute()).RequestPredicate,
+                },
+                ParameterType = typeof(string)
+            });
+
+            actionDescriptor.BoundProperties.Add(new ParameterDescriptor
+            {
+                Name = nameof(TestController.NullableProperty),
+                BindingInfo = new BindingInfo()
+                {
+                    BindingSource = BindingSource.Custom,
+
+                    // Simulates [BindProperty] on a property
+                    RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute()).RequestPredicate,
+                },
+                ParameterType = typeof(string)
+            });
+
+            var controllerContext = GetControllerContext(actionDescriptor);
+            controllerContext.HttpContext.Request.Method = "GET";
+
+            var controller = new TestController();
+            var arguments = new Dictionary<string, object>(StringComparer.Ordinal);
+
+            var binder = new StubModelBinder(ModelBindingResult.Success(model: null));
+            var factory = GetModelBinderFactory(binder);
+            var parameterBinder = GetParameterBinder(factory);
+
+            // Some non default value.
+            controller.NullableProperty = -1;
+
+            // Act
+            var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate(
+                parameterBinder,
+                factory,
+                TestModelMetadataProvider.CreateDefaultProvider(),
+                actionDescriptor);
+
+            await binderDelegate(controllerContext, controller, arguments);
+
+            // Assert
+            Assert.Equal(-1, controller.NullableProperty);
+            Assert.DoesNotContain("test-parameter", arguments.Keys);
+        }
+
+        [Fact]
+        public async Task BindActionArgumentsAsync_SupportsRequestPredicate_ForPropertiesAndParameters_Bound()
+        {
+            // Arrange
+            var actionDescriptor = GetActionDescriptor();
+
+            actionDescriptor.Parameters.Add(new ParameterDescriptor
+            {
+                Name = "test-parameter",
+                BindingInfo = new BindingInfo()
+                {
+                    BindingSource = BindingSource.Custom,
+
+                    // Simulates [BindProperty] on a parameter
+                    RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute()).RequestPredicate,
+                },
+                ParameterType = typeof(string)
+            });
+
+            actionDescriptor.BoundProperties.Add(new ParameterDescriptor
+            {
+                Name = nameof(TestController.NullableProperty),
+                BindingInfo = new BindingInfo()
+                {
+                    BindingSource = BindingSource.Custom,
+
+                    // Simulates [BindProperty] on a property
+                    RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute()).RequestPredicate,
+                },
+                ParameterType = typeof(string)
+            });
+
+            var controllerContext = GetControllerContext(actionDescriptor);
+            controllerContext.HttpContext.Request.Method = "POST";
+
+            var controller = new TestController();
+            var arguments = new Dictionary<string, object>(StringComparer.Ordinal);
+
+            var binder = new StubModelBinder(ModelBindingResult.Success(model: null));
+            var factory = GetModelBinderFactory(binder);
+            var parameterBinder = GetParameterBinder(factory);
+
+            // Some non default value.
+            controller.NullableProperty = -1;
+
+            // Act
+            var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate(
+                parameterBinder,
+                factory,
+                TestModelMetadataProvider.CreateDefaultProvider(),
+                actionDescriptor);
+
+            await binderDelegate(controllerContext, controller, arguments);
+
+            // Assert
+            Assert.Null(controller.NullableProperty);
+            Assert.Contains("test-parameter", arguments.Keys);
+            Assert.Null(arguments["test-parameter"]);
+        }
+
         // property name, property type, property accessor, input value, expected value
         public static TheoryData<string, Type, Func<object, object>, object, object> SkippedPropertyData
         {

test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs

@@ -728,7 +728,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
         public async Task PagePropertiesAreNotBoundInGetRequests()
         {
             // Arrange
-            var expected = "Id = 11, Name = , Age =";
+            var expected = "Id = 11, Name = Test-Name, Age =";
             var validationError = "The Name field is required.";
             var request = new HttpRequestMessage(HttpMethod.Get, "Pages/PropertyBinding/PageWithPropertyAndArgumentBinding?id=11")
             {

test/Microsoft.AspNetCore.Mvc.IntegrationTests/BindPropertyIntegrationTest.cs

@@ -0,0 +1,101 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc.Abstractions;
+using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.Extensions.Primitives;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.IntegrationTests
+{
+    // Integration tests for binding top level models with [BindProperty]
+    public class BindPropertyIntegrationTest
+    {
+        private class Person
+        {
+            public string Name { get; set; }
+        }
+
+        [Fact]
+        public async Task BindModelAsync_WithBindProperty_BindsModel_WhenRequestIsNotGet()
+        {
+            // Arrange
+            var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
+            var parameter = new ParameterDescriptor()
+            {
+                Name = "parameter",
+                ParameterType = typeof(Person),
+                BindingInfo = BindingInfo.GetBindingInfo(new[] { new BindPropertyAttribute() }),
+            };
+
+            var testContext = ModelBindingTestHelper.GetTestContext(request =>
+            {
+                request.Method = "POST";
+                request.QueryString = new QueryString("?parameter.Name=Joey");
+            });
+
+            // Act
+            var result = await parameterBinder.BindModelAsync(parameter, testContext);
+
+            // Assert
+            Assert.True(result.IsModelSet);
+
+            Assert.Equal("Joey", Assert.IsType<Person>(result.Model).Name);
+        }
+
+        [Fact]
+        public async Task BindModelAsync_WithBindProperty_SupportsGet_BindsModel_WhenRequestIsGet()
+        {
+            // Arrange
+            var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
+            var parameter = new ParameterDescriptor()
+            {
+                Name = "parameter",
+                ParameterType = typeof(Person),
+                BindingInfo = BindingInfo.GetBindingInfo(new[] { new BindPropertyAttribute() { SupportsGet = true } }),
+            };
+
+            var testContext = ModelBindingTestHelper.GetTestContext(request =>
+            {
+                request.Method = "GET";
+                request.QueryString = new QueryString("?parameter.Name=Joey");
+            });
+
+            // Act
+            var result = await parameterBinder.BindModelAsync(parameter, testContext);
+
+            // Assert
+            Assert.True(result.IsModelSet);
+
+            Assert.Equal("Joey", Assert.IsType<Person>(result.Model).Name);
+        }
+
+        [Fact]
+        public async Task BindModelAsync_WithBindProperty_DoesNotBindModel_WhenRequestIsGet()
+        {
+            // Arrange
+            var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
+            var parameter = new ParameterDescriptor()
+            {
+                Name = "parameter",
+                ParameterType = typeof(Person),
+                BindingInfo = BindingInfo.GetBindingInfo(new[] { new BindPropertyAttribute() }),
+            };
+
+            var testContext = ModelBindingTestHelper.GetTestContext(request =>
+            {
+                request.Method = "GET";
+                request.QueryString = new QueryString("?parameter.Name=Joey");
+            });
+
+            // Act
+            var result = await parameterBinder.BindModelAsync(parameter, testContext);
+
+            // Assert
+            Assert.False(result.IsModelSet);
+        }
+    }
+}

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageLoaderTest.cs

@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.Linq;
 using System.Reflection;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.ActionConstraints;
 using Microsoft.AspNetCore.Mvc.Filters;
 using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure;
@@ -576,7 +577,17 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                 p =>
                 {
                     Assert.Equal("Property", p.Property.Name);
-                    Assert.True(p.SupportsGet);
+                    Assert.NotNull(p.BindingInfo.RequestPredicate);
+                    Assert.True(p.BindingInfo.RequestPredicate(new ActionContext()
+                    {
+                        HttpContext = new DefaultHttpContext()
+                        {
+                            Request =
+                            {
+                                Method ="GET",
+                            }
+                        }
+                    }));
                 });
         }
 
@@ -603,7 +614,17 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                 p =>
                 {
                     Assert.Equal("Property", p.Property.Name);
-                    Assert.True(p.SupportsGet);
+                    Assert.NotNull(p.BindingInfo.RequestPredicate);
+                    Assert.True(p.BindingInfo.RequestPredicate(new ActionContext()
+                    {
+                        HttpContext = new DefaultHttpContext()
+                        {
+                            Request =
+                            {
+                                Method ="GET",
+                            }
+                        }
+                    }));
                 });
         }
 
@@ -628,7 +649,17 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                 p =>
                 {
                     Assert.Equal("Property", p.Property.Name);
-                    Assert.False(p.SupportsGet);
+                    Assert.NotNull(p.BindingInfo.RequestPredicate);
+                    Assert.False(p.BindingInfo.RequestPredicate(new ActionContext()
+                    {
+                        HttpContext = new DefaultHttpContext()
+                        {
+                            Request =
+                            {
+                                Method ="GET",
+                            }
+                        }
+                    }));
                 });
         }
 

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PagePropertyBinderFactoryTest.cs

@@ -324,7 +324,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
         [InlineData("Get")]
         [InlineData("GET")]
         [InlineData("gET")]
-        public async Task ModelBinderFactory_IgnoresPropertyWithoutSupportsGet_WhenRequestIsGet(string method)
+        public async Task ModelBinderFactory_BindsPropertyWithoutSupportsGet_WhenRequestIsGet(string method)
         {
             // Arrange
             var type = typeof(PageModelWithSupportsGetProperty).GetTypeInfo();
@@ -338,7 +338,11 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                         Name = nameof(PageModelWithSupportsGetProperty.SupportsGet),
                         ParameterType = typeof(string),
                         Property = type.GetProperty(nameof(PageModelWithSupportsGetProperty.SupportsGet)),
-                        SupportsGet = true,
+                        BindingInfo = new BindingInfo()
+                        {
+                            // Simulates placing a [BindProperty] on the property
+                            RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute() { SupportsGet = true }).RequestPredicate,
+                        }
                     },
                     new PageBoundPropertyDescriptor()
                     {
@@ -356,7 +360,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
             var binder = new TestParameterBinder(new Dictionary<string, object>()
             {
                 { "SupportsGet", "value" },
-                { "Default", "ignored" },
+                { "Default", "set" },
             });
 
             var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
@@ -366,12 +370,16 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
             {
                 PageContext = new PageContext()
                 {
-                    HttpContext = new DefaultHttpContext(),
+                    HttpContext = new DefaultHttpContext()
+                    {
+                        Request=
+                        {
+                            Method = method,
+                        }
+                    }
                 }
             };
 
-            page.HttpContext.Request.Method = method;
-
             var model = new PageModelWithSupportsGetProperty();
 
             // Act
@@ -379,7 +387,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
 
             // Assert
             Assert.Equal("value", model.SupportsGet);
-            Assert.Null(model.Default);
+            Assert.Equal("set", model.Default);
         }
 
         [Fact]
@@ -397,7 +405,10 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal
                         Name = nameof(PageModelWithSupportsGetProperty.SupportsGet),
                         ParameterType = typeof(string),
                         Property = type.GetProperty(nameof(PageModelWithSupportsGetProperty.SupportsGet)),
-                        SupportsGet = true,
+                        BindingInfo = new BindingInfo()
+                        {
+                            RequestPredicate = ((IRequestPredicateProvider)new BindPropertyAttribute() { SupportsGet = true }).RequestPredicate,
+                        }
                     },
                     new PageBoundPropertyDescriptor()
                     {