From fec6dd29403cee4bee887cefa4c1c997524b1706 Mon Sep 17 00:00:00 2001 From: Daniel Svensson Date: Wed, 1 Nov 2023 21:26:11 +0100 Subject: [PATCH 1/3] Add tests based on special case for #453 --- .../Data/EntityValidationTests.cs | 67 +++++++++++++++++-- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs b/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs index 7fded761c..b32b71958 100644 --- a/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs +++ b/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs @@ -25,7 +25,7 @@ public class EntityValidationTests : UnitTestBase public void Entity_Validation_Property_Fail_ReadOnly_Throws() { MockEntity entity = new MockEntity(); - ExceptionHelper.ExpectInvalidOperationException(delegate() + ExceptionHelper.ExpectInvalidOperationException(delegate () { entity.ReadOnlyProperty = "x"; }, "The 'ReadOnlyProperty' property is read only."); @@ -54,7 +54,7 @@ public void Entity_Validation_Property_ReadOnly_Missing() public void Entity_Validation_Property_Fail_NonPublic_Throws() { MockEntity entity = new MockEntity(); - ExceptionHelper.ExpectArgumentException(delegate() + ExceptionHelper.ExpectArgumentException(delegate () { entity.NonPublicProperty = "x"; }, "Type 'MockEntity' does not contain a public property named 'NonPublicProperty'.\r\nParameter name: propertyName"); @@ -65,7 +65,7 @@ public void Entity_Validation_Property_Fail_NonPublic_Throws() public void Entity_Validation_Property_Fail_Bogus_Throws() { MockEntity entity = new MockEntity(); - ExceptionHelper.ExpectArgumentException(delegate() + ExceptionHelper.ExpectArgumentException(delegate () { entity.BogusProperty = "x"; }, "Type 'MockEntity' does not contain a public property named 'XXX'.\r\nParameter name: propertyName"); @@ -144,7 +144,7 @@ public void Entity_Validation_Object_Fail_Null_Required_Throws() { MockEntity entity = new MockEntity(); ValidationContext context = new ValidationContext(entity, null, null); - ExceptionHelper.ExpectValidationException(delegate() + ExceptionHelper.ExpectValidationException(delegate () { System.ComponentModel.DataAnnotations.Validator.ValidateObject(entity, context); }, "The ReadWriteProperty field is required.", typeof(RequiredAttribute), null); @@ -228,7 +228,7 @@ public void ValidationResult_SameErrorMessageDifferentMembers() [Description("When a ValidationContext is provided to the DomainContext, it should be used for Property Validation")] public void ValidationContextUsedForPropertyValidation() { - Dictionary items = new Dictionary(); + Dictionary items = new Dictionary(); items.Add("TestMethod", "ValidationContextUsedForPropertyValidation"); ValidationContext providedValidationContext = new ValidationContext(this, null, items); @@ -469,6 +469,42 @@ public void ValidationErrors_PropertyErrorsAreReplacedFromValidateProperty() actualChanges.Clear(); } + + [TestMethod] + [Description("Test how object level validation interacts with property level validation")] + public void ValidationErrors_ObjectLevelValidation() + { + MockEntity_ObjectValidation entity = new MockEntity_ObjectValidation(); + INotifyDataErrorInfo notifier = (INotifyDataErrorInfo)entity; + + List actualChanges = new List(); + notifier.ErrorsChanged += (s, e) => actualChanges.Add(e.PropertyName); + + // Setting property directly will not catch any error + entity.OnlyObjectValidation = string.Empty; + + Assert.IsFalse(entity.HasValidationErrors, "No errors should be detected"); + Assert.AreEqual(0, actualChanges.Count, "No errors should be detected"); + + // Force object level validation + ((IEditableObject)entity).BeginEdit(); + ((IEditableObject)entity).EndEdit(); + + Assert.IsTrue(entity.HasValidationErrors, "Erros should be detected"); + Assert.AreEqual(1, entity.ValidationErrors.Count); + + var actualError = entity.ValidationErrors.First(); + Assert.AreEqual("Required", actualError.ErrorMessage); + CollectionAssert.AreEqual(new[] { nameof(MockEntity_ObjectValidation.OnlyObjectValidation) }, actualError.MemberNames.ToList(), "membernames"); + + Assert.AreEqual(1, actualChanges.Count, "1 errors should be detected"); + + // Setting property will remove object level validation + entity.OnlyObjectValidation = null; + Assert.IsFalse(entity.HasValidationErrors, "No errors should be detected"); + Assert.AreEqual(2, actualChanges.Count, "1 errors should be detected"); + } + [TestMethod] [WorkItem(854187)] [Description("Verifies that when an invalid entity's changes are rejected, the errors are cleared when RejectChanges is called")] @@ -943,6 +979,27 @@ public void CallValidateProperty(ValidationContext validationContext, object val } } + public class MockEntity_ObjectValidation : Entity, IValidatableObject + { + private string _onlyObjectValidation; + + public string OnlyObjectValidation + { + get => _onlyObjectValidation; + set + { + this.ValidateProperty(nameof(OnlyObjectValidation), value); + _onlyObjectValidation = value; + } + } + + IEnumerable IValidatableObject.Validate(ValidationContext validationContext) + { + if (string.IsNullOrEmpty(_onlyObjectValidation)) + yield return new ValidationResult("Required", new[] { nameof(OnlyObjectValidation) }); + } + } + public class MockEntity_Validation : MockEntity { public void Overload(int x) From 83f3996cca934608f93aac948096e6046ba2039b Mon Sep 17 00:00:00 2001 From: Daniel Svensson Date: Wed, 1 Nov 2023 21:53:14 +0100 Subject: [PATCH 2/3] Add possible fix for regression --- src/OpenRiaServices.Client/Framework/Entity.cs | 6 ++++++ .../Framework/Internal/MetaType.cs | 10 +++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/OpenRiaServices.Client/Framework/Entity.cs b/src/OpenRiaServices.Client/Framework/Entity.cs index d0da50e23..8757f4f3a 100644 --- a/src/OpenRiaServices.Client/Framework/Entity.cs +++ b/src/OpenRiaServices.Client/Framework/Entity.cs @@ -1309,6 +1309,12 @@ protected void ValidateProperty(string propertyName, object value) validationContext.MemberName = propertyName; this.ValidateProperty(validationContext, value); } + else if (MetaType.RequiresObjectValidation) + { + // Validation error must have been set by object level validation + // Clear it to mimic old behaviour where validate property was always called in these scenarios + this.ValidationResultCollection.ReplaceErrors(propertyName, Array.Empty()); + } } /// diff --git a/src/OpenRiaServices.Client/Framework/Internal/MetaType.cs b/src/OpenRiaServices.Client/Framework/Internal/MetaType.cs index 8100e5c25..31914d516 100644 --- a/src/OpenRiaServices.Client/Framework/Internal/MetaType.cs +++ b/src/OpenRiaServices.Client/Framework/Internal/MetaType.cs @@ -32,6 +32,7 @@ public sealed class MetaType [ThreadStatic] private static Dictionary s_metaTypes; private readonly bool _requiresValidation; + private readonly bool _requiresObjectValidation; private readonly Type[] _childTypes; private readonly Dictionary _metaMembers = new Dictionary(); private readonly ReadOnlyCollection _dataMembers; @@ -121,7 +122,8 @@ select attribute this.Type = type; _validationAttributes = new ReadOnlyCollection(this.Type.GetCustomAttributes(typeof(ValidationAttribute), true).OfType().ToArray()); - _requiresValidation = _requiresValidation || _validationAttributes.Any() || typeof(IValidatableObject).IsAssignableFrom(type); + _requiresObjectValidation = _validationAttributes.Any() || typeof(IValidatableObject).IsAssignableFrom(type); + _requiresValidation = _requiresValidation || _requiresObjectValidation; // for identity purposes, we need to make sure values are always ordered KeyMembers = new ReadOnlyCollection(_metaMembers.Values.Where(m => m.IsKeyMember).OrderBy(m => m.Name).ToArray()); @@ -262,6 +264,12 @@ public IEnumerable AssociationMembers /// public bool RequiresValidation => this._requiresValidation; + /// + /// Gets a value indicating whether the Type requires any Type level + /// validation. + /// + internal bool RequiresObjectValidation => this._requiresObjectValidation; + /// /// Gets a value indicating whether the Type has any members marked with /// CompositionAttribute. From 50024de4bb97271fa76b6904995ffe735deb1495 Mon Sep 17 00:00:00 2001 From: Daniel Svensson Date: Thu, 2 Nov 2023 10:21:28 +0100 Subject: [PATCH 3/3] fix CodeQL comment --- .../Data/EntityValidationTests.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs b/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs index b32b71958..a68b0c4b6 100644 --- a/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs +++ b/src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/EntityValidationTests.cs @@ -475,7 +475,7 @@ public void ValidationErrors_PropertyErrorsAreReplacedFromValidateProperty() public void ValidationErrors_ObjectLevelValidation() { MockEntity_ObjectValidation entity = new MockEntity_ObjectValidation(); - INotifyDataErrorInfo notifier = (INotifyDataErrorInfo)entity; + INotifyDataErrorInfo notifier = entity; List actualChanges = new List(); notifier.ErrorsChanged += (s, e) => actualChanges.Add(e.PropertyName); @@ -487,8 +487,9 @@ public void ValidationErrors_ObjectLevelValidation() Assert.AreEqual(0, actualChanges.Count, "No errors should be detected"); // Force object level validation - ((IEditableObject)entity).BeginEdit(); - ((IEditableObject)entity).EndEdit(); + IEditableObject editableObject = entity; + editableObject.BeginEdit(); + editableObject.EndEdit(); Assert.IsTrue(entity.HasValidationErrors, "Erros should be detected"); Assert.AreEqual(1, entity.ValidationErrors.Count); @@ -501,8 +502,8 @@ public void ValidationErrors_ObjectLevelValidation() // Setting property will remove object level validation entity.OnlyObjectValidation = null; - Assert.IsFalse(entity.HasValidationErrors, "No errors should be detected"); - Assert.AreEqual(2, actualChanges.Count, "1 errors should be detected"); + Assert.IsFalse(entity.HasValidationErrors, "Errors should have been removed"); + Assert.AreEqual(2, actualChanges.Count, "change notification should have happened when clearing error"); } [TestMethod]