diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 7390426bd9..bfad735496 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Linq.Expressions; using System.Reflection; using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Internal; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders @@ -47,6 +48,28 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return TaskCache.CompletedTask; } + // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as + // reflection does not provide information about the implicit parameterless constructor for a struct. + // This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression + // compile fails to construct it. + var modelTypeInfo = bindingContext.ModelType.GetTypeInfo(); + if (bindingContext.Model == null && + (modelTypeInfo.IsAbstract || + modelTypeInfo.GetConstructor(Type.EmptyTypes) == null)) + { + if (bindingContext.IsTopLevelObject) + { + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName)); + } + + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty( + modelTypeInfo.FullName, + bindingContext.ModelName, + bindingContext.ModelMetadata.ContainerType.FullName)); + } + // Perf: separated to avoid allocating a state machine when we don't // need to go async. return BindModelCoreAsync(bindingContext); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs index d8168f85c6..9c513b911d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs @@ -20,9 +20,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders throw new ArgumentNullException(nameof(context)); } - if (context.Metadata.IsComplexType && - !context.Metadata.IsCollectionType && - HasDefaultConstructor(context.Metadata.ModelType.GetTypeInfo())) + if (context.Metadata.IsComplexType && !context.Metadata.IsCollectionType) { var propertyBinders = new Dictionary(); for (var i = 0; i < context.Metadata.Properties.Count; i++) @@ -36,14 +34,5 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return null; } - - private bool HasDefaultConstructor(TypeInfo modelTypeInfo) - { - // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs. - // - Reflection does not provide information about the implicit parameterless constructor for a struct. - // - Also this binder would eventually fail to construct an instance of the struct as the Linq's - // NewExpression compile fails to construct it. - return !modelTypeInfo.IsAbstract && modelTypeInfo.GetConstructor(Type.EmptyTypes) != null; - } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index f1f70e2c2d..b16b71757b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1402,6 +1402,38 @@ namespace Microsoft.AspNetCore.Mvc.Core return string.Format(CultureInfo.CurrentCulture, GetString("Argument_InvalidOffsetLength"), p0, p1); } + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. + /// + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject + { + get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"); } + } + + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. + /// + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"), p0); + } + + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor. + /// + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty + { + get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty"); } + } + + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor. + /// + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty(object p0, object p1, object p2) + { + return string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty"), p0, p1, p2); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 37f5c7b895..870a5904a1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -391,4 +391,10 @@ '{0}' and '{1}' are out of bounds for the string. '{0}' and '{1}' are the parameters which combine to be out of bounds. + + Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. + + + Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor. + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs index 335166e3ea..b09d359227 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs @@ -61,11 +61,14 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal switch (part.NodeType) { case ExpressionType.Call: + // Will exit loop if at Method().Property or [i,j].Property. In that case (like [i].Property), + // don't cache and don't remove ".Model" (if that's .Property). + containsIndexers = true; + lastIsModel = false; + var methodExpression = (MethodCallExpression)part; if (IsSingleArgumentIndexer(methodExpression)) { - containsIndexers = true; - lastIsModel = false; length += "[99]".Length; part = methodExpression.Object; segmentCount++; diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionTextCache.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionTextCache.cs index f6ff6d2bd4..c9845c644e 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionTextCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionTextCache.cs @@ -37,6 +37,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal while (true) { + if (expression1 == null && expression2 == null) + { + return true; + } + if (expression1 == null || expression2 == null) { return false; @@ -47,31 +52,41 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return false; } - if (expression1.NodeType == ExpressionType.MemberAccess) + switch (expression1.NodeType) { - var memberExpression1 = (MemberExpression)expression1; - var memberName1 = memberExpression1.Member.Name; - expression1 = memberExpression1.Expression; + case ExpressionType.MemberAccess: + var memberExpression1 = (MemberExpression)expression1; + var memberName1 = memberExpression1.Member.Name; + expression1 = memberExpression1.Expression; - var memberExpression2 = (MemberExpression)expression2; - var memberName2 = memberExpression2.Member.Name; - expression2 = memberExpression2.Expression; + var memberExpression2 = (MemberExpression)expression2; + var memberName2 = memberExpression2.Member.Name; + expression2 = memberExpression2.Expression; - // If identifier contains "__", it is "reserved for use by the implementation" and likely compiler- - // or Razor-generated e.g. the name of a field in a delegate's generated class. - if (memberName1.Contains("__") && memberName2.Contains("__")) - { - return true; - } + // If identifier contains "__", it is "reserved for use by the implementation" and likely + // compiler- or Razor-generated e.g. the name of a field in a delegate's generated class. + if (memberName1.Contains("__") && memberName2.Contains("__")) + { + return true; + } - if (!string.Equals(memberName1, memberName2, StringComparison.OrdinalIgnoreCase)) - { + if (!string.Equals(memberName1, memberName2, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + break; + + case ExpressionType.ArrayIndex: + // Shouldn't be cached. Just in case, ensure indexers are all different. return false; - } - } - else - { - return true; + + case ExpressionType.Call: + // Shouldn't be cached. Just in case, ensure indexers and other calls are all different. + return false; + + default: + // Everything else terminates name generation. Haven't found a difference so far... + return true; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs index ba9f5c1b18..9dc044f814 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs @@ -55,107 +55,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType(result); } - [Theory] - [InlineData(typeof(PointStructWithExplicitConstructor))] - [InlineData(typeof(PointStructWithNoExplicitConstructor))] - public void Create_ForStructModel_ReturnsNull(Type modelType) - { - // Arrange - var provider = new ComplexTypeModelBinderProvider(); - var context = new TestModelBinderProviderContext(modelType); - - // Act - var result = provider.GetBinder(context); - - // Assert - Assert.Null(result); - } - - [Theory] - [InlineData(typeof(ClassWithNoDefaultConstructor))] - [InlineData(typeof(ClassWithStaticConstructor))] - [InlineData(typeof(ClassWithInternalDefaultConstructor))] - public void Create_ForModelTypeWithNoDefaultPublicConstructor_ReturnsNull(Type modelType) - { - // Arrange - var provider = new ComplexTypeModelBinderProvider(); - var context = new TestModelBinderProviderContext(modelType); - - // Act - var result = provider.GetBinder(context); - - // Assert - Assert.Null(result); - } - - [Fact] - public void Create_ForAbstractModelTypeWithDefaultPublicConstructor_ReturnsNull() - { - // Arrange - var provider = new ComplexTypeModelBinderProvider(); - var context = new TestModelBinderProviderContext(typeof(AbstractClassWithDefaultConstructor)); - - // Act - var result = provider.GetBinder(context); - - // Assert - Assert.Null(result); - } - - private struct PointStructWithNoExplicitConstructor - { - public double X { get; set; } - public double Y { get; set; } - } - - private struct PointStructWithExplicitConstructor - { - public PointStructWithExplicitConstructor(double x, double y) - { - X = x; - Y = y; - } - public double X { get; } - public double Y { get; } - } - private class Person { public string Name { get; set; } public int Age { get; set; } } - - private class ClassWithNoDefaultConstructor - { - public ClassWithNoDefaultConstructor(int id) { } - } - - private class ClassWithInternalDefaultConstructor - { - internal ClassWithInternalDefaultConstructor() { } - } - - private class ClassWithStaticConstructor - { - static ClassWithStaticConstructor() { } - - public ClassWithStaticConstructor(int id) { } - } - - private abstract class AbstractClassWithDefaultConstructor - { - private readonly string _name; - - public AbstractClassWithDefaultConstructor() - : this("James") - { - } - - public AbstractClassWithDefaultConstructor(string name) - { - _name = name; - } - } } } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index bf9eb4ffc1..e6470ab6f8 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -384,7 +384,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests } [Fact] - public async Task ActionParameter_ModelPropertyTypeWithNoDefaultConstructor_NoOps() + public async Task ActionParameter_ModelPropertyTypeWithNoParameterlessConstructor_ThrowsException() { // Arrange var parameterType = typeof(Class1); @@ -400,60 +400,63 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests }); var modelState = testContext.ModelState; - // Act - var result = await argumentBinder.BindModelAsync(parameter, testContext); - - // Assert - Assert.True(result.IsModelSet); - Assert.True(modelState.IsValid); - var model = Assert.IsType(result.Model); - Assert.Null(model.Property1); - var keyValuePair = Assert.Single(modelState); - Assert.Equal("Name", keyValuePair.Key); - Assert.Equal("James", keyValuePair.Value.AttemptedValue); - Assert.Equal("James", keyValuePair.Value.RawValue); + // Act & Assert + var exception = await Assert.ThrowsAsync(() => argumentBinder.BindModelAsync(parameter, testContext)); + Assert.Equal( + string.Format( + "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " + + "value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" + + " a non-null value in the '{2}' constructor.", + typeof(ClassWithNoDefaultConstructor).FullName, + nameof(Class1.Property1), + typeof(Class1).FullName), + exception.Message); } [Fact] - public async Task ActionParameter_BindingToStructModel_Fails() + public async Task ActionParameter_BindingToStructModel_ThrowsException() { // Arrange var parameterType = typeof(PointStruct); var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); var parameter = new ParameterDescriptor() { - ParameterType = parameterType + ParameterType = parameterType, + Name = "p" }; var testContext = ModelBindingTestHelper.GetTestContext(); // Act & Assert - var exception = await Assert.ThrowsAsync( - () => argumentBinder.BindModelAsync(parameter, testContext)); - + var exception = await Assert.ThrowsAsync(() => argumentBinder.BindModelAsync(parameter, testContext)); Assert.Equal( - string.Format("Could not create a model binder for model object of type '{0}'.", parameterType.FullName), + string.Format( + "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " + + "value types and must have a parameterless constructor.", + typeof(PointStruct).FullName), exception.Message); } [Theory] [InlineData(typeof(ClassWithNoDefaultConstructor))] [InlineData(typeof(AbstractClassWithNoDefaultConstructor))] - public async Task ActionParameter_NoDefaultConstructor_Fails(Type parameterType) + public async Task ActionParameter_BindingToTypeWithNoParameterlessConstructor_ThrowsException(Type parameterType) { // Arrange var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); var parameter = new ParameterDescriptor() { - ParameterType = parameterType + ParameterType = parameterType, + Name = "p" }; var testContext = ModelBindingTestHelper.GetTestContext(); // Act & Assert - var exception = await Assert.ThrowsAsync( - () => argumentBinder.BindModelAsync(parameter, testContext)); - + var exception = await Assert.ThrowsAsync(() => argumentBinder.BindModelAsync(parameter, testContext)); Assert.Equal( - string.Format("Could not create a model binder for model object of type '{0}'.", parameterType.FullName), + string.Format( + "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " + + "value types and must have a parameterless constructor.", + parameterType.FullName), exception.Message); } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index 982879aa23..47b8ac9945 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs @@ -1091,6 +1091,57 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(modelStateEntry.RawValue); } + private class AddressWithNoParameterlessConstructor + { + private readonly int _id; + public AddressWithNoParameterlessConstructor(int id) + { + _id = id; + } + public string Street { get; set; } + public string City { get; set; } + } + + [Fact] + public async Task TryUpdateModel_ExistingModelWithNoParameterlessConstructor_OverwritesBoundValues() + { + // Arrange + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = QueryString.Create("Street", "SomeStreet"); + }); + + var modelState = testContext.ModelState; + var model = new AddressWithNoParameterlessConstructor(10) + { + Street = "DefaultStreet", + City = "Toronto", + }; + var oldModel = model; + + // Act + var result = await TryUpdateModelAsync(model, string.Empty, testContext); + + // Assert + Assert.True(result); + + // Model + Assert.Same(oldModel, model); + Assert.Equal("SomeStreet", model.Street); + Assert.Equal("Toronto", model.City); + + // ModelState + Assert.True(modelState.IsValid); + + var entry = Assert.Single(modelState); + Assert.Equal("Street", entry.Key); + var state = entry.Value; + Assert.Equal("SomeStreet", state.AttemptedValue); + Assert.Equal("SomeStreet", state.RawValue); + Assert.Empty(state.Errors); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + } + private void UpdateRequest(HttpRequest request, string data, string name) { const string fileName = "text.txt"; diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs index eb06df6954..2193f33026 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Linq.Expressions; using Xunit; @@ -12,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { private readonly ExpressionTextCache _expressionTextCache = new ExpressionTextCache(); - public static IEnumerable ExpressionAndTexts + public static TheoryData ExpressionAndTexts { get { @@ -80,11 +81,35 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal (Expression, int>>)(model => model[2].PreferredCategories[i].CategoryId), "[2].PreferredCategories[3].CategoryId" }, + { + (Expression, string>>)(model => model.FirstOrDefault().Name), + "Name" + }, + { + (Expression, string>>)(model => model.FirstOrDefault().Model), + "Model" + }, + { + (Expression, int>>)(model => model.FirstOrDefault().SelectedCategory.CategoryId), + "SelectedCategory.CategoryId" + }, + { + (Expression, string>>)(model => model.FirstOrDefault().SelectedCategory.CategoryName.MainCategory), + "SelectedCategory.CategoryName.MainCategory" + }, + { + (Expression, int>>)(model => model.FirstOrDefault().PreferredCategories.Count), + "PreferredCategories.Count" + }, + { + (Expression, int>>)(model => model.FirstOrDefault().PreferredCategories.FirstOrDefault().CategoryId), + "CategoryId" + }, }; } } - public static IEnumerable CachedExpressions + public static TheoryData CachedExpressions { get { @@ -104,7 +129,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } - public static IEnumerable IndexerExpressions + public static TheoryData IndexerExpressions { get { @@ -123,7 +148,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } - public static IEnumerable EquivalentExpressions + public static TheoryData UnsupportedExpressions + { + get + { + var i = 2; + var j = 3; + + return new TheoryData + { + // Indexers that have multiple arguments. + (Expression>)(model => model[23][3].Name), + (Expression>)(model => model[i][3].Name), + (Expression>)(model => model[23][j].Name), + (Expression>)(model => model[i][j].Name), + // Calls that aren't indexers. + (Expression, string>>)(model => model.FirstOrDefault().Name), + (Expression, string>>)(model => model.FirstOrDefault().SelectedCategory.CategoryName.MainCategory), + (Expression, int>>)(model => model.FirstOrDefault().PreferredCategories.FirstOrDefault().CategoryId), + }; + } + } + + public static TheoryData EquivalentExpressions { get { @@ -167,7 +214,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } - public static IEnumerable NonEquivalentExpressions + public static TheoryData NonEquivalentExpressions { get { @@ -253,7 +300,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal [Theory] [MemberData(nameof(IndexerExpressions))] - public void GetExpressionText_DoesNotCacheIndexerExpression(LambdaExpression expression) + [MemberData(nameof(UnsupportedExpressions))] + public void GetExpressionText_DoesNotCacheIndexerOrUnspportedExpression(LambdaExpression expression) { // Act - 1 var text1 = ExpressionHelper.GetExpressionText(expression, _expressionTextCache);