From 1886d53d898b8790bcf289fd6521c228f08f9344 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 26 Jun 2017 18:58:04 -0700 Subject: [PATCH] Remove [BindProperty] on class This isn't a good fit with consistency with controllers. Discussed with @DamianEdwards and we agreed to remove this for now and bring it back in the future if there's a real need for it. --- .../BindPropertyAttribute.cs | 2 +- .../Infrastructure/PagesBaseClassAttribute.cs | 2 +- .../Internal/DefaultPageLoader.cs | 19 +--- .../Internal/DefaultPageLoaderTest.cs | 96 ------------------- 4 files changed, 4 insertions(+), 115 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/BindPropertyAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/BindPropertyAttribute.cs index 5056377712..4c12dd33d5 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/BindPropertyAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/BindPropertyAttribute.cs @@ -6,7 +6,7 @@ using Microsoft.AspNetCore.Mvc.ModelBinding; namespace Microsoft.AspNetCore.Mvc { - [AttributeUsage(AttributeTargets.Property | AttributeTargets.Class, AllowMultiple = false, Inherited = true)] + [AttributeUsage(AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public class BindPropertyAttribute : Attribute, IModelNameProvider, IBinderTypeProviderMetadata, IRequestPredicateProvider { private static readonly Func _supportsAllRequests = (c) => true; diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PagesBaseClassAttribute.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PagesBaseClassAttribute.cs index d16e84c72d..bf453ef994 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PagesBaseClassAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PagesBaseClassAttribute.cs @@ -7,7 +7,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure { /// /// An attribute for base classes for Pages and PageModels. Applying this attribute to a type - /// suppresses discovery of handler methods and bound properties for that type. + /// suppresses discovery of handler methods for that type. /// [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)] public class PagesBaseClassAttribute : Attribute diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs index a606355006..df4409b993 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs @@ -239,9 +239,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal { var properties = PropertyHelper.GetVisibleProperties(type.AsType()); - // If the type has a [BindPropertyAttribute] then we'll consider any and all public properties bindable. - var bindPropertyOnType = type.GetCustomAttribute(); - var results = new List(); for (var i = 0; i < properties.Length; i++) { @@ -249,24 +246,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal 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) + // property. So we won't bind this property. + if (bindingInfo == 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; - } - - 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, diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageLoaderTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageLoaderTest.cs index df27d80246..893e241a59 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageLoaderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageLoaderTest.cs @@ -537,31 +537,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal public int IgnoreMe { get; set; } } - // Additionally [BindProperty] on a property can opt-in a property - [Fact] - public void CreateBoundProperties_BindPropertyAttributeOnModel_OptsInAllProperties() - { - // Arrange - var type = typeof(ModelWithBindPropertyOnClass).GetTypeInfo(); - - // Act - var results = DefaultPageLoader.CreateBoundProperties(type); - - // Assert - Assert.Collection( - results.OrderBy(p => p.Property.Name), - p => - { - Assert.Equal("Property", p.Property.Name); - }); - } - - [BindProperty] - private class ModelWithBindPropertyOnClass : EmptyPageModel - { - public int Property { get; set; } - } - [Fact] public void CreateBoundProperties_SupportsGet_OnProperty() { @@ -599,77 +574,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal public int IgnoreMe { get; set; } } - [Fact] - public void CreateBoundProperties_SupportsGet_OnClass() - { - // Arrange - var type = typeof(ModelSupportsGetOnClass).GetTypeInfo(); - - // Act - var results = DefaultPageLoader.CreateBoundProperties(type); - - // Assert - Assert.Collection( - results.OrderBy(p => p.Property.Name), - p => - { - Assert.Equal("Property", p.Property.Name); - Assert.NotNull(p.BindingInfo.RequestPredicate); - Assert.True(p.BindingInfo.RequestPredicate(new ActionContext() - { - HttpContext = new DefaultHttpContext() - { - Request = - { - Method ="GET", - } - } - })); - }); - } - - [BindProperty(SupportsGet = true)] - private class ModelSupportsGetOnClass : EmptyPageModel - { - public int Property { get; set; } - } - - [Fact] - public void CreateBoundProperties_SupportsGet_Override() - { - // Arrange - var type = typeof(ModelSupportsGetOverride).GetTypeInfo(); - - // Act - var results = DefaultPageLoader.CreateBoundProperties(type); - - // Assert - Assert.Collection( - results.OrderBy(p => p.Property.Name), - p => - { - Assert.Equal("Property", p.Property.Name); - Assert.NotNull(p.BindingInfo.RequestPredicate); - Assert.False(p.BindingInfo.RequestPredicate(new ActionContext() - { - HttpContext = new DefaultHttpContext() - { - Request = - { - Method ="GET", - } - } - })); - }); - } - - [BindProperty(SupportsGet = true)] - private class ModelSupportsGetOverride : EmptyPageModel - { - [BindProperty(SupportsGet = false)] - public int Property { get; set; } - } - [Theory] [InlineData("Foo")] [InlineData("On")]