From 94e326f953dbb178a40ef6edd31105f029324157 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Fri, 20 Mar 2015 13:06:40 -0700 Subject: [PATCH] `CompositeModelBinder.TryBind()` should return `null` more often - #2129 - do not propagate results with `!IsModelSet`, allowing empty prefix fallback - adjust `ComplexModelDtoModelBinder` to at least fake-bind all properties - default values not consistently picked up otherwise nit: correct 2 test names in `KeyValuePairModelBinderTest` --- .../Binders/ComplexModelDtoModelBinder.cs | 18 ++++++++++-------- .../Binders/CompositeModelBinder.cs | 16 +++++++++++++++- .../Binders/MutableObjectModelBinder.cs | 3 ++- .../Binders/CompositeModelBinderTest.cs | 10 +++------- .../Binders/KeyValuePairModelBinderTest.cs | 4 ++-- 5 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDtoModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDtoModelBinder.cs index 78dc815269..65cfdbe381 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDtoModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDtoModelBinder.cs @@ -15,27 +15,29 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return null; } - ModelBindingHelper.ValidateBindingContext(bindingContext, - typeof(ComplexModelDto), - allowNullModel: false); + ModelBindingHelper.ValidateBindingContext(bindingContext, typeof(ComplexModelDto), allowNullModel: false); var dto = (ComplexModelDto)bindingContext.Model; foreach (var propertyMetadata in dto.PropertyMetadata) { var propertyModelName = ModelBindingHelper.CreatePropertyModelName( - bindingContext.ModelName, - propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName); + bindingContext.ModelName, + propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName); var propertyBindingContext = ModelBindingContext.GetChildModelBindingContext( bindingContext, propertyModelName, propertyMetadata); - // bind and propagate the values - // If we can't bind then leave the result missing (don't add a null). var modelBindingResult = await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(propertyBindingContext); - if (modelBindingResult != null) + if (modelBindingResult == null) + { + // Could not bind. Add a result so MutableObjectModelBinder will check for [DefaultValue]. + dto.Results[propertyMetadata] = + new ModelBindingResult(model: null, key: propertyModelName, isModelSet: false); + } + else { dto.Results[propertyMetadata] = modelBindingResult; } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CompositeModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CompositeModelBinder.cs index c52bed2140..7c1860514b 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CompositeModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CompositeModelBinder.cs @@ -97,7 +97,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var result = await binder.BindModelAsync(bindingContext); if (result != null) { - return result; + // Use returned ModelBindingResult if it either indicates the model was set or is related to a + // ModelState entry. The second 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.IsModelSet || bindingContext.ModelState.ContainsKey(bindingContext.ModelName)) + { + return result; + } + + // Current binder should have been able to bind value but found nothing. Exit loop in a way that + // tells caller to fall back to the empty prefix, if appropriate. Do not return result because it + // means only "other binders are not applicable". + break; } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs index 5fd4017227..3b4310e33e 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs @@ -410,7 +410,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } - // for each property that was bound, call the setter, recording exceptions as necessary + // For each property that ComplexModelDtoModelBinder attempted to bind, call the setter, recording + // exceptions as necessary. foreach (var entry in dto.Results) { var dtoResult = entry.Value; diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CompositeModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CompositeModelBinderTest.cs index 43a96d1612..7f0f0a978f 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CompositeModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CompositeModelBinderTest.cs @@ -112,10 +112,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task ModelBinder_ReturnsTrue_WithoutSettingValue() + public async Task ModelBinder_ReturnsNull_IfBinderMatchesButDoesNotSetModel() { // Arrange - var bindingContext = new ModelBindingContext { FallbackToEmptyPrefix = true, @@ -135,7 +134,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var modelBinder = new Mock(); modelBinder .Setup(mb => mb.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(new ModelBindingResult(null, "someName", false))); + .Returns(Task.FromResult(new ModelBindingResult(model: null, key: "someName", isModelSet: false))); var composite = CreateCompositeBinder(modelBinder.Object); @@ -143,10 +142,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var result = await composite.BindModelAsync(bindingContext); // Assert - Assert.NotNull(result); - Assert.False(result.IsModelSet); - Assert.Equal("someName", result.Key); - Assert.Null(result.Model); + Assert.Null(result); } [Fact] diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/KeyValuePairModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/KeyValuePairModelBinderTest.cs index daeb239ff4..4335ec30c9 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/KeyValuePairModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/KeyValuePairModelBinderTest.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test public class KeyValuePairModelBinderTest { [Fact] - public async Task BindModel_MissingKey_ReturnsTrue_AndAddsModelValidationError() + public async Task BindModel_MissingKey_ReturnsResult_AndAddsModelValidationError() { // Arrange var valueProvider = new SimpleHttpValueProvider(); @@ -36,7 +36,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test } [Fact] - public async Task BindModel_MissingValue_ReturnsTrue_AndAddsModelValidationError() + public async Task BindModel_MissingValue_ReturnsResult_AndAddsModelValidationError() { // Arrange var valueProvider = new SimpleHttpValueProvider();