diff --git a/CHANGELOG.md b/CHANGELOG.md index ce6ed30..fe5fa49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,25 @@ 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`. 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. +- 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 diff --git a/README.md b/README.md index cdf4fd5..9523f84 100644 --- a/README.md +++ b/README.md @@ -29,12 +29,23 @@ 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. - 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 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 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 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. diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index f816e4b..86376cb 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; @@ -7,6 +8,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.Serialization; using System.Threading; using RecursiveDataAnnotationsValidation.Attributes; @@ -26,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(); @@ -44,6 +52,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 +85,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 +99,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 +159,21 @@ public static bool IsUnsafeToWalk(this Type type) && UnsafeToWalkTypes.Any(unsafeType => unsafeType.IsAssignableFrom(t))); } + /// + /// 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 IsDefaultStruct(this object obj) + { + var type = obj.GetType(); + + return type.IsValueType && obj.Equals(DefaultValues.GetOrAdd(type, FormatterServices.GetUninitializedObject)); + } + private static bool IsInSystemNamespace(Type type) { var ns = type.Namespace; @@ -148,7 +195,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..06b71dd 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 + //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.IsDefaultStruct()) + { + 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,74 @@ 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) + { + //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)); + } + } + } + + return result; + } + + //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 + { + public ItemOfCollectionResult(string errorMessage, IEnumerable memberNames) + : base(errorMessage, memberNames) + { + } + } + /* 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..b292b9a 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,79 @@ 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()); + } + + // 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 + // System.Collections.Immutable is not part of net481. + [Fact] + public void Default_immutable_array_is_detected() + { + 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())).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 Object_of_a_class_is_not_a_default_struct() + { + Assert.False(new List().IsDefaultStruct()); + Assert.False("text".IsDefaultStruct()); + Assert.False(new object().IsDefaultStruct()); + } + // 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 new file mode 100644 index 0000000..65d8d37 --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/NestedCollectionEdgeCaseTests.cs @@ -0,0 +1,245 @@ +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 +{ + /// + /// 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. 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. 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 + /// 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. + /// Decision: let it propagate. The changelog entry for 2.4.0 says so. + /// + public class ItemThatThrowsWhenEnumerated + { + // Guard. A property that holds this collection throws, and so does an item (below). + [Fact] + public void Property_that_throws_when_enumerated_propagates() + { + var ex = Record.Exception(() => Run(new Holder { Value = new ThrowingBag() })); + + Assert.IsType(ex); + } + + [Fact] + public void Item_that_throws_when_enumerated_propagates() + { + var ex = Record.Exception(() => Run(new Holder> { Value = new List { new ThrowingBag() } })); + + Assert.IsType(ex); + } + } + + /// + /// 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 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 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. + /// 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 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() + { + var (valid, errors) = Run(new Holder>> + { + Value = new List> { default }, + }); + + Assert.True(valid); + Assert.Empty(errors); + } + + [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[]. + 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. + // See StructCollectionPropertyTests for a non-default array held in a property. + [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 + + [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/OddShapeTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs index 37ca83b..6149e53 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,135 @@ 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); + } + } + + /// + /// 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 + /// 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 +615,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 +648,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); + } } /// @@ -156,7 +702,9 @@ public void Items_of_the_collection_are_validated() /// 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..0c2e889 --- /dev/null +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs @@ -0,0 +1,289 @@ +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 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 + /// 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 + } +} 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 + } +}