From 8779cafbab39e9a57666a81e4eb25ebd70de7ae4 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Thu, 29 Jan 2015 09:31:05 -0800 Subject: [PATCH] Use `[Display(Order=x)]` to sort validation messages and properties - #964 - compute `ModelMetadata.Order` based on `[Display]` attribute - property affects e.g. `@Html.DisplayFor()` generation for complex objects - also affects order of messages in validation summaries - test new scenarios involving `ModelMetadata.Order` - per-property `ModelMetadata` and related tests - validation and `HtmlHelper` tests - add `HtmlHelperValidationSummaryTest` (which touches on #453) - update ModelBinding functional test to show use of `[Display(Order = x)]` nits: - move more `NullDisplayText` bits into proper slots (just above `Order`) - add doc comments for `ComputeNullDisplayText()` - add more assertions in tests using `ModelStateDictionary.HasReachedMaxErrors` - remove some trailing whitespace - avoid `Assert.True()` & `Assert.False()`; split some assertions up - `""` -> `string.Empty` in affected test classes - rename "DefaultEditorTemplatesTest~~s~~" class and file to follow guidelines - rename "ModelBindingTest~~s~~" class and file to follow guidelines FYI #1888 covers a predictable (or even just stable) order in the UI --- .../CachedDataAnnotationsModelMetadata.cs | 41 +- .../Metadata/CachedModelMetadata.cs | 41 +- .../Metadata/ModelMetadata.cs | 9 + .../Rendering/DefaultDisplayTemplatesTest.cs | 63 +++ ...Tests.cs => DefaultEditorTemplatesTest.cs} | 68 ++- .../HtmlHelperValidationSummaryTest.cs | 403 ++++++++++++++++++ .../ModelBindingWebSite.Vehicle.Details.html | 21 + ...elBindingWebSite.Vehicle.Edit.Invalid.html | 22 + .../ModelBindingWebSite.Vehicle.Edit.html | 20 + ...delBindingTests.cs => ModelBindingTest.cs} | 81 +++- .../CachedDataAnnotationsModelMetadataTest.cs | 55 +++ .../Metadata/ModelMetadataTest.cs | 244 ++++++++++- .../Validation/ModelStateDictionaryTest.cs | 33 +- .../Validation/ModelValidationNodeTest.cs | 201 ++++++++- .../Controllers/VehicleController.cs | 88 ++++ .../ViewModels/VehicleViewModel.cs | 6 + .../Views/Vehicle/Details.cshtml | 13 + .../Views/Vehicle/Edit.cshtml | 19 + 18 files changed, 1367 insertions(+), 61 deletions(-) rename test/Microsoft.AspNet.Mvc.Core.Test/Rendering/{DefaultEditorTemplatesTests.cs => DefaultEditorTemplatesTest.cs} (94%) create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/Rendering/HtmlHelperValidationSummaryTest.cs create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Details.html create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.Invalid.html create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.html rename test/Microsoft.AspNet.Mvc.FunctionalTests/{ModelBindingTests.cs => ModelBindingTest.cs} (94%) create mode 100644 test/WebSites/ModelBindingWebSite/Views/Vehicle/Details.cshtml create mode 100644 test/WebSites/ModelBindingWebSite/Views/Vehicle/Edit.cshtml diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedDataAnnotationsModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedDataAnnotationsModelMetadata.cs index 770b4d5c28..8aa42e6555 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedDataAnnotationsModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedDataAnnotationsModelMetadata.cs @@ -41,7 +41,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { // We want to respect the value set by the parameter (if any), and use the value specifed // on the type as a fallback. - // + // // We generalize this process, in case someone adds ordered providers (with count > 2) through // extensibility. foreach (var provider in PrototypeCache.BinderTypeProviders) @@ -84,13 +84,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding : base.ComputeConvertEmptyStringToNull(); } - protected override string ComputeNullDisplayText() - { - return PrototypeCache.DisplayFormat != null - ? PrototypeCache.DisplayFormat.NullDisplayText - : base.ComputeNullDisplayText(); - } - /// /// Calculate based on presence of a /// and its method. @@ -274,6 +267,38 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return (PrototypeCache.Required != null) || base.ComputeIsRequired(); } + /// + /// Calculate the value based on the presence of a + /// and its value. + /// + /// + /// Calculated value. + /// if a exists; + /// null otherwise. + /// + protected override string ComputeNullDisplayText() + { + return PrototypeCache.DisplayFormat != null + ? PrototypeCache.DisplayFormat.NullDisplayText + : base.ComputeNullDisplayText(); + } + + /// + /// Calculate the value based on presence of a + /// and its value. + /// + /// + /// Calculated value. if a + /// exists and its has been set; + /// 10000 otherwise. + /// + protected override int ComputeOrder() + { + var result = PrototypeCache.Display?.GetOrder(); + + return result ?? base.ComputeOrder(); + } + protected override string ComputeSimpleDisplayText() { if (Model != null && diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedModelMetadata.cs index e56f722949..6854684b12 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/CachedModelMetadata.cs @@ -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; namespace Microsoft.AspNet.Mvc.ModelBinding { @@ -17,7 +16,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public abstract class CachedModelMetadata : ModelMetadata { private bool _convertEmptyStringToNull; - private string _nullDisplayText; private string _dataTypeName; private string _description; private string _displayFormatString; @@ -29,6 +27,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private bool _isReadOnly; private bool _isComplexType; private bool _isRequired; + private string _nullDisplayText; + private int _order; private bool _showForDisplay; private bool _showForEdit; private IBinderMetadata _binderMetadata; @@ -37,7 +37,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private Type _binderType; private bool _convertEmptyStringToNullComputed; - private bool _nullDisplayTextComputed; private bool _dataTypeNameComputed; private bool _descriptionComputed; private bool _displayFormatStringComputed; @@ -49,6 +48,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private bool _isReadOnlyComputed; private bool _isComplexTypeComputed; private bool _isRequiredComputed; + private bool _nullDisplayTextComputed; + private bool _orderComputed; private bool _showForDisplayComputed; private bool _showForEditComputed; private bool _isBinderMetadataComputed; @@ -378,6 +379,27 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } + /// + public sealed override int Order + { + get + { + if (!_orderComputed) + { + _order = ComputeOrder(); + _orderComputed = true; + } + + return _order; + } + + set + { + _order = value; + _orderComputed = true; + } + } + public sealed override IPropertyBindingPredicateProvider PropertyBindingPredicateProvider { get @@ -575,11 +597,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return base.IsComplexType; } + /// + /// Calculate the value. + /// + /// Calculated value. protected virtual string ComputeNullDisplayText() { return base.NullDisplayText; } + /// + /// Calculate the value. + /// + /// Calculated value. + protected virtual int ComputeOrder() + { + return base.Order; + } + protected virtual bool ComputeShowForDisplay() { return base.ShowForDisplay; diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs index 69864bc684..b841024d50 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs @@ -160,6 +160,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding set { _isRequired = value; } } + /// + /// Gets or sets a value indicating where the current metadata should be ordered relative to other properties + /// in its containing type. + /// + /// + /// For example this property is used to order items in . + /// The default order is 10000. + /// + /// The order value of the current metadata. public virtual int Order { get { return _order; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultDisplayTemplatesTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultDisplayTemplatesTest.cs index 0e091b6dca..2a6d8c78ee 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultDisplayTemplatesTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultDisplayTemplatesTest.cs @@ -2,7 +2,11 @@ // 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.ComponentModel.DataAnnotations; +using System.Globalization; using System.Linq; +using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.Rendering; @@ -152,6 +156,48 @@ namespace Microsoft.AspNet.Mvc.Core Assert.Equal(expected, result); } + [Fact] + public void ObjectTemplate_OrdersProperties_AsExpected() + { + // Arrange + var model = new OrderedModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + var expectedProperties = new List + { + "OrderedProperty3", + "OrderedProperty2", + "OrderedProperty1", + "Property3", + "Property1", + "Property2", + "LastProperty", + }; + + var stringBuilder = new StringBuilder(); + foreach (var property in expectedProperties) + { + var label = string.Format( + CultureInfo.InvariantCulture, + "
{0}
", + property); + stringBuilder.AppendLine(label); + + var value = string.Format( + CultureInfo.InvariantCulture, + "
Model = (null), ModelType = System.String, PropertyName = {0}, " + + "SimpleDisplayText = (null)
", + property); + stringBuilder.AppendLine(value); + } + var expected = stringBuilder.ToString(); + + // Act + var result = DefaultDisplayTemplates.ObjectTemplate(html); + + // Assert + Assert.Equal(expected, result); + } + [Fact] public void HiddenInputTemplate_ReturnsValue() { @@ -309,5 +355,22 @@ namespace Microsoft.AspNet.Mvc.Core html.Display(expression: string.Empty, templateName: null, htmlFieldName: null, additionalViewData: null); viewEngine.Verify(); } + + private class OrderedModel + { + [Display(Order = 10001)] + public string LastProperty { get; set; } + + public string Property3 { get; set; } + public string Property1 { get; set; } + public string Property2 { get; set; } + + [Display(Order = 23)] + public string OrderedProperty3 { get; set; } + [Display(Order = 23)] + public string OrderedProperty2 { get; set; } + [Display(Order = 23)] + public string OrderedProperty1 { get; set; } + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTest.cs similarity index 94% rename from test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTests.cs rename to test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTest.cs index cff2bf9347..8bd15b2970 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/DefaultEditorTemplatesTest.cs @@ -3,7 +3,10 @@ using System; using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.Globalization; using System.Linq; +using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.Rendering; @@ -13,7 +16,7 @@ using Xunit; namespace Microsoft.AspNet.Mvc.Core { - public class DefaultEditorTemplatesTests + public class DefaultEditorTemplatesTest { // Mappings from templateName to expected result when using StubbyHtmlHelper. public static TheoryData TemplateNameData @@ -187,7 +190,7 @@ Environment.NewLine; var model = new DefaultTemplatesUtilities.ObjectTemplateModel { Property1 = "p1", Property2 = null }; var html = DefaultTemplatesUtilities.GetHtmlHelper(model); - + var metadata = html.ViewData.ModelMetadata.Properties["Property1"]; metadata.HideSurroundingHtml = true; @@ -198,6 +201,50 @@ Environment.NewLine; Assert.Equal(expected, result); } + [Fact] + public void ObjectTemplate_OrdersProperties_AsExpected() + { + // Arrange + var model = new OrderedModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + var expectedProperties = new List + { + "OrderedProperty3", + "OrderedProperty2", + "OrderedProperty1", + "Property3", + "Property1", + "Property2", + "LastProperty", + }; + + var stringBuilder = new StringBuilder(); + foreach (var property in expectedProperties) + { + var label = string.Format( + CultureInfo.InvariantCulture, + "
", + property); + stringBuilder.AppendLine(label); + + var value = string.Format( + CultureInfo.InvariantCulture, + "
Model = (null), ModelType = System.String, PropertyName = {0}, " + + "SimpleDisplayText = (null) " + + "" + + "
", + property); + stringBuilder.AppendLine(value); + } + var expected = stringBuilder.ToString(); + + // Act + var result = DefaultEditorTemplates.ObjectTemplate(html); + + // Assert + Assert.Equal(expected, result); + } + [Fact] public void HiddenInputTemplate_ReturnsValueAndHiddenInput() { @@ -702,6 +749,23 @@ Environment.NewLine; viewEngine.Verify(); } + private class OrderedModel + { + [Display(Order = 10001)] + public string LastProperty { get; set; } + + public string Property3 { get; set; } + public string Property1 { get; set; } + public string Property2 { get; set; } + + [Display(Order = 23)] + public string OrderedProperty3 { get; set; } + [Display(Order = 23)] + public string OrderedProperty2 { get; set; } + [Display(Order = 23)] + public string OrderedProperty1 { get; set; } + } + private class StubbyHtmlHelper : IHtmlHelper, ICanHasViewContext { private readonly IHtmlHelper _innerHelper; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/HtmlHelperValidationSummaryTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/HtmlHelperValidationSummaryTest.cs new file mode 100644 index 0000000000..d0173db950 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Rendering/HtmlHelperValidationSummaryTest.cs @@ -0,0 +1,403 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// 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.ComponentModel.DataAnnotations; +using Microsoft.AspNet.Mvc.ModelBinding; +using Xunit; + +namespace Microsoft.AspNet.Mvc.Rendering +{ + public class HtmlHelperValidationSummaryTest + { + // Message, HTML attributes, tag -> expected result. + public static TheoryData ValidValidationSummaryData + { + get + { + var attributes = new { @class = "wood smoke", attribute_name = "attribute-value", }; + var dictionary = new Dictionary + { + { "class", "wood smoke" }, + { "attribute-name", "attribute-value" }, + }; + + var basicDiv = "
" + + "
  • " + Environment.NewLine + + "
"; + var divWithAttributes = "
    " + + "
  • " + Environment.NewLine + + "
"; + var divWithMessage = "
" + + "This is my message" + Environment.NewLine + + "
  • " + Environment.NewLine + + "
"; + var divWithH3Message = "
" + + "

This is my message

" + Environment.NewLine + + "
  • " + Environment.NewLine + + "
"; + var divWithMessageAndAttributes = "
" + + "This is my message" + Environment.NewLine + + "
  • " + Environment.NewLine + + "
"; + var divWithH3MessageAndAttributes = "
" + + "

This is my message

" + Environment.NewLine + + "
  • " + Environment.NewLine + + "
"; + + return new TheoryData + { + { null, null, null, basicDiv }, + { null, null, "h3", basicDiv }, + { null, attributes, null, divWithAttributes }, + { null, attributes, "h3", divWithAttributes }, + { null, dictionary, null, divWithAttributes }, + { null, dictionary, "h3", divWithAttributes }, + { "This is my message", null, null, divWithMessage }, + { "This is my message", null, "h3", divWithH3Message }, + { "This is my message", attributes, null, divWithMessageAndAttributes }, + { "This is my message", attributes, "h3", divWithH3MessageAndAttributes }, + { "This is my message", dictionary, null, divWithMessageAndAttributes }, + { "This is my message", dictionary, "h3", divWithH3MessageAndAttributes }, + }; + } + } + + // Exclude property errors, client validation enabled -> expected result with model error, with property error. + public static TheoryData OneErrorValidationSummaryData + { + get + { + var basicDiv = "
    " + + "
  • " + Environment.NewLine + + "
"; + var divWithError = "
    " + + "
  • This is my validation message
    • " + Environment.NewLine + + "
"; + var divWithErrorAndSummary = "
    " + + "
  • This is my validation message
    • " + Environment.NewLine + + "
"; + + return new TheoryData + { + { false, false, divWithError, divWithError }, + { false, true, divWithErrorAndSummary, divWithErrorAndSummary }, + { true, false, divWithError, basicDiv }, + { true, true, divWithError, basicDiv }, + }; + } + } + + // Exclude property errors, prefix -> expected result + public static TheoryData MultipleErrorsValidationSummaryData + { + get + { + var basicDiv = "
    " + + "
  • " + Environment.NewLine + + "
"; + var divWithRootError = "
    " + + "
  • This is an error for the model root.
    • " + Environment.NewLine + + "
  • This is another error for the model root.
    • " + Environment.NewLine + + "
"; + var divWithProperty3Error = "
    " + + "
  • This is an error for Property3.
    • " + Environment.NewLine + + "
"; + var divWithAllErrors = "
    " + + "
  • This is an error for Property3.Property2.
    • " + Environment.NewLine + + "
  • This is an error for Property3.OrderedProperty3.
    • " + Environment.NewLine + + "
  • This is an error for Property3.OrderedProperty2.
    • " + Environment.NewLine + + "
  • This is an error for Property3.
    • " + Environment.NewLine + + "
  • This is an error for Property2.
    • " + Environment.NewLine + + "
  • This is another error for Property2.
    • " + Environment.NewLine + + "
  • This is an error for the model root.
    • " + Environment.NewLine + + "
  • This is another error for the model root.
    • " + Environment.NewLine + + "
"; + + return new TheoryData + { + { false, string.Empty, divWithAllErrors }, + { false, "Property2", divWithAllErrors }, + { false, "some.unrelated.prefix", divWithAllErrors }, + { true, string.Empty, divWithRootError }, + { true, "Property3", divWithProperty3Error }, + { true, "some.unrelated.prefix", basicDiv }, + }; + } + } + + [Theory] + [MemberData(nameof(ValidValidationSummaryData))] + public void ValidationSummary_AllValid_ReturnsExpectedDiv( + string message, + object htmlAttributes, + string tag, + string expected) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors: false, + message: message, + htmlAttributes: htmlAttributes, + tag: tag); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + [Theory] + [MemberData(nameof(ValidValidationSummaryData))] + public void ValidationSummary_ExcludePropertyErrorsAllValid_ReturnsEmpty( + string message, + object htmlAttributes, + string tag, + string ignored) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors: true, + message: message, + htmlAttributes: htmlAttributes, + tag: tag); + + // Assert + Assert.Equal(HtmlString.Empty, result); + } + + [Theory] + [MemberData(nameof(ValidValidationSummaryData))] + public void ValidationSummary_ClientValidationDisabledAllValid_ReturnsEmpty( + string message, + object htmlAttributes, + string tag, + string ignored) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + html.ViewContext.ClientValidationEnabled = false; + + // Act + var result = html.ValidationSummary( + excludePropertyErrors: false, + message: message, + htmlAttributes: htmlAttributes, + tag: tag); + + // Assert + Assert.Equal(HtmlString.Empty, result); + } + + [Theory] + [MemberData(nameof(OneErrorValidationSummaryData))] + public void ValidationSummary_InvalidModel_ReturnsExpectedDiv( + bool excludePropertyErrors, + bool clientValidationEnabled, + string expected, + string ignored) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + html.ViewContext.ClientValidationEnabled = clientValidationEnabled; + html.ViewData.ModelState.AddModelError(string.Empty, "This is my validation message"); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors: excludePropertyErrors, + message: null, + htmlAttributes: null, + tag: null); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + [Theory] + [MemberData(nameof(OneErrorValidationSummaryData))] + public void ValidationSummary_InvalidModelWithPrefix_ReturnsExpectedDiv( + bool excludePropertyErrors, + bool clientValidationEnabled, + string expected, + string ignored) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + html.ViewContext.ClientValidationEnabled = clientValidationEnabled; + html.ViewData.TemplateInfo.HtmlFieldPrefix = "this.is.my.prefix"; + html.ViewData.ModelState.AddModelError("this.is.my.prefix", "This is my validation message"); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors, + message: null, + htmlAttributes: null, + tag: null); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + [Theory] + [MemberData(nameof(OneErrorValidationSummaryData))] + public void ValidationSummary_OneInvalidProperty_ReturnsExpectedDiv( + bool excludePropertyErrors, + bool clientValidationEnabled, + string ignored, + string expected) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + html.ViewContext.ClientValidationEnabled = clientValidationEnabled; + html.ViewData.ModelState.AddModelError("Property1", "This is my validation message"); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors, + message: null, + htmlAttributes: null, + tag: null); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + [Theory] + [MemberData(nameof(MultipleErrorsValidationSummaryData))] + public void ValidationSummary_MultipleErrors_ReturnsExpectedDiv( + bool excludePropertyErrors, + string prefix, + string expected) + { + // Arrange + var model = new ValidationModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + html.ViewData.TemplateInfo.HtmlFieldPrefix = prefix; + AddMultipleErrors(html.ViewData.ModelState); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors, + message: null, + htmlAttributes: null, + tag: null); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + [Fact] + public void ValidationSummary_ErrorsInModelUsingOrder_SortsErrorsAsExpected() + { + // Arrange + var expected = "
    " + + "
  • This is an error for OrderedProperty3.
    • " + Environment.NewLine + + "
  • This is an error for OrderedProperty2.
    • " + Environment.NewLine + + "
  • This is another error for OrderedProperty2.
    • " + Environment.NewLine + + "
  • This is yet-another error for OrderedProperty2.
    • " + Environment.NewLine + + "
  • This is an error for OrderedProperty1.
    • " + Environment.NewLine + + "
  • This is an error for Property3.
    • " + Environment.NewLine + + "
  • This is an error for Property2.
    • " + Environment.NewLine + + "
  • This is another error for Property2.
    • " + Environment.NewLine + + "
  • This is an error for Property1.
    • " + Environment.NewLine + + "
  • This is another error for Property1.
    • " + Environment.NewLine + + "
  • This is an error for LastProperty.
    • " + Environment.NewLine + + "
"; + + var model = new OrderedModel(); + var html = DefaultTemplatesUtilities.GetHtmlHelper(model); + AddOrderedErrors(html.ViewData.ModelState); + + // Act + var result = html.ValidationSummary( + excludePropertyErrors: false, + message: null, + htmlAttributes: null, + tag: null); + + // Assert + Assert.Equal(expected, result.ToString()); + } + + // Adds errors for various parts of the model, including the root. + private void AddMultipleErrors(ModelStateDictionary modelState) + { + modelState.AddModelError("Property3.Property2", "This is an error for Property3.Property2."); + modelState.AddModelError("Property3.OrderedProperty3", "This is an error for Property3.OrderedProperty3."); + modelState.AddModelError("Property3.OrderedProperty2", "This is an error for Property3.OrderedProperty2."); + + modelState.AddModelError("Property3", "This is an error for Property3."); + modelState.AddModelError("Property3", new InvalidCastException("Exception will be ignored.")); + + modelState.AddModelError("Property2", "This is an error for Property2."); + modelState.AddModelError("Property2", "This is another error for Property2."); + + modelState.AddModelError(string.Empty, "This is an error for the model root."); + modelState.AddModelError(string.Empty, "This is another error for the model root."); + modelState.AddModelError(string.Empty, new InvalidOperationException("Another ignored Exception.")); + } + + // Adds one or more errors for all properties in OrderedModel. But adds errors out of order. + private void AddOrderedErrors(ModelStateDictionary modelState) + { + modelState.AddModelError("Property3", "This is an error for Property3."); + modelState.AddModelError("Property3", new InvalidCastException("An ignored Exception.")); + + modelState.AddModelError("Property2", "This is an error for Property2."); + modelState.AddModelError("Property2", "This is another error for Property2."); + + modelState.AddModelError("OrderedProperty3", "This is an error for OrderedProperty3."); + + modelState.AddModelError("OrderedProperty2", "This is an error for OrderedProperty2."); + modelState.AddModelError("OrderedProperty2", "This is another error for OrderedProperty2."); + + modelState.AddModelError("LastProperty", "This is an error for LastProperty."); + + modelState.AddModelError("Property1", "This is an error for Property1."); + modelState.AddModelError("Property1", "This is another error for Property1."); + + modelState.AddModelError("OrderedProperty1", "This is an error for OrderedProperty1."); + modelState.AddModelError("OrderedProperty2", "This is yet-another error for OrderedProperty2."); + } + + private class ValidationModel + { + public string Property1 { get; set; } + + public string Property2 { get; set; } + + public OrderedModel Property3 { get; set; } + } + + private class OrderedModel + { + [Display(Order = 10001)] + public string LastProperty { get; set; } + + public string Property3 { get; set; } + public string Property1 { get; set; } + public string Property2 { get; set; } + + [Display(Order = 23)] + public string OrderedProperty3 { get; set; } + [Display(Order = 23)] + public string OrderedProperty2 { get; set; } + [Display(Order = 23)] + public string OrderedProperty1 { get; set; } + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Details.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Details.html new file mode 100644 index 0000000000..230e53cccd --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Details.html @@ -0,0 +1,21 @@ +

Vehicle details

+
+
Model
+
the Fastener
+
Make
+
Fast Cars
+
Vin
+
87654321
+
Year
+
2013
+
LastUpdatedTrackingId
+
+ + +
+ InspectedDates +
+
+ 01/04/2001 00:00:00 -08:00 +
+
\ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.Invalid.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.Invalid.html new file mode 100644 index 0000000000..1ad9c88542 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.Invalid.html @@ -0,0 +1,22 @@ +
+
  • The field Vin must be a string with a maximum length of 8.
    • +
  • The field Year must be between 1980 and 2034.
    • +
  • The field InspectedDates must be a string or array type with a maximum length of '10'.
    • +
+
+
+
+
+
The field Vin must be a string with a maximum length of 8.
+
+
The field Year must be between 1980 and 2034.
+
+
+
+ +
+
+ +
+ +
\ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.html new file mode 100644 index 0000000000..a58f537d97 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources/ModelBindingWebSite.Vehicle.Edit.html @@ -0,0 +1,20 @@ +
+
  • +
+
+
+
+
+
+
+
+
+
+
+ +
+
+ +
+ +
\ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs similarity index 94% rename from test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTests.cs rename to test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs index 07a648c259..67107569b4 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs @@ -19,7 +19,7 @@ using Xunit; namespace Microsoft.AspNet.Mvc.FunctionalTests { - public class ModelBindingTests + public class ModelBindingTest { private readonly IServiceProvider _services = TestHelper.CreateServices("ModelBindingWebSite"); private readonly Action _app = new ModelBindingWebSite.Startup().Configure; @@ -1450,5 +1450,84 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // Round-tripped value includes descendent instances for all properties with data in the request. Assert.Equal("grandFatherName", employee.Parent.Parent.Name); } + + [Fact] + public async Task HtmlHelper_DisplayFor_ShowsPropertiesInModelMetadataOrder() + { + // Arrange + var expectedContent = await GetType().GetTypeInfo().Assembly.ReadResourceAsStringAsync( + "compiler/resources/ModelBindingWebSite.Vehicle.Details.html"); + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + var url = "http://localhost/vehicles/42"; + + // Act + var response = await client.GetAsync(url); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(expectedContent, body); + } + + [Fact] + public async Task HtmlHelper_EditorFor_ShowsPropertiesInModelMetadataOrder() + { + // Arrange + var expectedContent = await GetType().GetTypeInfo().Assembly.ReadResourceAsStringAsync( + "compiler/resources/ModelBindingWebSite.Vehicle.Edit.html"); + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + var url = "http://localhost/vehicles/42/edit"; + + // Act + var response = await client.GetAsync(url); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(expectedContent, body); + } + + [Fact] + public async Task HtmlHelper_EditorFor_ShowsPropertiesAndErrorsInModelMetadataOrder() + { + // Arrange + var expectedContent = await GetType().GetTypeInfo().Assembly.ReadResourceAsStringAsync( + "compiler/resources/ModelBindingWebSite.Vehicle.Edit.Invalid.html"); + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + var url = "http://localhost/vehicles/42/edit"; + var contentDictionary = new Dictionary + { + { "Make", "Fast Cars" }, + { "Model", "the Fastener" }, + { "InspectedDates[0]", "14/10/1979 00:00:00 -08:00" }, + { "InspectedDates[1]", "16/10/1979 00:00:00 -08:00" }, + { "InspectedDates[2]", "02/11/1979 00:00:00 -08:00" }, + { "InspectedDates[3]", "13/11/1979 00:00:00 -08:00" }, + { "InspectedDates[4]", "05/12/1979 00:00:00 -08:00" }, + { "InspectedDates[5]", "12/12/1979 00:00:00 -08:00" }, + { "InspectedDates[6]", "19/12/1979 00:00:00 -08:00" }, + { "InspectedDates[7]", "26/12/1979 00:00:00 -08:00" }, + { "InspectedDates[8]", "28/12/1979 00:00:00 -08:00" }, + { "InspectedDates[9]", "29/12/1979 00:00:00 -08:00" }, + { "InspectedDates[10]", "01/04/1980 00:00:00 -08:00" }, + { "Vin", "8765432112345678" }, + { "Year", "1979" }, + }; + var requestContent = new FormUrlEncodedContent(contentDictionary); + + // Act + var response = await client.PostAsync(url, requestContent); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(expectedContent, body); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/CachedDataAnnotationsModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/CachedDataAnnotationsModelMetadataTest.cs index d7d71d730d..9fe52fc77d 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/CachedDataAnnotationsModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/CachedDataAnnotationsModelMetadataTest.cs @@ -229,6 +229,61 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(expectedResult, result); } + public static TheoryData DisplayAttribute_OverridesOrderData + { + get + { + return new TheoryData + { + { + new DisplayAttribute(), ModelMetadata.DefaultOrder + }, + { + new DisplayAttribute { Order = int.MinValue }, int.MinValue + }, + { + new DisplayAttribute { Order = -100 }, -100 + }, + { + new DisplayAttribute { Order = -1 }, -1 + }, + { + new DisplayAttribute { Order = 0 }, 0 + }, + { + new DisplayAttribute { Order = 1 }, 1 + }, + { + new DisplayAttribute { Order = 200 }, 200 + }, + { + new DisplayAttribute { Order = int.MaxValue }, int.MaxValue + }, + }; + } + } + + [Theory] + [MemberData(nameof(DisplayAttribute_OverridesOrderData))] + public void DisplayAttribute_OverridesOrder(DisplayAttribute attribute, int expectedOrder) + { + // Arrange + var attributes = new[] { attribute }; + var provider = new DataAnnotationsModelMetadataProvider(); + var metadata = new CachedDataAnnotationsModelMetadata( + provider, + containerType: null, + modelType: typeof(object), + propertyName: null, + attributes: attributes); + + // Act + var result = metadata.Order; + + // Assert + Assert.Equal(expectedOrder, result); + } + [Fact] public void BinderMetadataIfPresent_Overrides_DefaultBinderMetadata() { diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs index 06668c7401..9f53c03c5e 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs @@ -7,9 +7,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; -#if ASPNET50 -using Moq; -#endif +using System.Reflection; using Xunit; namespace Microsoft.AspNet.Mvc.ModelBinding @@ -26,8 +24,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var binderMetadata = new TestBinderMetadata(); var predicateProvider = new DummyPropertyBindingPredicateProvider(); - var emptyPropertyList = new List(); - var nonEmptyPropertyList = new List() { "SomeProperty" }; + return new TheoryData, Func, object> { { m => m.ConvertEmptyStringToNull = false, m => m.ConvertEmptyStringToNull, false }, @@ -108,6 +105,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(typeof(string), metadata.ModelType); Assert.Equal("propertyName", metadata.PropertyName); + Assert.Equal(10000, ModelMetadata.DefaultOrder); Assert.Equal(ModelMetadata.DefaultOrder, metadata.Order); Assert.Null(metadata.BinderModelName); @@ -279,27 +277,168 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } // Properties -#if ASPNET50 + [Fact] - public void PropertiesCallsProvider() + public void PropertiesProperty_CallsProvider() { // Arrange - var modelType = typeof(string); - var propertyMetadata = new List(); - var provider = new Mock(); - var metadata = new ModelMetadata(provider.Object, null, null, modelType, null); - provider.Setup(p => p.GetMetadataForProperties(null, modelType)) - .Returns(propertyMetadata) - .Verifiable(); + var modelType = typeof(object); + var provider = new PropertiesModelMetadataProvider(new List()); + var metadata = new ModelMetadata( + provider, + containerType: null, + modelAccessor: null, + modelType: modelType, + propertyName: null); // Act var result = metadata.Properties; // Assert - Assert.Equal(propertyMetadata, result.ToList()); - provider.Verify(); + Assert.Empty(result); + Assert.Equal(1, provider.GetMetadataForPropertiesCalls); + } + + // Input (original) property names and expected (ordered) property names. + public static TheoryData, IEnumerable> PropertyNamesTheoryData + { + get + { + // ModelMetadata does not reorder properties Reflection returns without an Order override. + return new TheoryData, IEnumerable> + { + { + new List { "Property1", "Property2", "Property3", "Property4", }, + new List { "Property1", "Property2", "Property3", "Property4", } + }, + { + new List { "Property4", "Property3", "Property2", "Property1", }, + new List { "Property4", "Property3", "Property2", "Property1", } + }, + { + new List { "Delta", "Bravo", "Charlie", "Alpha", }, + new List { "Delta", "Bravo", "Charlie", "Alpha", } + }, + { + new List { "John", "Jonathan", "Jon", "Joan", }, + new List { "John", "Jonathan", "Jon", "Joan", } + }, + }; + } + } + + [Theory] + [MemberData(nameof(PropertyNamesTheoryData))] + public void PropertiesProperty_WithDefaultOrder_OrdersPropertyNamesAlphabetically( + IEnumerable originalNames, + IEnumerable expectedNames) + { + // Arrange + var modelType = typeof(object); + var provider = new PropertiesModelMetadataProvider(originalNames); + var metadata = new ModelMetadata( + provider, + containerType: null, + modelAccessor: null, + modelType: modelType, + propertyName: null); + + // Act + var result = metadata.Properties; + + // Assert + var propertyNames = result.Select(property => property.PropertyName); + Assert.Equal(expectedNames, propertyNames); + } + + // Input (original) property names, Order values, and expected (ordered) property names. + public static TheoryData>, IEnumerable> + PropertyNamesAndOrdersTheoryData + { + get + { + return new TheoryData>, IEnumerable> + { + { + new List> + { + new KeyValuePair("Property1", 23), + new KeyValuePair("Property2", 23), + new KeyValuePair("Property3", 23), + new KeyValuePair("Property4", 23), + }, + new List { "Property1", "Property2", "Property3", "Property4", } + }, + // Same order if already ordered using Order. + { + new List> + { + new KeyValuePair("Property4", 23), + new KeyValuePair("Property3", 24), + new KeyValuePair("Property2", 25), + new KeyValuePair("Property1", 26), + }, + new List { "Property4", "Property3", "Property2", "Property1", } + }, + // Rest of the orderings get updated within ModelMetadata. + { + new List> + { + new KeyValuePair("Property1", 26), + new KeyValuePair("Property2", 25), + new KeyValuePair("Property3", 24), + new KeyValuePair("Property4", 23), + }, + new List { "Property4", "Property3", "Property2", "Property1", } + }, + { + new List> + { + new KeyValuePair("Alpha", 26), + new KeyValuePair("Bravo", 24), + new KeyValuePair("Charlie", 23), + new KeyValuePair("Delta", 25), + }, + new List { "Charlie", "Bravo", "Delta", "Alpha", } + }, + // Jonathan and Jon will not be reordered. + { + new List> + { + new KeyValuePair("Joan", 1), + new KeyValuePair("Jonathan", 0), + new KeyValuePair("Jon", 0), + new KeyValuePair("John", -1), + }, + new List { "John", "Jonathan", "Jon", "Joan", } + }, + }; + } + } + + [Theory] + [MemberData(nameof(PropertyNamesAndOrdersTheoryData))] + public void PropertiesProperty_OrdersPropertyNamesUsingOrder_ThenAlphabetically( + IEnumerable> originalNamesAndOrders, + IEnumerable expectedNames) + { + // Arrange + var modelType = typeof(object); + var provider = new PropertiesModelMetadataProvider(originalNamesAndOrders); + var metadata = new ModelMetadata( + provider, + containerType: null, + modelAccessor: null, + modelType: modelType, + propertyName: null); + + // Act + var result = metadata.Properties; + + // Assert + var propertyNames = result.Select(property => property.PropertyName); + Assert.Equal(expectedNames, propertyNames); } -#endif [Theory] [MemberData(nameof(MetadataModifierData))] @@ -568,5 +707,76 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { public Func PropertyFilter { get; set; } } + + // Gives object type properties with provided names or names and Order values. + private class PropertiesModelMetadataProvider : IModelMetadataProvider + { + private List _properties = new List(); + + public PropertiesModelMetadataProvider(IEnumerable propertyNames) + { + foreach (var propertyName in propertyNames) + { + var metadata = new ModelMetadata( + this, + containerType: typeof(DummyContactModel), + modelAccessor: null, + modelType: typeof(string), + propertyName: propertyName); + + _properties.Add(metadata); + } + } + + public PropertiesModelMetadataProvider(IEnumerable> propertyNamesAndOrders) + { + foreach (var keyValuePair in propertyNamesAndOrders) + { + var metadata = new ModelMetadata( + this, + containerType: typeof(DummyContactModel), + modelAccessor: null, + modelType: typeof(string), + propertyName: keyValuePair.Key) + { + Order = keyValuePair.Value, + }; + + _properties.Add(metadata); + } + } + + public int GetMetadataForPropertiesCalls { get; private set; } + + public ModelMetadata GetMetadataForParameter( + Func modelAccessor, + [NotNull] MethodInfo methodInfo, + [NotNull] string parameterName) + { + throw new NotImplementedException(); + } + + public IEnumerable GetMetadataForProperties(object container, [NotNull] Type containerType) + { + Assert.Null(container); + Assert.Equal(typeof(object), containerType); + GetMetadataForPropertiesCalls++; + + return _properties; + } + + public ModelMetadata GetMetadataForProperty( + Func modelAccessor, + [NotNull] Type containerType, + [NotNull] string propertyName) + { + throw new NotImplementedException(); + } + + public ModelMetadata GetMetadataForType(Func modelAccessor, [NotNull] Type modelType) + { + throw new NotImplementedException(); + } + } } } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs index 4b16e03562..c82d8d98ae 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs @@ -382,14 +382,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding dictionary.AddModelError("key3", new Exception()); dictionary.AddModelError("key4", "error4"); dictionary.AddModelError("key5", "error5"); - dictionary.AddModelError("key6", "error6"); // Act and Assert Assert.True(dictionary.HasReachedMaxErrors); Assert.Equal(5, dictionary.ErrorCount); - var error = Assert.Single(dictionary[""].Errors); + var error = Assert.Single(dictionary[string.Empty].Errors); Assert.IsType(error.Exception); Assert.Equal(expected, error.Exception.Message); + + // TooManyModelErrorsException added instead of key5 error. + Assert.DoesNotContain("key5", dictionary.Keys); } [Fact] @@ -403,25 +405,35 @@ namespace Microsoft.AspNet.Mvc.ModelBinding }; // Act and Assert + Assert.False(dictionary.HasReachedMaxErrors); var result = dictionary.TryAddModelError("key1", "error1"); Assert.True(result); + Assert.False(dictionary.HasReachedMaxErrors); result = dictionary.TryAddModelError("key2", new Exception()); Assert.True(result); + Assert.False(dictionary.HasReachedMaxErrors); // Still room for TooManyModelErrorsException. result = dictionary.TryAddModelError("key3", "error3"); Assert.False(result); - result = dictionary.TryAddModelError("key4", "error4"); + Assert.True(dictionary.HasReachedMaxErrors); + result = dictionary.TryAddModelError("key4", "error4"); // no-op Assert.False(result); Assert.True(dictionary.HasReachedMaxErrors); Assert.Equal(3, dictionary.ErrorCount); Assert.Equal(3, dictionary.Count); - var error = Assert.Single(dictionary[""].Errors); + var error = Assert.Single(dictionary[string.Empty].Errors); Assert.IsType(error.Exception); Assert.Equal(expected, error.Exception.Message); + + // TooManyModelErrorsException added instead of key3 error. + Assert.DoesNotContain("key3", dictionary.Keys); + + // Last addition did nothing. + Assert.DoesNotContain("key4", dictionary.Keys); } [Fact] @@ -437,16 +449,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding dictionary.AddModelError("key2", "error2"); dictionary.AddModelError("key3", "error3"); dictionary.AddModelError("key3", new Exception()); - dictionary.AddModelError("key4", new InvalidOperationException()); - dictionary.AddModelError("key5", new FormatException()); // Act and Assert Assert.True(dictionary.HasReachedMaxErrors); Assert.Equal(4, dictionary.ErrorCount); Assert.Equal(4, dictionary.Count); - var error = Assert.Single(dictionary[""].Errors); + var error = Assert.Single(dictionary[string.Empty].Errors); Assert.IsType(error.Exception); Assert.Equal(expected, error.Exception.Message); + + // Second key3 model error resulted in TooManyModelErrorsException instead. + error = Assert.Single(dictionary["key3"].Errors); + Assert.Null(error.Exception); + Assert.Equal("error3", error.ErrorMessage); } [Fact] @@ -470,7 +485,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.False(result); Assert.Equal(3, dictionary.Count); - var error = Assert.Single(dictionary[""].Errors); + var error = Assert.Single(dictionary[string.Empty].Errors); Assert.IsType(error.Exception); Assert.Equal(expected, error.Exception.Message); } @@ -494,7 +509,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert Assert.Equal(3, copy.Count); - var error = Assert.Single(copy[""].Errors); + var error = Assert.Single(copy[string.Empty].Errors); Assert.IsType(error.Exception); Assert.Equal(expected, error.Exception.Message); } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs index b9138d6ba4..2e952dd08f 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs @@ -1,13 +1,11 @@ // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -#if ASPNET50 using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.Linq; using Microsoft.AspNet.Testing; -using Moq; using Xunit; namespace Microsoft.AspNet.Mvc.ModelBinding @@ -166,6 +164,97 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(expected, log); } + // Validation order is primarily important when MaxAllowedErrors has been overridden. + [Fact] + public void Validate_OrdersUsingModelMetadata() + { + // Proper order of invocation: + // 1. OnValidating() + // 2. Child validators -- ordered using ModelMetadata.Order. + // 3. OnValidated() + + // Arrange + var expected = new[] + { + "In OnValidating()", + "In LoggingValidatonAttribute.IsValid(OrderedProperty3)", + "In LoggingValidatonAttribute.IsValid(OrderedProperty2)", + "In LoggingValidatonAttribute.IsValid(OrderedProperty1)", + "In LoggingValidatonAttribute.IsValid(Property3)", + "In LoggingValidatonAttribute.IsValid(Property1)", + "In LoggingValidatonAttribute.IsValid(Property2)", + "In LoggingValidatonAttribute.IsValid(LastProperty)", + "In OnValidated()" + }; + + var log = new List(); + var model = new LoggingNonValidatableObject(log); + var provider = new DataAnnotationsModelMetadataProvider(); + var modelMetadata = provider.GetMetadataForType(() => model, model.GetType()); + var node = new ModelValidationNode(modelMetadata, "theKey") + { + ValidateAllProperties = true, + }; + node.Validating += (sender, e) => log.Add("In OnValidating()"); + node.Validated += (sender, e) => log.Add("In OnValidated()"); + var context = CreateContext(modelMetadata, provider); + + // Act + node.Validate(context); + + // Assert + Assert.Equal(expected, log); + } + + [Fact] + public void Validate_ChildNodes_OverridesOrdering() + { + // Proper order of invocation: + // 1. OnValidating() + // 2. Child validators -- ordered using ChildNodes, then ModelMetadata.Order. + // 3. OnValidated() + + // Arrange + var expected = new[] + { + "In OnValidating()", + "In LoggingValidatonAttribute.IsValid(LastProperty)", + "In LoggingValidatonAttribute.IsValid(OrderedProperty3)", + "In LoggingValidatonAttribute.IsValid(OrderedProperty2)", + "In LoggingValidatonAttribute.IsValid(OrderedProperty1)", + "In LoggingValidatonAttribute.IsValid(Property3)", + "In LoggingValidatonAttribute.IsValid(Property1)", + "In LoggingValidatonAttribute.IsValid(Property2)", + "In OnValidated()" + }; + + var log = new List(); + var model = new LoggingNonValidatableObject(log); + var provider = new DataAnnotationsModelMetadataProvider(); + var modelMetadata = provider.GetMetadataForType(() => model, model.GetType()); + var childMetadata = modelMetadata.Properties.FirstOrDefault( + property => property.PropertyName == "LastProperty"); + Assert.NotNull(childMetadata); // Guard + + var node = new ModelValidationNode(modelMetadata, "theKey") + { + ChildNodes = + { + new ModelValidationNode(childMetadata, "theKey.LastProperty") + }, + ValidateAllProperties = true, + }; + node.Validating += (sender, e) => log.Add("In OnValidating()"); + node.Validated += (sender, e) => log.Add("In OnValidated()"); + var context = CreateContext(modelMetadata, provider); + + // Act + node.Validate(context); + + // Assert + Assert.Equal(expected, log); + } + [Fact] public void Validate_SkipsRemainingValidationIfModelStateIsInvalid() { @@ -291,13 +380,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding node.Validate(context); // Assert - Assert.False(context.ModelState.ContainsKey("theKey.RequiredString")); - Assert.Equal("existing Error Text", - context.ModelState["theKey.RequiredString.Dummy"].Errors[0].ErrorMessage); - Assert.Equal("The field RangedInt must be between 10 and 30.", - context.ModelState["theKey.RangedInt"].Errors[0].ErrorMessage); - Assert.False(context.ModelState.ContainsKey("theKey.ValidString")); - Assert.False(context.ModelState.ContainsKey("theKey")); + var modelState = context.ModelState["theKey.RequiredString.Dummy"]; + Assert.NotNull(modelState); + var error = Assert.Single(modelState.Errors); + Assert.Equal("existing Error Text", error.ErrorMessage); + + modelState = context.ModelState["theKey.RangedInt"]; + Assert.NotNull(modelState); + error = Assert.Single(modelState.Errors); + Assert.Equal("The field RangedInt must be between 10 and 30.", error.ErrorMessage); + + Assert.DoesNotContain("theKey.RequiredString", context.ModelState.Keys); + Assert.DoesNotContain("theKey.ValidString", context.ModelState.Keys); + Assert.DoesNotContain("theKey", context.ModelState.Keys); } [Fact] @@ -311,6 +406,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding RangedInt = 0 /* error */, ValidString = "cat" /* error */ }; + var expectedMessage = ValidationAttributeUtil.GetRequiredErrorMessage("RequiredString"); var modelMetadata = GetModelMetadata(model); var node = new ModelValidationNode(modelMetadata, "theKey") @@ -326,11 +422,20 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert Assert.Equal(3, context.ModelState.Count); - Assert.IsType(context.ModelState[""].Errors[0].Exception); - Assert.Equal(ValidationAttributeUtil.GetRequiredErrorMessage("RequiredString"), - context.ModelState["theKey.RequiredString"].Errors[0].ErrorMessage); - Assert.False(context.ModelState.ContainsKey("theKey.RangedInt")); - Assert.False(context.ModelState.ContainsKey("theKey.ValidString")); + var modelState = context.ModelState[string.Empty]; + Assert.NotNull(modelState); + var error = Assert.Single(modelState.Errors); + Assert.IsType(error.Exception); + + // RequiredString is validated first due to ModelMetadata.Properties ordering (Reflection-based). + modelState = context.ModelState["theKey.RequiredString"]; + Assert.NotNull(modelState); + error = Assert.Single(modelState.Errors); + Assert.Equal(expectedMessage, error.ErrorMessage); + + // No room for the other validation errors. + Assert.DoesNotContain("theKey.RangedInt", context.ModelState.Keys); + Assert.DoesNotContain("theKey.ValidString", context.ModelState.Keys); } private static ModelMetadata GetModelMetadata() @@ -338,9 +443,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return new EmptyModelMetadataProvider().GetMetadataForType(null, typeof(object)); } - private static ModelMetadata GetModelMetadata(object o) + private static ModelMetadata GetModelMetadata(object model) { - return new DataAnnotationsModelMetadataProvider().GetMetadataForType(() => o, o.GetType()); + return new DataAnnotationsModelMetadataProvider().GetMetadataForType(() => model, model.GetType()); } private static ModelMetadata GetModelMetadata(Type type) @@ -348,15 +453,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return new DataAnnotationsModelMetadataProvider().GetMetadataForType(modelAccessor: null, modelType: type); } - private static ModelValidationContext CreateContext(ModelMetadata metadata = null) + private static ModelValidationContext CreateContext( + ModelMetadata metadata = null, + IModelMetadataProvider metadataProvider = null) { var providers = new IModelValidatorProvider[] { new DataAnnotationsModelValidatorProvider(), new DataMemberModelValidatorProvider() }; + if (metadataProvider == null) + { + metadataProvider = new EmptyModelMetadataProvider(); + } - return new ModelValidationContext(new EmptyModelMetadataProvider(), + return new ModelValidationContext(metadataProvider, new CompositeModelValidatorProvider(providers), new ModelStateDictionary(), metadata, @@ -378,6 +489,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public IEnumerable Validate(ValidationContext validationContext) { + Assert.Null(validationContext.MemberName); _log.Add("In IValidatableObject.Validate()"); yield return new ValidationResult("Sample error message", new[] { "InvalidStringProperty" }); } @@ -386,8 +498,56 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { protected override ValidationResult IsValid(object value, ValidationContext validationContext) { - LoggingValidatableObject lvo = (LoggingValidatableObject)value; - lvo._log.Add("In LoggingValidatonAttribute.IsValid()"); + var validatableObject = Assert.IsType(value); + Assert.NotNull(validationContext); + Assert.Equal("ValidStringProperty", validationContext.MemberName); + validatableObject._log.Add("In LoggingValidatonAttribute.IsValid()"); + return ValidationResult.Success; + } + } + } + + private sealed class LoggingNonValidatableObject + { + private readonly IList _log; + + public LoggingNonValidatableObject(IList log) + { + _log = log; + } + + [LoggingValidation] + [Display(Order = 10001)] + public string LastProperty { get; set; } + + [LoggingValidation] + public string Property3 { get; set; } + [LoggingValidation] + public string Property1 { get; set; } + [LoggingValidation] + public string Property2 { get; set; } + + [LoggingValidation] + [Display(Order = 23)] + public string OrderedProperty3 { get; set; } + [LoggingValidation] + [Display(Order = 23)] + public string OrderedProperty2 { get; set; } + [LoggingValidation] + [Display(Order = 23)] + public string OrderedProperty1 { get; set; } + + private sealed class LoggingValidationAttribute : ValidationAttribute + { + protected override ValidationResult IsValid(object value, ValidationContext validationContext) + { + Assert.Null(value); + Assert.NotNull(validationContext); + var nonValidatableObject = + Assert.IsType(validationContext.ObjectInstance); + + nonValidatableObject._log.Add( + string.Format("In LoggingValidatonAttribute.IsValid({0})", validationContext.MemberName)); return ValidationResult.Success; } } @@ -406,4 +566,3 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } } -#endif diff --git a/test/WebSites/ModelBindingWebSite/Controllers/VehicleController.cs b/test/WebSites/ModelBindingWebSite/Controllers/VehicleController.cs index b3b2575e55..ef26984c5d 100644 --- a/test/WebSites/ModelBindingWebSite/Controllers/VehicleController.cs +++ b/test/WebSites/ModelBindingWebSite/Controllers/VehicleController.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. // 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.ComponentModel.DataAnnotations; using System.Linq; @@ -13,6 +14,26 @@ namespace ModelBindingWebSite { public class VehicleController : Controller { + private static VehicleViewModel _vehicle = new VehicleViewModel + { + InspectedDates = new[] + { + // 01/04/2001 00:00:00 -08:00 + new DateTimeOffset( + year: 2001, + month: 4, + day: 1, + hour: 0, + minute: 0, + second: 0, + offset: TimeSpan.FromHours(-8)), + }, + Make = "Fast Cars", + Model = "the Fastener", + Vin = "87654321", + Year = 2013, + }; + [HttpPut("/api/vehicles/{id}")] [Produces("application/json")] public object UpdateVehicleApi( @@ -43,6 +64,73 @@ namespace ModelBindingWebSite return PartialView("UpdateSuccessful", model); } + [HttpGet("/vehicles/{id:int}")] + public IActionResult Details(int id) + { + if (id != 42) + { + return HttpNotFound(); + } + + return View(_vehicle); + } + + [HttpGet("/vehicles/{id:int}/edit")] + public IActionResult Edit(int id) + { + if (id != 42) + { + return HttpNotFound(); + } + + // Provide room for one additional inspection if not already full. + var vehicle = _vehicle; + var length = vehicle.InspectedDates.Length; + if (length < 10) + { + var array = new DateTimeOffset[length + 1]; + vehicle.InspectedDates.CopyTo(array, 0); + + // Don't update the stored VehicleViewModel instance. + vehicle = new VehicleViewModel + { + InspectedDates = array, + LastUpdatedTrackingId = vehicle.LastUpdatedTrackingId, + Make = vehicle.Make, + Model = vehicle.Model, + Vin = vehicle.Vin, + Year = vehicle.Year, + }; + } + + return View(vehicle); + } + + [HttpPost("/vehicles/{id:int}/edit")] + public IActionResult Edit(int id, VehicleViewModel vehicle) + { + if (id != 42) + { + return HttpNotFound(); + } + + if (!ModelState.IsValid) + { + return View(vehicle); + } + + if (vehicle.InspectedDates != null) + { + // Ignore empty inspection values. + var nonEmptyDates = vehicle.InspectedDates.Where(date => date != default(DateTimeOffset)).ToArray(); + vehicle.InspectedDates = nonEmptyDates; + } + + _vehicle = vehicle; + + return RedirectToAction(nameof(Details), new { id = id }); + } + public IDictionary> SerializeModelState() { Response.StatusCode = (int)HttpStatusCode.BadRequest; diff --git a/test/WebSites/ModelBindingWebSite/ViewModels/VehicleViewModel.cs b/test/WebSites/ModelBindingWebSite/ViewModels/VehicleViewModel.cs index 907ccbe7fb..db45af459b 100644 --- a/test/WebSites/ModelBindingWebSite/ViewModels/VehicleViewModel.cs +++ b/test/WebSites/ModelBindingWebSite/ViewModels/VehicleViewModel.cs @@ -10,22 +10,28 @@ namespace ModelBindingWebSite.ViewModels { public class VehicleViewModel : IValidatableObject { + // Placed using default Order (10000). [Required] [StringLength(8)] public string Vin { get; set; } + [Display(Order = 1)] public string Make { get; set; } + [Display(Order = 0)] public string Model { get; set; } + // Placed using default Order (10000). [Range(1980, 2034)] [CustomValidation(typeof(VehicleViewModel), nameof(ValidateYear))] public int Year { get; set; } + [Display(Order = 20000)] [Required] [MaxLength(10)] public DateTimeOffset[] InspectedDates { get; set; } + [Display(Order = 20000)] public string LastUpdatedTrackingId { get; set; } public IEnumerable Validate(ValidationContext validationContext) diff --git a/test/WebSites/ModelBindingWebSite/Views/Vehicle/Details.cshtml b/test/WebSites/ModelBindingWebSite/Views/Vehicle/Details.cshtml new file mode 100644 index 0000000000..b5886ddb4b --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Views/Vehicle/Details.cshtml @@ -0,0 +1,13 @@ +@model ModelBindingWebSite.ViewModels.VehicleViewModel + +

Vehicle details

+
+ @Html.DisplayFor(m => m) + @* DisplayFor ignores properties in complex objects that don't have simple types. Add the collection explicitly. *@ +
+ @Html.NameFor(m => m.InspectedDates) +
+
+ @Html.DisplayFor(m => m.InspectedDates) +
+
\ No newline at end of file diff --git a/test/WebSites/ModelBindingWebSite/Views/Vehicle/Edit.cshtml b/test/WebSites/ModelBindingWebSite/Views/Vehicle/Edit.cshtml new file mode 100644 index 0000000000..13bde77678 --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Views/Vehicle/Edit.cshtml @@ -0,0 +1,19 @@ +@model ModelBindingWebSite.ViewModels.VehicleViewModel + +
+ @using (Html.BeginForm()) + { + @Html.ValidationSummary() + @Html.EditorFor(m => m) + + @* EditorFor ignores properties in complex objects that don't have simple types. Add the collection explicitly. *@ +
+ @Html.LabelFor(m => m.InspectedDates) +
+
+ @Html.EditorFor(m => m.InspectedDates) +
+ + + } +
\ No newline at end of file