Skip to content

Commit

Permalink
Give ValidationContext.MemberName a distinct value from DisplayName
Browse files Browse the repository at this point in the history 
- #120
- revert to previous behavior if `webapi:UseLegacyValidationMemberName` application setting is `true`
  • Loading branch information
@dougbu
dougbu committed Feb 2, 2018
1 parent 2b669ca commit 9829dd8
Show file tree
Hide file tree
Showing 3 changed files with 274 additions and 13 deletions.
1 change: 1 addition & 0 deletions src/System.Web.Http/System.Web.Http.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
</Reference>
<Reference Include="System" />
<Reference Include="System.ComponentModel.DataAnnotations" />
<Reference Include="System.Configuration" />
<Reference Include="System.Core" />
<Reference Include="System.Data.Linq" />
<Reference Include="System.Net.Http" />
Expand Down
71 changes: 59 additions & 12 deletions src/System.Web.Http/Validation/Validators/DataAnnotationsModelValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,23 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel.DataAnnotations;
using System.Configuration;
using System.Linq;
using System.Web.Http.Metadata;

namespace System.Web.Http.Validation.Validators
{
public class DataAnnotationsModelValidator : ModelValidator
{
public DataAnnotationsModelValidator(IEnumerable<ModelValidatorProvider> validatorProviders, ValidationAttribute attribute)
internal static readonly string UseLegacyValidationMemberNameKey = "webapi:UseLegacyValidationMemberName";
private static bool _useLegacyValidationMemberName =
GetUseLegacyValidationMemberName(ConfigurationManager.AppSettings);

public DataAnnotationsModelValidator(
IEnumerable<ModelValidatorProvider> validatorProviders,
ValidationAttribute attribute)
: base(validatorProviders)
{
if (attribute == null)
Expand All @@ -21,6 +29,13 @@ public DataAnnotationsModelValidator(IEnumerable<ModelValidatorProvider> validat
Attribute = attribute;
}

// Internal for testing
internal static bool UseLegacyValidationMemberName
{
get { return _useLegacyValidationMemberName; }
set { _useLegacyValidationMemberName = value; }
}

protected internal ValidationAttribute Attribute { get; private set; }

public override bool IsRequired
Expand All @@ -30,25 +45,39 @@ public override bool IsRequired

public override IEnumerable<ModelValidationResult> Validate(ModelMetadata metadata, object container)
{
string memberName;
if (_useLegacyValidationMemberName)
{
// Using member name from a Display or DisplayFormat attribute is generally incorrect. This
// (configuration-controlled) override is provided only for corner cases where strict
// back-compatibility is required.
memberName = metadata.GetDisplayName();
}
else
{
// MemberName expression matches GetDisplayName() except it ignores Display and DisplayName
// attributes. ModelType fallback isn't great and can separate errors and attempted values in the
// ModelState. But, expression matches MVC, avoids a null MemberName, and is mostly back-compatible.
memberName = metadata.PropertyName ?? metadata.ModelType.Name;
}

// Per the WCF RIA Services team, instance can never be null (if you have
// no parent, you pass yourself for the "instance" parameter).

string memberName = metadata.GetDisplayName();
ValidationContext context = new ValidationContext(container ?? metadata.Model)
ValidationContext context = new ValidationContext(instance: container ?? metadata.Model)
{
DisplayName = memberName,
MemberName = memberName
DisplayName = metadata.GetDisplayName(),
MemberName = memberName,
};

ValidationResult result = Attribute.GetValidationResult(metadata.Model, context);

if (result != ValidationResult.Success)
{
// ModelValidationResult.MemberName is used by invoking validators (such as ModelValidationNode) to
// construct the ModelKey for ModelStateDictionary. When validating at type level we want to append the
// returned MemberNames if specified (e.g. person.Address.FirstName). For property validation, the
// ModelKey can be constructed using the ModelMetadata and we should ignore MemberName (we don't want
// (person.Name.Name). However the invoking validator does not have a way to distinguish between these two
// ModelValidationResult.MemberName is used by invoking validators (such as ModelValidationNode) to
// construct the ModelKey for ModelStateDictionary. When validating at type level we want to append the
// returned MemberNames if specified (e.g. person.Address.FirstName). For property validation, the
// ModelKey can be constructed using the ModelMetadata and we should ignore MemberName (we don't want
// (person.Name.Name). However the invoking validator does not have a way to distinguish between these two
// cases. Consequently we'll only set MemberName if this validation returns a MemberName that is different
// from the property being validated.

Expand All @@ -63,11 +92,29 @@ public override IEnumerable<ModelValidationResult> Validate(ModelMetadata metada
Message = result.ErrorMessage,
MemberName = errorMemberName
};

return new ModelValidationResult[] { validationResult };
}

return Enumerable.Empty<ModelValidationResult>();
}

// Internal for testing
internal static bool GetUseLegacyValidationMemberName(NameValueCollection appSettings)
{
var useLegacyMemberNameArray = appSettings.GetValues(UseLegacyValidationMemberNameKey);
if (useLegacyMemberNameArray != null &&
useLegacyMemberNameArray.Length > 0)
{
bool useLegacyMemberName;
if (bool.TryParse(useLegacyMemberNameArray[0], out useLegacyMemberName) &&
useLegacyMemberName)
{
return true;
}
}

return false;
}
}
}
215 changes: 214 additions & 1 deletion test/System.Web.Http.Test/Validation/Validators/DataAnnotationsModelValidatorTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Web.Http.Metadata;
using System.Web.Http.Metadata.Providers;
using System.Web.WebPages.TestUtils;
using Microsoft.TestCommon;
using Moq;
using Moq.Protected;
Expand Down Expand Up @@ -47,6 +49,90 @@ public void ValuesSet()
Assert.Same(attribute, validator.Attribute);
}

public static TheoryDataSet<NameValueCollection> FalseAppSettingsData
{
get
{
return new TheoryDataSet<NameValueCollection>
{
new NameValueCollection(),
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "false" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "False" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "false" },
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "true" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "garbage" },
},
};
}
}

[Theory]
[PropertyData("FalseAppSettingsData")]
public void GetUseLegacyValidationMemberName_ReturnsFalse(NameValueCollection appSettings)
{
// Arrange & Act
var result = DataAnnotationsModelValidator.GetUseLegacyValidationMemberName(appSettings);

// Assert
Assert.False(result);
}

public static TheoryDataSet<NameValueCollection> TrueAppSettingsData
{
get
{
return new TheoryDataSet<NameValueCollection>
{
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "true" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "True" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "true" },
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "false" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "true" },
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "false" },
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "garbage" },
},
new NameValueCollection
{
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "True" },
{ DataAnnotationsModelValidator.UseLegacyValidationMemberNameKey, "garbage" },
},
};
}
}

[Theory]
[PropertyData("TrueAppSettingsData")]
public void GetUseLegacyValidationMemberName_ReturnsTrue(NameValueCollection appSettings)
{
// Arrange & Act
var result = DataAnnotationsModelValidator.GetUseLegacyValidationMemberName(appSettings);

// Assert
Assert.True(result);
}

public static TheoryDataSet<ModelMetadata, string> ValidateSetsMemberNamePropertyDataSet
{
get
Expand All @@ -57,10 +143,14 @@ public static TheoryDataSet<ModelMetadata, string> ValidateSetsMemberNamePropert
_metadataProvider.GetMetadataForProperty(() => 15, typeof(string), "Length"),
"Length"
},
{
_metadataProvider.GetMetadataForProperty(() => string.Empty, typeof(AnnotatedModel), "Name"),
"Name"
},
{
_metadataProvider.GetMetadataForType(() => new object(), typeof(SampleModel)),
"SampleModel"
}
},
};
}
}
Expand Down Expand Up @@ -89,6 +179,123 @@ public void ValidateSetsMemberNamePropertyOfValidationContextForProperties(Model
attribute.VerifyAll();
}

[Fact]
public void ValidateSetsMemberNameProperty_UsingDisplayName()
{
AppDomainUtils.RunInSeparateAppDomain(ValidateSetsMemberNameProperty_UsingDisplayName_Inner);
}

private static void ValidateSetsMemberNameProperty_UsingDisplayName_Inner()
{
// Arrange
DataAnnotationsModelValidator.UseLegacyValidationMemberName = true;
var expectedMemberName = "Annotated Name";
var attribute = new Mock<ValidationAttribute> { CallBase = true };
attribute
.Protected()
.Setup<ValidationResult>("IsValid", ItExpr.IsAny<object>(), ItExpr.IsAny<ValidationContext>())
.Callback((object o, ValidationContext context) =>
{
Assert.Equal(expectedMemberName, context.MemberName);
})
.Returns(ValidationResult.Success)
.Verifiable();
var validator = new DataAnnotationsModelValidator(_noValidatorProviders, attribute.Object);
var metadata = _metadataProvider.GetMetadataForProperty(() => string.Empty, typeof(AnnotatedModel), "Name");

// Act
var results = validator.Validate(metadata, container: null);

// Assert
Assert.Empty(results);
attribute.VerifyAll();
}

// Confirm explicit false setting does not change Validate(...)'s behavior from its default.
[Fact]
public void ValidateSetsMemberNameProperty_NotUsingDisplayName()
{
AppDomainUtils.RunInSeparateAppDomain(ValidateSetsMemberNameProperty_NotUsingDisplayName_Inner);
}

private static void ValidateSetsMemberNameProperty_NotUsingDisplayName_Inner()
{
// Arrange
DataAnnotationsModelValidator.UseLegacyValidationMemberName = false;
var expectedMemberName = "Name";
var attribute = new Mock<ValidationAttribute> { CallBase = true };
attribute
.Protected()
.Setup<ValidationResult>("IsValid", ItExpr.IsAny<object>(), ItExpr.IsAny<ValidationContext>())
.Callback((object o, ValidationContext context) =>
{
Assert.Equal(expectedMemberName, context.MemberName);
})
.Returns(ValidationResult.Success)
.Verifiable();
var validator = new DataAnnotationsModelValidator(_noValidatorProviders, attribute.Object);
var metadata = _metadataProvider.GetMetadataForProperty(() => string.Empty, typeof(AnnotatedModel), "Name");

// Act
var results = validator.Validate(metadata, container: null);

// Assert
Assert.Empty(results);
attribute.VerifyAll();
}

public static TheoryDataSet<ModelMetadata, string> ValidateSetsDisplayNamePropertyDataSet
{
get
{
return new TheoryDataSet<ModelMetadata, string>
{
{
_metadataProvider.GetMetadataForProperty(() => 15, typeof(string), "Length"),
"Length"
},
{
_metadataProvider.GetMetadataForProperty(() => string.Empty, typeof(AnnotatedModel), "Name"),
"Annotated Name"
},
{
_metadataProvider.GetMetadataForType(() => new object(), typeof(SampleModel)),
"SampleModel"
},
};
}
}

[Theory]
[PropertyData("ValidateSetsDisplayNamePropertyDataSet")]
public void ValidateSetsDisplayNamePropertyOfValidationContextAsExpected(
ModelMetadata metadata,
string expectedDisplayName)
{
// Arrange
var attribute = new Mock<ValidationAttribute>
{
CallBase = true,
};
attribute
.Protected()
.Setup<ValidationResult>("IsValid", ItExpr.IsAny<object>(), ItExpr.IsAny<ValidationContext>())
.Callback(
(object o, ValidationContext context) => Assert.Equal(expectedDisplayName, context.DisplayName))
.Returns(ValidationResult.Success)
.Verifiable();
DataAnnotationsModelValidator validator = new DataAnnotationsModelValidator(
_noValidatorProviders,
attribute.Object);

// Act
IEnumerable<ModelValidationResult> results = validator.Validate(metadata, container: null);

// Assert
Assert.Empty(results);
attribute.VerifyAll();
}

[Fact]
public void ValidateWithIsValidTrue()
{
Expand Down Expand Up @@ -222,5 +429,11 @@ class SampleModel
{
public string Name { get; set; }
}

private class AnnotatedModel
{
[Display(Name = "Annotated Name")]
public string Name { get; set; }
}
}
}

0 comments on commit 9829dd8

Please sign in to comment.