From 5c8dfef15ee6bc378c6cfc5466ba244473ed5631 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Sun, 9 Sep 2018 21:48:23 -0700 Subject: [PATCH] Change `CollectionModelBinder` and `ComplexTypeModelBinder` to enforce `[BindRequired]` - #8180 - add an error when binding fails for top-level model - same case as when MVC creates "default" / empty model i.e. `ParameterBinder` can't detect this - update `CollectionModelBinder` subclasses and the various providers as well - controlled by existing `MvcOptions.AllowValidatingTopLevelNodes` option smaller issue: - change `ModelBinding_MissingBindRequiredMember` resource to mention parameters too --- .../Metadata/ModelBindingMessageProvider.cs | 4 +- .../ModelBinding/Binders/ArrayModelBinder.cs | 24 +++ .../Binders/ArrayModelBinderProvider.cs | 8 +- .../Binders/CollectionModelBinder.cs | 59 ++++++ .../Binders/CollectionModelBinderProvider.cs | 16 +- .../Binders/ComplexTypeModelBinder.cs | 51 ++++- .../Binders/ComplexTypeModelBinderProvider.cs | 7 +- .../Binders/DictionaryModelBinder.cs | 48 +++++ .../Binders/DictionaryModelBinderProvider.cs | 9 +- .../Properties/Resources.Designer.cs | 4 +- .../Resources.resx | 56 ++--- .../Binders/ArrayModelBinderProviderTest.cs | 25 +++ .../Binders/ArrayModelBinderTest.cs | 108 ++++++++-- .../CollectionModelBinderProviderTest.cs | 25 +++ .../Binders/CollectionModelBinderTest.cs | 109 ++++++++-- .../ComplexTypeModelBinderProviderTest.cs | 32 +++ .../Binders/ComplexTypeModelBinderTest.cs | 199 +++++++++++++++++- .../DictionaryModelBinderProviderTest.cs | 33 +++ .../Binders/DictionaryModelBinderTest.cs | 110 ++++++++-- .../TestModelBinderProviderContext.cs | 14 +- .../InputValidationTests.cs | 8 +- .../CollectionModelBinderIntegrationTest.cs | 4 +- .../ComplexTypeModelBinderIntegrationTest.cs | 16 +- 23 files changed, 843 insertions(+), 126 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Metadata/ModelBindingMessageProvider.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Metadata/ModelBindingMessageProvider.cs index 5ab37cfd8b..c91bfe4cb0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Metadata/ModelBindingMessageProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Metadata/ModelBindingMessageProvider.cs @@ -14,7 +14,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata /// Error message the model binding system adds when a property with an associated /// BindRequiredAttribute is not bound. /// - /// Default is "A value for the '{0}' property was not provided.". + /// + /// Default is "A value for the '{0}' parameter or property was not provided.". + /// public virtual Func MissingBindRequiredValueAccessor { get; } /// diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinder.cs index 96c6a31288..c884d8f685 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinder.cs @@ -38,11 +38,35 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// The for binding . /// /// The . + /// + /// The binder will not add an error for an unbound top-level model even if + /// is . + /// public ArrayModelBinder(IModelBinder elementBinder, ILoggerFactory loggerFactory) : base(elementBinder, loggerFactory) { } + /// + /// Creates a new . + /// + /// + /// The for binding . + /// + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + public ArrayModelBinder( + IModelBinder elementBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes) + : base(elementBinder, loggerFactory, allowValidatingTopLevelNodes) + { + } + /// public override bool CanCreateInstance(Type targetType) { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinderProvider.cs index 190390b9c7..c08263056e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ArrayModelBinderProvider.cs @@ -4,6 +4,7 @@ using System; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { @@ -27,7 +28,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var binderType = typeof(ArrayModelBinder<>).MakeGenericType(elementType); var loggerFactory = context.Services.GetRequiredService(); - return (IModelBinder)Activator.CreateInstance(binderType, elementBinder, loggerFactory); + var mvcOptions = context.Services.GetRequiredService>().Value; + return (IModelBinder)Activator.CreateInstance( + binderType, + elementBinder, + loggerFactory, + mvcOptions.AllowValidatingTopLevelNodes); } return null; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinder.cs index 16834ebadc..372a7da770 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinder.cs @@ -44,7 +44,29 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// /// The for binding elements. /// The . + /// + /// The binder will not add an error for an unbound top-level model even if + /// is . + /// public CollectionModelBinder(IModelBinder elementBinder, ILoggerFactory loggerFactory) + : this(elementBinder, loggerFactory, allowValidatingTopLevelNodes: false) + { + } + + /// + /// Creates a new . + /// + /// The for binding elements. + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + public CollectionModelBinder( + IModelBinder elementBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes) { if (elementBinder == null) { @@ -58,8 +80,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders ElementBinder = elementBinder; Logger = loggerFactory.CreateLogger(GetType()); + AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; } + // Internal for testing. + internal bool AllowValidatingTopLevelNodes { get; } + /// /// Gets the instances for binding collection elements. /// @@ -94,6 +120,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders model = CreateEmptyCollection(bindingContext.ModelType); } + if (AllowValidatingTopLevelNodes) + { + AddErrorIfBindingRequired(bindingContext); + } + bindingContext.Result = ModelBindingResult.Success(model); } @@ -161,6 +192,34 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders typeof(ICollection).IsAssignableFrom(targetType); } + /// + /// Add a to if + /// . + /// + /// The . + /// + /// + /// This method should be called only when is + /// and a top-level model was not bound. + /// + /// + /// For back-compatibility reasons, must have + /// equal to when a + /// top-level model is not bound. Therefore, ParameterBinder can not detect a + /// failure for collections. Add the error here. + /// + /// + protected void AddErrorIfBindingRequired(ModelBindingContext bindingContext) + { + var modelMetadata = bindingContext.ModelMetadata; + if (modelMetadata.IsBindingRequired) + { + var messageProvider = modelMetadata.ModelBindingMessageProvider; + var message = messageProvider.MissingBindRequiredValueAccessor(bindingContext.FieldName); + bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, message); + } + } + /// /// Create an assignable to . /// diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinderProvider.cs index bacc1f371f..5781289596 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/CollectionModelBinderProvider.cs @@ -7,6 +7,7 @@ using System.Reflection; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { @@ -32,7 +33,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders } var loggerFactory = context.Services.GetRequiredService(); - + var mvcOptions = context.Services.GetRequiredService>().Value; + // If the model type is ICollection<> then we can call its Add method, so we can always support it. var collectionType = ClosedGenericMatcher.ExtractGenericInterface(modelType, typeof(ICollection<>)); if (collectionType != null) @@ -41,7 +43,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType)); var binderType = typeof(CollectionModelBinder<>).MakeGenericType(collectionType.GenericTypeArguments); - return (IModelBinder)Activator.CreateInstance(binderType, elementBinder, loggerFactory); + return (IModelBinder)Activator.CreateInstance( + binderType, + elementBinder, + loggerFactory, + mvcOptions.AllowValidatingTopLevelNodes); } // If the model type is IEnumerable<> then we need to know if we can assign a List<> to it, since @@ -57,7 +63,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType)); var binderType = typeof(CollectionModelBinder<>).MakeGenericType(enumerableType.GenericTypeArguments); - return (IModelBinder)Activator.CreateInstance(binderType, elementBinder, loggerFactory); + return (IModelBinder)Activator.CreateInstance( + binderType, + elementBinder, + loggerFactory, + mvcOptions.AllowValidatingTopLevelNodes); } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 2ab1cdb70c..5c1e82a80c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -45,9 +45,33 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// The of binders to use for binding properties. /// /// The . + /// + /// The binder will not add an error for an unbound top-level model even if + /// is . + /// public ComplexTypeModelBinder( IDictionary propertyBinders, ILoggerFactory loggerFactory) + : this(propertyBinders, loggerFactory, allowValidatingTopLevelNodes: false) + { + } + + /// + /// Creates a new . + /// + /// + /// The of binders to use for binding properties. + /// + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + public ComplexTypeModelBinder( + IDictionary propertyBinders, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes) { if (propertyBinders == null) { @@ -61,8 +85,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders _propertyBinders = propertyBinders; _logger = loggerFactory.CreateLogger(); + AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; } + // Internal for testing. + internal bool AllowValidatingTopLevelNodes { get; } + /// public Task BindModelAsync(ModelBindingContext bindingContext) { @@ -91,9 +119,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders bindingContext.Model = CreateModel(bindingContext); } - for (var i = 0; i < bindingContext.ModelMetadata.Properties.Count; i++) + var modelMetadata = bindingContext.ModelMetadata; + var attemptedPropertyBinding = false; + for (var i = 0; i < modelMetadata.Properties.Count; i++) { - var property = bindingContext.ModelMetadata.Properties[i]; + var property = modelMetadata.Properties[i]; if (!CanBindProperty(bindingContext, property)) { continue; @@ -127,15 +157,32 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders if (result.IsModelSet) { + attemptedPropertyBinding = true; SetProperty(bindingContext, modelName, property, result); } else if (property.IsBindingRequired) { + attemptedPropertyBinding = true; var message = property.ModelBindingMessageProvider.MissingBindRequiredValueAccessor(fieldName); bindingContext.ModelState.TryAddModelError(modelName, message); } } + // Have we created a top-level model despite an inability to bind anything in said model and a lack of + // other IsBindingRequired errors? Does that violate [BindRequired] on the model? This case occurs when + // 1. The top-level model has no public settable properties. + // 2. All properties in a [BindRequired] model have [BindNever] or are otherwise excluded from binding. + // 3. No data exists for any property. + if (AllowValidatingTopLevelNodes && + !attemptedPropertyBinding && + bindingContext.IsTopLevelObject && + modelMetadata.IsBindingRequired) + { + var messageProvider = modelMetadata.ModelBindingMessageProvider; + var message = messageProvider.MissingBindRequiredValueAccessor(bindingContext.FieldName); + bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, message); + } + bindingContext.Result = ModelBindingResult.Success(bindingContext.Model); _logger.DoneAttemptingToBindModel(bindingContext); } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs index f0d6b5989c..ac8d2f13d3 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { @@ -31,7 +32,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders } var loggerFactory = context.Services.GetRequiredService(); - return new ComplexTypeModelBinder(propertyBinders, loggerFactory); + var mvcOptions = context.Services.GetRequiredService>().Value; + return new ComplexTypeModelBinder( + propertyBinders, + loggerFactory, + mvcOptions.AllowValidatingTopLevelNodes); } return null; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinder.cs index fbb0c53db1..0954f87e6c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinder.cs @@ -43,6 +43,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// The for . /// The for . /// The . + /// + /// The binder will not add an error for an unbound top-level model even if + /// is . + /// public DictionaryModelBinder(IModelBinder keyBinder, IModelBinder valueBinder, ILoggerFactory loggerFactory) : base(new KeyValuePairModelBinder(keyBinder, valueBinder, loggerFactory), loggerFactory) { @@ -54,6 +58,40 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders _valueBinder = valueBinder; } + /// + /// Creates a new . + /// + /// The for . + /// The for . + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + public DictionaryModelBinder( + IModelBinder keyBinder, + IModelBinder valueBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes) + : base( + new KeyValuePairModelBinder(keyBinder, valueBinder, loggerFactory), + loggerFactory, + // CollectionModelBinder should not check IsRequired, done in this model binder. + allowValidatingTopLevelNodes: false) + { + if (valueBinder == null) + { + throw new ArgumentNullException(nameof(valueBinder)); + } + + _valueBinder = valueBinder; + AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; + } + + // Internal for testing. + internal new bool AllowValidatingTopLevelNodes { get; } + /// public override async Task BindModelAsync(ModelBindingContext bindingContext) { @@ -85,6 +123,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { // No IEnumerableValueProvider available for the fallback approach. For example the user may have // replaced the ValueProvider with something other than a CompositeValueProvider. + if (AllowValidatingTopLevelNodes && bindingContext.IsTopLevelObject) + { + AddErrorIfBindingRequired(bindingContext); + } + return; } @@ -94,6 +137,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders if (keys.Count == 0) { // No entries with the expected keys. + if (AllowValidatingTopLevelNodes && bindingContext.IsTopLevelObject) + { + AddErrorIfBindingRequired(bindingContext); + } + return; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinderProvider.cs index 0d7db7c064..4ef0c83725 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/DictionaryModelBinderProvider.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { @@ -34,7 +35,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var binderType = typeof(DictionaryModelBinder<,>).MakeGenericType(dictionaryType.GenericTypeArguments); var loggerFactory = context.Services.GetRequiredService(); - return (IModelBinder)Activator.CreateInstance(binderType, keyBinder, valueBinder, loggerFactory); + var mvcOptions = context.Services.GetRequiredService>().Value; + return (IModelBinder)Activator.CreateInstance( + binderType, + keyBinder, + valueBinder, + loggerFactory, + mvcOptions.AllowValidatingTopLevelNodes); } return null; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index 70d895c8af..c00c53bcf8 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -795,7 +795,7 @@ namespace Microsoft.AspNetCore.Mvc.Core => GetString("ModelBinderUtil_ModelMetadataCannotBeNull"); /// - /// A value for the '{0}' property was not provided. + /// A value for the '{0}' parameter or property was not provided. /// internal static string ModelBinding_MissingBindRequiredMember { @@ -803,7 +803,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// A value for the '{0}' property was not provided. + /// A value for the '{0}' parameter or property was not provided. /// internal static string FormatModelBinding_MissingBindRequiredMember(object p0) => string.Format(CultureInfo.CurrentCulture, GetString("ModelBinding_MissingBindRequiredMember"), p0); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 5ec7fea828..30faf9ec02 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -1,17 +1,17 @@  - @@ -295,7 +295,7 @@ The binding context cannot have a null ModelMetadata. - A value for the '{0}' property was not provided. + A value for the '{0}' parameter or property was not provided. A non-empty request body is required. diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderProviderTest.cs index 83a96daa9a..537dd3bf4e 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderProviderTest.cs @@ -51,6 +51,31 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType(typeof(ArrayModelBinder<>).MakeGenericType(modelType.GetElementType()), result); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Create_ForArrayType_ReturnsBinder_WithExpectedAllowValidatingTopLevelNodes( + bool allowValidatingTopLevelNodes) + { + // Arrange + var provider = new ArrayModelBinderProvider(); + + var context = new TestModelBinderProviderContext(typeof(int[])); + context.MvcOptions.AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; + context.OnCreatingBinder(m => + { + Assert.Equal(typeof(int), m.ModelType); + return Mock.Of(); + }); + + // Act + var result = provider.GetBinder(context); + + // Assert + var binder = Assert.IsType>(result); + Assert.Equal(allowValidatingTopLevelNodes, binder.AllowValidatingTopLevelNodes); + } + [Fact] public void Create_ForModelMetadataReadOnly_ReturnsNull() { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs index 8c3a372f06..d3efdd8775 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.ModelBinding.Internal; @@ -42,13 +43,21 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Equal(new[] { 42, 84 }, array); } - [Fact] - public async Task ArrayModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() + private IActionResult ActionWithArrayParameter(string[] parameter) => null; + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task ArrayModelBinder_CreatesEmptyCollection_IfIsTopLevelObject( + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var binder = new ArrayModelBinder( new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.IsTopLevelObject = true; @@ -57,7 +66,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders bindingContext.ModelName = "modelName"; var metadataProvider = new TestModelMetadataProvider(); - bindingContext.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[])); + var parameter = typeof(ArrayModelBinderTest) + .GetMethod(nameof(ActionWithArrayParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); @@ -67,22 +82,74 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.Empty(Assert.IsType(bindingContext.Result.Model)); Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); } - [Theory] - [InlineData("")] - [InlineData("param")] - public async Task ArrayModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) + [Fact] + public async Task ArrayModelBinder_CreatesEmptyCollectionAndAddsError_IfIsTopLevelObject() { // Arrange var binder = new ArrayModelBinder( new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + var bindingContext = CreateContext(); + bindingContext.IsTopLevelObject = true; + bindingContext.FieldName = "fieldName"; + bindingContext.ModelName = "modelName"; + + var metadataProvider = new TestModelMetadataProvider(); + var parameter = typeof(ArrayModelBinderTest) + .GetMethod(nameof(ActionWithArrayParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); + + bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.Empty(Assert.IsType(bindingContext.Result.Model)); + Assert.True(bindingContext.Result.IsModelSet); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal("modelName", keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); + } + + [Theory] + [InlineData("", false, false)] + [InlineData("", true, false)] + [InlineData("", false, true)] + [InlineData("", true, true)] + [InlineData("param", false, false)] + [InlineData("param", true, false)] + [InlineData("param", false, true)] + [InlineData("param", true, true)] + public async Task ArrayModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject( + string prefix, + bool allowValidatingTopLevelNodes, + bool isBindingRequired) + { + // Arrange + var binder = new ArrayModelBinder( + new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.ModelName = ModelNames.CreatePropertyModelName(prefix, "ArrayProperty"); var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForProperty(typeof(ModelWithArrayProperty), nameof(ModelWithArrayProperty.ArrayProperty)) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); bindingContext.ModelMetadata = metadataProvider.GetMetadataForProperty( typeof(ModelWithArrayProperty), nameof(ModelWithArrayProperty.ArrayProperty)); @@ -94,6 +161,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.False(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); } public static TheoryData ArrayModelData @@ -177,23 +245,23 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders private static DefaultModelBindingContext GetBindingContext(IValueProvider valueProvider) { - var bindingContext = new DefaultModelBindingContext() - { - ModelName = "someName", - ModelState = new ModelStateDictionary(), - ValueProvider = valueProvider, - }; + var bindingContext = CreateContext(); + bindingContext.ModelName = "someName"; + bindingContext.ValueProvider = valueProvider; + return bindingContext; } private static DefaultModelBindingContext CreateContext() { - var modelBindingContext = new DefaultModelBindingContext() + var actionContext = new ActionContext { - ActionContext = new ActionContext() - { - HttpContext = new DefaultHttpContext(), - }, + HttpContext = new DefaultHttpContext(), + }; + var modelBindingContext = new DefaultModelBindingContext + { + ActionContext = actionContext, + ModelState = actionContext.ModelState, }; return modelBindingContext; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderProviderTest.cs index d0cd6091bb..d3ee57f216 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderProviderTest.cs @@ -66,6 +66,31 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType>(result); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Create_ForSupportedType_ReturnsBinder_WithExpectedAllowValidatingTopLevelNodes( + bool allowValidatingTopLevelNodes) + { + // Arrange + var provider = new CollectionModelBinderProvider(); + + var context = new TestModelBinderProviderContext(typeof(List)); + context.MvcOptions.AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; + context.OnCreatingBinder(m => + { + Assert.Equal(typeof(int), m.ModelType); + return Mock.Of(); + }); + + // Act + var result = provider.GetBinder(context); + + // Assert + var binder = Assert.IsType>(result); + Assert.Equal(allowValidatingTopLevelNodes, binder.AllowValidatingTopLevelNodes); + } + private class Person { public string Name { get; set; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderTest.cs index b34cd09da5..bc9cdf1981 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/CollectionModelBinderTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Globalization; using System.Linq; +using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Internal; @@ -211,13 +212,21 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Empty(boundCollection.Model); } - [Fact] - public async Task CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() + private IActionResult ActionWithListParameter(List parameter) => null; + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObject( + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var binder = new CollectionModelBinder( new StubModelBinder(result: ModelBindingResult.Failed()), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.IsTopLevelObject = true; @@ -226,7 +235,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders bindingContext.ModelName = "modelName"; var metadataProvider = new TestModelMetadataProvider(); - bindingContext.ModelMetadata = metadataProvider.GetMetadataForType(typeof(List)); + var parameter = typeof(CollectionModelBinderTest) + .GetMethod(nameof(ActionWithListParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); @@ -236,6 +251,45 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.Empty(Assert.IsType>(bindingContext.Result.Model)); Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); + } + + [Fact] + public async Task CollectionModelBinder_CreatesEmptyCollectionAndAddsError_IfIsTopLevelObject() + { + // Arrange + var binder = new CollectionModelBinder( + new StubModelBinder(result: ModelBindingResult.Failed()), + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + var bindingContext = CreateContext(); + bindingContext.IsTopLevelObject = true; + bindingContext.FieldName = "fieldName"; + bindingContext.ModelName = "modelName"; + + var metadataProvider = new TestModelMetadataProvider(); + var parameter = typeof(CollectionModelBinderTest) + .GetMethod(nameof(ActionWithListParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); + + bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.Empty(Assert.IsType>(bindingContext.Result.Model)); + Assert.True(bindingContext.Result.IsModelSet); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal("modelName", keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); } // Setup like CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObject except @@ -272,19 +326,32 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders } [Theory] - [InlineData("")] - [InlineData("param")] - public async Task CollectionModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) + [InlineData("", false, false)] + [InlineData("", true, false)] + [InlineData("", false, true)] + [InlineData("", true, true)] + [InlineData("param", false, false)] + [InlineData("param", true, false)] + [InlineData("param", false, true)] + [InlineData("param", true, true)] + public async Task CollectionModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject( + string prefix, + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var binder = new CollectionModelBinder( new StubModelBinder(result: ModelBindingResult.Failed()), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForProperty(typeof(ModelWithListProperty), nameof(ModelWithListProperty.ListProperty)) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); bindingContext.ModelMetadata = metadataProvider.GetMetadataForProperty( typeof(ModelWithListProperty), nameof(ModelWithListProperty.ListProperty)); @@ -296,6 +363,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.False(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); } // Model type -> can create instance. @@ -365,15 +433,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders typeof(ModelWithIListProperty), nameof(ModelWithIListProperty.ListProperty)); - var bindingContext = new DefaultModelBindingContext - { - ModelMetadata = metadata, - ModelName = "someName", - ModelState = new ModelStateDictionary(), - ValueProvider = valueProvider, - ValidationState = new ValidationStateDictionary(), - FieldName = "testfieldname", - }; + var bindingContext = CreateContext(); + bindingContext.FieldName = "testfieldname"; + bindingContext.ModelName = "someName"; + bindingContext.ModelMetadata = metadata; + bindingContext.ValueProvider = valueProvider; return bindingContext; } @@ -412,12 +476,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders private static DefaultModelBindingContext CreateContext() { + var actionContext = new ActionContext() + { + HttpContext = new DefaultHttpContext(), + }; var modelBindingContext = new DefaultModelBindingContext() { - ActionContext = new ActionContext() - { - HttpContext = new DefaultHttpContext(), - }, + ActionContext = actionContext, + ModelState = actionContext.ModelState, + ValidationState = new ValidationStateDictionary(), }; return modelBindingContext; 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 9dc044f814..45e27d4ddc 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs @@ -55,6 +55,38 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType(result); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Create_ForSupportedType_ReturnsBinder_WithExpectedAllowValidatingTopLevelNodes( + bool allowValidatingTopLevelNodes) + { + // Arrange + var provider = new ComplexTypeModelBinderProvider(); + + var context = new TestModelBinderProviderContext(typeof(Person)); + context.MvcOptions.AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; + context.OnCreatingBinder(m => + { + if (m.ModelType == typeof(int) || m.ModelType == typeof(string)) + { + return Mock.Of(); + } + else + { + Assert.False(true, "Not the right model type"); + return null; + } + }); + + // Act + var result = provider.GetBinder(context); + + // Assert + var binder = Assert.IsType(result); + Assert.Equal(allowValidatingTopLevelNodes, binder.AllowValidatingTopLevelNodes); + } + private class Person { public string Name { get; set; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs index 7eaedfa106..32314d7306 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Linq; +using System.Reflection; using System.Runtime.Serialization; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; @@ -275,8 +276,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Equal(expectedCanCreate, canCreate); } - [Fact] - public async Task BindModelAsync_CreatesModel_IfIsTopLevelObject() + private IActionResult ActionWithComplexParameter(Person parameter) => null; + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task BindModelAsync_CreatesModel_IfIsTopLevelObject( + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var mockValueProvider = new Mock(); @@ -287,6 +295,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Mock binder fails to bind all properties. var mockBinder = new StubModelBinder(); + var parameter = typeof(ComplexTypeModelBinderTest) + .GetMethod(nameof(ActionWithComplexParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); + var metadata = metadataProvider.GetMetadataForParameter(parameter); var bindingContext = new DefaultModelBindingContext { IsTopLevelObject = true, @@ -298,7 +314,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var model = new Person(); - var testableBinder = new Mock { CallBase = true }; + var testableBinder = new Mock(allowValidatingTopLevelNodes) + { + CallBase = true + }; testableBinder .Setup(o => o.CreateModelPublic(bindingContext)) .Returns(model) @@ -312,11 +331,149 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); + var returnedPerson = Assert.IsType(bindingContext.Result.Model); Assert.Same(model, returnedPerson); testableBinder.Verify(); } + [Fact] + public async Task BindModelAsync_CreatesModelAndAddsError_IfIsTopLevelObject_WithNoData() + { + // Arrange + var parameter = typeof(ComplexTypeModelBinderTest) + .GetMethod(nameof(ActionWithComplexParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + var metadata = metadataProvider.GetMetadataForParameter(parameter); + var bindingContext = new DefaultModelBindingContext + { + IsTopLevelObject = true, + FieldName = "fieldName", + ModelMetadata = metadata, + ModelName = string.Empty, + ValueProvider = new TestValueProvider(new Dictionary()), + ModelState = new ModelStateDictionary(), + }; + + // Mock binder fails to bind all properties. + var innerBinder = new StubModelBinder(); + var binders = new Dictionary(); + foreach (var property in metadataProvider.GetMetadataForProperties(typeof(Person))) + { + binders.Add(property, innerBinder); + } + + var binder = new ComplexTypeModelBinder( + binders, + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.IsType(bindingContext.Result.Model); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal(string.Empty, keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); + } + + private IActionResult ActionWithNoSettablePropertiesParameter(PersonWithNoProperties parameter) => null; + + [Fact] + public async Task BindModelAsync_CreatesModelAndAddsError_IfIsTopLevelObject_WithNoSettableProperties() + { + // Arrange + var parameter = typeof(ComplexTypeModelBinderTest) + .GetMethod( + nameof(ActionWithNoSettablePropertiesParameter), + BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + var metadata = metadataProvider.GetMetadataForParameter(parameter); + var bindingContext = new DefaultModelBindingContext + { + IsTopLevelObject = true, + FieldName = "fieldName", + ModelMetadata = metadata, + ModelName = string.Empty, + ValueProvider = new TestValueProvider(new Dictionary()), + ModelState = new ModelStateDictionary(), + }; + + var binder = new ComplexTypeModelBinder( + new Dictionary(), + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.IsType(bindingContext.Result.Model); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal(string.Empty, keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); + } + + private IActionResult ActionWithAllPropertiesExcludedParameter(PersonWithAllPropertiesExcluded parameter) => null; + + [Fact] + public async Task BindModelAsync_CreatesModelAndAddsError_IfIsTopLevelObject_WithAllPropertiesExcluded() + { + // Arrange + var parameter = typeof(ComplexTypeModelBinderTest) + .GetMethod( + nameof(ActionWithAllPropertiesExcludedParameter), + BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + var metadata = metadataProvider.GetMetadataForParameter(parameter); + var bindingContext = new DefaultModelBindingContext + { + IsTopLevelObject = true, + FieldName = "fieldName", + ModelMetadata = metadata, + ModelName = string.Empty, + ValueProvider = new TestValueProvider(new Dictionary()), + ModelState = new ModelStateDictionary(), + }; + + var binder = new ComplexTypeModelBinder( + new Dictionary(), + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.IsType(bindingContext.Result.Model); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal(string.Empty, keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); + } + [Theory] [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyInt), false)] // read-only value type [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyObject), true)] @@ -644,7 +801,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var modelError = Assert.Single(entry.Errors); Assert.Null(modelError.Exception); Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("A value for the 'Age' property was not provided.", modelError.ErrorMessage); + Assert.Equal("A value for the 'Age' parameter or property was not provided.", modelError.ErrorMessage); } [Fact] @@ -678,7 +835,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var modelError = Assert.Single(entry.Errors); Assert.Null(modelError.Exception); Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("A value for the 'Age' property was not provided.", modelError.ErrorMessage); + Assert.Equal("A value for the 'Age' parameter or property was not provided.", modelError.ErrorMessage); } [Fact] @@ -1203,6 +1360,23 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public string name = null; } + private class PersonWithAllPropertiesExcluded + { + [BindNever] + public DateTime DateOfBirth { get; set; } + + [BindNever] + public DateTime? DateOfDeath { get; set; } + + [BindNever] + public string FirstName { get; set; } + + [BindNever] + public string LastName { get; set; } + + public string NonUpdateableProperty { get; private set; } + } + private class PersonWithBindExclusion { [BindNever] @@ -1405,13 +1579,24 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { } + public TestableComplexTypeModelBinder(bool allowValidatingTopLevelNodes) + : this(new Dictionary(), allowValidatingTopLevelNodes) + { + } + public TestableComplexTypeModelBinder(IDictionary propertyBinders) : base(propertyBinders, NullLoggerFactory.Instance) { - Results = new Dictionary(); } - public Dictionary Results { get; } + public TestableComplexTypeModelBinder( + IDictionary propertyBinders, + bool allowValidatingTopLevelNodes) + : base(propertyBinders, NullLoggerFactory.Instance, allowValidatingTopLevelNodes) + { + } + + public Dictionary Results { get; } = new Dictionary(); public virtual Task BindPropertyPublic(ModelBindingContext bindingContext) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderProviderTest.cs index d60c3b2628..6d7add41c7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderProviderTest.cs @@ -60,6 +60,39 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType>(result); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Create_ForDictionaryType_ReturnsBinder_WithExpectedAllowValidatingTopLevelNodes( + bool allowValidatingTopLevelNodes) + { + // Arrange + var provider = new DictionaryModelBinderProvider(); + + var context = new TestModelBinderProviderContext(typeof(Dictionary)); + context.MvcOptions.AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; + context.OnCreatingBinder(m => + { + if (m.ModelType == typeof(KeyValuePair) || m.ModelType == typeof(string)) + { + return Mock.Of(); + } + else + { + Assert.False(true, "Not the right model type"); + return null; + } + }); + + // Act + var result = provider.GetBinder(context); + + // Assert + var binder = Assert.IsType>(result); + Assert.Equal(allowValidatingTopLevelNodes, binder.AllowValidatingTopLevelNodes); + Assert.False(((CollectionModelBinder>)binder).AllowValidatingTopLevelNodes); + } + private class Person { public string Name { get; set; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs index 2beb2a33f9..db059c0090 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Globalization; using System.Linq; +using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Internal; @@ -343,14 +344,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Equal(expectedDictionary, resultDictionary); } - [Fact] - public async Task DictionaryModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() + private IActionResult ActionWithDictionaryParameter(Dictionary parameter) => null; + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task DictionaryModelBinder_CreatesEmptyCollection_IfIsTopLevelObject( + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var binder = new DictionaryModelBinder( new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.IsTopLevelObject = true; @@ -359,7 +368,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders bindingContext.ModelName = "modelName"; var metadataProvider = new TestModelMetadataProvider(); - bindingContext.ModelMetadata = metadataProvider.GetMetadataForType(typeof(Dictionary)); + var parameter = typeof(DictionaryModelBinderTest) + .GetMethod(nameof(ActionWithDictionaryParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); @@ -369,23 +384,78 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.Empty(Assert.IsType>(bindingContext.Result.Model)); Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); + } + + [Fact] + public async Task DictionaryModelBinder_CreatesEmptyCollectionAndAddsError_IfIsTopLevelObject() + { + // Arrange + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), + new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance), + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes: true); + + var bindingContext = CreateContext(); + bindingContext.IsTopLevelObject = true; + bindingContext.FieldName = "fieldName"; + bindingContext.ModelName = "modelName"; + + var metadataProvider = new TestModelMetadataProvider(); + var parameter = typeof(DictionaryModelBinderTest) + .GetMethod(nameof(ActionWithDictionaryParameter), BindingFlags.Instance | BindingFlags.NonPublic) + .GetParameters()[0]; + metadataProvider + .ForParameter(parameter) + .BindingDetails(b => b.IsBindingRequired = true); + bindingContext.ModelMetadata = metadataProvider.GetMetadataForParameter(parameter); + + bindingContext.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.Empty(Assert.IsType>(bindingContext.Result.Model)); + Assert.True(bindingContext.Result.IsModelSet); + + var keyValuePair = Assert.Single(bindingContext.ModelState); + Assert.Equal("modelName", keyValuePair.Key); + var error = Assert.Single(keyValuePair.Value.Errors); + Assert.Equal("A value for the 'fieldName' parameter or property was not provided.", error.ErrorMessage); } [Theory] - [InlineData("")] - [InlineData("param")] - public async Task DictionaryModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) + [InlineData("", false, false)] + [InlineData("", true, false)] + [InlineData("", false, true)] + [InlineData("", true, true)] + [InlineData("param", false, false)] + [InlineData("param", true, false)] + [InlineData("param", false, true)] + [InlineData("param", true, true)] + public async Task DictionaryModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject( + string prefix, + bool allowValidatingTopLevelNodes, + bool isBindingRequired) { // Arrange var binder = new DictionaryModelBinder( new SimpleTypeModelBinder(typeof(int), NullLoggerFactory.Instance), new SimpleTypeModelBinder(typeof(int), NullLoggerFactory.Instance), - NullLoggerFactory.Instance); + NullLoggerFactory.Instance, + allowValidatingTopLevelNodes); var bindingContext = CreateContext(); bindingContext.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); var metadataProvider = new TestModelMetadataProvider(); + metadataProvider + .ForProperty( + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryProperty)) + .BindingDetails(b => b.IsBindingRequired = isBindingRequired); bindingContext.ModelMetadata = metadataProvider.GetMetadataForProperty( typeof(ModelWithDictionaryProperties), nameof(ModelWithDictionaryProperties.DictionaryProperty)); @@ -397,6 +467,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.False(bindingContext.Result.IsModelSet); + Assert.Equal(0, bindingContext.ModelState.ErrorCount); } // Model type -> can create instance. @@ -436,13 +507,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders private static DefaultModelBindingContext CreateContext() { + var actionContext = new ActionContext() + { + HttpContext = new DefaultHttpContext(), + }; var modelBindingContext = new DefaultModelBindingContext() { - ActionContext = new ActionContext() - { - HttpContext = new DefaultHttpContext(), - }, - ModelState = new ModelStateDictionary(), + ActionContext = actionContext, + ModelState = actionContext.ModelState, ValidationState = new ValidationStateDictionary(), }; @@ -495,14 +567,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders valueProvider.Add(kvp.Key, string.Empty); } - var bindingContext = new DefaultModelBindingContext - { - ModelMetadata = metadata, - ModelName = "someName", - ModelState = new ModelStateDictionary(), - ValueProvider = valueProvider, - ValidationState = new ValidationStateDictionary(), - }; + var bindingContext = CreateContext(); + bindingContext.ModelMetadata = metadata; + bindingContext.ModelName = "someName"; + bindingContext.ValueProvider = valueProvider; return bindingContext; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/TestModelBinderProviderContext.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/TestModelBinderProviderContext.cs index 514e012c76..d0c29dae8d 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/TestModelBinderProviderContext.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/TestModelBinderProviderContext.cs @@ -36,13 +36,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding BindingSource = Metadata.BindingSource, PropertyFilterProvider = Metadata.PropertyFilterProvider, }; - Services = GetServices(); + + (Services, MvcOptions) = GetServicesAndOptions(); } public override BindingInfo BindingInfo => _bindingInfo; public override ModelMetadata Metadata { get; } + public MvcOptions MvcOptions { get; } + public override IModelMetadataProvider MetadataProvider { get; } public override IServiceProvider Services { get; } @@ -77,12 +80,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding _binderCreators.Add((m) => m.Equals(metadata) ? binderCreator() : null); } - private static IServiceProvider GetServices() + private static (IServiceProvider, MvcOptions) GetServicesAndOptions() { var services = new ServiceCollection(); services.AddSingleton(); - services.AddSingleton(Options.Create(new MvcOptions())); - return services.BuildServiceProvider(); + + var mvcOptions = new MvcOptions(); + services.AddSingleton(Options.Create(mvcOptions)); + + return (services.BuildServiceProvider(), mvcOptions); } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputValidationTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputValidationTests.cs index 5c5c9e7189..2e87b4b44c 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputValidationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputValidationTests.cs @@ -89,10 +89,10 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests "The RequiredProp field is required.", errors["RequiredProp"]); Assert.Equal( - "A value for the 'BindRequiredProp' property was not provided.", + "A value for the 'BindRequiredProp' parameter or property was not provided.", errors["BindRequiredProp"]); Assert.Equal( - "A value for the 'RequiredAndBindRequiredProp' property was not provided.", + "A value for the 'RequiredAndBindRequiredProp' parameter or property was not provided.", errors["RequiredAndBindRequiredProp"]); Assert.Equal( "The field OptionalStringLengthProp must be a string with a maximum length of 5.", @@ -104,10 +104,10 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests "The requiredParam field is required.", errors["requiredParam"]); Assert.Equal( - "A value for the 'bindRequiredParam' property was not provided.", + "A value for the 'bindRequiredParam' parameter or property was not provided.", errors["bindRequiredParam"]); Assert.Equal( - "A value for the 'requiredAndBindRequiredParam' property was not provided.", + "A value for the 'requiredAndBindRequiredParam' parameter or property was not provided.", errors["requiredAndBindRequiredParam"]); Assert.Equal( "The field optionalStringLengthParam must be a string with a maximum length of 5.", diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index 7cb0c782d2..aec3949a3d 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -333,13 +333,13 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Equal(ModelValidationState.Invalid, entry.ValidationState); var error = Assert.Single(entry.Errors); - Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); entry = Assert.Single(modelState, kvp => kvp.Key == "parameter[1].Name").Value; Assert.Null(entry.RawValue); Assert.Equal(ModelValidationState.Invalid, entry.ValidationState); error = Assert.Single(entry.Errors); - Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs index 8f4fc6470f..beffb6d9a1 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs @@ -2123,7 +2123,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["Customer"].Errors); - Assert.Equal("A value for the 'Customer' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Customer' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2245,7 +2245,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["parameter.Customer.Name"].Errors); - Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2299,7 +2299,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["Customer.Name"].Errors); - Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2357,7 +2357,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["customParameter.Customer.Name"].Errors); - Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' parameter or property was not provided.", error.ErrorMessage); } private class Order12 @@ -2411,7 +2411,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["ProductName"].Errors); - Assert.Equal("A value for the 'ProductName' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'ProductName' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2463,7 +2463,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["customParameter.ProductName"].Errors); - Assert.Equal("A value for the 'ProductName' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'ProductName' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2563,7 +2563,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["OrderIds"].Errors); - Assert.Equal("A value for the 'OrderIds' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'OrderIds' parameter or property was not provided.", error.ErrorMessage); } [Fact] @@ -2615,7 +2615,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(entry.RawValue); Assert.Null(entry.AttemptedValue); var error = Assert.Single(modelState["customParameter.OrderIds"].Errors); - Assert.Equal("A value for the 'OrderIds' property was not provided.", error.ErrorMessage); + Assert.Equal("A value for the 'OrderIds' parameter or property was not provided.", error.ErrorMessage); } [Fact]