diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs index 4ad43c9168..f854657dd1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs @@ -65,10 +65,18 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(bindingContext)); } - // For compatibility with MVC 5.0 for top level object we want to consider an empty key instead of - // the parameter name/a custom name. In all other cases (like when binding body to a property) we - // consider the entire ModelName as a prefix. - var modelBindingKey = bindingContext.IsTopLevelObject ? string.Empty : bindingContext.ModelName; + // Special logic for body, treat the model name as string.Empty for the top level + // object, but allow an override via BinderModelName. The purpose of this is to try + // and be similar to the behavior for POCOs bound via traditional model binding. + string modelBindingKey; + if (bindingContext.IsTopLevelObject) + { + modelBindingKey = bindingContext.BinderModelName ?? string.Empty; + } + else + { + modelBindingKey = bindingContext.ModelName; + } var httpContext = bindingContext.OperationBindingContext.HttpContext; @@ -102,9 +110,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var result = await formatter.ReadAsync(formatterContext); var model = result.Model; - // Ensure a "modelBindingKey" entry exists whether or not formatting was successful. - bindingContext.ModelState.SetModelValue(modelBindingKey, rawValue: model, attemptedValue: null); - if (result.HasError) { // Formatter encountered an error. Do not use the model it returned. As above, tell the model diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs index 8df6e9b69c..0b0097e254 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs @@ -3,6 +3,7 @@ using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; namespace Microsoft.AspNetCore.Mvc.ModelBinding { @@ -16,7 +17,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { if (bindingContext.ModelType == typeof(CancellationToken)) { - var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted; + // We need to force boxing now, so we can insert the same reference to the boxed CancellationToken + // in both the ValidationState and ModelBindingResult. + // + // DO NOT simplify this code by removing the cast. + var model = (object)bindingContext.OperationBindingContext.HttpContext.RequestAborted; + bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true }); return ModelBindingResult.SuccessAsync(bindingContext.ModelName, model); } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs index 94e2893c39..c46f914720 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs @@ -76,12 +76,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding ValidationStateEntry entry; if (!bindingContext.ValidationState.TryGetValue(result.Model, out entry)) { - entry = new ValidationStateEntry(); + entry = new ValidationStateEntry() + { + Key = result.Key, + Metadata = bindingContext.ModelMetadata, + }; bindingContext.ValidationState.Add(result.Model, entry); } - - entry.Key = entry.Key ?? result.Key; - entry.Metadata = entry.Metadata ?? bindingContext.ModelMetadata; } return result; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs index 5a16880412..794dfb6ebd 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs @@ -43,15 +43,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private async Task BindModelCoreAsync(ModelBindingContext bindingContext) { + // If we're at the top level, then use the FieldName (paramter or property name). + // This handles the fact that there will be nothing in the ValueProviders for this parameter + // and so we'll do the right thing even though we 'fell-back' to the empty prefix. + var modelName = bindingContext.IsTopLevelObject + ? bindingContext.BinderModelName ?? bindingContext.FieldName + : bindingContext.ModelName; + object value; if (bindingContext.ModelType == typeof(IFormFile)) { - var postedFiles = await GetFormFilesAsync(bindingContext); + var postedFiles = await GetFormFilesAsync(modelName, bindingContext); value = postedFiles.FirstOrDefault(); } else if (typeof(IEnumerable).IsAssignableFrom(bindingContext.ModelType)) { - var postedFiles = await GetFormFilesAsync(bindingContext); + var postedFiles = await GetFormFilesAsync(modelName, bindingContext); value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles); } else @@ -67,9 +74,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } else { - bindingContext.ValidationState.Add(value, new ValidationStateEntry() { SuppressValidation = true }); + bindingContext.ValidationState.Add(value, new ValidationStateEntry() + { + Key = modelName, + SuppressValidation = true + }); + bindingContext.ModelState.SetModelValue( - bindingContext.ModelName, + modelName, rawValue: null, attemptedValue: null); @@ -77,7 +89,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } - private async Task> GetFormFilesAsync(ModelBindingContext bindingContext) + private async Task> GetFormFilesAsync(string modelName, ModelBindingContext bindingContext) { var request = bindingContext.OperationBindingContext.HttpContext.Request; var postedFiles = new List(); @@ -85,13 +97,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { var form = await request.ReadFormAsync(); - // If we're at the top level, then use the FieldName (paramter or property name). - // This handles the fact that there will be nothing in the ValueProviders for this parameter - // and so we'll do the right thing even though we 'fell-back' to the empty prefix. - var modelName = bindingContext.IsTopLevelObject - ? bindingContext.FieldName - : bindingContext.ModelName; - foreach (var file in form.Files) { ContentDispositionHeaderValue parsedContentDisposition; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index c9ee25b460..e4780832d7 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -77,11 +77,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// true if the object is valid, otherwise false. public bool Validate(ModelMetadata metadata, string key, object model) { - if (model == null) + if (model == null && key != null) { - if (_modelState.GetValidationState(key) != ModelValidationState.Valid) + var entry = _modelState[key]; + if (entry != null && entry.ValidationState != ModelValidationState.Valid) { - _modelState.MarkFieldValid(key); + entry.ValidationState = ModelValidationState.Valid; } return true; @@ -166,13 +167,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation SuppressValidation(key); return false; } - else if ((entry != null && entry.SuppressValidation)) + else if (entry != null && entry.SuppressValidation) { - SuppressValidation(key); + // Use the key on the entry, because we might not have entries in model state. + SuppressValidation(entry.Key); return true; } - using (StateManager.Recurse(this, key, metadata, model, strategy)) + using (StateManager.Recurse(this, key ?? string.Empty, metadata, model, strategy)) { if (_metadata.IsEnumerableType) { @@ -271,6 +273,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation private void SuppressValidation(string key) { + if (key == null) + { + // If the key is null, that means that we shouldn't expect any entries in ModelState for + // this value, so there's nothing to do. + return; + } + var entries = _modelState.FindKeysWithPrefix(key); foreach (var entry in entries) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs index 2179704e52..757b7cce60 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs @@ -10,7 +10,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Formatters; -using Microsoft.AspNetCore.Routing; using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -73,12 +72,40 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); Assert.Single(entry.Value.Errors); } + [Fact] + public async Task BindModel_NoInputFormatterFound_SetsModelStateError_RespectsBinderModelName() + { + // Arrange + var provider = new TestModelMetadataProvider(); + provider.ForType().BindingDetails(d => d.BindingSource = BindingSource.Body); + + var bindingContext = GetBindingContext(typeof(Person), metadataProvider: provider); + bindingContext.BinderModelName = "custom"; + + var binder = bindingContext.OperationBindingContext.ModelBinder; + + // Act + var binderResult = await binder.BindModelAsync(bindingContext); + + // Assert + + // Returns non-null because it understands the metadata type. + Assert.NotNull(binderResult); + Assert.False(binderResult.IsModelSet); + Assert.Null(binderResult.Model); + + // Key is the bindermodelname because this was a top-level binding. + var entry = Assert.Single(bindingContext.ModelState); + Assert.Equal("custom", entry.Key); + Assert.Single(entry.Value.Errors); + } + [Fact] public async Task BindModel_IsGreedy() { @@ -165,7 +192,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message; @@ -200,7 +227,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message; @@ -267,6 +294,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var bindingContext = new ModelBindingContext { + FieldName = "someField", IsTopLevelObject = true, ModelMetadata = metadataProvider.GetMetadataForType(modelType), ModelName = "someName", diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs index d2d9a4e35d..73afe109b1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs @@ -5,6 +5,7 @@ using System; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Xunit; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test @@ -60,7 +61,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test }, ModelBinder = new CancellationTokenModelBinder(), MetadataProvider = metadataProvider, - } + }, + ValidationState = new ValidationStateDictionary(), }; return bindingContext; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs index 109c761b91..73ba91af60 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs @@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var entry = bindingContext.ValidationState[result.Model]; Assert.True(entry.SuppressValidation); - Assert.Null(entry.Key); + Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); } @@ -59,7 +59,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var entry = bindingContext.ValidationState[result.Model]; Assert.True(entry.SuppressValidation); - Assert.Null(entry.Key); + Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); var files = Assert.IsAssignableFrom>(result.Model); diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs index 1b0f94549c..8d41962de0 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs @@ -44,11 +44,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // ModelState (not set unless inner binder sets it) Assert.True(modelState.IsValid); - var entry = modelState[string.Empty]; - Assert.Null(entry.AttemptedValue); - Assert.Null(entry.RawValue); - Assert.Empty(entry.Errors); - Assert.Equal(ModelValidationState.Valid, entry.ValidationState); + Assert.Empty(modelState); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs index 5c8e262da9..9edfd1e322 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs @@ -100,8 +100,12 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["CompanyName"]); Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Price"]); // Mono issue - https://github.com/aspnet/External/issues/19 - Assert.Equal(PlatformNormalizer.NormalizeContent("The Category field is required."), modelStateErrors["Category"]); - Assert.Equal(PlatformNormalizer.NormalizeContent("The Contact Us field is required."), modelStateErrors["Contact"]); + Assert.Equal( + PlatformNormalizer.NormalizeContent("The Category field is required."), + modelStateErrors["Category"]); + Assert.Equal( + PlatformNormalizer.NormalizeContent("The Contact Us field is required."), + modelStateErrors["Contact"]); Assert.Equal( PlatformNormalizer.NormalizeContent("The Detail2 field is required."), modelStateErrors["ProductDetails.Detail2"]); @@ -264,7 +268,9 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var modelStateErrors = CreateValidationDictionary(modelState); Assert.Equal(2, modelStateErrors.Count); Assert.Equal("The field Price must be between 100 and 200.", modelStateErrors["Price"]); - Assert.Equal("The field Contact must be a string with a maximum length of 10.", modelStateErrors["Contact"]); + Assert.Equal( + "The field Contact must be a string with a maximum length of 10.", + modelStateErrors["Contact"]); } [Fact] @@ -380,9 +386,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(modelBindingResult.Model); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Empty(entry.Key); - Assert.Null(entry.Value.RawValue); + Assert.Empty(modelState); } private class Person4 @@ -486,12 +490,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(0, boundPerson.Address.RequiredNumber); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Equal("CustomParameter.Address", entry.Key); - Assert.NotNull(entry.Value); - Assert.Null(entry.Value.AttemptedValue); - Assert.Same(boundPerson.Address, entry.Value.RawValue); - Assert.Empty(entry.Value.Errors); + Assert.Empty(modelState); } [Fact] @@ -527,16 +526,10 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(boundPerson.Address); Assert.False(modelState.IsValid); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(1, modelState.ErrorCount); - var state = modelState["CustomParameter.Address"]; - Assert.NotNull(state); - Assert.Null(state.AttemptedValue); - Assert.Null(state.RawValue); - Assert.Empty(state.Errors); - - state = modelState["CustomParameter.Address.Number"]; + var state = modelState["CustomParameter.Address.Number"]; Assert.NotNull(state); Assert.Null(state.AttemptedValue); Assert.Null(state.RawValue); @@ -597,9 +590,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(boundPerson); Assert.False(modelState.IsValid); - Assert.Equal(2, modelState.Keys.Count); - var address = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.Address").Value; - Assert.Equal(ModelValidationState.Unvalidated, address.ValidationState); + Assert.Equal(1, modelState.Keys.Count); var street = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.Address.Street").Value; Assert.Equal(ModelValidationState.Invalid, street.ValidationState); @@ -655,8 +646,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.IsType(modelBindingResult.Model); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Equal("CustomParameter.Address", entry.Key); + Assert.Empty(modelState); } private Dictionary CreateValidationDictionary(ModelStateDictionary modelState) diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs index 068b0aaacb..e070aa0b74 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs @@ -10,8 +10,6 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features.Internal; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Controllers; -using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.Extensions.Primitives; using Xunit; @@ -80,17 +78,13 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); - - entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Address").Value; - Assert.Null(entry.AttemptedValue); // ModelState entries for body don't include original text. - Assert.Same(model.Customer.Address, entry.RawValue); } [Fact] @@ -125,17 +119,13 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); var entry = Assert.Single(modelState, e => e.Key == "Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); - - entry = Assert.Single(modelState, e => e.Key == "Customer.Address").Value; - Assert.Null(entry.AttemptedValue); // ModelState entries for body don't include original text. - Assert.Same(model.Customer.Address, entry.RawValue); } [Fact] @@ -169,14 +159,11 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("bill", model.Customer.Name); Assert.Null(model.Customer.Address); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Address").Value; - Assert.Null(entry.AttemptedValue); - Assert.Null(entry.RawValue); - entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; + var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); } @@ -1355,13 +1342,9 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(1, modelState.Count); + Assert.Equal(0, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); - - var entry = Assert.Single(modelState, e => e.Key == "Customer.Address").Value; - Assert.Null(entry.AttemptedValue); - Assert.Same(model.Customer.Address, entry.RawValue); } private class Order10 diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs index 42da4b821b..9ad992e286 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.IO; using System.Text; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -1141,7 +1142,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // Act var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); - Assert.Equal(4, modelState.Count); + Assert.Equal(3, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); @@ -1159,14 +1160,6 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("46", entry.AttemptedValue); Assert.Equal("46", entry.RawValue); Assert.Equal(ModelValidationState.Skipped, entry.ValidationState); - - entry = Assert.Single(modelState, e => e.Key == "OfficeAddress").Value; - Assert.Null(entry.AttemptedValue); - var address = Assert.IsType
(entry.RawValue); - Assert.Equal(47, address.Zip); - - // Address itself is not excluded from validation. - Assert.Equal(ModelValidationState.Valid, entry.ValidationState); } [Fact] @@ -1197,7 +1190,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // We need to add another model state entry which should get marked as skipped so // we can prove that the JObject was skipped. - modelState.SetModelValue("message", "Hello", "Hello"); + modelState.SetModelValue("CustomParameter.message", "Hello", "Hello"); // Act var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); @@ -1209,13 +1202,154 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("Hello", message); Assert.True(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.message"); + Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState); + } + + // Regression test for https://github.com/aspnet/Mvc/issues/3743 + // + // A cancellation token that's bound with the empty prefix will end up suppressing + // the empty prefix. Since the empty prefix is a prefix of everything, this will + // basically result in clearing out all model errors, which is BAD. + // + // The fix is to treat non-user-input as have a key of null, which means that the MSD + // isn't even examined when it comes to supressing validation. + [Fact] + public async Task CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "cancellationToken", + ParameterType = typeof(CancellationToken) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry - we want this to be ignored. + modelState.SetModelValue("message", "Hello", "Hello"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + Assert.IsType(modelBindingResult.Model); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "message"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + // Similar to CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors - binding the body + // with the empty prefix should not cause unrelated modelstate entries to get suppressed. + [Fact] + public async Task FromBody_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors_Valid() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "Parameter1", + BindingInfo = new BindingInfo + { + BindingSource = BindingSource.Body + }, + ParameterType = typeof(Greeting) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext( + request => + { + request.Body = new MemoryStream(Encoding.UTF8.GetBytes("{ message: \"Hello\" }")); + request.ContentType = "application/json"; + }); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry which should not get changed. + modelState.SetModelValue("other.key", "1", "1"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + var message = Assert.IsType(modelBindingResult.Model).Message; + Assert.Equal("Hello", message); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "other.key"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + // Similar to CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors - binding the body + // with the empty prefix should not cause unrelated modelstate entries to get suppressed. + [Fact] + public async Task FromBody_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors_Invalid() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "Parameter1", + BindingInfo = new BindingInfo + { + BindingSource = BindingSource.Body + }, + ParameterType = typeof(Greeting) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext( + request => + { + // This string is too long and will have a validation error. + request.Body = new MemoryStream(Encoding.UTF8.GetBytes("{ message: \"Hello There\" }")); + request.ContentType = "application/json"; + }); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry which should not get changed. + modelState.SetModelValue("other.key", "1", "1"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + var message = Assert.IsType(modelBindingResult.Model).Message; + Assert.Equal("Hello There", message); + + Assert.False(modelState.IsValid); Assert.Equal(2, modelState.Count); - var entry = Assert.Single(modelState, kvp => kvp.Key == string.Empty); - Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState); + var entry = Assert.Single(modelState, kvp => kvp.Key == "Message"); + Assert.Equal(ModelValidationState.Invalid, entry.Value.ValidationState); - entry = Assert.Single(modelState, kvp => kvp.Key == "message"); - Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState); + entry = Assert.Single(modelState, kvp => kvp.Key == "other.key"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + private class Greeting + { + [StringLength(5)] + public string Message { get; set; } } private static void AssertRequiredError(string key, ModelError error)