Clean up `ModelBindingResult` constructor calls and related comments

- use named parameters more often
 - add more comments about returned `ModelBindingResult`
 - clean up `ModelBindingResult` doc comments
 - cleanup `using`s

Nits:
- cleanup trailing whitespace
- change `retVal` -> `result` in `KeyValuePairModelBinderTest`
This commit is contained in:
Doug Bunting 2015-03-20 13:02:23 -07:00
parent 103538b889
commit 64c8a6fa40
20 changed files with 102 additions and 79 deletions

View File

@ -6,8 +6,6 @@ using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.Core;
using Microsoft.AspNet.Mvc.ModelBinding;
using Microsoft.Framework.DependencyInjection;
using Microsoft.Framework.Internal;
namespace Microsoft.AspNet.Mvc.ModelBinding
@ -57,6 +55,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var unsupportedContentType = Resources.FormatUnsupportedContentType(
bindingContext.OperationBindingContext.HttpContext.Request.ContentType);
bindingContext.ModelState.AddModelError(bindingContext.ModelName, unsupportedContentType);
// This model binder is the only handler for the Body binding source.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
@ -69,9 +70,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
model = GetDefaultValueForType(bindingContext.ModelType);
bindingContext.ModelState.AddModelError(bindingContext.ModelName, ex);
// This model binder is the only handler for the Body binding source.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
// Success
// key is empty to ensure that the model name is not used as a prefix for validation.
return new ModelBindingResult(model, key: string.Empty, isModelSet: true);
}

View File

@ -26,7 +26,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
var requestServices = bindingContext.OperationBindingContext.HttpContext.RequestServices;
var model = requestServices.GetRequiredService(bindingContext.ModelType);
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, true));
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true));
}
}
}

View File

@ -33,7 +33,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var requestServices = bindingContext.OperationBindingContext.HttpContext.RequestServices;
var createFactory = _typeActivatorCache.GetOrAdd(bindingContext.BinderType, _createFactory);
var instance = createFactory(requestServices, arguments: null);
var modelBinder = instance as IModelBinder;
if (modelBinder == null)
{
@ -54,12 +54,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var result = await modelBinder.BindModelAsync(bindingContext);
var modelBindingResult = result != null ?
var modelBindingResult = result != null ?
new ModelBindingResult(result.Model, result.Key, result.IsModelSet) :
new ModelBindingResult(null, bindingContext.ModelName, false);
new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
// return a non null modelbinding result here, because this binder will handle all cases where the
// model binder is specified by metadata.
// A model binder was specified by metadata and this binder handles all such cases.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return modelBindingResult;
}
}

View File

@ -59,6 +59,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// <returns>
/// A <see cref="Task"/> which will complete when model binding has completed.
/// </returns>
/// <remarks>
/// Other model binders will never run if this method is called. Return <c>null</c> to skip other model binders
/// but allow higher-level handling e.g. falling back to empty prefix.
/// </remarks>
protected abstract Task<ModelBindingResult> BindModelCoreAsync([NotNull] ModelBindingContext bindingContext);
/// <inheritdoc />
@ -74,13 +78,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var result = await BindModelCoreAsync(context);
var modelBindingResult =
result != null ?
var modelBindingResult =
result != null ?
new ModelBindingResult(result.Model, result.Key, result.IsModelSet) :
new ModelBindingResult(null, context.ModelName, false);
new ModelBindingResult(model: null, key: context.ModelName, isModelSet: false);
// Prevent other model binders from running because this model binder is the only handler for
// its binding source.
// This model binder is the only handler for its binding source.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return modelBindingResult;
}
}

View File

@ -3,7 +3,6 @@
using System;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.ModelBinding.Internal;
using Microsoft.Framework.Internal;
namespace Microsoft.AspNet.Mvc.ModelBinding
@ -40,13 +39,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
try
{
var model = Convert.FromBase64String(value);
return new ModelBindingResult(model, bindingContext.ModelName, true);
return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true);
}
catch (Exception ex)
{
bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, ex);
}
// Matched the type (byte[]) only this binder supports.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
}

View File

@ -17,7 +17,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
if (bindingContext.ModelType == typeof(CancellationToken))
{
var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted;
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, true));
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true));
}
return Task.FromResult<ModelBindingResult>(null);

View File

@ -28,7 +28,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
BindComplexCollection(bindingContext);
var boundCollection = await bindCollectionTask;
var model = GetModel(boundCollection);
return new ModelBindingResult(model, bindingContext.ModelName, true);
return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true);
}
// Used when the ValueProvider contains the collection to be bound as a single element, e.g. the raw value

View File

@ -5,7 +5,6 @@ using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Framework.DependencyInjection;
using Microsoft.Framework.Internal;
namespace Microsoft.AspNet.Mvc.ModelBinding
@ -34,14 +33,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
public virtual async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingContext bindingContext)
{
var newBindingContext = CreateNewBindingContext(bindingContext,
bindingContext.ModelName);
var newBindingContext = CreateNewBindingContext(bindingContext, bindingContext.ModelName);
var modelBindingResult = await TryBind(newBindingContext);
if (modelBindingResult == null && !string.IsNullOrEmpty(bindingContext.ModelName)
&& bindingContext.FallbackToEmptyPrefix)
if (modelBindingResult == null &&
bindingContext.FallbackToEmptyPrefix &&
!string.IsNullOrEmpty(bindingContext.ModelName))
{
// fallback to empty prefix?
// Fall back to empty prefix.
newBindingContext = CreateNewBindingContext(bindingContext,
modelName: string.Empty);
modelBindingResult = await TryBind(newBindingContext);
@ -75,13 +74,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// {
// }
//
// In this case, for the model parameter the key would be SimpleType instead of model.SimpleType.
// In this case, for the model parameter the key would be SimpleType instead of model.SimpleType.
// (i.e here the prefix for the model key is empty).
// For the id parameter the key would be id.
return modelBindingResult;
}
}
// Fall through to update the ModelBindingResult's key.
return new ModelBindingResult(
modelBindingResult.Model,
bindingContext.ModelName,

View File

@ -46,7 +46,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
model = new FormCollection(new Dictionary<string, string[]>());
}
return new ModelBindingResult(model, bindingContext.ModelName, true);
return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true);
}
}
}

View File

@ -25,14 +25,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
var postedFiles = await GetFormFilesAsync(bindingContext);
var value = postedFiles.FirstOrDefault();
return new ModelBindingResult(value, bindingContext.ModelName, value != null);
return new ModelBindingResult(value, bindingContext.ModelName, isModelSet: value != null);
}
else if (typeof(IEnumerable<IFormFile>).GetTypeInfo().IsAssignableFrom(
bindingContext.ModelType.GetTypeInfo()))
{
var postedFiles = await GetFormFilesAsync(bindingContext);
var value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles);
return new ModelBindingResult(value, bindingContext.ModelName, value != null);
return new ModelBindingResult(value, bindingContext.ModelName, isModelSet: value != null);
}
return null;

View File

@ -22,10 +22,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var modelBindingResult = result != null ?
new ModelBindingResult(result.Model, result.Key, result.IsModelSet) :
new ModelBindingResult(null, bindingContext.ModelName, false);
new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
// Was able to resolve a binder type, hence we should tell the model binding system to return
// true so that none of the other model binders participate.
// Were able to resolve a binder type.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return modelBindingResult;
}

View File

@ -52,7 +52,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
}
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, model != null));
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet: model != null));
}
}
}

View File

@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.ModelBinding.Internal;
@ -25,6 +24,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
ModelBindingHelper.CastOrDefault<TKey>(keyResult.Model),
ModelBindingHelper.CastOrDefault<TValue>(valueResult.Model));
// Success
return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true);
}
else if (!keyResult.IsModelSet && valueResult.IsModelSet)
@ -32,6 +32,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
bindingContext.ModelState.TryAddModelError(
keyResult.Key,
Resources.KeyValuePair_BothKeyAndValueMustBePresent);
// Were able to get some data for this model.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
else if (keyResult.IsModelSet && !valueResult.IsModelSet)
@ -39,10 +42,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
bindingContext.ModelState.TryAddModelError(
valueResult.Key,
Resources.KeyValuePair_BothKeyAndValueMustBePresent);
// Were able to get some data for this model.
// Always tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
else
{
// Caller (GenericModelBinder) was able to resolve a binder type and will create a ModelBindingResult
// that exits current ModelBinding loop.
return null;
}
}
@ -58,16 +66,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
parentBindingContext,
propertyModelName,
propertyModelMetadata);
propertyBindingContext.BinderModelName = propertyModelMetadata.BinderModelName;
var modelBindingResult =
await propertyBindingContext.OperationBindingContext.ModelBinder.BindModelAsync(propertyBindingContext);
var modelBindingResult = await propertyBindingContext.OperationBindingContext.ModelBinder.BindModelAsync(
propertyBindingContext);
if (modelBindingResult != null)
{
return modelBindingResult;
}
// Always return a ModelBindingResult to avoid an NRE in BindModelAsync.
return new ModelBindingResult(model: default(TModel), key: propertyModelName, isModelSet: false);
}
}

View File

@ -40,8 +40,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// post-processing, e.g. property setters and hooking up validation
ProcessDto(bindingContext, (ComplexModelDto)result.Model);
return new ModelBindingResult(
bindingContext.Model,
bindingContext.ModelName,
bindingContext.Model,
bindingContext.ModelName,
isModelSet: true);
}
@ -53,6 +53,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
internal async Task<bool> CanCreateModel(MutableObjectBinderContext context)
{
var bindingContext = context.ModelBindingContext;
var isTopLevelObject = bindingContext.ModelMetadata.ContainerType == null;
var hasExplicitAlias = bindingContext.BinderModelName != null;
@ -117,7 +118,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// However, because a property might specify a custom binding source ([FromForm]), it's not correct
// for us to just try bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName),
// because that may include ALL value providers - that would lead us to mistakenly create the model
// when the data is coming from a source we should use (ex: value found in query string, but the
// when the data is coming from a source we should use (ex: value found in query string, but the
// model has [FromForm]).
//
// To do this we need to enumerate the properties, and see which of them provide a binding source
@ -133,7 +134,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// If a property does not have a binding source, then it's fair game for any value provider.
//
// If any property meets the above conditions and has a value from valueproviders, then we'll
// create the model and try to bind it. OR if ALL properties of the model have a greedy source,
// create the model and try to bind it. OR if ALL properties of the model have a greedy source,
// then we go ahead and create it.
//
var isAnyPropertyEnabledForValueProviderBasedBinding = false;

View File

@ -31,14 +31,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
newModel = valueProviderResult.ConvertTo(bindingContext.ModelType);
ModelBindingHelper.ReplaceEmptyStringWithNull(bindingContext.ModelMetadata, ref newModel);
return new ModelBindingResult(newModel, bindingContext.ModelName, true);
return new ModelBindingResult(newModel, bindingContext.ModelName, isModelSet: true);
}
catch (Exception ex)
{
bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, ex);
}
return new ModelBindingResult(null, bindingContext.ModelName, false);
// Were able to find a converter for the type but conversion failed.
// Tell the model binding system to skip other model binders i.e. return non-null.
return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
}
}
}

View File

@ -20,7 +20,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);
var model = valueProviderResult.RawValue;
ModelBindingHelper.ReplaceEmptyStringWithNull(bindingContext.ModelMetadata, ref model);
return new ModelBindingResult(model, bindingContext.ModelName, true);
return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true);
}
internal static async Task<ValueProviderResult> GetCompatibleValueProviderResult(ModelBindingContext context)

View File

@ -23,22 +23,28 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
/// <summary>
/// Gets or sets the model associated with this context.
/// Gets the model associated with this context.
/// </summary>
public object Model { get; }
/// <summary>
/// Gets or sets the model name which was used to bind the model.
///
/// <para>
/// Gets the model name which was used to bind the model.
/// </para>
/// <para>
/// This property can be used during validation to add model state for a bound model.
/// </para>
/// </summary>
public string Key { get; }
/// <summary>
/// Gets or sets a value indicating whether or not the <see cref="Model"/> value has been set.
///
/// <para>
/// Gets a value indicating whether or not the <see cref="Model"/> value has been set.
/// </para>
/// <para>
/// This property can be used to distinguish between a model binder which does not find a value and
/// the case where a model binder sets the <c>null</c> value.
/// </para>
/// </summary>
public bool IsModelSet { get; }
}

View File

@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
if (bindingContext.ModelType == typeof(HttpRequestMessage))
{
var model = bindingContext.OperationBindingContext.HttpContext.GetHttpRequestMessage();
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, true));
return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true));
}
return Task.FromResult<ModelBindingResult>(null);

View File

@ -4,7 +4,6 @@
#if DNX451
using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Threading.Tasks;
using Moq;
@ -25,11 +24,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.BindModelAsync(bindingContext);
var result = await binder.BindModelAsync(bindingContext);
// Assert
Assert.NotNull(retVal);
Assert.Null(retVal.Model);
Assert.NotNull(result);
Assert.Null(result.Model);
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal("someName", bindingContext.ModelName);
var error = Assert.Single(bindingContext.ModelState["someName.Key"].Errors);
@ -47,11 +46,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.BindModelAsync(bindingContext);
var result = await binder.BindModelAsync(bindingContext);
// Assert
Assert.NotNull(retVal);
Assert.Null(retVal.Model);
Assert.NotNull(result);
Assert.Null(result.Model);
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal("someName", bindingContext.ModelName);
Assert.Equal(bindingContext.ModelState["someName.Value"].Errors.First().ErrorMessage, "A value is required.");
@ -73,10 +72,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.BindModelAsync(bindingContext);
var result = await binder.BindModelAsync(bindingContext);
// Assert
Assert.Null(retVal);
Assert.Null(result);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(0, bindingContext.ModelState.ErrorCount);
}
@ -92,11 +91,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.BindModelAsync(bindingContext);
var result = await binder.BindModelAsync(bindingContext);
// Assert
Assert.NotNull(retVal);
Assert.Equal(new KeyValuePair<int, string>(42, "some-value"), retVal.Model);
Assert.NotNull(result);
Assert.Equal(new KeyValuePair<int, string>(42, "some-value"), result.Model);
}
[Fact]
@ -107,11 +106,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.TryBindStrongModel<int>(bindingContext, "key");
var result = await binder.TryBindStrongModel<int>(bindingContext, "key");
// Assert
Assert.True(retVal.IsModelSet);
Assert.Equal(42, retVal.Model);
Assert.True(result.IsModelSet);
Assert.Equal(42, result.Model);
Assert.Empty(bindingContext.ModelState);
}
@ -133,11 +132,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
var binder = new KeyValuePairModelBinder<int, string>();
// Act
var retVal = await binder.TryBindStrongModel<int>(bindingContext, "key");
var result = await binder.TryBindStrongModel<int>(bindingContext, "key");
// Assert
Assert.True(retVal.IsModelSet);
Assert.Null(retVal.Model);
Assert.True(result.IsModelSet);
Assert.Null(result.Model);
Assert.Empty(bindingContext.ModelState);
}

View File

@ -7,7 +7,6 @@ using System.Collections.Generic;
using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.AspNet.Http.Core;
using Microsoft.AspNet.Testing;
@ -106,17 +105,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
[Fact]
public async Task CanCreateModel_ReturnsFalse_ForNonTopLevelModel_IfModelIsMarkedWithBinderMetadata()
{
// Get the property metadata so that it is not a top level object.
var modelMetadata = GetMetadataForType(typeof(Document))
.Properties
.First(metadata => metadata.PropertyName == nameof(Document.SubDocument));
.Properties
.First(metadata => metadata.PropertyName == nameof(Document.SubDocument));
var bindingContext = new MutableObjectBinderContext
{
ModelBindingContext = new ModelBindingContext
{
// Get the property metadata so that it is not a top level object.
ModelMetadata = GetMetadataForType(typeof(Document))
.Properties
.First(metadata => metadata.PropertyName == nameof(Document.SubDocument)),
ModelMetadata = modelMetadata,
OperationBindingContext = new OperationBindingContext
{
ValidatorProvider = Mock.Of<IModelValidatorProvider>(),
@ -381,7 +378,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
.Verifiable();
testableBinder
.Setup(o => o.GetMetadataForProperties(bindingContext))
.Returns(new ModelMetadata[0]);
.Returns(new ModelMetadata[0]);
// Act
var retValue = await testableBinder.Object.BindModelAsync(bindingContext);
@ -389,8 +386,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
Assert.NotNull(retValue);
Assert.True(retValue.IsModelSet);
Assert.Same(model, retValue.Model);
Assert.IsType<Person>(retValue.Model);
var returnedPerson = Assert.IsType<Person>(retValue.Model);
Assert.Same(model, returnedPerson);
testableBinder.Verify();
}
@ -442,8 +439,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
Assert.NotNull(retValue);
Assert.True(retValue.IsModelSet);
Assert.Same(model, retValue.Model);
Assert.IsType<Person>(retValue.Model);
var returnedPerson = Assert.IsType<Person>(retValue.Model);
Assert.Same(model, returnedPerson);
testableBinder.Verify();
}