From 1631ca1a47823f8c23c16b336cef27105fcc6708 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 9 Dec 2014 15:12:25 -0800 Subject: [PATCH] Fix for #1681 - Model state errors are wrong with attributes like [FromHeader] The issue here is that a model state error is added with the model name 'doubled'. This is on a fairly obscure code path and the code dates back to the original WSR git checkin of webapi. There are no wsr tests that verify this behavior. The cause here is that our 'greedy' model binders (like FromHeaderModelBinder) return 'true' whether or not they successfully found a model, because they don't want other model binders to run. This also has an effect on the validation system. That means that validators will run and attempt to validate the model (which may be null). That's that rare case where we get to this code path. --- .../Validation/ModelValidationNode.cs | 4 +-- .../ModelBindingFromHeaderTest.cs | 5 ++-- .../Validation/ModelValidationNodeTest.cs | 27 +++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs index 74009f8d87..c5a577149e 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs @@ -190,11 +190,9 @@ private void ValidateThis(ModelValidationContext validationContext, ModelValidat // If the Model at the current node is null and there is no parent, we cannot validate, and the // DataAnnotationsModelValidator will throw. So we intercept here to provide a catch-all value-required // validation error - var modelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, - ModelMetadata.GetDisplayName()); if (parentNode == null && ModelMetadata.Model == null) { - modelState.TryAddModelError(modelStateKey, Resources.Validation_ValueNotFound); + modelState.TryAddModelError(ModelStateKey, Resources.Validation_ValueNotFound); return; } diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs index d578524d07..437e72f10f 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs @@ -96,10 +96,9 @@ public async Task FromHeader_BindHeader_ToString_OnParameter_NoValues(string hea Assert.Null(result.HeaderValue); Assert.Null(result.HeaderValues); - - // This is a bug - the model state error key is wrong here. + var error = Assert.Single(result.ModelStateErrors); - Assert.Equal("transactionId.transactionId", error); + Assert.Equal("transactionId", error); } // The action that this test hits will echo back the model-bound values diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs index 4825e972aa..5bb344a2c6 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs @@ -2,6 +2,7 @@ // 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; @@ -245,6 +246,27 @@ public void Validate_SkipsValidationIfSuppressed() Assert.Empty(log); } + [Fact] + public void Validate_ValidatesIfModelIsNull() + { + // Arrange + var modelMetadata = GetModelMetadata(typeof(ValidateAllPropertiesModel)); + var node = new ModelValidationNode(modelMetadata, "theKey"); + + var context = CreateContext(modelMetadata); + + // Act + node.Validate(context); + + // Assert + var modelState = Assert.Single(context.ModelState); + Assert.Equal("theKey", modelState.Key); + Assert.Equal(ModelValidationState.Invalid, modelState.Value.ValidationState); + + var error = Assert.Single(modelState.Value.Errors); + Assert.Equal("A value is required but was not present in the request.", error.ErrorMessage); + } + [Fact] [ReplaceCulture] public void Validate_ValidateAllProperties_AddsValidationErrors() @@ -321,6 +343,11 @@ private static ModelMetadata GetModelMetadata(object o) return new DataAnnotationsModelMetadataProvider().GetMetadataForType(() => o, o.GetType()); } + private static ModelMetadata GetModelMetadata(Type type) + { + return new DataAnnotationsModelMetadataProvider().GetMetadataForType(modelAccessor: null, modelType: type); + } + private static ModelValidationContext CreateContext(ModelMetadata metadata = null) { var providers = new IModelValidatorProvider[]