diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs index 532368e395..38c2147946 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs @@ -65,10 +65,7 @@ private async Task BindModelCoreAsync(ModelBindingContext bi 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; + var modelBindingKey = bindingContext.IsTopLevelObject ? bindingContext.FieldName : bindingContext.ModelName; var httpContext = bindingContext.OperationBindingContext.HttpContext; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs index 8df6e9b69c..7031d7d4d3 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,9 @@ public Task BindModelAsync(ModelBindingContext bindingContex { if (bindingContext.ModelType == typeof(CancellationToken)) { - var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted; + // Force boxing so we can maintain a reference to this token in validation state. + 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 @@ private async Task RunModelBinders(ModelBindingContext bindi 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..d17cce631f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs @@ -43,15 +43,22 @@ public Task BindModelAsync(ModelBindingContext bindingContex 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.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 @@ private async Task BindModelCoreAsync(ModelBindingContext bi } 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 @@ private async Task BindModelCoreAsync(ModelBindingContext bi } } - 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 @@ private async Task> GetFormFilesAsync(ModelBindingContext bindin { 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..01b41805a9 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -168,11 +168,12 @@ private bool Visit(ModelMetadata metadata, string key, object model) } 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 +272,13 @@ private bool VisitChildren(IValidationStrategy strategy) 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 5d57a147a7..0975cd276d 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 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,9 +72,9 @@ public async Task BindModel_NoInputFormatterFound_SetsModelStateError() Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the fieldName because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); - Assert.Equal(string.Empty, entry.Key); + Assert.Equal("someName", entry.Key); Assert.Single(entry.Value.Errors); } @@ -165,9 +164,9 @@ public async Task CustomFormatterDeserializationException_AddedToModelState() Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the field name because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); - Assert.Equal(string.Empty, entry.Key); + Assert.Equal("someName", entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message; Assert.Equal("Your input is bad!", errorMessage); } @@ -200,9 +199,9 @@ public async Task NullFormatterError_AddedToModelState() Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the field name because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); - Assert.Equal(string.Empty, entry.Key); + Assert.Equal("someName", entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).ErrorMessage; Assert.Equal("Unsupported content type 'text/xyz'.", errorMessage); } @@ -267,6 +266,7 @@ private static ModelBindingContext GetBindingContext( var bindingContext = new ModelBindingContext { + FieldName = "someName", 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.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 @@ private static ModelBindingContext GetBindingContext(Type modelType) }, 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 @@ public async Task FormFileModelBinder_SuppressesValidation() 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 @@ public async Task FormFileModelBinder_ExpectMultipleFiles_BindSuccessful() 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/BodyValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs index 5c8e262da9..6863a08400 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs @@ -97,17 +97,21 @@ public async Task ModelMetaDataTypeAttribute_InvalidPropertiesAndSubPropertiesOn Assert.NotNull(boundPerson); Assert.False(modelState.IsValid); var modelStateErrors = CreateValidationDictionary(modelState); - Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["CompanyName"]); - Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Price"]); + Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["Parameter1.CompanyName"]); + Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Parameter1.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["Parameter1.Category"]); + Assert.Equal( + PlatformNormalizer.NormalizeContent("The Contact Us field is required."), + modelStateErrors["Parameter1.Contact"]); Assert.Equal( PlatformNormalizer.NormalizeContent("The Detail2 field is required."), - modelStateErrors["ProductDetails.Detail2"]); + modelStateErrors["Parameter1.ProductDetails.Detail2"]); Assert.Equal( PlatformNormalizer.NormalizeContent("The Detail3 field is required."), - modelStateErrors["ProductDetails.Detail3"]); + modelStateErrors["Parameter1.ProductDetails.Detail3"]); } [Fact] @@ -147,7 +151,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidComplexTypePropertyOnBaseCla var modelStateErrors = CreateValidationDictionary(modelState); Assert.Equal( PlatformNormalizer.NormalizeContent("The ProductDetails field is required."), - modelStateErrors["ProductDetails"]); + modelStateErrors["Parameter1.ProductDetails"]); } [Fact] @@ -187,7 +191,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidClassAttributeOnBaseClass_Ha Assert.False(modelState.IsValid); var modelStateErrors = CreateValidationDictionary(modelState); Assert.Single(modelStateErrors); - Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors[""]); + Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors["Parameter1"]); } [Fact] @@ -263,8 +267,10 @@ public async Task ModelMetaDataTypeAttribute_InvalidPropertiesOnDerivedClass_Has Assert.False(modelState.IsValid); 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 Price must be between 100 and 200.", modelStateErrors["Parameter1.Price"]); + Assert.Equal( + "The field Contact must be a string with a maximum length of 10.", + modelStateErrors["Parameter1.Contact"]); } [Fact] @@ -304,7 +310,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidClassAttributeOnBaseClassPro Assert.False(modelState.IsValid); var modelStateErrors = CreateValidationDictionary(modelState); Assert.Single(modelStateErrors); - Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors[""]); + Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors["Parameter1"]); } [Fact] @@ -381,7 +387,7 @@ public async Task FromBodyOnActionParameter_EmptyBody_BindsToNullValue() Assert.True(modelState.IsValid); var entry = Assert.Single(modelState); - Assert.Empty(entry.Key); + Assert.Equal("CustomParameter", entry.Key); Assert.Null(entry.Value.RawValue); } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs index 42da4b821b..4bc290849c 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -5,6 +5,7 @@ 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; @@ -1197,7 +1198,7 @@ public async Task FromBody_JToken_ExcludedFromValidation() // 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); @@ -1211,13 +1212,55 @@ public async Task FromBody_JToken_ExcludedFromValidation() Assert.True(modelState.IsValid); Assert.Equal(2, modelState.Count); - var entry = Assert.Single(modelState, kvp => kvp.Key == string.Empty); + var entry = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter"); Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState); - entry = Assert.Single(modelState, kvp => kvp.Key == "message"); + 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); + } + private static void AssertRequiredError(string key, ModelError error) { // Mono issue - https://github.com/aspnet/External/issues/19