From ba81a0f254b228c251e4dd1e09ec283a41f706eb Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 18:00:05 -0400 Subject: [PATCH 1/3] test: specs for enumerating each collection once A struct reached by two routes is to be reported once where the validator can tell: a collection is enumerated once, and the properties that framework types declare on an item or root collection are not walked. Specs: a shared array, a list in a property and an item (still validated as an object), an ArraySegment, LinkedList and dictionary item, a root LinkedList, and an object outside an ArraySegment, which is no longer reached. Guards: a class object is reported once, a computed struct copy is still reported twice, and Tuple and KeyValuePair keep their walked properties. Rename StructItemsReportedTwiceTests to StructsReachedByTwoRoutesTests. Co-Authored-By: Claude Opus 5.5 --- .../Extensions/TypeExtensionsTests.cs | 39 +++ .../StructCollectionPropertyTests.cs | 3 +- .../StructItemsReportedTwiceTests.cs | 187 -------------- .../StructsReachedByTwoRoutesTests.cs | 236 ++++++++++++++++++ 4 files changed, 276 insertions(+), 189 deletions(-) delete mode 100644 test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs create mode 100644 test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs diff --git a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs index 57a494f..e2e8483 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs @@ -274,6 +274,45 @@ public sealed class SealedStructBagHolder public ChildBagStruct Bag { get; set; } } + public class OwnedLinkedList : LinkedList + { + public Child Owner { get; set; } + } + + // A collection is enumerated, so the properties that a framework type declares on it are + // not walked: they repeat the items, such as LinkedList.First and Last. The properties that + // your own collection type adds are walked. + [Fact] + public void Framework_properties_of_a_collection_are_not_walked() + { + Assert.Empty(typeof(LinkedList).GetWalkedProperties()); + Assert.Equal( + new[] { nameof(OwnedLinkedList.Owner) }, + typeof(OwnedLinkedList).GetWalkedProperties().ConvertAll(p => p.Name)); + } + + // A framework type that is not a collection keeps its properties walked, so the objects + // in a Tuple or a KeyValuePair are reached. + [Fact] + public void Framework_properties_of_a_type_that_is_not_a_collection_are_walked() + { + Assert.Equal( + new[] { nameof(Tuple.Item1) }, + typeof(Tuple).GetWalkedProperties().ConvertAll(p => p.Name)); + Assert.Equal( + new[] { nameof(KeyValuePair.Value) }, + typeof(KeyValuePair).GetWalkedProperties().ConvertAll(p => p.Name)); + } + + // Check 4 of IsLeafType uses the walked properties. ArraySegment has an Array property + // of type int[], which used to count as walked. It holds only ints, so it is a leaf type. + [Fact] + public void Array_segment_of_values_is_a_leaf_type() + { + Assert.True(typeof(ArraySegment).IsLeafType()); + Assert.False(typeof(ArraySegment).IsLeafType()); + } + #if NET8_0_OR_GREATER public class ImmutableArrayHolder { diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs index ec9ef8a..2c32405 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs @@ -219,8 +219,7 @@ public class SkippedBag // The items of a struct collection are objects of a class, so the reference check applies to // them as to any other object: one that two properties share is validated, and reported, once. - // Only an item that is itself a struct is reported once for each route (see - // StructItemsReportedTwiceTests). + // A struct item has no identity of its own (see StructsReachedByTwoRoutesTests). [Fact] public void Object_shared_by_two_struct_collection_properties_is_reported_once() { diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs deleted file mode 100644 index 4d3c196..0000000 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs +++ /dev/null @@ -1,187 +0,0 @@ -using System; -using System.Collections.Generic; -using System.ComponentModel.DataAnnotations; -using Xunit; - -namespace RecursiveDataAnnotationsValidation.Tests -{ - /// - /// A struct that the validator reaches by two routes is reported once for each route. - /// A class object is reported once. - /// - /// Why. The validator remembers each object it has validated, by reference, so that a shared - /// object or a cycle is not walked again (see ObjectReferenceComparer). A class instance has one - /// identity. A struct has none: it is copied each time it is read from a property, and each - /// time an enumerator returns it. The validator holds it as an object, which boxes the copy into - /// a new object. Two boxed copies of the same struct are never the same reference, so the second - /// one looks new and is validated again. - /// See: https://learn.microsoft.com/dotnet/csharp/programming-guide/types/boxing-and-unboxing - /// - /// Where it happens. - /// - Two properties hold the same array of structs. This is so in 2.3.3 too. - /// - Two properties return the same struct, such as a property and a computed copy of it. - /// Since 3.0 the validator walks a struct property that has something to validate. - /// - An item that is a collection returns its structs by enumeration and also through a public - /// property, such as ArraySegment<T>.Array. Since 3.0 the validator enumerates an item that - /// is a collection and also walks its properties, so such an item reports each struct twice. - /// Before, only the property route found them. A LinkedList<T> item reports each struct four - /// times, because its First node also reaches the list through its List property, and the - /// struct through its Value and ValueRef properties. - /// See: https://learn.microsoft.com/dotnet/api/system.arraysegment-1.array - /// - /// What it is not. The invalid struct is reported, so no model passes that failed before. The - /// result list has an extra entry with a different member name. A caller that shows the whole - /// list shows the same error twice, and a caller that counts the errors counts too many. - /// For a collection type of your own, mark the property that repeats the items with - /// [SkipRecursiveValidation], and each struct is reported once. A framework type, such as - /// ArraySegment, has no such property to mark. - /// - /// These are limitation guards. They pass today. If the validator learns to match a struct by - /// more than reference, they fail on purpose, so the change is deliberate. A fix has to decide - /// which of the paths to keep. The paths here start with the property route, which 2.3.3 reported. - /// - public class StructItemsReportedTwiceTests - { - public struct Line - { - [Required] - public string Text { get; set; } - } - - public class Leaf - { - [Required] - public string Name { get; set; } - } - - public class Holder - { - public T Value { get; set; } - } - - public class TwoArrays - { - public Line[] First { get; set; } - public Line[] Second { get; set; } - } - - public class TwoLists - { - public List First { get; set; } - public List Second { get; set; } - } - - public class LineAndCopy - { - public Line Line { get; set; } - - public Line Copy => Line; - } - - private const string TextRequired = " | The Text field is required."; - - private static (bool Valid, List Errors) Run(object model) - { - var results = new List(); - var valid = new RecursiveDataAnnotationValidator().TryValidateObjectRecursive(model, results); - return (valid, ResultText.Describe(results)); - } - - // Contrast. A class object that two properties hold is validated once. - [Fact] - public void Class_object_reached_by_two_properties_is_reported_once() - { - var shared = new List { new Leaf() }; - - var (valid, errors) = Run(new TwoLists { First = shared, Second = shared }); - - Assert.False(valid); - Assert.Equal(ResultText.Expect("First[0].Name | The Name field is required."), errors); - } - - // Also so in 2.3.3. The array is one object, but the validator enumerates the collection - // that each property holds, and each enumeration boxes a new copy of the struct. - [Fact] - public void Struct_reached_by_two_properties_is_reported_twice() - { - var shared = new[] { new Line() }; - - var (valid, errors) = Run(new TwoArrays { First = shared, Second = shared }); - - Assert.False(valid); - Assert.Equal(ResultText.Expect("First[0].Text" + TextRequired, "Second[0].Text" + TextRequired), errors); - } - - // New in 3.0. 2.3.3 did not walk a struct property, so it reported nothing. Each read of - // a struct property returns a copy, so Line and Copy are two structs to the validator. - [Fact] - public void Struct_returned_by_two_properties_is_reported_twice() - { - var (valid, errors) = Run(new LineAndCopy { Line = new Line() }); - - Assert.False(valid); - Assert.Equal(ResultText.Expect("Line.Text" + TextRequired, "Copy.Text" + TextRequired), errors); - } - - // New in 3.0. 2.3.3 reports only the first path, through the Array property. - [Fact] - public void Struct_in_an_array_segment_item_is_reported_twice() - { - var segment = new ArraySegment(new[] { new Line() }); - - var (valid, errors) = Run(new Holder> { Value = new List { segment } }); - - Assert.False(valid); - Assert.Equal( - ResultText.Expect("Value[0].Array[0].Text" + TextRequired, "Value[0][0].Text" + TextRequired), - errors); - } - - // A struct that is valid adds no entry for either route. - [Fact] - public void Valid_struct_in_an_array_segment_item_is_not_reported() - { - var segment = new ArraySegment(new[] { new Line { Text = "a" } }); - - var (valid, errors) = Run(new Holder> { Value = new List { segment } }); - - Assert.True(valid); - Assert.Empty(errors); - } - - // The same item of class objects is reported once, because the second route finds that the - // objects have been validated. - [Fact] - public void Class_objects_in_an_array_segment_item_are_reported_once() - { - var segment = new ArraySegment(new[] { new Leaf() }); - - var (valid, errors) = Run(new Holder> { Value = new List { segment } }); - - Assert.False(valid); - Assert.Single(errors); - } - -#if NET8_0_OR_GREATER - // LinkedListNode.ValueRef is new in .NET 6, so this shape is not the same on .NET Framework. - // First reaches the list through its List property, and the struct through Value and ValueRef. - // See: https://learn.microsoft.com/dotnet/api/system.collections.generic.linkedlistnode-1.valueref - [Fact] - public void Struct_in_a_linked_list_item_is_reported_four_times() - { - var list = new LinkedList(new[] { new Line() }); - - var (valid, errors) = Run(new Holder>> { Value = new List> { list } }); - - Assert.False(valid); - Assert.Equal( - ResultText.Expect( - "Value[0].First.List[0].Text" + TextRequired, - "Value[0].First.Value.Text" + TextRequired, - "Value[0].First.ValueRef.Text" + TextRequired, - "Value[0][0].Text" + TextRequired), - errors); - } -#endif - } -} diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs new file mode 100644 index 0000000..e8510f9 --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs @@ -0,0 +1,236 @@ +using System; +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using Xunit; + +namespace RecursiveDataAnnotationsValidation.Tests +{ + /// + /// A struct that the validator reaches by two routes. A class object is reported once, at the + /// shortest path. A struct is reported once too, wherever the validator can tell that the two + /// routes lead to the same struct. + /// + /// Why it is hard. The validator remembers each object it has validated, by reference, so that + /// a shared object or a cycle is not walked again (see ObjectReferenceComparer). A class + /// instance has one identity. A struct has none: it is copied each time it is read from a + /// property, and each time an enumerator returns it. The validator holds it as an object, which + /// boxes the copy into a new object. Two boxed copies of the same struct are never the same + /// reference, so the second one looks new. + /// See: https://learn.microsoft.com/dotnet/csharp/programming-guide/types/boxing-and-unboxing + /// + /// How 3.0 avoids most of the routes: + /// - Each collection is enumerated once. The collection itself is an object with an identity, + /// so an array that two properties hold yields its structs once, at the shortest path. A + /// collection that a second route reaches as an item is still validated as an object there, + /// so its own attributes run, but it is not enumerated again. + /// - A collection that is an item or the root object is enumerated, and the properties that a + /// framework type declares on it are not walked, as for a collection that a property holds. + /// Those properties repeat the items: ArraySegment<T>.Array, LinkedList<T>.First, + /// Dictionary<TKey, TValue>.Values. The properties that a collection type of your own adds + /// are still walked. + /// See: https://learn.microsoft.com/dotnet/api/system.arraysegment-1.array + /// + /// What is left. Two properties that each return a struct, such as a property and a computed + /// copy of it, give two separate values. The validator reports each one. Mark the copy with + /// [SkipRecursiveValidation] to report it once. + /// + public class StructsReachedByTwoRoutesTests + { + public struct Line + { + [Required] + public string Text { get; set; } + } + + public class Leaf + { + [Required] + public string Name { get; set; } + } + + public class Holder + { + public T Value { get; set; } + } + + public class TwoArrays + { + public Line[] First { get; set; } + public Line[] Second { get; set; } + } + + public class TwoLists + { + public List First { get; set; } + public List Second { get; set; } + } + + public class LineAndCopy + { + public Line Line { get; set; } + + public Line Copy => Line; + } + + // A collection type of your own, with an attribute on a property it adds. + public class PagedLines : List + { + [Required] + public string Cursor { get; set; } + } + + public class ListInPropertyAndItem + { + public PagedLines Lines { get; set; } + + public List Items { get; set; } + } + + private const string TextRequired = " | The Text field is required."; + + private static (bool Valid, List Errors) Run(object model) + { + var results = new List(); + var valid = new RecursiveDataAnnotationValidator().TryValidateObjectRecursive(model, results); + return (valid, ResultText.Describe(results)); + } + + // Guard. A class object that two properties hold is validated once. + [Fact] + public void Class_object_reached_by_two_properties_is_reported_once() + { + var shared = new List { new Leaf() }; + + var (valid, errors) = Run(new TwoLists { First = shared, Second = shared }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("First[0].Name | The Name field is required."), errors); + } + + // Spec. 2.3.3 reported Second[0].Text too, because it enumerated the array once for + // each property. + [Fact] + public void Struct_in_an_array_that_two_properties_hold_is_reported_once() + { + var shared = new[] { new Line() }; + + var (valid, errors) = Run(new TwoArrays { First = shared, Second = shared }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("First[0].Text" + TextRequired), errors); + } + + // Spec. The list is reached through Lines and as an item of Items. It is enumerated once, + // through the shorter path. As an item it is still validated as an object, so the Cursor + // error is reported there. + [Fact] + public void Collection_in_a_property_and_in_an_item_is_enumerated_once_and_validated() + { + var lines = new PagedLines { new Line() }; + + var (valid, errors) = Run(new ListInPropertyAndItem { Lines = lines, Items = new List { lines } }); + + Assert.False(valid); + Assert.Equal( + ResultText.Expect("Lines[0].Text" + TextRequired, "Items[0].Cursor | The Cursor field is required."), + errors); + } + + // Limitation guard. Each read of a struct property returns a copy, so Line and Copy are + // two structs to the validator. + [Fact] + public void Struct_returned_by_two_properties_is_reported_twice() + { + var (valid, errors) = Run(new LineAndCopy { Line = new Line() }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Line.Text" + TextRequired, "Copy.Text" + TextRequired), errors); + } + + // Spec. Before, the Array property gave a second route: Value[0].Array[0].Text. + [Fact] + public void Struct_in_an_array_segment_item_is_reported_once() + { + var segment = new ArraySegment(new[] { new Line() }); + + var (valid, errors) = Run(new Holder> { Value = new List { segment } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Text" + TextRequired), errors); + } + + // Guard. A struct that is valid adds no entry. + [Fact] + public void Valid_struct_in_an_array_segment_item_is_not_reported() + { + var segment = new ArraySegment(new[] { new Line { Text = "a" } }); + + var (valid, errors) = Run(new Holder> { Value = new List { segment } }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Guard. Class objects were reported once before, too, at the index. + [Fact] + public void Class_objects_in_an_array_segment_item_are_reported_once() + { + var segment = new ArraySegment(new[] { new Leaf() }); + + var (valid, errors) = Run(new Holder> { Value = new List { segment } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name | The Name field is required."), errors); + } + + // Spec. The segment holds only the first element of the array. The second one is not part + // of the model, as when a property holds the segment. Before, Array reached it. + [Fact] + public void Object_outside_an_array_segment_item_is_not_validated() + { + var array = new[] { new Leaf { Name = "in the segment" }, new Leaf() }; + var segment = new ArraySegment(array, 0, 1); + + var (valid, errors) = Run(new Holder> { Value = new List { segment } }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Spec. Before, First gave three more routes: First.List[0], First.Value and, from .NET 6 + // on, First.ValueRef. + // See: https://learn.microsoft.com/dotnet/api/system.collections.generic.linkedlistnode-1.valueref + [Fact] + public void Struct_in_a_linked_list_item_is_reported_once() + { + var list = new LinkedList(new[] { new Line() }); + + var (valid, errors) = Run(new Holder>> { Value = new List> { list } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Text" + TextRequired), errors); + } + + // Spec. The root object is walked like an item. + [Fact] + public void Struct_in_a_root_linked_list_is_reported_once() + { + var (valid, errors) = Run(new LinkedList(new[] { new Line() })); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("[0].Text" + TextRequired), errors); + } + + // Spec. Before, the Values property gave a second route: Value[0].Values[0].Text. + [Fact] + public void Struct_value_in_a_dictionary_item_is_reported_once() + { + var map = new Dictionary { ["a"] = new Line() }; + + var (valid, errors) = Run(new Holder>> { Value = new List> { map } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Value.Text" + TextRequired), errors); + } + } +} From 7ef8dcad21dcbf27f325baeda790331639565273 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 18:41:44 -0400 Subject: [PATCH 2/3] fix: report a struct once when a collection repeats it For a collection that is an item or the root object, the properties that framework types declare are no longer walked, as for a collection that a property holds. They repeat the items: ArraySegment.Array, LinkedList.First, Dictionary.Values. The rule lives in GetWalkedProperties, so IsLeafType agrees: int[] and ArraySegment are now leaf types. The Array.SyncRoot special case is no longer needed. The walk remembers each collection it enumerated. A second route enumerates it again, so an item that a Validate method added is still found, but skips its struct items, which the first route reported. Class items were already dropped by reference. The spec for a list in a property and an item is renamed, because the list is enumerated twice; only its struct is reported once. Co-Authored-By: Claude Opus 5.5 --- .../Extensions/TypeExtensions.cs | 13 +++++++- .../RecursiveDataAnnotationValidator.cs | 33 +++++++++++-------- .../Extensions/TypeExtensionsTests.cs | 5 ++- .../StructsReachedByTwoRoutesTests.cs | 18 +++++----- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index 01b369f..a58531e 100644 --- a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs +++ b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs @@ -152,10 +152,21 @@ public static bool IsWalked(this PropertyInfo property) /// ). A static property is left out: it holds data of the type, not /// of the object, and Validator ignores it too. Type.GetProperties() with no arguments /// would return public static properties as well. + /// For a collection, the properties that a framework type declares are left out too, such as + /// Array.SyncRoot, LinkedList.First or Dictionary.Values. The validator enumerates the + /// collection, and those properties repeat its items, so a struct item would be reported once + /// for each route. A collection that a property holds is only enumerated, so an item or root + /// collection is now walked the same way. The properties that a collection type of your own + /// adds are still walked. /// public static List GetWalkedProperties(this Type type) { - return type.GetProperties(BindingFlags.Public | BindingFlags.Instance).Where(IsWalked).ToList(); + var isCollection = typeof(IEnumerable).IsAssignableFrom(type); + + return type.GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Where(property => property.IsWalked() + && !(isCollection && IsInSystemNamespace(property.DeclaringType))) + .ToList(); } /// diff --git a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs index 147201c..d2f7c82 100644 --- a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs +++ b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs @@ -147,6 +147,13 @@ private sealed class GraphWalk //every object that was validated but not walked, because it Equals an ancestor on its path private readonly HashSet stoppedObjects = new HashSet(ObjectReferenceComparer.Instance); + //every collection that was enumerated, compared by reference. A collection that a second + //route reaches is enumerated again, so an item that was added in the meantime, such as by a + //Validate method, is found. Its struct items are skipped the second time: a struct has no + //identity of its own, so queuedObjects cannot tell that the copy is the same struct, and it + //would be reported once for each route. Its class items are dropped by queuedObjects. + private readonly HashSet enumeratedCollections = new HashSet(ObjectReferenceComparer.Instance); + private readonly ICollection validationResults; private readonly IServiceProvider serviceProvider; private readonly IDictionary validationContextItems; @@ -229,7 +236,8 @@ EqualsAncestor ancestors } //Queues each item of a collection. The path of an item is the path of the collection - //plus its index, and its depth is itemDepth. + //plus its index, and its depth is itemDepth. A collection that was enumerated before skips + //its struct items (see enumeratedCollections). private void EnqueueItems( IEnumerable items, PathStep collectionPath, @@ -237,6 +245,8 @@ private void EnqueueItems( EqualsAncestor ancestors ) { + var enumeratedBefore = !enumeratedCollections.Add(items); + var arrayIndex = -1; foreach (var item in items) { @@ -245,6 +255,7 @@ EqualsAncestor ancestors //NOTE: Possibly should have a separate case for Dictionary which reports on the key if (item == null) continue; + if (enumeratedBefore && item.GetType().IsValueType) continue; Enqueue(item, collectionPath, null, arrayIndex, itemDepth, ancestors); } } @@ -291,24 +302,22 @@ private bool Walk(WorkItem item) //A collection that gets here is the root object or an item of another collection. It is //validated as an object above, so its own attributes run. Then its items are queued, - //before its properties. An object that the collection also returns from a property, - //such as Array.SyncRoot, is then reported at its index (Value[0][0], or [0] for the root) - //and not through the property (Value[0].SyncRoot[0]). A collection that a property - //holds never gets here: it waits in the queue as a step of its own (see below). + //before its properties. A collection that a property holds never gets here: it waits in + //the queue as a step of its own (see below). //A collection of leaf types is skipped, like a collection that a property holds. A default //struct, such as an ImmutableArray nobody set, is skipped: it holds nothing and enumerating it throws. - var enumeratedItems = false; if (obj is IEnumerable items && !type.IsCollectionOfLeafType() && !obj.IsDefaultStruct()) { - enumeratedItems = true; EnqueueItems(items, item.Path, item.Depth + 1, ancestors); } - //IsWalked leaves out properties declared by framework types that throw or never end when - //read, such as Uri.Segments on a relative Uri, DirectoryInfo.Root, or the properties of - //a Thread or Process read from the wrong thread or process (see IsUnsafeToWalk) + //GetWalkedProperties leaves out properties declared by framework types that throw or never + //end when read, such as Uri.Segments on a relative Uri, DirectoryInfo.Root, or the properties + //of a Thread or Process read from the wrong thread or process (see IsUnsafeToWalk). For a + //collection, it also leaves out the properties that framework types declare, such as + //Array.SyncRoot or LinkedList.First, because they repeat the items that were just queued. var properties = type.GetWalkedProperties(); foreach (var property in properties) @@ -332,10 +341,6 @@ private bool Walk(WorkItem item) continue; case IEnumerable asEnumerable: - //an item that was enumerated above can return itself from a property, such as - //Array.SyncRoot, and enumerating it a second time would find nothing new - if (enumeratedItems && ReferenceEquals(value, obj)) continue; - //the property is one level and the index of each item is another queue.Enqueue(new WorkItem( asEnumerable, diff --git a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs index e2e8483..1a975f1 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs @@ -123,6 +123,10 @@ public class MixedSequence : IEnumerable, IEnumerable [Theory] [InlineData(typeof(byte[]))] [InlineData(typeof(int[,]))] + // An int[] is a sealed collection of ints. Its only property of a reference type, + // Array.SyncRoot, is declared by a framework type, so it is not walked, and int[] is a + // leaf type. A jagged array of ints holds nothing to validate. + [InlineData(typeof(int[][]))] [InlineData(typeof(int?[]))] [InlineData(typeof(List))] [InlineData(typeof(HashSet))] @@ -156,7 +160,6 @@ public void Collection_of_leaf_type_is_detected(Type type) [Theory] [InlineData(typeof(Child[]))] [InlineData(typeof(object[]))] - [InlineData(typeof(int[][]))] [InlineData(typeof(List))] [InlineData(typeof(Dictionary))] [InlineData(typeof(Dictionary))] diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs index e8510f9..35bf09a 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructsReachedByTwoRoutesTests.cs @@ -19,10 +19,12 @@ namespace RecursiveDataAnnotationsValidation.Tests /// See: https://learn.microsoft.com/dotnet/csharp/programming-guide/types/boxing-and-unboxing /// /// How 3.0 avoids most of the routes: - /// - Each collection is enumerated once. The collection itself is an object with an identity, - /// so an array that two properties hold yields its structs once, at the shortest path. A - /// collection that a second route reaches as an item is still validated as an object there, - /// so its own attributes run, but it is not enumerated again. + /// - The collection itself is an object with an identity. The validator remembers each + /// collection it enumerated. A second route enumerates it again, so an item that was added in + /// the meantime is found, but skips its struct items, which the first route reported. So an + /// array that two properties hold yields its structs once, at the shortest path. A collection + /// that a second route reaches as an item is still validated as an object there, so its own + /// attributes run. /// - A collection that is an item or the root object is enumerated, and the properties that a /// framework type declares on it are not walked, as for a collection that a property holds. /// Those properties repeat the items: ArraySegment<T>.Array, LinkedList<T>.First, @@ -120,11 +122,11 @@ public void Struct_in_an_array_that_two_properties_hold_is_reported_once() Assert.Equal(ResultText.Expect("First[0].Text" + TextRequired), errors); } - // Spec. The list is reached through Lines and as an item of Items. It is enumerated once, - // through the shorter path. As an item it is still validated as an object, so the Cursor - // error is reported there. + // Spec. The list is reached through Lines and as an item of Items. Its struct is reported + // through the shorter path. As an item the list is still validated as an object, so the + // Cursor error is reported there. [Fact] - public void Collection_in_a_property_and_in_an_item_is_enumerated_once_and_validated() + public void Collection_in_a_property_and_in_an_item_reports_its_struct_once() { var lines = new PagedLines { new Line() }; From b1919241247953b47c5961031657d701699ec97a Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 18:42:12 -0400 Subject: [PATCH 3/3] docs: describe how a struct reached twice is reported README: a struct in a shared collection is reported once, and the properties that framework types declare on a collection item are not walked. Only a computed copy of a struct is still reported twice. Changelog: a BREAKING entry for the removed routes and the objects that only those routes reached, and a narrower struct known limit. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++- README.md | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06bc642..62e7d05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,8 @@ Breaking changes are marked **BREAKING**. - A collection that a property holds is enumerated later than before: after the objects at its own level and above are validated, not when the property is read. A lazy sequence runs later. A change that a `Validate` method makes to the collection is now seen. An item whose `Validate` adds to its own list no longer throws `Collection was modified`. When a collection throws, the exception is the same, but the result list holds different partial results. - The NuGet package title now reads "Recursive DataAnnotations Validation". It was misspelled "Recurisive". - The NuGet package now has a description. It showed "Package Description" before. -- Known limit: an invalid struct that an item collection returns by enumeration and also through one of its properties is reported twice, with two member names, for example `Value[0][0].Text` and `Value[0].Array[0].Text` for an `ArraySegment`. The validator cannot match two copies of a struct by reference. A struct that two properties hold was already reported twice. Two properties that return the same struct, such as a property and a computed copy of it, now report it twice too. Objects of a class are reported once. +- **BREAKING** A struct that the validator reaches by two routes is reported once where the validator can tell. For a collection that is an item or the root object, the properties that framework types declare, such as `ArraySegment.Array`, `LinkedList.First` and `Dictionary.Values`, are no longer walked, as for a collection that a property holds. A collection that a second route reaches is enumerated again, so an item added in the meantime is found, but its struct items are not reported again. Change any code that counts the results: `Value[0].Array[0].Text`, `Value[0].First.Value.Text` and `Value[0].Values[0].Text` are gone, and so is `Second[0].Text` for an array that `First` also holds. An object that only such a property reached is no longer validated, such as an item of the array outside an `ArraySegment`, or the comparer of a dictionary. A jagged array of simple values, such as `int[][]`, is no longer enumerated, because it cannot hold an invalid object. +- Known limit: two properties that each return the same struct, such as a property and a computed copy of it, report it twice, with two member names. The validator cannot match two copies of a struct by reference. Objects of a class are reported once. - README: say what `Validator.TryValidateObject` leaves out, show the usage as C#, and tell the `Tuple` classes, which are walked, from value tuples, which are not. ### Fixed diff --git a/README.md b/README.md index b1bca82..1fce1c6 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot ### Shared objects, cycles and computed properties - Each object is validated once, even when several properties point to it. This also stops cycles, such as a child that points back to its parent. The error names the shortest path to the object (see "Order of results" below). -- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value, so it reports an invalid struct once for each route that reaches it. For example, two properties that hold the same array of structs report each struct twice, so do two properties that return the same struct, such as a property and a computed copy of it, and so does a `List` that holds an `ArraySegment` of structs, because the segment returns its items by enumeration and through its `Array` property. The error is not lost, but the result list has a duplicate with a different member name. For a collection type of your own, mark the property that repeats the items with `[SkipRecursiveValidation]`. +- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value. It does remember each collection, so a struct in an array that two properties hold is reported once, at the shortest path. Two properties that each return the struct, such as a property and a computed copy of it, give two separate values, and the validator reports each one. The error is not lost, but the result list has a duplicate with a different member name. Mark the copy with `[SkipRecursiveValidation]`. - Objects are compared by reference. Two separate objects that are `Equals` to each other, such as records with the same values or entities with the same `Id`, are each validated. - Only public instance properties are walked. A static property holds data of the type, not of your object, so it is not walked, as `Validator` ignores it too. - A property that builds a new object on each read, such as `public Money Zero => new Money(0)`, could make the walk go on forever. So when an object's type overrides `Equals`, and the object equals an object of the same type, a base type or a derived type on its own path from the root, the validator checks that object's own attributes but does not walk into its properties from there. The check uses the shortest path to the object. A longer path that does not pass an equal object walks into it. An invalid object that every path reaches through an equal ancestor is not reached. This can happen when a shared object, such as a customer, is reached first through an equal ancestor. @@ -60,7 +60,7 @@ An object that several paths reach is validated once, and its errors name the sh - The items of a collection that a property holds are validated. The error names the property and the index of the item: `Items[1].Name`. - An item that is itself a collection is validated too, at each level: `Matrix[0][2].Name`. This holds for lists, arrays, sets, dictionaries and your own collection types. A dictionary is enumerated as `KeyValuePair` items, so a value that is an object is reported as `Map[0].Value.Name`. A value that is a struct is walked when it has something to validate, as it is when a property holds it (see "Structs" below). -- An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. +- An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. The properties that a framework type declares on it, such as `ArraySegment.Array`, `LinkedList.First` or `Dictionary.Values`, are not walked, because they repeat the items. A collection that a property holds is treated the same way. The properties that your own collection type adds are walked. - A collection of simple values, such as `List` or `string[]`, is not enumerated, because it cannot hold an invalid object. - A collection that you pass to the validator as the root object is validated as an object, then its items are validated. The error starts with the index of the item: `[1].Name`. - A collection that is a struct, such as `ImmutableArray`, is validated when a property holds it and when it is an item of another collection: `Lines[0].Sku`.