From 98b3f055e190f57b7460a4b41e1973717e21d078 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 20 Aug 2015 20:33:06 -0700 Subject: [PATCH] Change ModelBinding to use a single pass --- .../ModelBinding/ModelBindingContext.cs | 18 +- .../ModelBinding/ModelBindingResult.cs | 7 - .../ModelBinding/CollectionModelBinder.cs | 4 +- .../ModelBinding/CompositeModelBinder.cs | 93 +++---- .../ModelBinding/FormFileModelBinder.cs | 11 +- .../ModelBinding/KeyValuePairModelBinder.cs | 4 +- .../ModelBinding/MutableObjectModelBinder.cs | 4 +- .../ModelBinding/ArrayModelBinderTest.cs | 28 +- .../ModelBinding/BodyModelBinderTests.cs | 4 - .../ModelBinding/CollectionModelBinderTest.cs | 30 +- .../ModelBinding/CompositeModelBinderTest.cs | 47 +--- .../ModelBinding/DictionaryModelBinderTest.cs | 33 +-- .../ModelBinding/FormFileModelBinderTest.cs | 32 +++ .../KeyValuePairModelBinderTest.cs | 28 +- .../ModelBinding/ModelBindingContextTest.cs | 1 - .../MutableObjectModelBinderTest.cs | 30 +- .../ModelBindingHelperTest.cs | 2 +- .../ModelBindingTest.cs | 10 +- ...nderTypeBasedModelBinderIntegrationTest.cs | 4 +- .../ModelPrefixSelectionIntegrationTest.cs | 256 ++++++++++++++++++ 20 files changed, 367 insertions(+), 279 deletions(-) create mode 100644 test/Microsoft.AspNet.Mvc.IntegrationTests/ModelPrefixSelectionIntegrationTest.cs diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingContext.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingContext.cs index 9b161125f8..9dc3ead100 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingContext.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingContext.cs @@ -11,9 +11,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public class ModelBindingContext { - private static readonly Func - _defaultPropertyFilter = (context, propertyName) => true; - /// /// Initializes a new instance of the class. /// @@ -167,19 +164,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// Passed into the model binding system. public bool IsTopLevelObject { get; set; } - /// - /// Gets or sets an indication that the model binding system will make another binding attempt (e.g. fall back - /// to the empty prefix) after this one. - /// - /// - /// Not passed into the model binding system but instead set by the top-level binder. With built-in binders, - /// true only in binders called directly from a - /// Microsoft.AspNet.Mvc.ModelBinding.CompositeModelBinder that was passed a - /// with true. false - /// otherwise. - /// - public bool IsFirstChanceBinding { get; set; } - /// /// Gets or sets the associated with this context. /// @@ -191,4 +175,4 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public Func PropertyFilter { get; set; } } -} \ No newline at end of file +} diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingResult.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingResult.cs index 941afaf25a..8850f96c6c 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingResult.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelBindingResult.cs @@ -15,7 +15,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public ModelBindingResult(string key) { Key = key; - IsFatalError = true; } /// @@ -62,12 +61,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public string Key { get; } - /// - /// Gets a value indicating the caller should not attempt binding again. This attempt encountered a fatal - /// error. - /// - public bool IsFatalError { get; } - /// /// /// Gets a value indicating whether or not the value has been set. diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs index a2e3561e41..6df8fc7510 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs @@ -29,9 +29,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var model = bindingContext.Model; if (!bindingContext.ValueProvider.ContainsPrefix(bindingContext.ModelName)) { - // If this is the fallback case and we failed to find data for a top-level model, then generate a + // If we failed to find data for a top-level model, then generate a // default 'empty' model (or use existing Model) and return it. - if (!bindingContext.IsFirstChanceBinding && bindingContext.IsTopLevelObject) + if (bindingContext.IsTopLevelObject) { if (model == null) { diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs index be58a547e9..93b2cff5e8 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.Core; @@ -35,29 +34,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public virtual async Task BindModelAsync([NotNull] ModelBindingContext bindingContext) { - // Will there be a last chance (fallback) binding attempt? - var isFirstChanceBinding = bindingContext.FallbackToEmptyPrefix && - !string.IsNullOrEmpty(bindingContext.ModelName); - - var newBindingContext = CreateNewBindingContext(bindingContext, bindingContext.ModelName); + var newBindingContext = CreateNewBindingContext(bindingContext); if (newBindingContext == null) { // Unable to find a value provider for this binding source. Binding will fail. return null; } - newBindingContext.IsFirstChanceBinding = isFirstChanceBinding; - var modelBindingResult = await TryBind(newBindingContext); - - if (modelBindingResult == null && isFirstChanceBinding) - { - // Fall back to empty prefix. - newBindingContext = CreateNewBindingContext(bindingContext, modelName: string.Empty); - Debug.Assert(newBindingContext != null, "Should have failed on first attempt."); - - modelBindingResult = await TryBind(newBindingContext); - } - + var modelBindingResult = await RunModelBinders(newBindingContext); if (modelBindingResult == null) { // Unable to bind or something went wrong. @@ -99,7 +83,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding modelBindingResult.ValidationNode); } - private async Task TryBind(ModelBindingContext bindingContext) + private async Task RunModelBinders(ModelBindingContext bindingContext) { RuntimeHelpers.EnsureSufficientExecutionStack(); @@ -108,18 +92,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var result = await binder.BindModelAsync(bindingContext); if (result != null) { - // Use returned ModelBindingResult if it indicates the model was set, indicates the binder - // encountered a fatal error, or is related to a ModelState entry. - // - // The second condition is necessary because the BodyModelBinder unconditionally binds during the - // first attempt and does not always create ModelState values using ModelName. - // - // The third condition is necessary because the ModelState entry would never be validated if + // This condition is necessary because the ModelState entry would never be validated if // caller fell back to the empty prefix, leading to an possibly-incorrect !IsValid. In most // (hopefully all) cases, the ModelState entry exists because some binders add errors before // returning a result with !IsModelSet. Those binders often cannot run twice anyhow. - if (result.IsFatalError || - result.IsModelSet || + if (result.IsModelSet || bindingContext.ModelState.ContainsKey(bindingContext.ModelName)) { return result; @@ -136,26 +113,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return null; } - private static ModelBindingContext CreateNewBindingContext( - ModelBindingContext oldBindingContext, - string modelName) + private static ModelBindingContext CreateNewBindingContext(ModelBindingContext oldBindingContext) { - var newBindingContext = new ModelBindingContext - { - Model = oldBindingContext.Model, - ModelMetadata = oldBindingContext.ModelMetadata, - ModelName = modelName, - FieldName = oldBindingContext.FieldName, - ModelState = oldBindingContext.ModelState, - ValueProvider = oldBindingContext.ValueProvider, - OperationBindingContext = oldBindingContext.OperationBindingContext, - PropertyFilter = oldBindingContext.PropertyFilter, - BinderModelName = oldBindingContext.BinderModelName, - BindingSource = oldBindingContext.BindingSource, - BinderType = oldBindingContext.BinderType, - IsTopLevelObject = oldBindingContext.IsTopLevelObject, - }; - // If the property has a specified data binding sources, we need to filter the set of value providers // to just those that match. We can skip filtering when IsGreedy == true, because that can't use // value providers. @@ -174,15 +133,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // public IActionResult UpdatePerson([FromForm] Person person) { } // // In this example, [FromQuery] overrides the ambient data source (form). + IValueProvider valueProvider = oldBindingContext.ValueProvider; var bindingSource = oldBindingContext.BindingSource; if (bindingSource != null && !bindingSource.IsGreedy) { - var valueProvider = - oldBindingContext.OperationBindingContext.ValueProvider as IBindingSourceValueProvider; - if (valueProvider != null) + var bindingSourceValueProvider = valueProvider as IBindingSourceValueProvider; + if (bindingSourceValueProvider != null) { - newBindingContext.ValueProvider = valueProvider.Filter(bindingSource); - if (newBindingContext.ValueProvider == null) + valueProvider = bindingSourceValueProvider.Filter(bindingSource); + if (valueProvider == null) { // Unable to find a value provider for this binding source. return null; @@ -190,6 +149,36 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } + var newBindingContext = new ModelBindingContext + { + Model = oldBindingContext.Model, + ModelMetadata = oldBindingContext.ModelMetadata, + FieldName = oldBindingContext.FieldName, + ModelState = oldBindingContext.ModelState, + ValueProvider = valueProvider, + OperationBindingContext = oldBindingContext.OperationBindingContext, + PropertyFilter = oldBindingContext.PropertyFilter, + BinderModelName = oldBindingContext.BinderModelName, + BindingSource = oldBindingContext.BindingSource, + BinderType = oldBindingContext.BinderType, + IsTopLevelObject = oldBindingContext.IsTopLevelObject, + }; + + if (bindingSource != null && bindingSource.IsGreedy) + { + newBindingContext.ModelName = oldBindingContext.ModelName; + } + else if ( + !oldBindingContext.FallbackToEmptyPrefix || + newBindingContext.ValueProvider.ContainsPrefix(oldBindingContext.ModelName)) + { + newBindingContext.ModelName = oldBindingContext.ModelName; + } + else + { + newBindingContext.ModelName = string.Empty; + } + return newBindingContext; } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs index ef9b8ef5b9..119147d89f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs @@ -69,6 +69,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { var form = await request.ReadFormAsync(); + // If we're at the top level, then use the FieldName (paramter or property name). + // This handles the fact that there will be nothing in the ValueProviders for this parameter + // and so we'll do the right thing even though we 'fell-back' to the empty prefix. + var modelName = bindingContext.IsTopLevelObject + ? bindingContext.FieldName + : bindingContext.ModelName; + foreach (var file in form.Files) { ContentDispositionHeaderValue parsedContentDisposition; @@ -82,8 +89,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding continue; } - var modelName = HeaderUtilities.RemoveQuotes(parsedContentDisposition.Name); - if (modelName.Equals(bindingContext.ModelName, StringComparison.OrdinalIgnoreCase)) + var fileName = HeaderUtilities.RemoveQuotes(parsedContentDisposition.Name); + if (fileName.Equals(modelName, StringComparison.OrdinalIgnoreCase)) { postedFiles.Add(file); } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs index 4a54e68747..acaf075d0f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs @@ -62,9 +62,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } else { - // If this is the fallback case and we failed to find data for a top-level model, then generate a + // If we failed to find data for a top-level model, then generate a // default 'empty' model and return it. - if (!bindingContext.IsFirstChanceBinding && bindingContext.IsTopLevelObject) + if (bindingContext.IsTopLevelObject) { var model = new KeyValuePair(); diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index 7e65459545..484c04d9d5 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -96,8 +96,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } // Create the object if: - // 1. It is a top level model and no later fallback (to empty prefix) will occur. - if (isTopLevelObject && !bindingContext.IsFirstChanceBinding) + // 1. It is a top level model. + if (isTopLevelObject) { return true; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs index 7c8caeeb23..82f398c658 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs @@ -37,33 +37,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task ArrayModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding() - { - // Arrange - var binder = new ArrayModelBinder(); - - var context = CreateContext(); - context.IsFirstChanceBinding = true; - context.IsTopLevelObject = true; - - // Explicit prefix and empty model name both ignored. - context.BinderModelName = "prefix"; - context.ModelName = string.Empty; - - var metadataProvider = context.OperationBindingContext.MetadataProvider; - context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[])); - - context.ValueProvider = new TestValueProvider(new Dictionary()); - - // Act - var result = await binder.BindModelAsync(context); - - // Assert - Assert.Null(result); - } - - [Fact] - public async Task ArrayModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding() + public async Task ArrayModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange var binder = new ArrayModelBinder(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs index 451a3d48bc..6b0a1da765 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs @@ -76,7 +76,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Returns non-null because it understands the metadata type. Assert.NotNull(binderResult); - Assert.True(binderResult.IsFatalError); Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.ValidationNode); Assert.Null(binderResult.Model); @@ -103,7 +102,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert Assert.NotNull(binderResult); - Assert.True(binderResult.IsFatalError); Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.ValidationNode); } @@ -172,7 +170,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Returns non-null because it understands the metadata type. Assert.NotNull(binderResult); - Assert.True(binderResult.IsFatalError); Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.ValidationNode); Assert.Null(binderResult.Model); @@ -209,7 +206,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Returns non-null result because it understands the metadata type. Assert.NotNull(binderResult); - Assert.True(binderResult.IsFatalError); Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); Assert.Null(binderResult.ValidationNode); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs index 5855ec4008..65bcf56bfa 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs @@ -227,33 +227,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task CollectionModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding() - { - // Arrange - var binder = new CollectionModelBinder(); - - var context = CreateContext(); - context.IsTopLevelObject = true; - context.IsFirstChanceBinding = true; - - // Explicit prefix and empty model name both ignored. - context.BinderModelName = "prefix"; - context.ModelName = string.Empty; - - var metadataProvider = context.OperationBindingContext.MetadataProvider; - context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(List)); - - context.ValueProvider = new TestValueProvider(new Dictionary()); - - // Act - var result = await binder.BindModelAsync(context); - - // Assert - Assert.Null(result); - } - - [Fact] - public async Task CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding() + public async Task CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange var binder = new CollectionModelBinder(); @@ -284,7 +258,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Same(result.ValidationNode.ModelMetadata, context.ModelMetadata); } - // Setup like CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding except + // Setup like CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObject except // Model already has a value. [Fact] public async Task CollectionModelBinder_DoesNotCreateEmptyCollection_IfModelNonNull() diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeModelBinderTest.cs index 2a7da19eca..9fb43421fc 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CompositeModelBinderTest.cs @@ -6,7 +6,6 @@ using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.Threading.Tasks; -using Microsoft.AspNet.Mvc.ModelBinding.Validation; using Moq; using Xunit; @@ -65,6 +64,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var bindingContext = new ModelBindingContext { FallbackToEmptyPrefix = true, + IsTopLevelObject = true, ModelMetadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(List)), ModelName = "someName", ModelState = new ModelStateDictionary(), @@ -136,49 +136,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Null(result); } - [Fact] - public async Task ModelBinder_FallsBackToEmpty_IfBinderMatchesButDoesNotSetModel() - { - // Arrange - var bindingContext = new ModelBindingContext - { - FallbackToEmptyPrefix = true, - ModelMetadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(List)), - ModelName = "someName", - ModelState = new ModelStateDictionary(), - OperationBindingContext = new OperationBindingContext(), - ValueProvider = new SimpleValueProvider - { - { "someOtherName", "dummyValue" } - }, - }; - - var count = 0; - var modelBinder = new Mock(); - modelBinder - .Setup(mb => mb.BindModelAsync(It.IsAny())) - .Callback(context => - { - // Expect two calls; the second with empty ModelName. - Assert.InRange(count, 0, 1); - count++; - if (count == 1) - { - Assert.Equal("someName", context.ModelName); - } - else - { - Assert.Empty(context.ModelName); - } - }) - .Returns(Task.FromResult(new ModelBindingResult(model: null, key: "someName", isModelSet: false))); - - var composite = CreateCompositeBinder(modelBinder.Object); - - // Act & Assert - var result = await composite.BindModelAsync(bindingContext); - } - [Fact] public async Task ModelBinder_DoesNotFallBackToEmpty_IfFallbackToEmptyPrefixFalse() { @@ -322,6 +279,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test ModelMetadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(int)), ModelState = new ModelStateDictionary(), OperationBindingContext = new OperationBindingContext(), + ValueProvider = new SimpleValueProvider(), }; // Act @@ -493,6 +451,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var bindingContext = new ModelBindingContext { FallbackToEmptyPrefix = true, + IsTopLevelObject = true, ModelMetadata = metadataProvider.GetMetadataForType(type), ModelName = "parameter", ModelState = new ModelStateDictionary(), diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs index bdabc7ae90..658826e1d3 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs @@ -133,7 +133,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test // Assert Assert.NotNull(result); - Assert.False(result.IsFatalError); Assert.True(result.IsModelSet); Assert.Equal(modelName, result.Key); Assert.NotNull(result.ValidationNode); @@ -171,7 +170,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test // Assert Assert.NotNull(result); - Assert.False(result.IsFatalError); Assert.True(result.IsModelSet); Assert.Equal("prefix", result.Key); Assert.NotNull(result.ValidationNode); @@ -223,7 +221,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test // Assert Assert.NotNull(result); - Assert.False(result.IsFatalError); Assert.True(result.IsModelSet); Assert.Equal("prefix", result.Key); Assert.NotNull(result.ValidationNode); @@ -265,7 +262,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test // Assert Assert.NotNull(result); - Assert.False(result.IsFatalError); Assert.True(result.IsModelSet); Assert.Equal("prefix", result.Key); Assert.NotNull(result.ValidationNode); @@ -300,7 +296,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test // Assert Assert.NotNull(result); - Assert.False(result.IsFatalError); Assert.True(result.IsModelSet); Assert.Equal(modelName, result.Key); Assert.NotNull(result.ValidationNode); @@ -310,33 +305,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task DictionaryModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding() - { - // Arrange - var binder = new DictionaryModelBinder(); - - var context = CreateContext(); - context.IsTopLevelObject = true; - context.IsFirstChanceBinding = true; - - // Explicit prefix and empty model name both ignored. - context.BinderModelName = "prefix"; - context.ModelName = string.Empty; - - var metadataProvider = context.OperationBindingContext.MetadataProvider; - context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(Dictionary)); - - context.ValueProvider = new TestValueProvider(new Dictionary()); - - // Act - var result = await binder.BindModelAsync(context); - - // Assert - Assert.Null(result); - } - - [Fact] - public async Task DictionaryModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding() + public async Task DictionaryModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange var binder = new DictionaryModelBinder(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs index 135b8388fe..72ed422443 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs @@ -9,6 +9,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Http.Internal; +using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -116,6 +117,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Null(result.Model); } + [Theory] + [InlineData(true, "FieldName")] + [InlineData(false, "ModelName")] + public async Task FormFileModelBinder_UsesFieldNameForTopLevelObject(bool isTopLevel, string expected) + { + // Arrange + var formFiles = new FormFileCollection(); + formFiles.Add(GetMockFormFile("FieldName", "file1.txt")); + formFiles.Add(GetMockFormFile("ModelName", "file1.txt")); + var httpContext = GetMockHttpContext(GetMockFormCollection(formFiles)); + + var bindingContext = GetBindingContext(typeof(IFormFile), httpContext); + bindingContext.IsTopLevelObject = isTopLevel; + bindingContext.FieldName = "FieldName"; + bindingContext.ModelName = "ModelName"; + + var binder = new FormFileModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + var file = Assert.IsAssignableFrom(result.Model); + + ContentDispositionHeaderValue contentDisposition; + ContentDispositionHeaderValue.TryParse(file.ContentDisposition, out contentDisposition); + Assert.Equal(expected, contentDisposition.Name); + } + [Fact] public async Task FormFileModelBinder_ReturnsNull_WithEmptyContentDisposition() { diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs index 5935dd4009..1fa38234cb 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs @@ -148,33 +148,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task KeyValuePairModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding() - { - // Arrange - var binder = new KeyValuePairModelBinder(); - - var context = CreateContext(); - context.IsTopLevelObject = true; - context.IsFirstChanceBinding = true; - - // Explicit prefix and empty model name both ignored. - context.BinderModelName = "prefix"; - context.ModelName = string.Empty; - - var metadataProvider = context.OperationBindingContext.MetadataProvider; - context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(KeyValuePair)); - - context.ValueProvider = new TestValueProvider(new Dictionary()); - - // Act - var result = await binder.BindModelAsync(context); - - // Assert - Assert.Null(result); - } - - [Fact] - public async Task KeyValuePairModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding() + public async Task KeyValuePairModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange var binder = new KeyValuePairModelBinder(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ModelBindingContextTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ModelBindingContextTest.cs index cebc30f69a..02cf9fdfcc 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ModelBindingContextTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ModelBindingContextTest.cs @@ -46,7 +46,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Same(newModelMetadata.BindingSource, newBindingContext.BindingSource); Assert.False(newBindingContext.FallbackToEmptyPrefix); Assert.Equal("fieldName", newBindingContext.FieldName); - Assert.False(newBindingContext.IsFirstChanceBinding); Assert.False(newBindingContext.IsTopLevelObject); Assert.Null(newBindingContext.Model); Assert.Same(newModelMetadata, newBindingContext.ModelMetadata); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs index 5f5e9c9453..279c9d3c8f 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -20,27 +20,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public class MutableObjectModelBinderTest { [Theory] - [InlineData(true, true, "", "", false)] - [InlineData(true, false, "", "", true)] - [InlineData(false, true, "", "", false)] // !isTopLevelObject && isFirstChanceBinding cases are unexpected - [InlineData(false, false, "", "", false)] - [InlineData(true, true, "prefix", "", false)] - [InlineData(true, false, "prefix", "", true)] - [InlineData(false, true, "prefix", "", false)] - [InlineData(false, false, "prefix", "", false)] - [InlineData(true, true, "", "dummyModelName", false)] - [InlineData(true, false, "", "dummyModelName", true)] - [InlineData(false, true, "", "dummyModelName", false)] - [InlineData(false, false, "", "dummyModelName", false)] - [InlineData(true, true, "prefix", "dummyModelName", false)] - [InlineData(true, false, "prefix", "dummyModelName", true)] - [InlineData(false, true, "prefix", "dummyModelName", false)] - [InlineData(false, false, "prefix", "dummyModelName", false)] - public void CanCreateModel_ReturnsTrue_IfIsTopLevelObjectAndNotIsFirstChanceBinding( + [InlineData(true, true)] + [InlineData(false, false)] + public void CanCreateModel_ReturnsTrue_IfIsTopLevelObject( bool isTopLevelObject, - bool isFirstChanceBinding, - string binderModelName, - string modelName, bool expectedCanCreate) { var mockValueProvider = new Mock(); @@ -54,7 +37,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding ModelBindingContext = new ModelBindingContext { IsTopLevelObject = isTopLevelObject, - IsFirstChanceBinding = isFirstChanceBinding, // Random type. ModelMetadata = metadataProvider.GetMetadataForType(typeof(Person)), @@ -65,10 +47,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding MetadataProvider = metadataProvider, ValidatorProvider = Mock.Of(), }, - - // CanCreateModel() ignores the BinderModelName and ModelName properties. - BinderModelName = binderModelName, - ModelName = modelName, }, }; @@ -467,7 +445,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } [Fact] - public async Task BindModel_InitsInstance_IfIsTopLevelObjectAndNotIsFirstChanceBinding() + public async Task BindModel_InitsInstance_IfIsTopLevelObject() { // Arrange var mockValueProvider = new Mock(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs index 3ce93de31d..8a20e2efa3 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs @@ -25,7 +25,7 @@ namespace Microsoft.AspNet.Mvc.Test return new TheoryData { null, - new ModelBindingResult("someKey"), // IsFatalError true as well as IsModelSet false. + new ModelBindingResult("someKey"), // IsModelSet false. new ModelBindingResult(model: null, key: "someKey", isModelSet: false), }; } diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs index eaa7ede274..660d92e459 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs @@ -734,10 +734,14 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // Assert var json = JsonConvert.DeserializeObject>(response); Assert.Equal(3, json.Count); + + // The model prefix 'model' is used in the modelstate keys because the key 'model' is present in the + // query string. This causes modelbinding to commit to using the prefix. + // // Mono issue - https://github.com/aspnet/External/issues/19 - Assert.Equal(PlatformNormalizer.NormalizeContent("The Field1 field is required."), json["Field1"]); - Assert.Equal(PlatformNormalizer.NormalizeContent("The Field2 field is required."), json["Field2"]); - Assert.Equal(PlatformNormalizer.NormalizeContent("The Field3 field is required."), json["Field3"]); + Assert.Equal(PlatformNormalizer.NormalizeContent("The Field1 field is required."), json["model.Field1"]); + Assert.Equal(PlatformNormalizer.NormalizeContent("The Field2 field is required."), json["model.Field2"]); + Assert.Equal(PlatformNormalizer.NormalizeContent("The Field3 field is required."), json["model.Field3"]); } [Fact] diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs index 3663e6b6e9..987bb1affd 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs @@ -226,7 +226,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests // ModelBindingResult Assert.NotNull(modelBindingResult); - Assert.Equal("Parameter1", modelBindingResult.Key); + Assert.Equal(string.Empty, modelBindingResult.Key); // Model var boundPerson = Assert.IsType(modelBindingResult.Model); @@ -237,7 +237,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.True(modelState.IsValid); Assert.Equal(1, modelState.Keys.Count); - var key = Assert.Single(modelState.Keys, k => k == "Parameter1.Address.Street"); + var key = Assert.Single(modelState.Keys, k => k == "Address.Street"); Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); Assert.NotNull(modelState[key].RawValue); // Value is set by test model binder, no need to validate it. } diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ModelPrefixSelectionIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ModelPrefixSelectionIntegrationTest.cs new file mode 100644 index 0000000000..d7f233f637 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ModelPrefixSelectionIntegrationTest.cs @@ -0,0 +1,256 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNet.Http; +using Microsoft.AspNet.Http.Internal; +using Microsoft.AspNet.Mvc.ModelBinding; +using Microsoft.Framework.Primitives; +using Xunit; + +namespace Microsoft.AspNet.Mvc.IntegrationTests +{ + // Integration tests for the decision logic about how a model-name/prefix is selected at the top-level + // of ModelBinding. + public class ModelPrefixSelectionIntegrationTest + { + private class Person1 + { + [FromForm] + public string Name { get; set; } + } + + [Fact] + public async Task ComplexModel_PrefixSelected_ByValueProvider() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Person1), + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + // This will cause selection of the "parameter" prefix. + request.QueryString = new QueryString("?parameter="); + + // This value won't be used, because we select the "parameter" prefix. + request.Form = new FormCollection(new Dictionary() + { + { "Name", "Billy" }, + }); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal("parameter", modelBindingResult.Key); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Null(model.Name); + + Assert.Equal(0, modelState.Count); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + + private class Person2 + { + [FromForm] + public string Name { get; set; } + } + + [Fact] + public async Task ComplexModel_PrefixSelected_ByValueProviderValue_WithFilteredValueProviders() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Person2), + BindingInfo = new BindingInfo() + { + BindingSource = BindingSource.Query, + }, + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + // This will cause selection of the "parameter" prefix. + request.QueryString = new QueryString("?parameter="); + + // This value won't be used, because we select the "parameter" prefix. + request.Form = new FormCollection(new Dictionary() + { + { "Name", "Billy" }, + }); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal("parameter", modelBindingResult.Key); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Null(model.Name); + + Assert.Equal(0, modelState.Count); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + + private class Person3 + { + [FromForm] + public string Name { get; set; } + } + + [Fact] + public async Task ComplexModel_EmptyPrefixSelected_NoMatchingValueProviderValue() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Person3), + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + // This can't be used because of [FromForm] on the property. + request.QueryString = new QueryString("?Name="); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal(string.Empty, modelBindingResult.Key); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Null(model.Name); + + Assert.Equal(0, modelState.Count); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + + private class Person4 + { + [FromForm] + public string Name { get; set; } + } + + [Fact] + public async Task ComplexModel_EmptyPrefixSelected_NoMatchingValueProviderValue_WithFilteredValueProviders() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Person4), + BindingInfo = new BindingInfo() + { + BindingSource = BindingSource.Query, + }, + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + // This will only match empty prefix, but can't be used because of [FromForm] on the property. + request.QueryString = new QueryString("?Name="); + + // This value won't be used to select a prefix, because we're only looking at the query string. + request.Form = new FormCollection(new Dictionary() + { + { "parameter", string.Empty }, + }); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal(string.Empty, modelBindingResult.Key); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Null(model.Name); + + Assert.Equal(0, modelState.Count); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + + private class Person5 + { + [FromForm] + public string Name { get; set; } + } + + [Fact] + public async Task ComplexModel_EmptyPrefixSelected_NoMatchingValueProviderValue_WithFilteredValueProviders_NoValues() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Person5), + BindingInfo = new BindingInfo() + { + BindingSource = BindingSource.Query, + }, + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + // This value won't be used to select a prefix, because we're only looking at the query string. + request.Form = new FormCollection(new Dictionary() + { + { "parameter", string.Empty }, + }); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal(string.Empty, modelBindingResult.Key); + + var model = Assert.IsType(modelBindingResult.Model); + Assert.Null(model.Name); + + Assert.Equal(0, modelState.Count); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + } +}