Make `CompositeValueProvider` consistent with `IBindingSourceValueProvider` contract

- #2907
- return `null` if `Filter()` finds no matching value provider in collection
- fix lack of defensiveness in model binders' use of `IBindingSourceValueProvider` implementations
- remove test workaround for unusual `CompositeValueProvider` behaviour
This commit is contained in:
Doug Bunting 2015-08-05 17:00:16 -07:00
parent 1aef84b50d
commit 4a813773d0
5 changed files with 28 additions and 15 deletions

View File

@ -3,6 +3,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.Core;
@ -39,6 +40,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
!string.IsNullOrEmpty(bindingContext.ModelName);
var newBindingContext = CreateNewBindingContext(bindingContext, bindingContext.ModelName);
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);
@ -46,12 +53,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
// 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);
}
if (modelBindingResult == null)
{
return null; // something went wrong
// Unable to bind or something went wrong.
return null;
}
bindingContext.OperationBindingContext.BodyBindingState =
@ -176,6 +186,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
if (valueProvider != null)
{
newBindingContext.ValueProvider = valueProvider.Filter(bindingSource);
if (newBindingContext.ValueProvider == null)
{
// Unable to find a value provider for this binding source.
return null;
}
}
}

View File

@ -137,6 +137,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
}
if (filteredValueProviders.Count == 0)
{
// Do not create an empty CompositeValueProvider.
return null;
}
if (filteredValueProviders.Count == Count)
{
// No need for a new CompositeValueProvider.

View File

@ -115,7 +115,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
private async Task<bool> CanValueBindAnyModelProperties(MutableObjectBinderContext context)
{
// If there are no properties on the model, there is nothing to bind. We are here means this is not a top
// If there are no properties on the model, there is nothing to bind. We are here means this is not a top
// level object. So we return false.
if (context.PropertyMetadata == null || context.PropertyMetadata.Count == 0)
{
@ -196,6 +196,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
if (rootValueProvider != null)
{
valueProvider = rootValueProvider.Filter(bindingSource);
if (valueProvider == null)
{
// Unable to find a value provider for this binding source. Binding will fail.
return false;
}
}
}

View File

@ -28,14 +28,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
return new CompositeValueProvider(new[] { emptyValueProvider, valueProvider });
}
protected override void CheckFilterExcludeResult(IValueProvider result)
{
// CompositeValueProvider returns an empty instance rather than null. CompositeModelBinder and
// MutableObjectModelBinder depend on this empty instance.
var compositeProvider = Assert.IsType<CompositeValueProvider>(result);
Assert.Empty(compositeProvider);
}
#if DNX451
[Fact]
public async Task GetKeysFromPrefixAsync_ReturnsResultFromFirstValueProviderThatReturnsValues()

View File

@ -273,11 +273,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var result = provider.Filter(bindingSource);
// Assert
CheckFilterExcludeResult(result);
}
protected virtual void CheckFilterExcludeResult(IValueProvider result)
{
Assert.Null(result);
}