diff --git a/CHANGELOG.md b/CHANGELOG.md index eb0811b..06bc642 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Breaking changes are marked **BREAKING**. - 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`. - An array or `ArrayList` passed as the root object: `SyncRoot[0].Name` is now `[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. The same holds for a struct collection that a property holds, such as `ImmutableArray`: one that throws when enumerated now throws from validation, and one that builds new objects on each read stops at the maximum depth and fails the validation. To avoid it, mark the property that holds the collection with `[SkipRecursiveValidation]`. A collection passed as the root object is now enumerated too, so a lazy sequence passed in runs during validation. +- **BREAKING** The validator now reads the properties of a struct that a property holds, when the struct has something to validate. A getter of such a struct that throws now throws from validation. To avoid it, mark the property with `[SkipRecursiveValidation]`. The properties of `GCHandle`, of the SqlTypes, such as `SqlString`, which implement `INullable`, and of `ValueTask` are no longer walked, because reading them throws or waits. A null `SqlString` or a `GCHandle` that is not allocated, in a property declared as `object` or in a `List`, no longer throws, and `ValueTask.Result` is no longer read, so an invalid object in the result of a `ValueTask` is not reported. - **BREAKING** An error of a whole nested object now has the path of the object as its member name. This covers a class-level attribute and an `IValidatableObject.Validate` result with no member names, or with a null or empty one. Before, such an error had no member names once nested, so a caller could not tell which object failed, and a null or empty name gave the path with a dot at the end. Change any code that matches these names: - No member names: none is now `Range`, or `Items[1]` for an item. - A null or empty member name: `Value[0].` is now `Value[0]`. @@ -35,15 +36,16 @@ Breaking changes are marked **BREAKING**. - A collection that a property holds is enumerated later than before: after the objects at its own level and above are validated, not when the property is read. A lazy sequence runs later. A change that a `Validate` method makes to the collection is now seen. An item whose `Validate` adds to its own list no longer throws `Collection was modified`. When a collection throws, the exception is the same, but the result list holds different partial results. - The NuGet package title now reads "Recursive DataAnnotations Validation". It was misspelled "Recurisive". - The NuGet package now has a description. It showed "Package Description" before. -- Known limit: an invalid struct that an item collection returns by enumeration and also through one of its properties is reported twice, with two member names, for example `Value[0][0].Text` and `Value[0].Array[0].Text` for an `ArraySegment`. The validator cannot match two copies of a struct by reference. A struct that two properties hold was already reported twice. Objects of a class are reported once. +- Known limit: an invalid struct that an item collection returns by enumeration and also through one of its properties is reported twice, with two member names, for example `Value[0][0].Text` and `Value[0].Array[0].Text` for an `ArraySegment`. The validator cannot match two copies of a struct by reference. A struct that two properties hold was already reported twice. Two properties that return the same struct, such as a property and a computed copy of it, now report it twice too. Objects of a class are reported once. - README: say what `Validator.TryValidateObject` leaves out, show the usage as C#, and tell the `Tuple` classes, which are walked, from value tuples, which are not. ### Fixed -- A struct collection that a property holds, such as `ImmutableArray` or a struct of your own that implements `IEnumerable`, is now validated. Before, the validator skipped every property whose type is a struct, so an invalid object inside passed, although the same struct was validated as an item of a list. The error is reported like one for a class collection, for example `Lines[0].Sku`. A model that passed before can fail now. This includes an item of a list, or the value of a dictionary, that holds such a struct. It was skipped before as having nothing to walk. A struct property that is not a collection, such as a `Money`, is still not walked. +- A struct collection that a property holds, such as `ImmutableArray` or a struct of your own that implements `IEnumerable`, is now validated. Before, the validator skipped every property whose type is a struct, so an invalid object inside passed, although the same struct was validated as an item of a list. The error is reported like one for a class collection, for example `Lines[0].Sku`. A model that passed before can fail now. This includes an item of a list, or the value of a dictionary, that holds such a struct. It was skipped before as having nothing to walk. +- A struct that a property holds is now walked when it has something to validate: a validation attribute on the struct or on one of its properties, `IValidatableObject`, or a property that holds an object, such as the `Value` of a `KeyValuePair`. Before, only a struct collection was walked as a property, so `[Range]` on the `Amount` of a `Money` property, or an invalid object in a `KeyValuePair` property, passed, although the same struct was validated as an item of a list. The error names the path, for example `Price.Amount` or `Pair.Value.Name`. A model that passed before can fail now. A struct with nothing to validate, such as a `DateTime`, is still skipped, and so is a property of a struct that returns its own type, such as `DateTime.Date`. The items of a value tuple are fields, so they are still not walked. - 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`. The item is still validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. A model that passed before can fail now. - A collection that you pass to the validator as the root object, such as a `List`, is now enumerated. Before, its items were not validated, so the list passed even when an item was invalid. The error starts with the index of the item, for example `[1].Name`. The collection is still validated as an object first, so its own attributes run. A model that passed before can fail now. -- A struct item, or a property declared as a struct, that is its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. The validator compares the memory of the struct with its default, and does not call the `Equals` of your struct, which can say "equal" for a struct that holds objects, or throw. +- A struct collection that is its default value, as an item or in a property declared as that struct, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped, because it holds nothing and enumerating it throws. The validator compares the memory of the struct with its default, and does not call the `Equals` of your struct, which can say "equal" for a struct that holds objects, or throw. ## 2.3.3 - 2026-10-01 diff --git a/README.md b/README.md index ce2a926..b1bca82 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot ### Shared objects, cycles and computed properties - Each object is validated once, even when several properties point to it. This also stops cycles, such as a child that points back to its parent. The error names the shortest path to the object (see "Order of results" below). -- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value, so it reports an invalid struct once for each route that reaches it. For example, two properties that hold the same array of structs report each struct twice, and so does a `List` that holds an `ArraySegment` of structs, because the segment returns its items by enumeration and through its `Array` property. The error is not lost, but the result list has a duplicate with a different member name. For a collection type of your own, mark the property that repeats the items with `[SkipRecursiveValidation]`. +- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value, so it reports an invalid struct once for each route that reaches it. For example, two properties that hold the same array of structs report each struct twice, so do two properties that return the same struct, such as a property and a computed copy of it, and so does a `List` that holds an `ArraySegment` of structs, because the segment returns its items by enumeration and through its `Array` property. The error is not lost, but the result list has a duplicate with a different member name. For a collection type of your own, mark the property that repeats the items with `[SkipRecursiveValidation]`. - Objects are compared by reference. Two separate objects that are `Equals` to each other, such as records with the same values or entities with the same `Id`, are each validated. - Only public instance properties are walked. A static property holds data of the type, not of your object, so it is not walked, as `Validator` ignores it too. - A property that builds a new object on each read, such as `public Money Zero => new Money(0)`, could make the walk go on forever. So when an object's type overrides `Equals`, and the object equals an object of the same type, a base type or a derived type on its own path from the root, the validator checks that object's own attributes but does not walk into its properties from there. The check uses the shortest path to the object. A longer path that does not pass an equal object walks into it. An invalid object that every path reaches through an equal ancestor is not reached. This can happen when a shared object, such as a customer, is reached first through an equal ancestor. @@ -59,20 +59,28 @@ An object that several paths reach is validated once, and its errors name the sh ### 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 collection is validated, as it is when a property holds it (see below). A value that is any other struct, such as a `Money`, is not walked, because the validator reads only properties of reference types. +- An item that is itself a collection is validated too, at each level: `Matrix[0][2].Name`. This holds for lists, arrays, sets, dictionaries and your own collection types. A dictionary is enumerated as `KeyValuePair` items, so a value that is an object is reported as `Map[0].Value.Name`. A value that is a struct is walked when it has something to validate, as it is when a property holds it (see "Structs" below). - An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. - A collection of simple values, such as `List` or `string[]`, is not enumerated, because it cannot hold an invalid object. - A collection that you pass to the validator as the root object is validated as an object, then its items are validated. The error starts with the index of the item: `[1].Name`. -- A collection that is a struct, such as `ImmutableArray`, is validated when a property holds it and when it is an item of another collection: `Lines[0].Sku`. The validator walks a property whose type is a class, or a struct that is a collection of objects. A struct that is not a collection, such as a `Money`, a value tuple like `(Child, int)` or a `KeyValuePair` property, is not walked, because the validator only reads properties of reference types. +- A collection that is a struct, such as `ImmutableArray`, is validated when a property holds it and when it is an item of another collection: `Lines[0].Sku`. - 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 collection that is its default value, such as an `ImmutableArray` or an `ArraySegment` that nobody set, is skipped when it is an item or when the property is declared as that struct, because it holds nothing and enumerating it throws. The validator compares the memory of the struct with its default. It does not call the `Equals` of your struct. A property declared as an interface or as `object` is always enumerated. +### Structs + +- A struct that a property holds is walked when it has something to validate: a validation attribute on the struct or on one of its properties, `IValidatableObject`, or a property that holds an object, such as the `Value` of a `KeyValuePair`. The error names the path, as for a class: `Price.Amount`, `Pair.Value.Name`. +- A struct with nothing to validate, such as an `int`, a `Guid` or a `DateTime`, is skipped. +- A struct is validated even when it is its default value. A default `Money` with `[Range(1, 10)]` on its `Amount` fails. Only a struct collection that is its default value is skipped (see "Collections" above). +- A property of a struct that returns its own struct type, such as `DateTime.Date`, is not walked. Each read returns a new copy, so the walk would not end on its own. +- A value tuple such as `(Child, int)` keeps its items in fields, not in properties. `Validator` reads only properties, so the items of a value tuple are not walked. Use a class, a record or a `Tuple` instead. + ### 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. +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`, `Process`, `GCHandle`, the SqlTypes such as `SqlString`, which implement `INullable`, and `ValueTask`. Reading those properties throws, waits or never ends, for example `Uri.Segments` on a relative `Uri`, or `ValueTask.Result` before the task has finished, and they carry no validation attributes. An object of one of these types is still validated. Attributes on the property that holds it, such as `[Required]` on a `Uri` property, still run. A subclass of your own, such as a class derived from `Uri`, is validated too, and the properties it adds are walked. -Other framework types are walked, so your objects inside them are validated. Examples are the `Tuple` classes, such as `Tuple`, the `KeyValuePair` items of a dictionary, and collections. +Other framework types are walked, so your objects inside them are validated. Examples are the `Tuple` classes, such as `Tuple`, `KeyValuePair`, and collections. ## Build Status diff --git a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs index 6f179a3..01b369f 100644 --- a/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs +++ b/src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs @@ -38,7 +38,7 @@ internal static class TypeExtensions private static readonly ConcurrentDictionary UnsafeToWalkCache = new ConcurrentDictionary(); - // Framework types whose properties throw, or never end, when the validator walks them. + // Framework types whose properties throw, wait, or never end, when the validator walks them. // They carry no validation attributes. Only the properties these types and their framework // subclasses declare are skipped, so a user subclass still has its own properties walked. private static readonly Type[] UnsafeToWalkTypes = @@ -51,6 +51,15 @@ internal static class TypeExtensions typeof(FileSystemInfo), // DirectoryInfo.Root returns a new DirectoryInfo on each read typeof(Thread), // CurrentCulture and others throw when read from another thread typeof(Process), // StartInfo and others throw for a process this object did not start + typeof(System.Runtime.InteropServices.GCHandle), // Target throws when the handle is not allocated + typeof(System.Data.SqlTypes.INullable), // a null SqlString and others throw SqlNullValueException + }; + + // Generic framework types for the same list that netstandard2.0 cannot name, matched by the + // full name of the generic type definition. + private static readonly string[] UnsafeToWalkGenericTypeNames = + { + "System.Threading.Tasks.ValueTask`1", // Result waits for a task that has not finished }; // The types IsLeafType is deciding on this thread. A type that yields itself, such as @@ -122,13 +131,15 @@ public static bool IsLeafType(this Type type) /// /// True for a property the validator walks into: readable, not an indexer, not marked - /// [SkipRecursiveValidation], of a reference type other than string or of a struct that is a - /// collection of items that can have attributes, and not declared by a framework type whose - /// properties are unsafe to read (see ). + /// [SkipRecursiveValidation], of a reference type other than string or of a struct that is + /// not a leaf type, and not declared by a framework type whose properties are unsafe to read + /// (see ). A struct property that returns the struct's own type, + /// such as DateTime.Date, is not walked (see ). /// public static bool IsWalked(this PropertyInfo property) { return property.PropertyType != typeof(string) + && !ReturnsItsOwnStructType(property) && IsWalkedType(property.PropertyType) && property.CanRead && property.GetIndexParameters().Length == 0 @@ -160,16 +171,18 @@ public static bool OverridesEquals(this Type type) /// /// True for a framework type whose own properties the validator does not walk, because - /// reading them throws or never ends, such as Uri.Segments on a relative Uri. Framework - /// types derived from one count too, such as RuntimeType, the type of typeof(...). - /// A type outside the System namespaces, such as a user's subclass of Uri, does not count, - /// so the properties it adds are walked. See . + /// reading them throws, waits or never ends, such as Uri.Segments on a relative Uri. Framework + /// types derived from one, or that implement one, count too, such as RuntimeType, the type of + /// typeof(...), and SqlString, which implements INullable. A type outside the System + /// namespaces, such as a user's subclass of Uri, does not count, so the properties it adds are + /// walked. See and . /// public static bool IsUnsafeToWalk(this Type type) { return UnsafeToWalkCache.GetOrAdd(type, t => IsInSystemNamespace(t) - && UnsafeToWalkTypes.Any(unsafeType => unsafeType.IsAssignableFrom(t))); + && (UnsafeToWalkTypes.Any(unsafeType => unsafeType.IsAssignableFrom(t)) + || (t.IsGenericType && UnsafeToWalkGenericTypeNames.Contains(t.GetGenericTypeDefinition().FullName)))); } /// @@ -193,17 +206,30 @@ public static bool IsDefaultStruct(this object obj) && RuntimeHelpers.Equals(obj, DefaultValues.GetOrAdd(type, FormatterServices.GetUninitializedObject)); } - // A property of a reference type is walked. A property of a struct is not, because a struct such - // as a Point or a Money has nothing to walk into. The exception is a struct that is a collection - // of items that can have attributes, such as ImmutableArray of a class, because the same - // struct is enumerated when it is an item of another collection, and a model must not pass - // because the collection sits one level higher. A Nullable is checked as T. + // A property of a reference type is walked. A property of a struct is walked when the struct + // is not a leaf type: it has a validation attribute, implements IValidatableObject, has a + // property the validator walks, such as KeyValuePair.Value, or is a collection of items that + // can have attributes, such as ImmutableArray of a class. The same struct is validated when + // it is an item of a collection, and a model must not pass because the struct sits in a + // property instead. A struct with nothing to validate, such as an int or a DateTime, is a leaf + // type and is skipped. A Nullable is checked as T. private static bool IsWalkedType(Type type) { type = Nullable.GetUnderlyingType(type) ?? type; - return !type.IsValueType - || (typeof(IEnumerable).IsAssignableFrom(type) && !type.IsCollectionOfLeafType()); + return !type.IsValueType || !type.IsLeafType(); + } + + // True for a property of a struct that returns that same struct type, or a Nullable of it, + // such as DateTime.Date. Each read returns a new copy, which the walk cannot match by + // reference, so walking it would not end on its own. It also keeps a struct such as DateTime + // a leaf type: deciding that would otherwise ask about DateTime while DateTime is being + // decided, and that guard answers "not a leaf type" (see IsLeafType). + private static bool ReturnsItsOwnStructType(PropertyInfo property) + { + var type = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType; + + return type.IsValueType && type == property.DeclaringType; } private static bool IsInSystemNamespace(Type type) diff --git a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs index c6c77a2..57a494f 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs @@ -16,7 +16,7 @@ namespace RecursiveDataAnnotationsValidation.Tests.Extensions /// 3. It does not implement IValidatableObject. Validator calls Validate() on any item /// that implements it. /// 4. No property the validator would walk into: a readable, non-indexer property of a - /// reference type other than string, or of a struct that is a collection of objects. + /// reference type other than string, or of a struct that is not a leaf type itself. /// 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 @@ -207,11 +207,22 @@ public class StructBagHolder public PlainPoint Point { get; set; } public KeyValuePair Pair { get; set; } + + public PointWithChild WithChild { get; set; } + + public SelfValidatingPoint SelfValidating { get; set; } + + public SelfValidatingPoint? MaybeSelfValidating { get; set; } + + public DateTime When { get; set; } + + public DateTime? MaybeWhen { get; set; } } - // A property of a struct is walked only when the struct is a collection of items that can have - // attributes. A struct that is not a collection is not, and neither is a KeyValuePair, which - // is not an IEnumerable. + // A property of a struct is walked when the struct is not a leaf type: it is a collection of + // items that can have attributes, or it has something to validate itself, such as an + // IValidatableObject or a property that the validator walks. A KeyValuePair is not an + // IEnumerable, but its Value can hold an object. [Fact] public void Struct_collection_property_is_walked() { @@ -220,10 +231,34 @@ public void Struct_collection_property_is_walked() } [Fact] - public void Struct_property_that_is_not_a_collection_is_not_walked() + public void Struct_property_with_something_to_validate_is_walked() + { + Assert.True(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.Pair)).IsWalked()); + Assert.True(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.WithChild)).IsWalked()); + Assert.True(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.SelfValidating)).IsWalked()); + Assert.True(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.MaybeSelfValidating)).IsWalked()); + } + + [Fact] + public void Struct_property_of_a_leaf_type_is_not_walked() { Assert.False(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.Point)).IsWalked()); - Assert.False(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.Pair)).IsWalked()); + Assert.False(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.When)).IsWalked()); + Assert.False(typeof(StructBagHolder).GetProperty(nameof(StructBagHolder.MaybeWhen)).IsWalked()); + } + + // DateTime.Date returns a DateTime. A property that returns its own struct type is not walked, + // so DateTime has no walked property and stays a leaf type. Without that rule, deciding + // whether DateTime is a leaf type would ask about DateTime again, and the guard against + // that answers "not a leaf type". Every DateTime property would then be walked. + // See: https://learn.microsoft.com/dotnet/api/system.datetime.date + [Fact] + public void Struct_property_of_its_own_type_is_not_walked() + { + Assert.False(typeof(DateTime).GetProperty(nameof(DateTime.Date)).IsWalked()); + Assert.True(typeof(DateTime).IsLeafType()); + Assert.True(typeof(DateTimeOffset).IsLeafType()); + Assert.True(typeof(TimeSpan).IsLeafType()); } // Check 4 of IsLeafType uses IsWalked. A type whose only walked member is a struct collection @@ -325,7 +360,12 @@ public void Object_of_a_class_is_not_a_default_struct() Assert.False(new object().IsDefaultStruct()); } - // Framework types whose properties throw or never end when walked, and types derived from them. + // Framework types whose properties throw, block or never end when walked, and types derived + // from them. A null SqlString throws SqlNullValueException from CompareInfo, a GCHandle that + // is not allocated throws from Target, and ValueTask.Result waits for a task that has not + // finished. Each SqlTypes type implements INullable, so INullable covers all of them. + // See: https://learn.microsoft.com/dotnet/api/system.data.sqltypes.inullable + // See: https://learn.microsoft.com/dotnet/api/system.threading.tasks.valuetask-1.result [Theory] [InlineData(typeof(Type))] [InlineData(typeof(System.Reflection.MethodInfo))] @@ -336,11 +376,47 @@ public void Object_of_a_class_is_not_a_default_struct() [InlineData(typeof(Uri))] [InlineData(typeof(System.IO.DirectoryInfo))] [InlineData(typeof(System.IO.FileInfo))] + [InlineData(typeof(System.Runtime.InteropServices.GCHandle))] + [InlineData(typeof(System.Data.SqlTypes.SqlString))] + [InlineData(typeof(System.Data.SqlTypes.SqlDecimal))] + [InlineData(typeof(System.Data.SqlTypes.SqlBytes))] +#if NET8_0_OR_GREATER + // ValueTask is not part of .NET Framework 4.8.1 without a package. + [InlineData(typeof(System.Threading.Tasks.ValueTask))] +#endif public void Unsafe_to_walk_type_is_detected(Type type) { Assert.True(type.IsUnsafeToWalk()); } + // A denied struct has no property left to walk, so it is a leaf type, and a property of + // that type is skipped. + [Fact] + public void Denied_struct_is_a_leaf_type() + { + Assert.True(typeof(System.Runtime.InteropServices.GCHandle).IsLeafType()); + Assert.True(typeof(System.Data.SqlTypes.SqlString).IsLeafType()); +#if NET8_0_OR_GREATER + Assert.True(typeof(System.Threading.Tasks.ValueTask).IsLeafType()); +#endif + } + + // INullable is denied only for framework types. A type of your own that implements it is + // walked like any other. + public class UserNullable : System.Data.SqlTypes.INullable + { + public bool IsNull => false; + + public Child Owner { get; set; } + } + + [Fact] + public void User_type_that_implements_INullable_is_not_detected() + { + Assert.False(typeof(UserNullable).IsUnsafeToWalk()); + Assert.True(typeof(UserNullable).GetProperty(nameof(UserNullable.Owner)).IsWalked()); + } + [Fact] public void Runtime_type_object_is_unsafe_to_walk() { @@ -358,6 +434,7 @@ public void Runtime_type_object_is_unsafe_to_walk() [InlineData(typeof(System.IO.Stream))] [InlineData(typeof(Exception))] [InlineData(typeof(System.Threading.Tasks.Task))] + [InlineData(typeof(System.Threading.CancellationToken))] public void Walkable_type_is_not_detected(Type type) { Assert.False(type.IsUnsafeToWalk()); diff --git a/test/RecursiveDataAnnotationsValidation.Tests/FrameworkTypeWalkTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/FrameworkTypeWalkTests.cs index 6077e40..6b51915 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/FrameworkTypeWalkTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/FrameworkTypeWalkTests.cs @@ -86,6 +86,13 @@ public class ItemHolder public List Items { get; set; } } + public class TypedHolder + { + public T Value { get; set; } + + public Leaf Sibling { get; set; } = new Leaf(); + } + // Runs the validator on its own thread, so a hang fails the test instead of the whole run. // The thread is a background thread, so a hung walk does not keep the test host alive. // A thread in the pool would also work, but a hung one would stay in the pool for good. @@ -312,6 +319,97 @@ public void Process_in_a_list_is_walked_without_error() } } + /// + /// Framework types that 3.0 adds to the deny list, because 3.0 walks a struct property that + /// has something to validate, and these structs have a property of a reference type. + /// Reading that property throws or waits: + /// - A null SqlString or SqlDecimal throws SqlNullValueException from CompareInfo or Data. + /// SqlBytes is a class with the same problem. All SqlTypes types implement INullable. + /// - A GCHandle that is not allocated throws InvalidOperationException from Target. + /// - ValueTask<T>.Result waits for a task that has not finished. When the ValueTask + /// wraps an IValueTaskSource, Result asks the source for its result. A pooled source can + /// then be reused, so a second read gets the result of another operation. + /// Each type was already walked in a property declared as object and as an item of a + /// List<object>, so those shapes threw, or read Result, before 3.0 too. These tests + /// fail against 2.3.3. + /// See: https://learn.microsoft.com/dotnet/api/system.data.sqltypes.inullable + /// See: https://learn.microsoft.com/dotnet/api/system.runtime.interopservices.gchandle.target + /// See: https://learn.microsoft.com/dotnet/api/system.threading.tasks.valuetask-1 + /// + public class FixedSince30 + { + private static void AssertWalkedWithoutErrorInEveryShape(Func create) + { + AssertWalkedWithoutError(() => create()); + + var (valid, errors) = Run(new TypedHolder { Value = create() }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Sibling.Name | Name is required"), errors); + } + + [Fact] + public void Null_SqlString_is_walked_without_error() + { + AssertWalkedWithoutErrorInEveryShape(() => System.Data.SqlTypes.SqlString.Null); + } + + [Fact] + public void Null_SqlDecimal_is_walked_without_error() + { + AssertWalkedWithoutErrorInEveryShape(() => System.Data.SqlTypes.SqlDecimal.Null); + } + + [Fact] + public void Null_SqlBytes_is_walked_without_error() + { + AssertWalkedWithoutErrorInEveryShape(() => System.Data.SqlTypes.SqlBytes.Null); + } + + [Fact] + public void GCHandle_that_is_not_allocated_is_walked_without_error() + { + AssertWalkedWithoutErrorInEveryShape(() => default(System.Runtime.InteropServices.GCHandle)); + } + +#if NET8_0_OR_GREATER + // A source that never finishes and counts the reads of its result. A ValueTask over a + // Task that never finishes would make Result wait, and hang the run. + // See: https://learn.microsoft.com/dotnet/api/system.threading.tasks.sources.ivaluetasksource-1 + private sealed class PendingSource : System.Threading.Tasks.Sources.IValueTaskSource + { + public int ResultReads; + + public Leaf GetResult(short token) + { + ResultReads++; + return new Leaf(); + } + + public System.Threading.Tasks.Sources.ValueTaskSourceStatus GetStatus(short token) => + System.Threading.Tasks.Sources.ValueTaskSourceStatus.Pending; + + public void OnCompleted( + Action continuation, + object state, + short token, + System.Threading.Tasks.Sources.ValueTaskSourceOnCompletedFlags flags) + { + } + } + + [Fact] + public void Result_of_a_ValueTask_is_not_read() + { + var source = new PendingSource(); + + AssertWalkedWithoutErrorInEveryShape(() => new ValueTask(source, 0)); + + Assert.Equal(0, source.ResultReads); + } +#endif + } + /// /// Limitation guards for a Task whose result is a reference type. The walk reads /// Task<T>.Result, and Result throws for a task that faulted or was canceled. The diff --git a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs index 0e6fa11..b2401b3 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs @@ -18,9 +18,11 @@ namespace RecursiveDataAnnotationsValidation.Tests /// /// Object shapes that are unusual, made up or taken from real framework types, to see what the /// validator does with them. The rule that the validator walks into every public property of a - /// reference type, and enumerates every IEnumerable it finds in one, explains all results. + /// reference type, or of a struct that has something to validate, and enumerates every + /// IEnumerable it finds in one, explains all results. /// A property is walked when it is readable, not an indexer, not marked - /// [SkipRecursiveValidation], and of a reference type other than string. See IsWalked. + /// [SkipRecursiveValidation], and of a reference type other than string, or of a struct that is + /// not a leaf type. See IsWalked. /// Each test is one of: /// - A guard. It passes today and keeps a behavior that callers may rely on. /// - A limitation guard. It passes today and shows a shape that is silently not validated. @@ -28,8 +30,9 @@ namespace RecursiveDataAnnotationsValidation.Tests /// - 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, and the collections in - /// CollectionsInsideCollections and CollectionAsRootObject, which 3.0 enumerates. + /// framework types in MembersThatThrow, which are no longer walked, the collections in + /// CollectionsInsideCollections and CollectionAsRootObject, which 3.0 enumerates, and the + /// struct properties in StructProperties, which 3.0 walks. /// 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. @@ -78,8 +81,7 @@ private static (bool Valid, List Errors) Run(object model) /// 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. + /// IEnumerable, is enumerated like any other, so an item that hides objects does not pass. /// See: https://learn.microsoft.com/dotnet/api/system.array.syncroot /// public class CollectionsInsideCollections @@ -302,15 +304,15 @@ public void List_of_immutable_lists_is_validated() } #endif - // StructsAreNotWalked.LeafBag is a readonly struct that implements IEnumerable of Leaf. + // StructProperties.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> + var (valid, errors) = Run(new Holder> { - Value = new List { new StructsAreNotWalked.LeafBag(new Leaf()) }, + Value = new List { new StructProperties.LeafBag(new Leaf()) }, }); Assert.False(valid); @@ -322,7 +324,7 @@ public void Object_array_holding_a_struct_collection_is_validated() { var (valid, errors) = Run(new Holder { - Value = new object[] { new StructsAreNotWalked.LeafBag(new Leaf()) }, + Value = new object[] { new StructProperties.LeafBag(new Leaf()) }, }); Assert.False(valid); @@ -767,17 +769,24 @@ public void Validate_on_a_collection_item_runs() } /// - /// Structs. The validator walks into properties of reference types only, so a struct - /// 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. The exception is a struct that is a collection, - /// such as ImmutableArray<T>: its items are validated, as an item of another collection - /// and as a property (see 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. + /// Structs. A struct property is walked when the struct has something to validate: a + /// validation attribute on the struct or on one of its properties, IValidatableObject, or a + /// property that the validator walks, such as the Value of a KeyValuePair<string, Leaf>. + /// A struct with nothing to validate, such as an int, a Guid or a DateTime, is a leaf type + /// and is skipped (see IsLeafType). Up to 2.3.3, only a struct that is a collection was + /// walked as a property, so an invalid struct in a property passed, although the same + /// struct was validated as an item of a list. + /// A struct is copied each time it is read, and the walk boxes the copy, so the walk cannot + /// tell that two reads give the same struct. A property that returns its own struct type, + /// such as DateTime.Date, is therefore not walked: each read is a new struct, and the walk + /// would not end on its own. + /// A value tuple such as (Leaf, int) keeps its items in the fields Item1 and Item2, not in + /// properties. Validator reads properties only, so the items of a value tuple are not walked. /// See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/struct + /// See: https://learn.microsoft.com/dotnet/csharp/programming-guide/types/boxing-and-unboxing + /// See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/value-tuples /// - public class StructsAreNotWalked + public class StructProperties { public struct Money { @@ -801,28 +810,115 @@ public LeafBag(params Leaf[] items) IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); } + // No attribute of its own. It holds an object of a class. + public struct Wrapper + { + public Leaf Inner { get; set; } + } + + // Next returns its own struct type, as DateTime.Date does. Each read is a new struct + // with a Count that is out of range. + public struct Counter + { + [Range(1, 10)] + public int Count { get; set; } + + public Counter Next => new Counter { Count = Count + 100 }; + } + + private const string AmountOutOfRange = " | The field Amount must be between 1 and 10."; + + // Spec. Up to 2.3.3 this passed. [Fact] - public void Struct_property_with_an_attribute_is_not_validated() + public void Struct_property_with_an_attribute_is_validated() { var (valid, errors) = Run(new Holder { Value = new Money { Amount = 99 } }); - Assert.True(valid); - Assert.Empty(errors); + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Amount" + AmountOutOfRange), errors); } + // Spec. A Nullable that has a value is boxed as a Money, so it is walked the same way. + // See: https://learn.microsoft.com/dotnet/csharp/language-reference/builtin-types/nullable-value-types#boxing-and-unboxing [Fact] - public void Nullable_struct_property_with_an_attribute_is_not_validated() + public void Nullable_struct_property_with_an_attribute_is_validated() { var (valid, errors) = Run(new Holder { Value = new Money { Amount = 99 } }); + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Amount" + AmountOutOfRange), errors); + } + + // Spec. Record structs with positional `[property: ...]` attributes are a modern way to + // model a value. + [Fact] + public void Record_struct_property_with_an_attribute_is_validated() + { + var (valid, errors) = Run(new Holder { Value = new Coordinates(200) }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Latitude | The field Latitude must be between -90 and 90."), errors); + } + + // Spec. A struct is not skipped because it is its default value, unlike a struct + // collection that nobody set. A default Money has an Amount of 0, which is out of range. + [Fact] + public void Default_struct_property_is_validated() + { + var (valid, errors) = Run(new Holder()); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Amount" + AmountOutOfRange), errors); + } + + // Spec. A dictionary item is a KeyValuePair and was already walked. A KeyValuePair + // property now is too. + [Fact] + public void KeyValuePair_property_is_walked() + { + var (valid, errors) = Run(new Holder> + { + Value = new KeyValuePair("a", new Leaf()) + }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Value.Name" + NameRequired), errors); + } + + // Spec. + [Fact] + public void Struct_that_holds_an_object_is_walked() + { + var (valid, errors) = Run(new Holder { Value = new Wrapper { Inner = new Leaf() } }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Value.Inner.Name" + NameRequired), errors); + } + + // Guard. + [Fact] + public void Valid_struct_properties_pass() + { + Assert.True(Run(new Holder { Value = new Money { Amount = 5 } }).Valid); + Assert.True(Run(new Holder { Value = new Wrapper { Inner = new Leaf { Name = "ok" } } }).Valid); + Assert.True(Run(new Holder()).Valid); + } + + // Guard. Next is not walked, so its Count of 101 is not reported. Count itself is checked. + [Fact] + public void Struct_property_of_its_own_type_is_not_walked() + { + var (valid, errors) = Run(new Holder { Value = new Counter { Count = 1 } }); + Assert.True(valid); Assert.Empty(errors); } + // Limitation guard. Item1 is a field, so the invalid Leaf in it is not reached. [Fact] - public void Record_struct_property_with_an_attribute_is_not_validated() + public void Value_tuple_items_are_not_walked() { - var (valid, errors) = Run(new Holder { Value = new Coordinates(200) }); + var (valid, errors) = Run(new Holder<(Leaf, int)> { Value = (new Leaf(), 1) }); Assert.True(valid); Assert.Empty(errors); diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs index 71728a5..ec9ef8a 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs @@ -26,8 +26,8 @@ namespace RecursiveDataAnnotationsValidation.Tests /// that is not a collection of leaf types, and a Nullable of such a struct. 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 that is not a collection, such as a Money, is still skipped. An attribute on - /// its members is still not checked (see OddShapeTests.StructsAreNotWalked). + /// - A struct that is not a collection, such as a Money, is walked when it has something to + /// validate, and skipped when it is a leaf type (see OddShapeTests.StructProperties). /// - A struct in a property that is its default value, such as a default ImmutableArray or /// ArraySegment, is skipped, as an item is (see NestedCollectionEdgeCaseTests, Case 2). It /// passed before, and enumerating it would add a crash. The check compares memory, and does diff --git a/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs index 838a645..4d3c196 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/StructItemsReportedTwiceTests.cs @@ -19,11 +19,14 @@ namespace RecursiveDataAnnotationsValidation.Tests /// /// Where it happens. /// - Two properties hold the same array of structs. This is so in 2.3.3 too. + /// - Two properties return the same struct, such as a property and a computed copy of it. + /// Since 3.0 the validator walks a struct property that has something to validate. /// - An item that is a collection returns its structs by enumeration and also through a public /// property, such as ArraySegment<T>.Array. Since 3.0 the validator enumerates an item that /// is a collection and also walks its properties, so such an item reports each struct twice. - /// Before, only the property route found them. A LinkedList<T> item reports each struct three - /// times, because its First node also reaches the list through two properties. + /// Before, only the property route found them. A LinkedList<T> item reports each struct four + /// times, because its First node also reaches the list through its List property, and the + /// struct through its Value and ValueRef properties. /// See: https://learn.microsoft.com/dotnet/api/system.arraysegment-1.array /// /// What it is not. The invalid struct is reported, so no model passes that failed before. The @@ -68,6 +71,13 @@ public class TwoLists public List Second { get; set; } } + public class LineAndCopy + { + public Line Line { get; set; } + + public Line Copy => Line; + } + private const string TextRequired = " | The Text field is required."; private static (bool Valid, List Errors) Run(object model) @@ -102,6 +112,17 @@ public void Struct_reached_by_two_properties_is_reported_twice() Assert.Equal(ResultText.Expect("First[0].Text" + TextRequired, "Second[0].Text" + TextRequired), errors); } + // New in 3.0. 2.3.3 did not walk a struct property, so it reported nothing. Each read of + // a struct property returns a copy, so Line and Copy are two structs to the validator. + [Fact] + public void Struct_returned_by_two_properties_is_reported_twice() + { + var (valid, errors) = Run(new LineAndCopy { Line = new Line() }); + + Assert.False(valid); + Assert.Equal(ResultText.Expect("Line.Text" + TextRequired, "Copy.Text" + TextRequired), errors); + } + // New in 3.0. 2.3.3 reports only the first path, through the Array property. [Fact] public void Struct_in_an_array_segment_item_is_reported_twice() @@ -143,10 +164,10 @@ public void Class_objects_in_an_array_segment_item_are_reported_once() #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. + // First reaches the list through its List property, and the struct through Value and ValueRef. // See: https://learn.microsoft.com/dotnet/api/system.collections.generic.linkedlistnode-1.valueref [Fact] - public void Struct_in_a_linked_list_item_is_reported_three_times() + public void Struct_in_a_linked_list_item_is_reported_four_times() { var list = new LinkedList(new[] { new Line() }); @@ -156,6 +177,7 @@ public void Struct_in_a_linked_list_item_is_reported_three_times() Assert.Equal( ResultText.Expect( "Value[0].First.List[0].Text" + TextRequired, + "Value[0].First.Value.Text" + TextRequired, "Value[0].First.ValueRef.Text" + TextRequired, "Value[0][0].Text" + TextRequired), errors); diff --git a/test/RecursiveDataAnnotationsValidation.Tests/UpgradeCompatibilityTests.cs b/test/RecursiveDataAnnotationsValidation.Tests/UpgradeCompatibilityTests.cs index 1640923..3a59388 100644 --- a/test/RecursiveDataAnnotationsValidation.Tests/UpgradeCompatibilityTests.cs +++ b/test/RecursiveDataAnnotationsValidation.Tests/UpgradeCompatibilityTests.cs @@ -32,6 +32,7 @@ namespace RecursiveDataAnnotationsValidation.Tests /// - A null object or context, and a property hidden with `new` (fixed in 2.2.4). /// - The order of the results, and the path of an object that two routes reach, which 3.0 /// changed with its breadth-first walk (BreadthFirstWalkTests). + /// - A struct held in a property, which 3.0 walks (OddShapeTests.StructProperties). /// /// The messages are the framework's default English messages, or a custom ErrorMessage where /// .NET Framework and .NET might word a default differently. @@ -1044,7 +1045,8 @@ public void Collections_of_types_with_nothing_to_validate_pass() /// /// A struct in a collection is boxed and validated like any object, so its property - /// attributes run. A struct that is not a collection, held directly in a property, is not walked. + /// attributes run. A struct held directly in a property is walked by 3.0 only, so it is + /// not here (see OddShapeTests.StructProperties). /// See: https://learn.microsoft.com/dotnet/csharp/programming-guide/types/boxing-and-unboxing /// public struct CheckedPoint @@ -1056,8 +1058,6 @@ public struct CheckedPoint public class CheckedPoints { public List Points { get; set; } - - public CheckedPoint Single { get; set; } } [Fact] @@ -1066,7 +1066,6 @@ public void Struct_items_with_attributes_are_validated() var points = new CheckedPoints { Points = new List { new CheckedPoint { X = 1 }, new CheckedPoint { X = 11 } }, - Single = new CheckedPoint { X = 99 }, }; Assert.False(Validate(points, out var results));