From 3761c700271984c432a75586fd4b44960eab4e0a Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Thu, 1 Oct 2026 23:48:56 -0400 Subject: [PATCH 01/12] test: add specs for items that are collections An item that is itself a collection is validated as an object but never enumerated, so a List of List, a List of HashSet, or an object array that holds a List passes with invalid objects inside. Arrays and ArrayList are caught by accident through SyncRoot. The new specs state the fixed behavior and fail until the fix lands: index at each level (Value[0][0].Name), null items, repeated and cyclic collections, sealed and struct collections, and an item that has its own attributes. Guards pin what must keep working: attributes and IValidatableObject on a collection item, leaf collections not being enumerated, and a collection as the root object. Open cases are skipped with their reasoning in NestedCollectionEdgeCaseTests. Co-Authored-By: Claude Sonnet 5.5 --- .../NestedCollectionEdgeCaseTests.cs | 166 ++++++ .../OddShapeTests.cs | 479 +++++++++++++++++- 2 files changed, 629 insertions(+), 16 deletions(-) create mode 100644 test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs diff --git a/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs new file mode 100644 index 0000000..a6ff05a --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs @@ -0,0 +1,166 @@ +using System; +using System.Collections; +using System.Collections.Generic; +#if NET8_0_OR_GREATER +using System.Collections.Immutable; +#endif +using System.ComponentModel.DataAnnotations; +using Xunit; + +namespace RecursiveDataAnnotationsValidation.Tests +{ + /// + /// Open cases for the fix that enumerates an item that is itself a collection (see + /// OddShapeTests.CollectionsInsideCollections). In 2.3.3 and earlier an item that is a + /// collection is validated as an object but never enumerated. After the fix it is also + /// enumerated, and enumerating can do two things that validating the object never did: + /// it can throw, and it can run for a long time. None of these cases is decided yet. + /// Each case has the reasoning and a recommendation. The specs are skipped until a ruling. + /// + /// What the validator does today when it enumerates a collection that a property holds: + /// - It does not catch anything. The exception leaves TryValidateObjectRecursive (see + /// ThrowingMemberTests). A foreach calls GetEnumerator and MoveNext directly, not through + /// reflection, so the exception is not wrapped in a TargetInvocationException. + /// - It never stops a long or endless sequence. An iterator that never ends makes the walk + /// hang. ValidatorHardeningTests says that lazy or infinite sequences of objects need a + /// separate decision. A hang cannot be a test, because it stops the test run, so there is + /// no spec for it here. A collection item behaves the same as a property after the fix. + /// + /// A fix that enumerates items therefore changes what a model can do to the caller. A model + /// that passes on 2.3.3 can throw on 2.4, if one of its items is a collection that throws + /// when it is enumerated. Only the cases below are realistic. A collection that throws on + /// enumeration is a broken collection, but a default struct is a common way to get one. + /// + public class NestedCollectionEdgeCaseTests + { + public class Leaf + { + [Required] + public string Name { get; set; } + } + + public class Holder + { + public T Value { get; set; } + } + + 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)); + } + + /// + /// A collection that throws when it is enumerated. It is not sealed, so the validator cannot + /// tell from the type that it has nothing to validate (see IsLeafType), and it enumerates it. + /// + public class ThrowingBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new InvalidOperationException("enumeration failed"); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + /// + /// Case 1: an item whose enumeration throws. + /// Reasoning: + /// - The validator does not catch exceptions anywhere, and the guard below shows that a + /// property that holds this collection already throws. An item should do the same, so one + /// rule covers both and the caller handles one kind of failure. + /// - Catching would turn a broken model into "valid" or "invalid", and neither is true. The + /// caller could not tell a bad model from a bad collection. + /// - The cost is that a model which passes on 2.3.3 can throw on 2.4, because 2.3.3 never + /// enumerates the item. This is a rare shape: a collection that throws on enumeration. + /// Recommendation: let it propagate. Add a line to the changelog entry. + /// + public class ItemThatThrowsWhenEnumerated + { + // Guard. This passes today, and shows the rule that the spec below extends to an item. + [Fact] + public void Property_that_throws_when_enumerated_propagates_today() + { + var ex = Record.Exception(() => Run(new Holder { Value = new ThrowingBag() })); + + Assert.IsType(ex); + } + + [Fact(Skip = "Open. Whether an item that throws when enumerated should propagate like a property is not decided.")] + public void Item_that_throws_when_enumerated_propagates() + { + var ex = Record.Exception(() => Run(new Holder> { Value = new List { new ThrowingBag() } })); + + Assert.IsType(ex); + } + + // Passes on 2.3.3, because nothing enumerates the item. If the ruling is to propagate, + // this guard is deleted with the spec above unskipped, so the change is deliberate. + [Fact] + public void Item_that_throws_when_enumerated_is_not_enumerated_today() + { + var (valid, errors) = Run(new Holder> { Value = new List { new ThrowingBag() } }); + + Assert.True(valid); + Assert.Empty(errors); + } + } + +#if NET8_0_OR_GREATER + /// + /// Case 2: an item that is a default ImmutableArray. + /// An ImmutableArray is a struct that wraps an array. default(ImmutableArray) wraps null, + /// and enumerating it throws InvalidOperationException. A model gets one by accident: a + /// field that nobody set, a deserializer that skipped a missing JSON property, or a + /// constructor that left it out. It is the common way to meet a collection that throws. + /// See: https://learn.microsoft.com/dotnet/api/system.collections.immutable.immutablearray-1.isdefault + /// Reasoning: + /// - A default array holds no objects, so "valid" is the correct answer, and the answer on + /// 2.3.3. A fix that makes it throw turns a passing model into a crash for a reason the + /// caller did not cause and cannot see in the model. + /// - The same array held in a property is not walked at all (StructsAreNotWalked), so it + /// passes today and after the fix. Only an item would throw. That difference is a trap. + /// - The fix can skip a value that is a default ImmutableArray. It checks IsDefault, which + /// needs the type, so this is a special case for one framework type. I know of no other + /// framework collection that throws when it is default. Other struct collections are + /// the caller's own types, and Case 1 covers them. + /// Recommendation: skip a default ImmutableArray, and validate a non-default one. + /// The alternative is Case 1 for this type too. It is less work, and it breaks models. + /// + public class DefaultImmutableArrayItem + { + [Fact(Skip = "Open. A default ImmutableArray item must not throw, but the fix is not decided.")] + public void Default_immutable_array_item_is_treated_as_empty() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { default }, + }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact(Skip = "Open. A default ImmutableArray item must not throw, but the fix is not decided.")] + public void Default_immutable_array_in_an_object_array_is_treated_as_empty() + { + // The item is boxed as an object, which is how the validator sees it in an object[]. + var (valid, errors) = Run(new Holder { Value = new object[] { default(ImmutableArray) } }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Guard. The item that is not default is validated by the spec in OddShapeTests, and a + // default array held in a property passes today and must keep passing. + [Fact] + public void Default_immutable_array_in_a_property_is_not_walked() + { + var (valid, errors) = Run(new Holder> { Value = default }); + + Assert.True(valid); + Assert.Empty(errors); + } + } +#endif + } +} diff --git a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs index 37ca83b..913be10 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs @@ -1,6 +1,10 @@ using System; using System.Collections; using System.Collections.Generic; +#if NET8_0_OR_GREATER +using System.Collections.Immutable; +#endif +using System.Collections.ObjectModel; using System.ComponentModel.DataAnnotations; using System.Dynamic; using System.IO; @@ -22,8 +26,10 @@ namespace RecursiveDataAnnotationsValidation.Tests /// - A limitation guard. It passes today and shows a shape that is silently not validated. /// If the validator learns to walk that shape, the test fails on purpose, so the change is deliberate. /// - An open test. It is skipped and states the behavior a fix would give. + /// - A spec. It is not skipped and fails until the fix lands. It states the behavior the fix gives. /// Every result below is the same on release 2.2.0 and on the current code, except the - /// framework types in MembersThatThrow, which are no longer walked. + /// framework types in MembersThatThrow, which are no longer walked, and the collections in + /// CollectionsInsideCollections, which 2.4.0 enumerates. /// Not covered here, because it stops the test run: a Task that has not completed makes the /// walk read Task.Result, which waits forever. The open decision, with skipped specs, is in /// TaskPropertyTests. @@ -51,18 +57,34 @@ private static (bool Valid, List Errors) Run(object model) private const string NameRequired = " | The Name field is required."; /// - /// A collection inside a collection. The validator walks the properties of an item and - /// enumerates a property that is a collection. It never enumerates an item that is itself a - /// collection, because an item is not a property. So a List of List, or an object array that - /// holds a List, hides the objects inside. - /// A dictionary works, because its item is a KeyValuePair whose Value is a property. - /// A jagged array works by accident: the item is an array, and an array has a public - /// SyncRoot property that returns the array itself, which the validator walks and enumerates. + /// A collection inside a collection. Releases 2.0.0 to 2.3.3 walk the properties of an item + /// and enumerate a property that is a collection. They never enumerate an item that is itself + /// a collection, because an item is not a property. So a List of List, a List of HashSet, or + /// an object array that holds a List passed validation with invalid objects inside. + /// Some shapes worked by accident. An array or an ArrayList has a public SyncRoot property + /// that returns the collection itself. The validator walked that property and enumerated it, + /// so a jagged array reported the path "Value[0].SyncRoot[0].Name". A List of Dictionary + /// worked through the public Values property, with the path "Value[0].Values[0].Name". + /// Release 2.4.0 enumerates an item that is a collection, and reports the index of each level: + /// "Value[0][0].Name". The tests in this class state that behavior. Most fail on 2.3.3 and + /// earlier: the shapes that passed silently report nothing, and the shapes that worked by + /// accident report a different path. The guards for null items, empty collections and + /// collections of values pass on both. + /// An item that is a collection is still validated as an object first, as before: its own + /// attributes run and its own properties are walked. Then it is enumerated. Enumerating + /// must not remove an error that 2.3.3 reports (see CollectionsWithMembersOfTheirOwn). + /// The items are enumerated before the properties are walked. An object is validated once, + /// so a Leaf that is reachable both ways is reported at the enumeration path. + /// A dictionary item is enumerated as KeyValuePair items, so a List of Dictionary reports + /// "Value[0][0].Value.Name", the same member names as a dictionary held in a property. + /// A collection of a struct type, such as ImmutableArray or a readonly struct that implements + /// IEnumerable, is enumerated like any other. The validator does not walk into a struct + /// that is held in a property, but an item that hides objects must not pass. /// See: https://learn.microsoft.com/dotnet/api/system.array.syncroot /// public class CollectionsInsideCollections { - [Fact(Skip = "Not fixed yet. An item that is itself a collection is not enumerated, so its objects are not validated.")] + [Fact] public void List_of_lists_is_validated() { var (valid, errors) = Run(new Holder>> @@ -71,10 +93,10 @@ public void List_of_lists_is_validated() }); Assert.False(valid); - Assert.Single(errors); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); } - [Fact(Skip = "Not fixed yet. An item that is itself a collection is not enumerated, so its objects are not validated.")] + [Fact] public void Object_array_holding_a_list_is_validated() { var (valid, errors) = Run(new Holder @@ -83,7 +105,353 @@ public void Object_array_holding_a_list_is_validated() }); Assert.False(valid); - Assert.Single(errors); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void List_of_hash_sets_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new HashSet { new Leaf() } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void List_of_read_only_collections_is_validated() + { + // A read-only collection is typical for a type that exposes IReadOnlyList of IReadOnlyList. + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new ReadOnlyCollection(new[] { new Leaf() }) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void Three_levels_of_lists_are_validated() + { + var (valid, errors) = Run(new Holder>>> + { + Value = new List>> + { + new List> { new List { new Leaf() } }, + }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0][0].Name" + NameRequired), errors); + } + + [Fact] + public void Path_has_the_index_of_each_level() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> + { + new List { new Leaf { Name = "a" }, new Leaf { Name = "b" } }, + new List { new Leaf { Name = "c" }, new Leaf() }, + }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[1][1].Name" + NameRequired), errors); + } + + [Fact] + public void Null_items_count_in_the_index_at_each_level() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> + { + null, + new List { null, new Leaf() }, + }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[1][1].Name" + NameRequired), errors); + } + + [Fact] + public void Each_invalid_object_is_reported_with_its_own_path() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> + { + new List { new Leaf() }, + new List { new Leaf() }, + }, + }); + + Assert.False(valid); + Assert.Equal( + ResultText.Expect("Value[0][0].Name" + NameRequired, "Value[1][0].Name" + NameRequired), + errors); + } + + [Fact] + public void Inner_list_that_appears_twice_is_reported_once() + { + // The validator visits each object once, so an object that is reachable twice is + // reported at the first path only. The same holds for an inner collection. + var shared = new List { new Leaf() }; + + var (valid, errors) = Run(new Holder>> + { + Value = new List> { shared, shared }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + // The test above passes even if the inner list is walked twice, because the Leaf is + // marked as visited on the first walk. Counting the enumerations shows that the shared + // collection is walked once. + [Fact] + public void Inner_list_that_appears_twice_is_enumerated_once() + { + var shared = new CountingLeaves(); + + var (valid, errors) = Run(new Holder> { Value = new List { shared, shared } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + Assert.Equal(1, shared.Enumerations); + } + + public class CountingLeaves : IEnumerable + { + public int Enumerations { get; private set; } + + public IEnumerator GetEnumerator() + { + Enumerations++; + yield return new Leaf(); + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + // A sealed class cannot have a derived type, and this one has no attributes and no + // properties, so the type check IsLeafType says that it can never produce a result. That + // check looks at the type only, not at the Leaf objects the class yields when enumerated. + // A List of SealedBag is then skipped as a collection of leaf values, and a fix that + // enumerates only the items that fail the check would still miss it. + // See: https://learn.microsoft.com/dotnet/csharp/language-reference/keywords/sealed + [Fact] + public void List_of_sealed_collections_is_validated() + { + var (valid, errors) = Run(new Holder> + { + Value = new List { new SealedBag(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void Object_array_holding_a_sealed_collection_is_validated() + { + var (valid, errors) = Run(new Holder + { + Value = new object[] { new SealedBag(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + public sealed class SealedBag : IEnumerable + { + private readonly Leaf[] _items; + + public SealedBag(params Leaf[] items) + { + _items = items; + } + + public IEnumerator GetEnumerator() => ((IEnumerable)_items).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + +#if NET8_0_OR_GREATER + // ImmutableList is sealed, and System.Collections.Immutable is not part of net481. + // See: https://learn.microsoft.com/dotnet/api/system.collections.immutable.immutablelist-1 + [Fact] + public void List_of_immutable_lists_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { ImmutableList.Create(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } +#endif + + // StructsAreNotWalked.LeafBag is a readonly struct that implements IEnumerable of Leaf. + // A boxed struct is not a sealed class, but IsLeafType treats both the same way, so this + // is the struct form of the SealedBag gap above. + [Fact] + public void List_of_struct_collections_is_validated() + { + var (valid, errors) = Run(new Holder> + { + Value = new List { new StructsAreNotWalked.LeafBag(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void Object_array_holding_a_struct_collection_is_validated() + { + var (valid, errors) = Run(new Holder + { + Value = new object[] { new StructsAreNotWalked.LeafBag(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + +#if NET8_0_OR_GREATER + // ImmutableArray is a struct. See the limitation guards in StructsAreNotWalked for the + // same type held in a property. + // See: https://learn.microsoft.com/dotnet/api/system.collections.immutable.immutablearray-1 + [Fact] + public void List_of_immutable_arrays_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { ImmutableArray.Create(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } +#endif + + [Fact] + public void Lists_that_contain_each_other_end_and_report_the_invalid_object_once() + { + // The outer list is also an item of the inner list, so enumerating items that are + // collections must not loop. The invalid object is on the inner list. + var outer = new List(); + var inner = new List { outer, new Leaf() }; + outer.Add(inner); + + var (valid, errors) = Run(new Holder> { Value = outer }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][1].Name" + NameRequired), errors); + } + + [Fact] + public void Null_and_empty_inner_collections_are_ignored() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { null, new List(), new List { null } }, + }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Items that are collections of values are not enumerated, like a property of that + // type (see IsCollectionOfLeafType). This test counts the enumerations that start. + // The result is the same either way, so only a count can tell the two apart. + // An iterator method runs no code until the first MoveNext, so the count goes up there. + [Fact] + public void Inner_collection_of_values_is_not_enumerated() + { + var numbers = new CountingNumbers(); + + var (valid, errors) = Run(new Holder> { Value = new List { numbers } }); + + Assert.True(valid); + Assert.Empty(errors); + Assert.Equal(0, numbers.Enumerations); + } + + public class CountingNumbers : IEnumerable + { + public int Enumerations { get; private set; } + + public IEnumerator GetEnumerator() + { + Enumerations++; + yield return 1; + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + [Fact] + public void List_of_arrays_is_validated() + { + var (valid, errors) = Run(new Holder> + { + Value = new List { new[] { new Leaf() } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void Object_array_holding_an_array_is_validated() + { + var (valid, errors) = Run(new Holder + { + Value = new object[] { new[] { new Leaf() } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void List_of_array_lists_is_validated() + { + // ArrayList is not generic, and like an array it has a public SyncRoot property. + var (valid, errors) = Run(new Holder> + { + Value = new List { new ArrayList { new Leaf() } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + [Fact] + public void List_of_dictionaries_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> + { + new Dictionary { ["k"] = new Leaf() }, + }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Value.Name" + NameRequired), errors); } [Fact] @@ -98,10 +466,9 @@ public void Dictionary_of_lists_is_validated_through_the_pair_value() Assert.Equal(ResultText.Expect("Value[0].Value[0].Name" + NameRequired), errors); } - // The path has SyncRoot in it. If nested collections are enumerated properly, the - // path becomes "Value[0][0].Name" and this test fails on purpose. + // Up to release 2.3.3 the path was "Value[0].SyncRoot[0].Name". [Fact] - public void Jagged_array_is_validated_through_SyncRoot() + public void Jagged_array_is_validated() { var (valid, errors) = Run(new Holder { @@ -109,7 +476,36 @@ public void Jagged_array_is_validated_through_SyncRoot() }); Assert.False(valid); - Assert.Equal(ResultText.Expect("Value[0].SyncRoot[0].Name" + NameRequired), errors); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + } + + /// + /// A collection as the object passed to the validator. The validator validates that object + /// and walks its properties, but never enumerates it, because an item is only reached + /// through a property. So a List that is the root hides its items, and a root array reports + /// its items through SyncRoot. This is not changed by the fix for nested collections. + /// These are limitation guards. If the validator learns to enumerate a root collection, + /// the tests fail on purpose, so the change is deliberate. The path format for it is open. + /// + public class CollectionAsRootObject + { + [Fact] + public void Items_of_a_root_list_are_not_validated() + { + var (valid, errors) = Run(new List { new Leaf() }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact] + public void Items_of_a_root_array_are_validated_through_SyncRoot() + { + var (valid, errors) = Run(new[] { new Leaf() }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("SyncRoot[0].Name" + NameRequired), errors); } } @@ -120,6 +516,10 @@ public void Jagged_array_is_validated_through_SyncRoot() /// The items are still validated. /// Whether the attributes of the collection object should run is open. Running them would /// fail models that pass today. + /// This holds for a collection held in a property. A collection that is an item of another + /// collection is validated as an object, so its attributes and IValidatableObject run today. + /// Enumerating such an item (see CollectionsInsideCollections) must keep those errors, + /// because dropping them would let a model that fails on 2.3.3 pass. /// public class CollectionsWithMembersOfTheirOwn { @@ -149,6 +549,53 @@ public void Items_of_the_collection_are_validated() Assert.False(valid); Assert.Equal(ResultText.Expect("Value[0].Name" + NameRequired), errors); } + + // Guard. An item that is a collection is validated as an object, so its attribute runs. + // This passes on 2.3.3. A fix that only enumerates the item would drop this error. + [Fact] + public void Attributes_on_a_collection_item_are_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new PagedList { Cursor = null } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Cursor | The Cursor field is required."), errors); + } + + // Spec. The item has its own error and an invalid object inside. Both are reported. + [Fact] + public void Attributes_and_items_of_a_collection_item_are_both_validated() + { + var page = new PagedList { Cursor = null }; + page.Add(new Leaf()); + + var (valid, errors) = Run(new Holder>> { Value = new List> { page } }); + + Assert.False(valid); + Assert.Equal( + ResultText.Expect("Value[0].Cursor | The Cursor field is required.", "Value[0][0].Name" + NameRequired), + errors); + } + + public class ValidatableBag : List, IValidatableObject + { + public IEnumerable Validate(ValidationContext validationContext) + { + yield return new ValidationResult("The bag is not valid.", new[] { "Total" }); + } + } + + // Guard. IValidatableObject on an item that is a collection runs on 2.3.3, and must keep running. + [Fact] + public void Validate_on_a_collection_item_runs() + { + var (valid, errors) = Run(new Holder> { Value = new List { new ValidatableBag() } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Total | The bag is not valid."), errors); + } } /// From fe7e1c6c8580c086b78ffc412e706b89f4caed30 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Thu, 1 Oct 2026 23:51:55 -0400 Subject: [PATCH 02/12] fix: validate the objects inside an item that is a collection The validator enumerated a collection that a property holds, but never an item that is itself a collection. A List of List, a List of HashSet, or an object array that holds a List passed with invalid objects inside. Arrays and ArrayList were reached by accident through SyncRoot. TryValidateObjectRecursive now takes enumerateItems. An item that is a collection is validated as an object first, so its attributes and IValidatableObject still run, then its items are enumerated before its properties are walked. The path has the index of each level: Value[0][0].Name. A collection of leaf types is not enumerated. IsLeafType now treats a sealed class or struct that is a collection as a leaf only when it is a collection of leaf types. Without that, a List of ImmutableList or of a sealed collection was skipped as a list of values. A type that yields itself is answered as not a leaf, to end the check. A default ImmutableArray item is skipped, because enumerating it throws and it holds nothing. Any other item that throws when enumerated throws from validation, like a collection that a property holds. Co-Authored-By: Claude Sonnet 5.5 --- .../Extensions/TypeExtensions.cs | 45 +++++- .../RecursiveDataAnnotationValidator.cs | 137 ++++++++++++++---- .../Extensions/TypeExtensionsTests.cs | 90 ++++++++++++ .../NestedCollectionEdgeCaseTests.cs | 39 ++--- 4 files changed, 252 insertions(+), 59 deletions(-) diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index f816e4b..681a484 100644 --- a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs +++ b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Collections; using System.Collections.Concurrent; using System.Collections.Generic; using System.ComponentModel; @@ -44,6 +45,11 @@ internal static class TypeExtensions typeof(Process), // StartInfo and others throw for a process this object did not start }; + // The types IsLeafType is deciding on this thread. A type that yields itself, such as + // `sealed class Node : IEnumerable`, asks about its own type while it is being decided. + [ThreadStatic] + private static HashSet _typesBeingChecked; + private static int _typeDescriptorVersion; static TypeExtensions() @@ -72,6 +78,10 @@ public static bool IsCollectionOfLeafType(this Type collectionType) /// 2. No validation attribute on any of its properties. /// 3. It does not implement IValidatableObject. /// 4. No property the validator walks into (see ). + /// 5. If it is a collection, it is a collection of leaf types (see ), + /// because the validator enumerates an item that is a collection, and the items it + /// yields can have attributes of their own. A collection of an unknown item type, such as + /// a non-generic one, is not a leaf type. /// Checks 1 and 2 use TypeDescriptor, like Validator, so attributes added at runtime count. /// public static bool IsLeafType(this Type type) @@ -82,7 +92,22 @@ public static bool IsLeafType(this Type type) if (LeafTypeCache.TryGetValue(type, out var cached) && cached.Version == version) return cached.IsLeaf; - var isLeaf = FindIsLeafType(type); + var typesBeingChecked = _typesBeingChecked ?? (_typesBeingChecked = new HashSet()); + + // A type that is already being decided is not a leaf type. That is the safe answer, + // because the validator then looks at the type instead of skipping it. + if (!typesBeingChecked.Add(type)) return false; + + bool isLeaf; + try + { + isLeaf = FindIsLeafType(type); + } + finally + { + typesBeingChecked.Remove(type); + } + LeafTypeCache[type] = (version, isLeaf); return isLeaf; } @@ -127,6 +152,21 @@ public static bool IsUnsafeToWalk(this Type type) && UnsafeToWalkTypes.Any(unsafeType => unsafeType.IsAssignableFrom(t))); } + /// + /// True for a default ImmutableArray, such as an array field that nobody set. It wraps null, + /// so enumerating it throws InvalidOperationException, and it holds nothing to validate. + /// The type is found by name, because the library does not reference System.Collections.Immutable. + /// + public static bool IsDefaultImmutableArray(this object obj) + { + var type = obj.GetType(); + + return type.IsValueType + && type.IsGenericType + && type.GetGenericTypeDefinition().FullName == "System.Collections.Immutable.ImmutableArray`1" + && (bool)type.GetProperty("IsDefault").GetValue(obj, null); + } + private static bool IsInSystemNamespace(Type type) { var ns = type.Namespace; @@ -148,7 +188,8 @@ private static bool FindIsLeafType(Type type) return (type.IsValueType || type.IsSealed) && !HasValidationAttributes(type) && !typeof(IValidatableObject).IsAssignableFrom(type) - && !type.GetProperties().Any(IsWalked); + && !type.GetProperties().Any(IsWalked) + && (!typeof(IEnumerable).IsAssignableFrom(type) || type.IsCollectionOfLeafType()); } /// diff --git a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs index 6d0322a..7f01dc9 100644 --- a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs +++ b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs @@ -172,13 +172,17 @@ private static bool EqualsAnAncestor(object obj, Type type, List equalit //validatedObjects holds every object visited so far, compared by reference. //equalityPath holds the objects on the path from the root to this object whose type overrides Equals. + //enumerateItems is true for an item of a collection. If the item is itself a collection, its + //items are validated too. A collection that a property holds is enumerated by the caller, and + //the root object is never enumerated. private bool TryValidateObjectRecursive( object obj, ICollection validationResults, ISet validatedObjects, List equalityPath, IServiceProvider serviceProvider, - IDictionary validationContextItems + IDictionary validationContextItems, + bool enumerateItems = false ) { var type = obj.GetType(); @@ -211,6 +215,33 @@ IDictionary validationContextItems var result = TryValidateObject(obj, validationResults, serviceProvider, validationContextItems); + //An item that is a collection is validated as an object above, so its own attributes run. + //Then its items are validated, before its properties are walked. An object that the + //collection also returns from a property, such as Array.SyncRoot, is then reported at its + //index (Value[0][0]) and not through the property (Value[0].SyncRoot[0]). + //A collection of leaf types is skipped, like a collection that a property holds. A default + //ImmutableArray is skipped, because enumerating it throws and it holds nothing. + var enumeratedItems = false; + if (enumerateItems + && obj is IEnumerable items + && !type.IsCollectionOfLeafType() + && !obj.IsDefaultImmutableArray()) + { + enumeratedItems = true; + if (!TryValidateItems( + items, + "", + validationResults, + validatedObjects, + equalityPath, + serviceProvider, + validationContextItems + )) + { + result = false; + } + } + //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) @@ -231,40 +262,24 @@ IDictionary validationContextItems continue; case IEnumerable asEnumerable: - var arrayIndex = -1; - foreach (var item in 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; + + if (!TryValidateItems( + asEnumerable, + property.Name, + validationResults, + validatedObjects, + equalityPath, + serviceProvider, + validationContextItems + )) { - arrayIndex++; - - //NOTE: Possibly should have a separate case for Dictionary which reports on the key - - if (item == null) continue; - nestedResults = new List(); - if (!TryValidateObjectRecursive( - item, - nestedResults, - validatedObjects, - equalityPath, - serviceProvider, - validationContextItems - )) - { - result = false; - foreach (var validationResult in nestedResults) - { - var property1 = property; - validationResults.Add( - new ValidationResult( - validationResult.ErrorMessage, - validationResult.MemberNames - .Select(x => $"{property1.Name}[{arrayIndex}].{x}") - .ToList() - )); - } - } + result = false; } break; - + default: nestedResults = new List(); if (!TryValidateObjectRecursive( @@ -296,6 +311,64 @@ IDictionary validationContextItems return result; } + //Validates each item of a collection. The result of an item is added to validationResults with + //its member names prefixed by the property name, if there is one, and the index of the item. + private bool TryValidateItems( + IEnumerable items, + string propertyName, + ICollection validationResults, + ISet validatedObjects, + List equalityPath, + IServiceProvider serviceProvider, + IDictionary validationContextItems + ) + { + var result = true; + var arrayIndex = -1; + foreach (var item in items) + { + arrayIndex++; + + //NOTE: Possibly should have a separate case for Dictionary which reports on the key + + if (item == null) continue; + var nestedResults = new List(); + if (!TryValidateObjectRecursive( + item, + nestedResults, + validatedObjects, + equalityPath, + serviceProvider, + validationContextItems, + enumerateItems: true + )) + { + result = false; + var index = arrayIndex; + foreach (var validationResult in nestedResults) + { + validationResults.Add( + new ValidationResult( + validationResult.ErrorMessage, + validationResult.MemberNames + .Select(x => ItemMemberName(propertyName, index, x)) + .ToList() + )); + } + } + } + + return result; + } + + //The name of a member of an item: Items[1].Name. The member name of an item that is a + //collection starts with its own index, so the indexes are joined without a dot: Items[1][0].Name. + private static string ItemMemberName(string propertyName, int index, string memberName) + { + var separator = memberName.StartsWith("[", StringComparison.Ordinal) ? "" : "."; + return propertyName + "[" + index + "]" + separator + memberName; + } + /* Note 1: * * Background information of why we don't use ValidationContext.ObjectInstance here, even though it is tempting. diff --git a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs index 830a674..a06dc04 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs @@ -17,6 +17,10 @@ namespace RecursiveDataAnnotationsValidation.Tests.Extensions /// that implements it. /// 4. No property the validator would walk into: a readable, non-indexer property of a /// reference type other than string. + /// 5. If it is a collection, it is a collection of leaf types. The validator enumerates an + /// item that is a collection, so a sealed class or a struct that yields objects must not + /// be skipped. A collection that yields an unknown type, such as a non-generic one, counts + /// as one that yields objects. /// Checks 1 and 2 use TypeDescriptor, like Validator, so attributes added at runtime count. /// The type must also be a value type or a sealed class. Otherwise a collection declared /// as List<Base> could hold a derived object that has its own attributes. @@ -46,6 +50,38 @@ public sealed class SealedTag public int Weight { get; set; } } + // Collections of leaf types and of objects, sealed or a struct, for check 5. + public sealed class SealedIntBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + IEnumerator IEnumerable.GetEnumerator() => throw new NotSupportedException(); + } + + public sealed class SealedChildBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + IEnumerator IEnumerable.GetEnumerator() => throw new NotSupportedException(); + } + + public struct ChildBagStruct : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + IEnumerator IEnumerable.GetEnumerator() => throw new NotSupportedException(); + } + + public sealed class NonGenericBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + } + + // Yields its own type. Deciding that it is a leaf type asks whether it is a leaf type, and + // without a guard that would never end. It is not a leaf type, because it yields objects. + public sealed class SelfYieldingBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + IEnumerator IEnumerable.GetEnumerator() => throw new NotSupportedException(); + } + // Fails check 3. public struct SelfValidatingPoint : IValidatableObject { @@ -107,6 +143,11 @@ public class MixedSequence : IEnumerable, IEnumerable [InlineData(typeof(List))] [InlineData(typeof(List))] [InlineData(typeof(List))] + [InlineData(typeof(List))] +#if NET8_0_OR_GREATER + [InlineData(typeof(List>))] + [InlineData(typeof(List>))] +#endif public void Collection_of_leaf_type_is_detected(Type type) { Assert.True(type.IsCollectionOfLeafType()); @@ -127,11 +168,60 @@ public void Collection_of_leaf_type_is_detected(Type type) [InlineData(typeof(List))] [InlineData(typeof(List))] [InlineData(typeof(List))] + [InlineData(typeof(List))] + [InlineData(typeof(List))] + [InlineData(typeof(List))] + [InlineData(typeof(List))] + [InlineData(typeof(SelfYieldingBag[]))] +#if NET8_0_OR_GREATER + [InlineData(typeof(List>))] + [InlineData(typeof(List>))] +#endif public void Collection_that_can_yield_other_types_is_not_detected(Type type) { Assert.False(type.IsCollectionOfLeafType()); } + [Fact] + public void Sealed_collection_of_objects_is_not_a_leaf_type() + { + Assert.False(typeof(SealedChildBag).IsLeafType()); + Assert.False(typeof(ChildBagStruct).IsLeafType()); + Assert.False(typeof(NonGenericBag).IsLeafType()); + Assert.False(typeof(SelfYieldingBag).IsLeafType()); + } + + [Fact] + public void Sealed_collection_of_values_is_a_leaf_type() + { + Assert.True(typeof(SealedIntBag).IsLeafType()); + Assert.True(typeof(string).IsLeafType()); + } + +#if NET8_0_OR_GREATER + // The library does not reference System.Collections.Immutable, so it finds the type by name. + [Fact] + public void Default_immutable_array_is_detected() + { + Assert.True(((object)default(System.Collections.Immutable.ImmutableArray)).IsDefaultImmutableArray()); + } + + [Fact] + public void Immutable_array_that_has_a_value_is_not_detected() + { + Assert.False(((object)System.Collections.Immutable.ImmutableArray.Create(new Child())).IsDefaultImmutableArray()); + Assert.False(((object)System.Collections.Immutable.ImmutableArray.Empty).IsDefaultImmutableArray()); + } + + [Fact] + public void Other_objects_are_not_a_default_immutable_array() + { + Assert.False(new List().IsDefaultImmutableArray()); + Assert.False(((object)default(int)).IsDefaultImmutableArray()); + Assert.False("text".IsDefaultImmutableArray()); + } +#endif + // Framework types whose properties throw or never end when walked, and types derived from them. [Theory] [InlineData(typeof(Type))] diff --git a/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs index a6ff05a..7d0a445 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs @@ -10,12 +10,11 @@ namespace RecursiveDataAnnotationsValidation.Tests { /// - /// Open cases for the fix that enumerates an item that is itself a collection (see + /// Edge cases of the fix that enumerates an item that is itself a collection (see /// OddShapeTests.CollectionsInsideCollections). In 2.3.3 and earlier an item that is a - /// collection is validated as an object but never enumerated. After the fix it is also + /// collection is validated as an object but never enumerated. Since 2.4.0 it is also /// enumerated, and enumerating can do two things that validating the object never did: - /// it can throw, and it can run for a long time. None of these cases is decided yet. - /// Each case has the reasoning and a recommendation. The specs are skipped until a ruling. + /// it can throw, and it can run for a long time. Each case has the reasoning for the decision. /// /// What the validator does today when it enumerates a collection that a property holds: /// - It does not catch anything. The exception leaves TryValidateObjectRecursive (see @@ -72,37 +71,26 @@ public class ThrowingBag : IEnumerable /// caller could not tell a bad model from a bad collection. /// - The cost is that a model which passes on 2.3.3 can throw on 2.4, because 2.3.3 never /// enumerates the item. This is a rare shape: a collection that throws on enumeration. - /// Recommendation: let it propagate. Add a line to the changelog entry. + /// Decision: let it propagate. The changelog entry for 2.4.0 says so. /// public class ItemThatThrowsWhenEnumerated { - // Guard. This passes today, and shows the rule that the spec below extends to an item. + // Guard. A property that holds this collection throws, and so does an item (below). [Fact] - public void Property_that_throws_when_enumerated_propagates_today() + public void Property_that_throws_when_enumerated_propagates() { var ex = Record.Exception(() => Run(new Holder { Value = new ThrowingBag() })); Assert.IsType(ex); } - [Fact(Skip = "Open. Whether an item that throws when enumerated should propagate like a property is not decided.")] + [Fact] public void Item_that_throws_when_enumerated_propagates() { var ex = Record.Exception(() => Run(new Holder> { Value = new List { new ThrowingBag() } })); Assert.IsType(ex); } - - // Passes on 2.3.3, because nothing enumerates the item. If the ruling is to propagate, - // this guard is deleted with the spec above unskipped, so the change is deliberate. - [Fact] - public void Item_that_throws_when_enumerated_is_not_enumerated_today() - { - var (valid, errors) = Run(new Holder> { Value = new List { new ThrowingBag() } }); - - Assert.True(valid); - Assert.Empty(errors); - } } #if NET8_0_OR_GREATER @@ -120,15 +108,15 @@ public void Item_that_throws_when_enumerated_is_not_enumerated_today() /// - The same array held in a property is not walked at all (StructsAreNotWalked), so it /// passes today and after the fix. Only an item would throw. That difference is a trap. /// - The fix can skip a value that is a default ImmutableArray. It checks IsDefault, which - /// needs the type, so this is a special case for one framework type. I know of no other - /// framework collection that throws when it is default. Other struct collections are + /// needs the type, so this is a special case for one framework type. No other + /// framework collection is known to throw when it is default. Other struct collections are /// the caller's own types, and Case 1 covers them. - /// Recommendation: skip a default ImmutableArray, and validate a non-default one. - /// The alternative is Case 1 for this type too. It is less work, and it breaks models. + /// Decision: skip a default ImmutableArray, and validate a non-default one. The alternative + /// was Case 1 for this type too. It is less work, and it breaks models. /// public class DefaultImmutableArrayItem { - [Fact(Skip = "Open. A default ImmutableArray item must not throw, but the fix is not decided.")] + [Fact] public void Default_immutable_array_item_is_treated_as_empty() { var (valid, errors) = Run(new Holder>> @@ -140,7 +128,7 @@ public void Default_immutable_array_item_is_treated_as_empty() Assert.Empty(errors); } - [Fact(Skip = "Open. A default ImmutableArray item must not throw, but the fix is not decided.")] + [Fact] public void Default_immutable_array_in_an_object_array_is_treated_as_empty() { // The item is boxed as an object, which is how the validator sees it in an object[]. @@ -152,6 +140,7 @@ public void Default_immutable_array_in_an_object_array_is_treated_as_empty() // Guard. The item that is not default is validated by the spec in OddShapeTests, and a // default array held in a property passes today and must keep passing. + // See StructCollectionPropertyTests for a non-default array held in a property. [Fact] public void Default_immutable_array_in_a_property_is_not_walked() { From 7d19c4c7b1909eb656a8f4b983ae20225f2b7dac Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Thu, 1 Oct 2026 23:51:55 -0400 Subject: [PATCH 03/12] test: add specs for a struct collection held in a property A struct collection such as ImmutableArray is now validated as an item of another collection, but a property that holds it is still not read, because the validator walks reference-type properties only. The same invalid objects pass or fail depending on one level of nesting. A guard pins the gap. Skipped specs state the planned behavior for the next release, with the plan and the steps for converting the guards. Co-Authored-By: Claude Sonnet 5.5 --- .../OddShapeTests.cs | 4 +- .../StructCollectionPropertyTests.cs | 288 ++++++++++++++++++ 2 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs diff --git a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs index 913be10..5ad3aac 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs @@ -603,7 +603,9 @@ public void Validate_on_a_collection_item_runs() /// property is checked for its own validation attributes by Validator when the parent is /// validated, but nothing inside the struct is walked. A property of the struct that /// carries an attribute is never checked, and neither are the items of a collection that is - /// itself a struct, such as ImmutableArray<T>. + /// itself a struct, such as ImmutableArray<T>, when a property holds it. A struct + /// collection that is an item of another collection is enumerated since 2.4.0. The planned + /// fix for the property case is in StructCollectionPropertyTests. /// These are limitation guards. Record structs with positional `[property: ...]` /// attributes are a modern way to model a value, so this one may surprise callers. /// See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs new file mode 100644 index 0000000..0717f36 --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs @@ -0,0 +1,288 @@ +using System; +using System.Collections; +using System.Collections.Generic; +#if NET8_0_OR_GREATER +using System.Collections.Immutable; +#endif +using System.ComponentModel.DataAnnotations; +using Xunit; + +namespace RecursiveDataAnnotationsValidation.Tests +{ + /// + /// A collection that is a struct, held in a property. Not fixed in 2.4.0. The specs here are + /// skipped and describe the behavior planned for the next release. + /// + /// The inconsistency. The validator walks only the properties of a reference type (see + /// IsWalked), so a property whose type is a struct is never read. That includes a struct + /// that is a collection, such as ImmutableArray<T>. Since 2.4.0 the validator enumerates an + /// item that is a collection, and it does so for a struct too, because an item is validated as + /// an object whatever its type is. So the same ImmutableArray of invalid objects is: + /// - validated when it is an item, such as List<ImmutableArray<T>>, and + /// - not validated, silently, when it is the value of a property. + /// A model can pass validation because the collection sits one level higher. + /// Both behaviors are pinned below, in a guard that passes today and fails on purpose when the + /// property case is fixed. + /// + /// Why a struct property is skipped. The rule suits a Point or a Money, where the validator + /// has nothing to walk into. It covers a struct that is a collection as a side effect, and + /// nothing in the code or the tests records a decision about collections. A struct property with + /// an attribute on the struct's own members is still not walked (see + /// OddShapeTests.StructsAreNotWalked), and this fix does not change that. A property that holds + /// a struct is only enumerated when the struct is a collection, so that a model does not pass + /// because the collection sits one level higher. + /// See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct + /// + /// The plan for the fix: + /// - IsWalked admits a property of a struct type when the struct is a collection that is not a + /// collection of leaf types, and a Nullable of such a struct. A struct that is not a + /// collection, such as a Money, is still skipped. + /// - The walk reads the value, which is boxed, and enumerates it like any other collection. + /// The path is the same as for a collection that is a class: Value[0].Name. + /// - A default ImmutableArray in a property is skipped, as an item is (see + /// NestedCollectionEdgeCaseTests). It passes today, so enumerating it would add a crash. + /// - A struct collection that throws when enumerated throws from the property too, like an + /// item and like a class. This is a change for the few models that hold one. + /// - Check 4 of IsLeafType uses IsWalked, so a struct with such a property is no longer a + /// leaf type. That is the right answer, because the validator now looks inside it. + /// + /// When the fix lands: + /// - Unskip the specs below. + /// - Delete Struct_collection_is_validated_as_an_item_but_not_as_a_property, which pins the gap. + /// - Replace OddShapeTests.StructsAreNotWalked.Struct_collection_items_are_not_validated, which + /// pins the same gap, and update the comment of that class, which mentions ImmutableArray. + /// - Add a Fixed entry to the changelog. A model that passes today can fail. List the changed + /// member name format as BREAKING only if one exists. A property that was never walked has no + /// path to change. + /// + public class StructCollectionPropertyTests + { + private const string Reason = "Next release. A struct collection held in a property is not enumerated yet."; + + public class Leaf + { + [Required] + public string Name { get; set; } + } + + public class Holder + { + public T Value { get; set; } + } + + private const string NameRequired = " | The Name 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)); + } + + public readonly struct LeafBag : IEnumerable + { + private readonly Leaf[] _items; + + public LeafBag(params Leaf[] items) + { + _items = items; + } + + public IEnumerator GetEnumerator() => ((IEnumerable)_items).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + /// + /// A struct cannot count its own enumerations, because it is copied. This one counts in an + /// object that every copy shares. + /// + public readonly struct CountedNumbers : IEnumerable + { + public CountedNumbers(Counter counter) + { + Counter = counter; + } + + public Counter Counter { get; } + + public IEnumerator GetEnumerator() + { + Counter.Enumerations++; + yield return 1; + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + public class Counter + { + public int Enumerations { get; set; } + } + + public readonly struct ThrowingStructBag : IEnumerable + { + public IEnumerator GetEnumerator() => throw new InvalidOperationException("enumeration failed"); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + + /// + /// Guard that pins the gap. The same struct is validated as an item, and passes as a + /// property. It fails on purpose when the property case is fixed. + /// + [Fact] + public void Struct_collection_is_validated_as_an_item_but_not_as_a_property() + { + var asItem = Run(new Holder> { Value = new List { new LeafBag(new Leaf()) } }); + var asProperty = Run(new Holder { Value = new LeafBag(new Leaf()) }); + + Assert.False(asItem.Valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), asItem.Errors); + Assert.True(asProperty.Valid); + Assert.Empty(asProperty.Errors); + } + + [Fact(Skip = Reason)] + public void Struct_collection_property_is_validated() + { + var (valid, errors) = Run(new Holder { Value = new LeafBag(new Leaf()) }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Name" + NameRequired), errors); + } + + // A nullable struct is boxed as the struct itself when it has a value. + // See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/nullable-value-types#boxing-and-unboxing + [Fact(Skip = Reason)] + public void Nullable_struct_collection_property_is_validated() + { + var (valid, errors) = Run(new Holder { Value = new LeafBag(new Leaf()) }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Name" + NameRequired), errors); + } + + [Fact] + public void Nullable_struct_collection_property_without_a_value_is_valid() + { + var (valid, errors) = Run(new Holder { Value = null }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact(Skip = Reason)] + public void Struct_collection_property_below_a_class_property_is_validated() + { + var (valid, errors) = Run(new Holder> + { + Value = new Holder { Value = new LeafBag(new Leaf()) }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Value[0].Name" + NameRequired), errors); + } + + [Fact(Skip = Reason)] + public void Struct_collection_property_in_an_item_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new Holder { Value = new LeafBag(new Leaf()) } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Value[0].Name" + NameRequired), errors); + } + + // The error of each object is reported once, and the valid ones are not reported. + [Fact(Skip = Reason)] + public void Only_the_invalid_items_of_a_struct_collection_property_are_reported() + { + var (valid, errors) = Run(new Holder + { + Value = new LeafBag(new Leaf { Name = "a" }, null, new Leaf(), new Leaf { Name = "b" }), + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[2].Name" + NameRequired), errors); + } + + // Guard. A struct collection of leaf types has nothing to validate, like a List of int, so + // it is not enumerated. The count shows it, because the result is the same either way. + [Fact] + public void Struct_collection_of_values_is_not_enumerated() + { + var counter = new Counter(); + + var (valid, errors) = Run(new Holder { Value = new CountedNumbers(counter) }); + + Assert.True(valid); + Assert.Empty(errors); + Assert.Equal(0, counter.Enumerations); + } + + // A struct collection that throws when it is enumerated throws from a property, like an + // item (NestedCollectionEdgeCaseTests, Case 1). It passes today, because it is not read. + [Fact(Skip = Reason)] + public void Struct_collection_property_that_throws_when_enumerated_propagates() + { + var ex = Record.Exception(() => Run(new Holder { Value = new ThrowingStructBag() })); + + Assert.IsType(ex); + } + + [Fact] + public void Struct_collection_that_throws_when_enumerated_passes_today() + { + var (valid, errors) = Run(new Holder { Value = new ThrowingStructBag() }); + + Assert.True(valid); + Assert.Empty(errors); + } + +#if NET8_0_OR_GREATER + [Fact(Skip = Reason)] + public void Immutable_array_property_is_validated() + { + var (valid, errors) = Run(new Holder> { Value = ImmutableArray.Create(new Leaf()) }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].Name" + NameRequired), errors); + } + + [Fact(Skip = Reason)] + public void Immutable_array_of_immutable_arrays_property_is_validated() + { + var (valid, errors) = Run(new Holder>> + { + Value = ImmutableArray.Create(ImmutableArray.Create(new Leaf())), + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].Name" + NameRequired), errors); + } + + // Guard. A default ImmutableArray in a property passes today and must pass after the fix. + // Enumerating it throws, so the fix has to skip it, as it does for an item. + [Fact] + public void Default_immutable_array_property_is_valid() + { + var (valid, errors) = Run(new Holder> { Value = default }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact] + public void Immutable_array_of_values_property_is_valid() + { + var (valid, errors) = Run(new Holder> { Value = ImmutableArray.Create(1, 2) }); + + Assert.True(valid); + Assert.Empty(errors); + } +#endif + } +} From f61293285c799ca4f5498896500fd07ee40a68ec Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Thu, 1 Oct 2026 23:51:55 -0400 Subject: [PATCH 04/12] docs: describe the nested collection fix in the changelog and README Mark the two changed member name formats as BREAKING, and add a Collections section to the README. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 12 ++++++++++++ README.md | 10 ++++++++++ 2 files changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce6ed30..852aa4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ Breaking changes are marked **BREAKING**. ## Unreleased +### Fixed + +- Objects inside an item that is itself a collection are now validated. Before, a `List>`, a `List>`, an `object[]` that holds a list, and a list of a sealed or struct collection such as `ImmutableList` passed validation even when an object inside was invalid. The error is reported with the index of each level, for example `Value[0][0].Name`. +- **BREAKING** for these two cases: the member name changed, because the path used to go through a public property of the inner collection. + - An array or an `ArrayList` inside a collection: `Value[0].SyncRoot[0].Name` is now `Value[0][0].Name`. + - A dictionary or a `Hashtable` inside a collection: `Value[0].Values[0].Name` is now `Value[0][0].Value.Name`. + + Code that matches these member names must use the new form. A model that passed before can also fail now, if it holds an invalid object in a nested collection. +- An item that is a collection is still validated as an object first, so its own attributes and `IValidatableObject.Validate` run, as before. Its items are then validated as well. +- An item that throws when it is enumerated now throws from validation, like a collection that a property holds. A default `ImmutableArray` item is skipped, because it holds nothing and enumerating it throws. +- A struct collection such as `ImmutableArray` is validated when it is an item, but not yet when a property holds it. That case is planned for the next release. + ### Changed - The NuGet package title now reads "Recursive DataAnnotations Validation". It was misspelled "Recurisive". diff --git a/README.md b/README.md index cdf4fd5..67cf695 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,16 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot - For a type that does not override `Equals`, mark such a property with `[SkipRecursiveValidation]`. Otherwise the walk overflows the stack, which ends the process. - Two records that reference each other can also overflow the stack. A record's generated `Equals` compares properties in declaration order, so it follows the reference forever if it reaches it before a property that differs. This happens when the records have equal values, or when the reference is declared first. On .NET Framework it always happens, because the framework's `Validator` calls the record's generated `GetHashCode`, which follows the reference too. Marking the reference with `[SkipRecursiveValidation]` avoids the walk, but not the `GetHashCode` call on .NET Framework. There, override `GetHashCode` so it does not include the reference. +### Collections + +- 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 its values are reported as `Map[0].Value.Name`. +- An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. +- 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 not enumerated. Wrap it in an object with a property. +- A collection that is a struct, such as `ImmutableArray`, is validated when it is an item of another collection, but not when a property holds it. The validator only reads properties of reference types. +- The validator does not catch exceptions. If a collection throws when it is enumerated, the exception reaches your code. A default `ImmutableArray` item is skipped, because it holds nothing. + ### Framework types that are not walked The validator does not walk the properties that these framework types declare: `Type` and other `MemberInfo` types, `Assembly`, `Module`, delegates, `Uri`, `FileSystemInfo`, which covers `DirectoryInfo` and `FileInfo`, `Thread`, and `Process`. Reading those properties throws or never ends, for example `Uri.Segments` on a relative `Uri`, and they carry no validation attributes. From 0fd64fbd8ec44b208b11fd0792c9253a8824ec9f Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:04:37 -0400 Subject: [PATCH 05/12] test: add specs for null and bracketed member names on items A class-level attribute that passes ValidationContext.MemberName, or a Validate method that yields a null member name, gives an item result with a null member name. Release 2.4.0 read the name to decide whether it was an index, so it threw NullReferenceException. A name that starts with "[" changed from "Value[0].[Totals]" to "Value[0][Totals]". The tests pin the paths of 2.3.3 for these names and state the path of a nested collection. They fail until the fix lands. Co-Authored-By: Claude Sonnet 5.5 --- .../OddShapeTests.cs | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs index 5ad3aac..6149e53 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs @@ -480,6 +480,105 @@ public void Jagged_array_is_validated() } } + /// + /// Member names that are not plain property names, on an item of a collection. + /// - A class-level attribute that passes ValidationContext.MemberName gives a null member + /// name, because no member is being validated. This is the usual way to write one. + /// The same holds for an IValidatableObject that yields a null member name. + /// The validator prefixes the path to each name, and a null name gives "Value[0]." with + /// nothing after the dot. Code that builds the path must not throw on null. + /// - A member name that starts with "[", such as "[Totals]", is a name the item chose. It + /// is not the index of a nested collection, so the path keeps the dot: "Value[0].[Totals]". + /// The tests also run on 2.3.3, which gives the same paths. Release 2.4.0 first read the + /// name to decide whether it was an index, and threw NullReferenceException on null. + /// See: https://learn.microsoft.com/dotnet/api/system.componentmodel.dataannotations.validationcontext.membername + /// + public class UnusualMemberNames + { + [AttributeUsage(AttributeTargets.Class)] + public class ClassLevelAttribute : ValidationAttribute + { + protected override ValidationResult IsValid(object value, ValidationContext validationContext) => + new ValidationResult("The item is not valid.", new[] { validationContext.MemberName }); + } + + [ClassLevel] + public class ClassLevelItem + { + } + + public class SelfValidatingItem : IValidatableObject + { + public string MemberName { get; set; } + + public IEnumerable Validate(ValidationContext validationContext) + { + yield return new ValidationResult("The item is not valid.", new[] { MemberName }); + } + } + + [Fact] + public void Null_member_name_from_a_class_level_attribute_on_an_item() + { + var (valid, errors) = Run(new Holder> { Value = new List { new ClassLevelItem() } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0]. | The item is not valid."), errors); + } + + [Fact] + public void Null_member_name_from_Validate_on_an_item() + { + var (valid, errors) = Run(new Holder> { Value = new List { new SelfValidatingItem() } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0]. | The item is not valid."), errors); + } + + // In a nested collection the path has the index of each level, then the empty name. + // Nested collections are enumerated from 2.4.0, so 2.3.3 reports nothing here. + [Fact] + public void Null_member_name_on_an_item_of_a_nested_collection() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new List { new ClassLevelItem() } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0]. | The item is not valid."), errors); + } + + // A name that starts with a bracket on an item of a nested collection is still a name. + // The path is the index of each level, a dot, then the name. + [Fact] + public void Member_name_that_starts_with_a_bracket_on_an_item_of_a_nested_collection() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> + { + new List { new SelfValidatingItem { MemberName = "[Totals]" } }, + }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0][0].[Totals] | The item is not valid."), errors); + } + + [Fact] + public void Member_name_that_starts_with_a_bracket_keeps_the_dot() + { + var (valid, errors) = Run(new Holder> + { + Value = new List { new SelfValidatingItem { MemberName = "[Totals]" } }, + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value[0].[Totals] | The item is not valid."), errors); + } + } + /// /// A collection as the object passed to the validator. The validator validates that object /// and walks its properties, but never enumerates it, because an item is only reached From 16d67b9591bde8be4ac50c46a078e6c48b97e95c Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:04:38 -0400 Subject: [PATCH 06/12] fix: do not read member names to join item paths The validator joined the path of an item that is a collection by looking at the start of each member name. A null name threw NullReferenceException, and a name that starts with "[" lost its dot. A private ValidationResult subclass now marks the results whose names start with an item index, so the names the items choose are never inspected. A null name gives "Value[0]." as in 2.3.3. Co-Authored-By: Claude Sonnet 5.5 --- .../RecursiveDataAnnotationValidator.cs | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs index 7f01dc9..7cc3cd9 100644 --- a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs +++ b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs @@ -347,13 +347,17 @@ IDictionary validationContextItems var index = arrayIndex; foreach (var validationResult in nestedResults) { - validationResults.Add( - new ValidationResult( - validationResult.ErrorMessage, - validationResult.MemberNames - .Select(x => ItemMemberName(propertyName, index, x)) - .ToList() - )); + //the member names of an item that is a collection start with the index of its own items + var startsWithIndex = validationResult is ItemOfCollectionResult; + var memberNames = validationResult.MemberNames + .Select(x => propertyName + "[" + index + "]" + (startsWithIndex ? "" : ".") + x) + .ToList(); + + //With no property name, the result is for an item that is a collection: the + //caller puts its own index in front of these names, with no dot. + validationResults.Add(propertyName.Length == 0 + ? new ItemOfCollectionResult(validationResult.ErrorMessage, memberNames) + : new ValidationResult(validationResult.ErrorMessage, memberNames)); } } } @@ -361,12 +365,18 @@ IDictionary validationContextItems return result; } - //The name of a member of an item: Items[1].Name. The member name of an item that is a - //collection starts with its own index, so the indexes are joined without a dot: Items[1][0].Name. - private static string ItemMemberName(string propertyName, int index, string memberName) + //A result whose member names start with the index of an item, such as "[0].Name", and not + //with a property name. It marks the names that the caller must join without a dot. + //The names the items choose are not looked at, so a name that starts with "[", or a null + //name, is joined like any other name. A result of this type never reaches the caller, + //because the object passed to the validator is not enumerated, and every other result + //is rebuilt with a property name in front. + private sealed class ItemOfCollectionResult : ValidationResult { - var separator = memberName.StartsWith("[", StringComparison.Ordinal) ? "" : "."; - return propertyName + "[" + index + "]" + separator + memberName; + public ItemOfCollectionResult(string errorMessage, IEnumerable memberNames) + : base(errorMessage, memberNames) + { + } } /* Note 1: From 8f7aaf430fcc153d231b15bc3353345199acd3d3 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:04:38 -0400 Subject: [PATCH 07/12] docs: list every changed member name and the new limits The BREAKING entry now states the rule: an item that is a collection is enumerated before its properties are walked, so any path through a property of the item changed. List the verified examples. Add the lazy sequence, hang and stack depth notes, and fix the README claim about dictionary values that are structs. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 11 +++++++---- README.md | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 852aa4e..09ca409 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,13 +11,16 @@ Breaking changes are marked **BREAKING**. ### Fixed - Objects inside an item that is itself a collection are now validated. Before, a `List>`, a `List>`, an `object[]` that holds a list, and a list of a sealed or struct collection such as `ImmutableList` passed validation even when an object inside was invalid. The error is reported with the index of each level, for example `Value[0][0].Name`. -- **BREAKING** for these two cases: the member name changed, because the path used to go through a public property of the inner collection. - - An array or an `ArrayList` inside a collection: `Value[0].SyncRoot[0].Name` is now `Value[0][0].Name`. - - A dictionary or a `Hashtable` inside a collection: `Value[0].Values[0].Name` is now `Value[0][0].Value.Name`. +- **BREAKING** for an item that is a collection and has a property that returns its items or an object in them: the member name changed. The validator used to reach such a collection through that property. It now enumerates the items first and reports each at its index. The name of an object in the items is now `Value[0][0].Name`, and each item of a dictionary or a `Hashtable` is a `KeyValuePair` or `DictionaryEntry`. These names changed: + - An array, a multi-dimensional array or an `ArrayList` inside a collection: `Value[0].SyncRoot[0].Name` is now `Value[0][0].Name`. + - A dictionary or a `Hashtable` inside a collection: `Value[0].Values[0].Name` is now `Value[0][0].Value.Name`, and `Value[0].Keys[0].Name` is now `Value[0][0].Key.Name`. + - A `LinkedList`: `Value[0].First.List[1].Name` is now `Value[0][1].Name`. A `SortedSet`: `Value[0].Min.Name` is now `Value[0][0].Name`. + - A collection of your own that exposes its items through a property, such as a `List` subclass with a `View` property: `Value[0].View[0].Name` is now `Value[0][0].Name`. Code that matches these member names must use the new form. A model that passed before can also fail now, if it holds an invalid object in a nested collection. - An item that is a collection is still validated as an object first, so its own attributes and `IValidatableObject.Validate` run, as before. Its items are then validated as well. -- An item that throws when it is enumerated now throws from validation, like a collection that a property holds. A default `ImmutableArray` item is skipped, because it holds nothing and enumerating it throws. +- An item that is a collection is now enumerated, so a lazy sequence in an item runs, as it does when a property holds it. An item that throws when it is enumerated now throws from validation, and an item that never ends makes validation hang. A default `ImmutableArray` item is skipped, because it holds nothing and enumerating it throws. +- A deeply nested collection uses more of the stack. On a 1 MB stack, a chain of `List` that holds the next list passes at 1,100 levels and overflows at 1,200, which ends the process (net8.0, macOS, Release). Before, such a list passed without being enumerated. The validator still has no maximum depth. - A struct collection such as `ImmutableArray` is validated when it is an item, but not yet when a property holds it. That case is planned for the next release. ### Changed diff --git a/README.md b/README.md index 67cf695..6666dc0 100644 --- a/README.md +++ b/README.md @@ -38,12 +38,12 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot ### Collections - 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 its values are reported as `Map[0].Value.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 not walked, because the validator reads only properties of reference types. - An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. - 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 not enumerated. Wrap it in an object with a property. - A collection that is a struct, such as `ImmutableArray`, is validated when it is an item of another collection, but not when a property holds it. The validator only reads properties of reference types. -- The validator does not catch exceptions. If a collection throws when it is enumerated, the exception reaches your code. A default `ImmutableArray` item is skipped, because it holds nothing. +- The validator runs each collection it enumerates, so a lazy sequence, such as a LINQ query or an iterator, runs during validation. A sequence that never ends makes validation hang. The validator does not catch exceptions, so an exception that a collection throws when it is enumerated reaches your code. A default `ImmutableArray` item is skipped, because it holds nothing. ### Framework types that are not walked From 1a66bc9c64cc4b10e7caa30dd66cca738e94e7a1 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:18:20 -0400 Subject: [PATCH 08/12] test: pin that a struct reached by two routes is reported twice The validator remembers objects by reference, and a boxed struct copy is never the same reference. A struct that two properties hold was already reported twice in 2.3.3. Since 2.4.0 an item collection that returns its structs by enumeration and through a property, such as ArraySegment, reports each one twice, and LinkedList three times. Limitation guards, so a later fix is a deliberate change. Co-Authored-By: Claude Sonnet 5.5 --- .../StructItemsReportedTwiceTests.cs | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs new file mode 100644 index 0000000..b687ed3 --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs @@ -0,0 +1,165 @@ +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. + /// - An item that is a collection returns its structs by enumeration and also through a public + /// property, such as ArraySegment<T>.Array. Since 2.4.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 three + /// times, because its First node also reaches the list through two 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; } + } + + 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 2.4.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 node through 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_three_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.ValueRef.Text" + TextRequired, + "Value[0][0].Text" + TextRequired), + errors); + } +#endif + } +} From 11ce579cc978b2c97ce7065fc221b033c0efaffd Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:18:20 -0400 Subject: [PATCH 09/12] fix: skip any struct collection item that equals its default A default ArraySegment, or a struct of your own that wraps a null array, threw when the item was enumerated, where 2.3.3 returned valid. The check for a default ImmutableArray found the type by name and covered only that one. IsDefaultStruct compares the struct to its default value with its own Equals, so it needs no list of types. Tests cover ImmutableArray, ArraySegment and a struct of the caller's own, boxed and unboxed, and the cases that must still be validated: a segment with an array, an empty segment, and a segment of bytes. Co-Authored-By: Claude Sonnet 5.5 --- .../Extensions/TypeExtensions.cs | 25 ++-- .../RecursiveDataAnnotationValidator.cs | 4 +- .../Extensions/TypeExtensionsTests.cs | 37 ++++-- .../NestedCollectionEdgeCaseTests.cs | 122 +++++++++++++++--- .../StructCollectionPropertyTests.cs | 5 +- 5 files changed, 155 insertions(+), 38 deletions(-) diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index 681a484..493c2a9 100644 --- a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs +++ b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs @@ -8,6 +8,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.Serialization; using System.Threading; using RecursiveDataAnnotationsValidation.Attributes; @@ -27,6 +28,12 @@ internal static class TypeExtensions private static readonly ConcurrentDictionary OverridesEqualsCache = new ConcurrentDictionary(); + // A boxed default(T) for each struct type. GetUninitializedObject returns zeroed memory, so + // it is default(T) even for a struct that declares a parameterless constructor, which + // Activator.CreateInstance would run. + private static readonly ConcurrentDictionary DefaultValues = + new ConcurrentDictionary(); + private static readonly ConcurrentDictionary UnsafeToWalkCache = new ConcurrentDictionary(); @@ -153,21 +160,21 @@ public static bool IsUnsafeToWalk(this Type type) } /// - /// True for a default ImmutableArray, such as an array field that nobody set. It wraps null, - /// so enumerating it throws InvalidOperationException, and it holds nothing to validate. - /// The type is found by name, because the library does not reference System.Collections.Immutable. + /// True for a struct that equals its default value, such as an ImmutableArray or an + /// ArraySegment that nobody set, or a struct of your own whose fields are all null or zero. + /// A default struct collection usually holds nothing to validate, and enumerating one throws: + /// InvalidOperationException for these two framework types, NullReferenceException for a + /// struct that wraps an array. The validator skips it, so a model that has an unset struct + /// field does not start to throw. Equality is the struct's own Equals. /// - public static bool IsDefaultImmutableArray(this object obj) + public static bool IsDefaultStruct(this object obj) { var type = obj.GetType(); - return type.IsValueType - && type.IsGenericType - && type.GetGenericTypeDefinition().FullName == "System.Collections.Immutable.ImmutableArray`1" - && (bool)type.GetProperty("IsDefault").GetValue(obj, null); + return type.IsValueType && obj.Equals(DefaultValues.GetOrAdd(type, FormatterServices.GetUninitializedObject)); } - private static bool IsInSystemNamespace(Type type) + private static bool IsInSystemNamespace(Type type) { var ns = type.Namespace; return ns != null && (ns == "System" || ns.StartsWith("System.", StringComparison.Ordinal)); diff --git a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs index 7cc3cd9..06b71dd 100644 --- a/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs +++ b/src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs @@ -220,12 +220,12 @@ private bool TryValidateObjectRecursive( //collection also returns from a property, such as Array.SyncRoot, is then reported at its //index (Value[0][0]) and not through the property (Value[0].SyncRoot[0]). //A collection of leaf types is skipped, like a collection that a property holds. A default - //ImmutableArray is skipped, because enumerating it throws and it holds nothing. + //struct, such as an ImmutableArray nobody set, is skipped: it holds nothing and enumerating it throws. var enumeratedItems = false; if (enumerateItems && obj is IEnumerable items && !type.IsCollectionOfLeafType() - && !obj.IsDefaultImmutableArray()) + && !obj.IsDefaultStruct()) { enumeratedItems = true; if (!TryValidateItems( diff --git a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs index a06dc04..b292b9a 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs @@ -198,29 +198,48 @@ public void Sealed_collection_of_values_is_a_leaf_type() Assert.True(typeof(string).IsLeafType()); } + // A default struct collection holds nothing, and enumerating one throws, so the validator skips it. + // The check is the struct's own Equals against default(T), so it needs no list of types. + [Fact] + public void Default_struct_is_detected() + { + Assert.True(((object)default(ArraySegment)).IsDefaultStruct()); + Assert.True(((object)default(PlainPoint)).IsDefaultStruct()); + Assert.True(((object)default(int)).IsDefaultStruct()); + } + #if NET8_0_OR_GREATER - // The library does not reference System.Collections.Immutable, so it finds the type by name. + // System.Collections.Immutable is not part of net481. [Fact] public void Default_immutable_array_is_detected() { - Assert.True(((object)default(System.Collections.Immutable.ImmutableArray)).IsDefaultImmutableArray()); + Assert.True(((object)default(System.Collections.Immutable.ImmutableArray)).IsDefaultStruct()); } [Fact] public void Immutable_array_that_has_a_value_is_not_detected() { - Assert.False(((object)System.Collections.Immutable.ImmutableArray.Create(new Child())).IsDefaultImmutableArray()); - Assert.False(((object)System.Collections.Immutable.ImmutableArray.Empty).IsDefaultImmutableArray()); + Assert.False(((object)System.Collections.Immutable.ImmutableArray.Create(new Child())).IsDefaultStruct()); + Assert.False(((object)System.Collections.Immutable.ImmutableArray.Empty).IsDefaultStruct()); + } +#endif + + [Fact] + public void Struct_that_has_a_value_is_not_detected() + { + Assert.False(((object)new ArraySegment(new[] { new Child() })).IsDefaultStruct()); + Assert.False(((object)new ArraySegment(new Child[0])).IsDefaultStruct()); + Assert.False(((object)new PlainPoint { X = 1 }).IsDefaultStruct()); + Assert.False(((object)5).IsDefaultStruct()); } [Fact] - public void Other_objects_are_not_a_default_immutable_array() + public void Object_of_a_class_is_not_a_default_struct() { - Assert.False(new List().IsDefaultImmutableArray()); - Assert.False(((object)default(int)).IsDefaultImmutableArray()); - Assert.False("text".IsDefaultImmutableArray()); + Assert.False(new List().IsDefaultStruct()); + Assert.False("text".IsDefaultStruct()); + Assert.False(new object().IsDefaultStruct()); } -#endif // Framework types whose properties throw or never end when walked, and types derived from them. [Theory] diff --git a/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs index 7d0a445..65d8d37 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs @@ -93,29 +93,54 @@ public void Item_that_throws_when_enumerated_propagates() } } -#if NET8_0_OR_GREATER /// - /// Case 2: an item that is a default ImmutableArray. - /// An ImmutableArray is a struct that wraps an array. default(ImmutableArray) wraps null, - /// and enumerating it throws InvalidOperationException. A model gets one by accident: a - /// field that nobody set, a deserializer that skipped a missing JSON property, or a - /// constructor that left it out. It is the common way to meet a collection that throws. + /// Case 2: an item that is a default struct collection. + /// A struct collection that nobody set equals default(T), and enumerating it throws: + /// - default(ImmutableArray) wraps null and throws InvalidOperationException. + /// - default(ArraySegment) has a null Array and throws InvalidOperationException. + /// - A struct of your own that wraps an array throws NullReferenceException. + /// A model gets one by accident: a field that nobody set, a deserializer that skipped a + /// missing JSON property, or a constructor that left it out. /// See: https://learn.microsoft.com/dotnet/api/system.collections.immutable.immutablearray-1.isdefault + /// See: https://learn.microsoft.com/dotnet/api/system.arraysegment-1.array + /// A struct collection of simple values, such as ArraySegment of byte, is never enumerated, + /// so only a collection of objects can throw. /// Reasoning: - /// - A default array holds no objects, so "valid" is the correct answer, and the answer on + /// - A default struct holds no objects, so "valid" is the correct answer, and the answer on /// 2.3.3. A fix that makes it throw turns a passing model into a crash for a reason the /// caller did not cause and cannot see in the model. - /// - The same array held in a property is not walked at all (StructsAreNotWalked), so it + /// - The same struct held in a property is not walked at all (StructsAreNotWalked), so it /// passes today and after the fix. Only an item would throw. That difference is a trap. - /// - The fix can skip a value that is a default ImmutableArray. It checks IsDefault, which - /// needs the type, so this is a special case for one framework type. No other - /// framework collection is known to throw when it is default. Other struct collections are - /// the caller's own types, and Case 1 covers them. - /// Decision: skip a default ImmutableArray, and validate a non-default one. The alternative - /// was Case 1 for this type too. It is less work, and it breaks models. + /// Options that were weighed: + /// - A. Skip a default ImmutableArray only. It leaves ArraySegment and the caller's own + /// structs to throw. + /// - B. Skip any struct item that equals default(T). It covers all of them, and needs no + /// list of types and no reference to System.Collections.Immutable. + /// The cost is a struct whose default value really yields objects. That is skipped, and + /// it was not validated before either. + /// - C. Skip ArraySegment too. That is a list that grows with each framework type. + /// - D. Catch the exception and treat the item as empty. It hides a bug in a collection of + /// the caller's own, and the validator catches nothing else. + /// Decision: B. /// - public class DefaultImmutableArrayItem + public class DefaultStructCollectionItem { + /// A struct collection that wraps an array. Its default value wraps null. + public readonly struct LeafBag : IEnumerable + { + private readonly Leaf[] _items; + + public LeafBag(params Leaf[] items) + { + _items = items; + } + + public IEnumerator GetEnumerator() => ((IEnumerable)_items).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + +#if NET8_0_OR_GREATER [Fact] public void Default_immutable_array_item_is_treated_as_empty() { @@ -149,7 +174,72 @@ public void Default_immutable_array_in_a_property_is_not_walked() Assert.True(valid); Assert.Empty(errors); } - } #endif + + [Fact] + public void Default_array_segment_item_is_treated_as_empty() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { default }, + }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact] + public void Default_array_segment_in_an_object_array_is_treated_as_empty() + { + var (valid, errors) = Run(new Holder { Value = new object[] { default(ArraySegment) } }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [Fact] + public void Default_struct_collection_of_your_own_is_treated_as_empty() + { + var (valid, errors) = Run(new Holder> { Value = new List { default } }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Guard. The segment is not default, so it is enumerated, and the invalid object is found. + [Fact] + public void Array_segment_that_has_an_array_is_validated() + { + 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); + } + + // Guard. An empty segment is not default. It has an array, so it is enumerated and holds nothing. + [Fact] + public void Empty_array_segment_is_valid() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { new ArraySegment(new Leaf[0]) }, + }); + + Assert.True(valid); + Assert.Empty(errors); + } + + // Guard. A segment of simple values is never enumerated, so a default one cannot throw. + [Fact] + public void Default_array_segment_of_bytes_is_valid() + { + var (valid, errors) = Run(new Holder>> { Value = new List> { default } }); + + Assert.True(valid); + Assert.Empty(errors); + } + } } } diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs index 0717f36..0c2e889 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs @@ -39,8 +39,9 @@ namespace RecursiveDataAnnotationsValidation.Tests /// collection, such as a Money, is still skipped. /// - The walk reads the value, which is boxed, and enumerates it like any other collection. /// The path is the same as for a collection that is a class: Value[0].Name. - /// - A default ImmutableArray in a property is skipped, as an item is (see - /// NestedCollectionEdgeCaseTests). It passes today, so enumerating it would add a crash. + /// - A struct in a property that equals its default value, such as a default ImmutableArray or + /// ArraySegment, is skipped, as an item is (see NestedCollectionEdgeCaseTests, Case 2). It + /// passes today, so enumerating it would add a crash. /// - A struct collection that throws when enumerated throws from the property too, like an /// item and like a class. This is a change for the few models that hold one. /// - Check 4 of IsLeafType uses IsWalked, so a struct with such a property is no longer a From 3d8412f9ca74f63a95363e24eb5158c621bcd607 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:18:20 -0400 Subject: [PATCH 10/12] docs: note the struct duplicate and the default struct rule Add the gotcha that a struct reached by two routes is reported twice, with the way out for a collection of the caller's own. Say that any struct item that equals its default is skipped, not only ImmutableArray. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 3 ++- README.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09ca409..94ec1d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,9 @@ Breaking changes are marked **BREAKING**. Code that matches these member names must use the new form. A model that passed before can also fail now, if it holds an invalid object in a nested collection. - An item that is a collection is still validated as an object first, so its own attributes and `IValidatableObject.Validate` run, as before. Its items are then validated as well. -- An item that is a collection is now enumerated, so a lazy sequence in an item runs, as it does when a property holds it. An item that throws when it is enumerated now throws from validation, and an item that never ends makes validation hang. A default `ImmutableArray` item is skipped, because it holds nothing and enumerating it throws. +- An item that is a collection is now enumerated, so a lazy sequence in an item runs, as it does when a property holds it. An item that throws when it is enumerated now throws from validation, and an item that never ends makes validation hang. A struct item that equals its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. - A deeply nested collection uses more of the stack. On a 1 MB stack, a chain of `List` that holds the next list passes at 1,100 levels and overflows at 1,200, which ends the process (net8.0, macOS, Release). Before, such a list passed without being enumerated. The validator still has no maximum depth. +- 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. Objects of a class are reported once. - A struct collection such as `ImmutableArray` is validated when it is an item, but not yet when a property holds it. That case is planned for the next release. ### Changed diff --git a/README.md b/README.md index 6666dc0..9523f84 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,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. +- 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, 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]`. - 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. - Public static properties are walked, as well as instance properties. - 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. An invalid object below it is not reached. @@ -43,7 +44,7 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot - 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 not enumerated. Wrap it in an object with a property. - A collection that is a struct, such as `ImmutableArray`, is validated when it is an item of another collection, but not when a property holds it. The validator only reads properties of reference types. -- The validator runs each collection it enumerates, so a lazy sequence, such as a LINQ query or an iterator, runs during validation. A sequence that never ends makes validation hang. The validator does not catch exceptions, so an exception that a collection throws when it is enumerated reaches your code. A default `ImmutableArray` item is skipped, because it holds nothing. +- The validator runs each collection it enumerates, so a lazy sequence, such as a LINQ query or an iterator, runs during validation. A sequence that never ends makes validation hang. The validator does not catch exceptions, so an exception that a collection throws when it is enumerated reaches your code. A struct item that equals its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. ### Framework types that are not walked From 9620704a2e56fb1bad0097fcfe7877567165dbe2 Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:20:05 -0400 Subject: [PATCH 11/12] docs: group the changelog entries for the nested collection fix Replace the long BREAKING bullet with one BREAKING entry for the changed member names, with one line per shape, and one for enumeration side effects. Move the stack and known-limit notes to plain entries. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94ec1d7..fe5fa49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,23 +10,23 @@ Breaking changes are marked **BREAKING**. ### Fixed -- Objects inside an item that is itself a collection are now validated. Before, a `List>`, a `List>`, an `object[]` that holds a list, and a list of a sealed or struct collection such as `ImmutableList` passed validation even when an object inside was invalid. The error is reported with the index of each level, for example `Value[0][0].Name`. -- **BREAKING** for an item that is a collection and has a property that returns its items or an object in them: the member name changed. The validator used to reach such a collection through that property. It now enumerates the items first and reports each at its index. The name of an object in the items is now `Value[0][0].Name`, and each item of a dictionary or a `Hashtable` is a `KeyValuePair` or `DictionaryEntry`. These names changed: - - An array, a multi-dimensional array or an `ArrayList` inside a collection: `Value[0].SyncRoot[0].Name` is now `Value[0][0].Name`. - - A dictionary or a `Hashtable` inside a collection: `Value[0].Values[0].Name` is now `Value[0][0].Value.Name`, and `Value[0].Keys[0].Name` is now `Value[0][0].Key.Name`. - - A `LinkedList`: `Value[0].First.List[1].Name` is now `Value[0][1].Name`. A `SortedSet`: `Value[0].Min.Name` is now `Value[0][0].Name`. - - A collection of your own that exposes its items through a property, such as a `List` subclass with a `View` property: `Value[0].View[0].Name` is now `Value[0][0].Name`. - - Code that matches these member names must use the new form. A model that passed before can also fail now, if it holds an invalid object in a nested collection. +- Objects inside an item that is itself a collection are now validated. Before, a `List>`, a `List>`, an `object[]` that holds a list, and a list of a sealed or struct collection such as `ImmutableList` passed validation even when an object inside was invalid. The error is reported with the index of each level, for example `Value[0][0].Name`. A model that passed before can fail now. - An item that is a collection is still validated as an object first, so its own attributes and `IValidatableObject.Validate` run, as before. Its items are then validated as well. -- An item that is a collection is now enumerated, so a lazy sequence in an item runs, as it does when a property holds it. An item that throws when it is enumerated now throws from validation, and an item that never ends makes validation hang. A struct item that equals its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. -- A deeply nested collection uses more of the stack. On a 1 MB stack, a chain of `List` that holds the next list passes at 1,100 levels and overflows at 1,200, which ends the process (net8.0, macOS, Release). Before, such a list passed without being enumerated. The validator still has no maximum depth. -- 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. Objects of a class are reported once. -- A struct collection such as `ImmutableArray` is validated when it is an item, but not yet when a property holds it. That case is planned for the next release. +- A struct item that equals its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. ### Changed +- **BREAKING** The member names of an object in an item that is a collection. The validator now enumerates the items first, so the path no longer goes through a property of the item. Change any code that matches these names: + - Array, multi-dimensional array, `ArrayList`: `Value[0].SyncRoot[0].Name` is now `Value[0][0].Name`. + - Dictionary, `Hashtable`: `Value[0].Values[0].Name` is now `Value[0][0].Value.Name`, and `Value[0].Keys[0].Name` is now `Value[0][0].Key.Name`. + - `LinkedList`: `Value[0].First.List[1].Name` is now `Value[0][1].Name`. + - `SortedSet`: `Value[0].Min.Name` is now `Value[0][0].Name`. + - A collection of your own with a property that returns its items, such as `View`: `Value[0].View[0].Name` is now `Value[0][0].Name`. +- **BREAKING** An item that is a collection is now enumerated, so a lazy sequence in an item runs, as it does when a property holds it. An item that throws when enumerated now throws from validation, and an item that never ends makes validation hang. To avoid it, mark the property that holds the collection with `[SkipRecursiveValidation]`. +- A deeply nested collection uses more of the stack. On a 1 MB stack, a chain of `List` that holds the next list passes at 1,100 levels and overflows at 1,200, which ends the process (net8.0, macOS, Release). Before, such a list passed without being enumerated. The validator still has no maximum depth. - The NuGet package title now reads "Recursive DataAnnotations Validation". It was misspelled "Recurisive". +- Known limit: a struct collection such as `ImmutableArray` is validated when it is an item, but not yet when a property holds it. +- 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. Objects of a class are reported once. ## 2.3.3 - 2026-10-01 From 6528d1f91a746c98d5397033ece384d275f0310e Mon Sep 17 00:00:00 2001 From: Thomas Harold Date: Fri, 2 Oct 2026 00:20:18 -0400 Subject: [PATCH 12/12] style: fix the indentation of IsInSystemNamespace Co-Authored-By: Claude Sonnet 5.5 --- .../Extensions/TypeExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index 493c2a9..86376cb 100644 --- a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs +++ b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs @@ -174,7 +174,7 @@ public static bool IsDefaultStruct(this object obj) return type.IsValueType && obj.Equals(DefaultValues.GetOrAdd(type, FormatterServices.GetUninitializedObject)); } - private static bool IsInSystemNamespace(Type type) + private static bool IsInSystemNamespace(Type type) { var ns = type.Namespace; return ns != null && (ns == "System" || ns.StartsWith("System.", StringComparison.Ordinal));