diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs index d78b1d1e13..0662d57a79 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs @@ -249,6 +249,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding EnforceBindRequiredAndValidate( baseObjectValidator, actionContext, + parameter, metadata, modelBindingContext, modelBindingResult); @@ -276,6 +277,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private void EnforceBindRequiredAndValidate( ObjectModelValidator baseObjectValidator, ActionContext actionContext, + ParameterDescriptor parameter, ModelMetadata metadata, ModelBindingContext modelBindingContext, ModelBindingResult modelBindingResult) @@ -287,7 +289,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var message = metadata.ModelBindingMessageProvider.MissingBindRequiredValueAccessor(modelName); actionContext.ModelState.TryAddModelError(modelName, message); } - else if (modelBindingResult.IsModelSet || metadata.IsRequired) + else if (modelBindingResult.IsModelSet) { // Enforce any other validation rules baseObjectValidator.Validate( @@ -297,6 +299,35 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding modelBindingResult.Model, metadata); } + else if (metadata.IsRequired) + { + // We need to special case the model name for cases where a 'fallback' to empty + // prefix occurred but binding wasn't successful. For these cases there will be no + // entry in validation state to match and determine the correct key. + // + // See https://github.com/aspnet/Mvc/issues/7503 + // + // This is to avoid adding validation errors for an 'empty' prefix when a simple + // type fails to bind. The fix for #7503 uncovered this issue, and was likely the + // original problem being worked around that regressed #7503. + string modelName = modelBindingContext.ModelName; + + if (string.IsNullOrEmpty(modelBindingContext.ModelName) && + (parameter.BindingInfo?.BinderModelName ?? metadata.BinderModelName) == null) + { + // If we get here then this is a fallback case. The model name wasn't explicitly set + // and we ended up with an empty prefix. + modelName = modelBindingContext.FieldName; + } + + // Run validation, we expect this to validate [Required]. + baseObjectValidator.Validate( + actionContext, + modelBindingContext.ValidationState, + modelName, + modelBindingResult.Model, + metadata); + } } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index 9f621f3b9e..499959df27 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -146,13 +146,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation var result = results[i]; var key = ModelNames.CreatePropertyModelName(Key, result.MemberName); - // If this is a top-level parameter/property, the key would be empty. - // So, use the name of the top-level property/property. - if (string.IsNullOrEmpty(key) && Metadata.Name != null) - { - key = Metadata.Name; - } - + // It's OK for key to be the empty string here. This can happen when a top + // level object implements IValidatableObject. ModelState.TryAddModelError(key, result.Message); } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs index 461a3c8bca..5cee8ae5a6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs @@ -381,7 +381,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding ModelMetadata metadata) { // Arrange - var expectedKey = metadata.Name ?? string.Empty; + var expectedKey = string.Empty; var expectedFieldName = metadata.Name ?? nameof(Person); var actionContext = GetControllerContext(); diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index 8b24b6d273..4e349de9b9 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -578,7 +578,10 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests [InlineData("displayNameStringLengthParam", "abc", true)] [InlineData("displayNameStringLengthParam", "abcTooLong", false, "My Display Name")] public async Task ActionParameter_EnforcesDataAnnotationsAttributes( - string paramName, string input, bool isValid, string displayName = null) + string paramName, + string input, + bool isValid, + string displayName = null) { // Arrange var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); @@ -614,11 +617,114 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(isValid, modelState.IsValid); if (!isValid) { - var message = modelState[paramName].Errors.Single().ErrorMessage; + var entry = modelState[paramName]; + Assert.NotNull(entry); + + var message = entry.Errors.Single().ErrorMessage; Assert.Contains(displayName ?? parameter.Name, message); } } + [Fact] + public async Task ActionParameter_CanRunIValidatableObject_EmptyPrefix() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameterInfo = BindingAndValidationController.ValidatableObjectParameterInfo; + var parameter = new ParameterDescriptor() + { + Name = parameterInfo.Name, + ParameterType = parameterInfo.ParameterType, + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = QueryString.Create(nameof(ModelWithIValidatableObject.FirstName), "Billy"); + }); + + var modelState = testContext.ModelState; + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var modelMetadata = modelMetadataProvider + .GetMetadataForParameter(parameterInfo); + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync( + parameter, + testContext, + modelMetadataProvider, + modelMetadata); + + // Assert + Assert.True(modelBindingResult.IsModelSet, "model is set"); + Assert.False(modelState.IsValid, "model is valid"); + + var entry = modelState[string.Empty]; + Assert.NotNull(entry); + var message = entry.Errors.Single().ErrorMessage; + Assert.Equal("Not valid.", message); + + entry = modelState[nameof(ModelWithIValidatableObject.FirstName)]; + Assert.NotNull(entry); + message = entry.Errors.Single().ErrorMessage; + Assert.Equal("FirstName Not valid.", message); + } + + [Fact] + public async Task ActionParameter_CanRunIValidatableObject_WithPrefix() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameterInfo = BindingAndValidationController.ValidatableObjectParameterInfo; + var parameter = new ParameterDescriptor() + { + Name = parameterInfo.Name, + ParameterType = parameterInfo.ParameterType, + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + var key = ModelNames.CreatePropertyModelName(parameter.Name, nameof(ModelWithIValidatableObject.FirstName)); + request.QueryString = QueryString.Create(key, "Billy"); + }); + + var modelState = testContext.ModelState; + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var modelMetadata = modelMetadataProvider + .GetMetadataForParameter(parameterInfo); + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync( + parameter, + testContext, + modelMetadataProvider, + modelMetadata); + + // Assert + Assert.True(modelBindingResult.IsModelSet, "model is set"); + Assert.False(modelState.IsValid, "model is valid"); + + var entry = modelState[parameter.Name]; + Assert.NotNull(entry); + var message = entry.Errors.Single().ErrorMessage; + Assert.Equal("Not valid.", message); + + entry = modelState[ModelNames.CreatePropertyModelName(parameter.Name, nameof(ModelWithIValidatableObject.FirstName))]; + Assert.NotNull(entry); + message = entry.Errors.Single().ErrorMessage; + Assert.Equal("FirstName Not valid.", message); + } + + private class ModelWithIValidatableObject : IValidatableObject + { + public string FirstName { get; set; } + + public IEnumerable Validate(ValidationContext validationContext) + { + yield return new ValidationResult("Not valid."); + yield return new ValidationResult("FirstName Not valid.", new string[] { nameof(FirstName) }); + } + } + private struct PointStruct { public PointStruct(double x, double y) @@ -669,7 +775,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests [BindNever] int bindNeverParam, [BindRequired] int bindRequiredParam, [Required, StringLength(3)] string requiredAndStringLengthParam, - [Display(Name = "My Display Name"), StringLength(3)] string displayNameStringLengthParam) + [Display(Name = "My Display Name"), StringLength(3)] string displayNameStringLengthParam, + ModelWithIValidatableObject validatableObject) { } @@ -682,6 +789,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests public static ParameterInfo BindRequiredParamInfo => MyActionMethodInfo.GetParameters()[1]; + public static ParameterInfo ValidatableObjectParameterInfo => MyActionMethodInfo.GetParameters()[4]; + public static ParameterInfo GetParameterInfo(string parameterName) { return MyActionMethodInfo