From e1abb47b98d502f4d6c78700f7dffa48ef346f5d Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 29 Mar 2016 08:00:48 -0700 Subject: [PATCH] Fix #4238 IFormFile model binder suppresses validation This change no longer suppresses validation for IFormFile and IFormFileCollection model values. This will allow the use of [Required] on an IFormFile model, or a custom attribute for validating IFormFileCollection. These types already have ValidateChildren = false, so we don't recurse into them. --- .../ModelBinding/Binders/FormCollectionModelBinder.cs | 3 +-- .../ModelBinding/Binders/FormFileModelBinder.cs | 3 ++- .../ModelBinding/Binders/FormCollectionModelBinderTest.cs | 5 +---- .../ModelBinding/Binders/FormFileModelBinderTest.cs | 6 +++--- .../FormFileModelBindingIntegrationTest.cs | 8 ++++---- .../TryUpdateModelIntegrationTest.cs | 2 +- 6 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormCollectionModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormCollectionModelBinder.cs index 94e4f34359..e3c10f2e67 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormCollectionModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormCollectionModelBinder.cs @@ -36,8 +36,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { model = new EmptyFormCollection(); } - - bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true }); + bindingContext.Result = ModelBindingResult.Success(bindingContext.ModelName, model); } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormFileModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormFileModelBinder.cs index a2572f581b..a9eb164714 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormFileModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/FormFileModelBinder.cs @@ -98,10 +98,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders } } + // We need to add a ValidationState entry because the modelName might be non-standard. Otherwise + // the entry we create in model state might not be marked as valid. bindingContext.ValidationState.Add(value, new ValidationStateEntry() { Key = modelName, - SuppressValidation = true }); bindingContext.ModelState.SetModelValue( diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormCollectionModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormCollectionModelBinderTest.cs index c1833789c4..a9b3b90061 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormCollectionModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormCollectionModelBinderTest.cs @@ -36,10 +36,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.NotEqual(default(ModelBindingResult), result); Assert.True(result.IsModelSet); - var entry = bindingContext.ValidationState[result.Model]; - Assert.True(entry.SuppressValidation); - Assert.Null(entry.Key); - Assert.Null(entry.Metadata); + Assert.Empty(bindingContext.ValidationState); var form = Assert.IsAssignableFrom(result.Model); Assert.Equal(2, form.Count); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormFileModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormFileModelBinderTest.cs index cd99808e0e..d874396e17 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormFileModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/FormFileModelBinderTest.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public class FormFileModelBinderTest { [Fact] - public async Task FormFileModelBinder_SuppressesValidation() + public async Task FormFileModelBinder_SingleFile_BindSuccessful() { // Arrange var formFiles = new FormFileCollection(); @@ -33,7 +33,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.True(result.IsModelSet); var entry = bindingContext.ValidationState[result.Model]; - Assert.True(entry.SuppressValidation); + Assert.False(entry.SuppressValidation); Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); } @@ -55,7 +55,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.True(result.IsModelSet); var entry = bindingContext.ValidationState[result.Model]; - Assert.True(entry.SuppressValidation); + Assert.False(entry.SuppressValidation); Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs index 3186996089..f91cf5aeae 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs @@ -74,7 +74,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var key = Assert.Single(modelState.Keys, k => k == "Address.File"); Assert.Null(modelState[key].RawValue); Assert.Empty(modelState[key].Errors); - Assert.Equal(ModelValidationState.Skipped, modelState[key].ValidationState); + Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } private class ListContainer1 @@ -125,7 +125,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var modelStateEntry = kvp.Value; Assert.NotNull(modelStateEntry); Assert.Empty(modelStateEntry.Errors); - Assert.Equal(ModelValidationState.Skipped, modelStateEntry.ValidationState); + Assert.Equal(ModelValidationState.Valid, modelStateEntry.ValidationState); Assert.Null(modelStateEntry.AttemptedValue); Assert.Null(modelStateEntry.RawValue); } @@ -216,7 +216,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var modelStateEntry = kvp.Value; Assert.NotNull(modelStateEntry); Assert.Empty(modelStateEntry.Errors); - Assert.Equal(ModelValidationState.Skipped, modelStateEntry.ValidationState); + Assert.Equal(ModelValidationState.Valid, modelStateEntry.ValidationState); Assert.Null(modelStateEntry.AttemptedValue); Assert.Null(modelStateEntry.RawValue); } @@ -265,7 +265,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var entry = Assert.Single(modelState); Assert.Equal("CustomParameter", entry.Key); Assert.Empty(entry.Value.Errors); - Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState); + Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState); Assert.Null(entry.Value.AttemptedValue); Assert.Null(entry.Value.RawValue); } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index 02602539bc..2294081190 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs @@ -1000,7 +1000,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var modelStateEntry = kvp.Value; Assert.NotNull(modelStateEntry); Assert.Empty(modelStateEntry.Errors); - Assert.Equal(ModelValidationState.Skipped, modelStateEntry.ValidationState); + Assert.Equal(ModelValidationState.Valid, modelStateEntry.ValidationState); Assert.Null(modelStateEntry.AttemptedValue); Assert.Null(modelStateEntry.RawValue); }