Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
- add single-parameter `ValueProviderResult` constructor
 - use in most of the greedy and type-matching model binders
- revert changes to `FormCollectionModelBinder` and related tests
 - do _not_ add a `ModelState` entry in this binder
- add doc comments in `ValueProviderResult` class
- simplify logic in `FormFileModelBinder`
 - exit early instead of using `typeMatched` variable

nits:
- use new `ValueProviderResult` constructor in many existing tests
- `""` -> `string.Empty` and `vpr` -> `valueProviderResult` in `ValueProviderResultTest`
- improve some test names in `BodyValidationIntegrationTests`
- do not check `Message` of a Json.NET `Exception`
  • Loading branch information
dougbu committed Jun 22, 2015
1 parent a218289 commit f0ae34d
Show file tree
Hide file tree
Showing 12 changed files with 179 additions and 170 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,59 @@

namespace Microsoft.AspNet.Mvc.ModelBinding
{
/// <summary>
/// Result of an <see cref="IValueProvider.GetValueAsync"/> operation.
/// </summary>
public class ValueProviderResult
{
private static readonly CultureInfo _staticCulture = CultureInfo.InvariantCulture;
private CultureInfo _instanceCulture;

// default constructor so that subclassed types can set the properties themselves
/// <summary>
/// Instantiates a new instance of the <see cref="ValueProviderResult"/> class. Subclass must at least set
/// <see cref="Culture"/> to avoid <see cref="Exception"/>s in <see cref="ConvertTo(Type, CultureInfo)"/>.
/// </summary>

This comment has been minimized.

Copy link
@rynowak

rynowak Jun 23, 2015

Member

This seems like over-sharing. Why do we even have this protected constructor?

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 23, 2015

Author Member

I can't find any subclasses but are we expecting users to create them? If not I'll remove this constructor and the protected setters on a couple of the properties.

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 23, 2015

Author Member

@Eilon this protected constructor as well as the protected AttemptedValue and RawValue setters were directly inherited from our old Web Stack Runtime. There as well, I cannot find any subclasses. What are the scenarios for creating a ValueProviderResult subclass and are they relevant in MVC 6?

Note in-the-box value providers and model binders construct ValueProviderResults directly.

This comment has been minimized.

Copy link
@rynowak

rynowak Jun 23, 2015

Member

The scenario is probably to override Convert. IMO it's probably fine to leave as-is.

What set me off is seeing that we had to take the trouble to document that you are required to set a culture. Maybe the right thing to do is set it InvariantCulture so that you get reasonable behavior by default.

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 23, 2015

Author Member

I agree ConvertTo() is virtual for subclasses but that doesn't explain allowing subclasses to change the property values. Would expect subclasses to call base(rawValue) or base(rawValue, attemptedValue, culture) from their constructors.

Separately Culture gets _staticCulture automatically and that's CultureInfo.InvariantCulture. I'm only passing _staticCulture in the constructor below because null would be a bit obscure.

This comment has been minimized.

Copy link
@Eilon

Eilon Jun 24, 2015

Member

Sorry I can't think of any scenarios and I don't recall the history.

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 24, 2015

Author Member

Sounds good. I'll remove the protected bits of this class but leave ConvertTo() virtual. That'll save me documenting something that doesn't need to exist.

protected ValueProviderResult()
{
}

/// <summary>
/// Instantiates a new instance of the <see cref="ValueProviderResult"/> class with given
/// <paramref name="rawValue"/>. Initializes <see cref="Culture"/> to
/// <see cref="CultureInfo.InvariantCulture"/>.

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 23, 2015

Author Member

@rynowak Hmm, should I add <remarks> mentioning that this constructor is probably not the right choice in mainline value provider scenarios? We use it for cases that don't involve value providers and in value provider mocks. Not sure we want to encourage InvariantCulture too much...

/// </summary>
/// <param name="rawValue">The <see cref="RawValue"/> value of the new instance.</param>
public ValueProviderResult(object rawValue)
: this(rawValue, attemptedValue: null, culture: _staticCulture)
{
}

/// <summary>
/// Instantiates a new instance of the <see cref="ValueProviderResult"/> class with given
/// <paramref name="rawValue"/>, <paramref name="attemptedValue"/>, and <paramref name="culture"/>.
/// </summary>
/// <param name="rawValue">The <see cref="RawValue"/> value of the new instance.</param>
/// <param name="attemptedValue">The <see cref="AttemptedValue"/> value of the new instance.</param>
/// <param name="culture">The <see cref="Culture"/> value of the new instance.</param>
public ValueProviderResult(object rawValue, string attemptedValue, CultureInfo culture)
{
RawValue = rawValue;
AttemptedValue = attemptedValue;
Culture = culture;
}

/// <summary>
/// <see cref="string"/> conversion of <see cref="RawValue"/>.
/// </summary>
/// <remarks>
/// Used in helpers that generate <c>&lt;textarea&gt;</c> elements as well as some error messages.
/// </remarks>
public string AttemptedValue { get; protected set; }

/// <summary>
/// <see cref="CultureInfo"/> to use in <see cref="ConvertTo(Type)"/> or
/// <see cref="ConvertTo(Type, CultureInfo)"/> if passed <see cref="CultureInfo"/> is <c>null</c>.
/// </summary>
public CultureInfo Culture
{
get
Expand All @@ -44,13 +78,37 @@ public CultureInfo Culture
protected set { _instanceCulture = value; }
}

/// <summary>
/// The provided <see cref="object"/>.
/// </summary>
public object RawValue { get; protected set; }

/// <summary>
/// Converts <see cref="RawValue"/> to the given <paramref name="type"/>. Uses <see cref="Culture"/> for
/// <see cref="TypeConverter"/> operations.
/// </summary>
/// <param name="type">The target <see cref="Type"/> of the conversion.</param>
/// <returns>
/// <see cref="RawValue"/> converted to the given <paramref name="type"/>. <c>null</c> if the conversion fails.
/// </returns>
public object ConvertTo(Type type)
{
return ConvertTo(type, culture: null);
}

/// <summary>
/// Converts <see cref="RawValue"/> to the given <paramref name="type"/> using the given
/// <paramref name="culture"/>.
/// </summary>
/// <param name="type">The target <see cref="Type"/> of the conversion.</param>
/// <param name="culture">
/// The <see cref="CultureInfo"/> to use for <see cref="TypeConverter"/> operations. Uses
/// <see cref="Culture"/> if this parameter is <c>null</c>.
/// </param>
/// <returns>
/// <see cref="RawValue"/> converted to the given <paramref name="type"/> using the given
/// <paramref name="culture"/>. <c>null</c> if the conversion fails.
/// </returns>
public virtual object ConvertTo([NotNull] Type type, CultureInfo culture)
{
var value = RawValue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected async override Task<ModelBindingResult> BindModelCoreAsync(
return new ModelBindingResult(modelBindingKey);
}

var valueProviderResult = new ValueProviderResult(model, attemptedValue: null, culture: null);
var valueProviderResult = new ValueProviderResult(rawValue: model);
bindingContext.ModelState.SetModelValue(modelBindingKey, valueProviderResult);

var validationNode = new ModelValidationNode(modelBindingKey, bindingContext.ModelMetadata, model)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingConte
return null;
}

object model;
object model = null;
var request = bindingContext.OperationBindingContext.HttpContext.Request;
if (request.HasFormContentType)
{
Expand All @@ -45,9 +45,6 @@ public async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingConte
model = new FormCollection(new Dictionary<string, string[]>());
}

var valueProviderResult = new ValueProviderResult(model, attemptedValue: null, culture: null);
bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);

var validationNode =
new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model);
return new ModelBindingResult(
Expand Down
37 changes: 17 additions & 20 deletions src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,41 +22,38 @@ public class FormFileModelBinder : IModelBinder
/// <inheritdoc />
public async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingContext bindingContext)
{
var typeMatched = false;
object value = null;
object value;
if (bindingContext.ModelType == typeof(IFormFile))
{
typeMatched = true;
var postedFiles = await GetFormFilesAsync(bindingContext);
value = postedFiles.FirstOrDefault();
}
else if (typeof(IEnumerable<IFormFile>).IsAssignableFrom(bindingContext.ModelType))
{
typeMatched = true;
var postedFiles = await GetFormFilesAsync(bindingContext);
value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles);
}

if (typeMatched)
else
{
ModelValidationNode validationNode = null;
if (value != null)
{
validationNode =
new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, value);
// This binder does not support the requested type.
return null;
}

var valueProviderResult = new ValueProviderResult(value, attemptedValue: null, culture: null);
bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);
}
ModelValidationNode validationNode = null;
if (value != null)
{
validationNode =
new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, value);

return new ModelBindingResult(
value,
bindingContext.ModelName,
isModelSet: value != null,
validationNode: validationNode);
var valueProviderResult = new ValueProviderResult(rawValue: value);
bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);
}

return null;
return new ModelBindingResult(
value,
bindingContext.ModelName,
isModelSet: value != null,
validationNode: validationNode);
}

private async Task<List<IFormFile>> GetFormFilesAsync(ModelBindingContext bindingContext)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.Globalization;
#if DNXCORE50
using System.Reflection;
#endif
Expand Down Expand Up @@ -61,7 +62,7 @@ protected override Task<ModelBindingResult> BindModelCoreAsync([NotNull] ModelBi
model);

var attemptedValue = (model as string) ?? request.Headers.Get(headerName);
var valueProviderResult = new ValueProviderResult(model, attemptedValue, culture: null);
var valueProviderResult = new ValueProviderResult(model, attemptedValue, CultureInfo.InvariantCulture);

This comment has been minimized.

Copy link
@rynowak

rynowak Jun 23, 2015

Member

should we add a another constructor that sets the culture to Invariant?

This comment has been minimized.

Copy link
@dougbu

dougbu Jun 23, 2015

Author Member

You mean ValueProviderResult(object rawValue, string attemptedValue)? Might shorten some lines in our tests but I'd rather not further delay this PR.

bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Globalization;
using Microsoft.Framework.Internal;
using Xunit;
Expand Down Expand Up @@ -75,7 +74,7 @@ public void MarkFieldSkipped_Throws_IfStateIsInvalid()

// Act
var exception = Assert.Throws<InvalidOperationException>(() => source.MarkFieldSkipped("key"));

// Assert
Assert.Equal(
"A field previously marked invalid should not be marked skipped.",
Expand Down Expand Up @@ -138,7 +137,7 @@ public void MarkFieldValid_Throws_IfStateIsInvalid()

// Act
var exception = Assert.Throws<InvalidOperationException>(() => source.MarkFieldValid("key"));

// Assert
Assert.Equal(
"A field previously marked invalid should not be marked valid.",
Expand Down Expand Up @@ -278,7 +277,7 @@ public void GetFieldValidationState_ReturnsValidIfModelStateDoesNotContainErrors
// Arrange
var validState = new ModelState
{
Value = new ValueProviderResult(null, null, null),
Value = new ValueProviderResult(rawValue: null),
ValidationState = ModelValidationState.Valid
};
var msd = new ModelStateDictionary
Expand Down Expand Up @@ -317,7 +316,7 @@ public void GetFieldValidationState_IndexedPrefix_ReturnsValidIfModelStateDoesNo
// Arrange
var validState = new ModelState
{
Value = new ValueProviderResult(null, null, null),
Value = new ValueProviderResult(rawValue: null),
ValidationState = ModelValidationState.Valid
};
var msd = new ModelStateDictionary
Expand Down
Loading

0 comments on commit f0ae34d

Please sign in to comment.