Walk public instance properties only, not static ones

CHANGELOG.md

@@ -34,6 +34,7 @@ Breaking changes are marked **BREAKING**.
   - 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]`.
   - An error of the root object keeps its member names, as before.
+- **BREAKING** Static properties are no longer walked. Only public instance properties are, as in `Validator`, MVC model validation and System.Text.Json. Before, a public static property was walked like an instance one, so a shared static object that was invalid failed every model of that type, with an error such as `Shared.Name`. A model that failed only because of a static property passes now. Attributes on a static property never ran, and still do not.
 - **BREAKING** The validator walks the graph breadth first, not depth first. Change any code that depends on the order of the results or on the member name of an object that two paths reach:
   - The results come shallowest first. The errors of one object stay together and in the same order. An item of a collection that a property holds is two levels below the object, so for a model with `First`, `Items` and `Last`, the order was `First`, `Items[0]`, `Items[1]`, `Last` and is now `First`, `Last`, `Items[0]`, `Items[1]`. Sort the list if you need a fixed order.
   - An object that two paths reach is reported with the shortest path. Before, it was the first path in property order. If two paths are equally short, the first property still wins. In a graph with links back to parents, such as the entities of an ORM, the member names are now much shorter. For example, in a store where each order shares a product with the next order, the second order in `Store.Orders` was reported as `Orders[0].Lines[1].Product.Lines[1].Order.Code` when a longer path reached it first, and is now `Orders[1].Code`.

README.md

@@ -35,7 +35,7 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot
 - 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<object>` that holds an `ArraySegment<T>` 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.
+- 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.
 - For a type that does not override `Equals`, mark such a property with `[SkipRecursiveValidation]`. Otherwise the walk goes on until it reaches the maximum depth (see below), and the validation fails.
 - 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.

src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs

@@ -136,6 +136,17 @@ public static bool IsWalked(this PropertyInfo property)
                 && !property.DeclaringType.IsUnsafeToWalk();
         }
 
+        /// <summary>
+        /// The public instance properties of the type that the validator walks (see
+        /// <see cref="IsWalked"/>). A static property is left out: it holds data of the type, not
+        /// of the object, and Validator ignores it too. Type.GetProperties() with no arguments
+        /// would return public static properties as well.
+        /// </summary>
+        public static List<PropertyInfo> GetWalkedProperties(this Type type)
+        {
+            return type.GetProperties(BindingFlags.Public | BindingFlags.Instance).Where(IsWalked).ToList();
+        }
+
         /// <summary>
         /// True when the type's Equals(object) is not object.Equals, so two different objects
         /// of the type can be equal. Records, structs (through ValueType.Equals) and classes
@@ -216,7 +227,7 @@ private static bool FindIsLeafType(Type type)
             return (type.IsValueType || type.IsSealed)
                 && !HasValidationAttributes(type)
                 && !typeof(IValidatableObject).IsAssignableFrom(type)
-                && !type.GetProperties().Any(IsWalked)
+                && type.GetWalkedProperties().Count == 0
                 && (!typeof(IEnumerable).IsAssignableFrom(type) || type.IsCollectionOfLeafType());
         }
 

src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs

@@ -309,7 +309,7 @@ private bool Walk(WorkItem item)
                 //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)
-                var properties = type.GetProperties().Where(prop => prop.IsWalked()).ToList();
+                var properties = type.GetWalkedProperties();
 
                 foreach (var property in properties)
                 {

test/RecursiveDataAnnotationsValidation.Tests/OddShapeTests.cs

@@ -874,8 +874,11 @@ public void Attribute_with_the_property_target_is_validated()
 
         /// <summary>
         /// Members that the validator reads, with effects a model author may not expect.
-        /// - A static property is walked, like an instance one. A shared static object that is
-        ///   invalid fails every model of that type. Type.GetProperties includes static members.
+        /// - A static property is not walked. It holds data of the type, not of the model, and
+        ///   Validator, MVC model validation and System.Text.Json all ignore static properties.
+        ///   Up to 2.3.3 it was walked, because Type.GetProperties() with no arguments returns
+        ///   public static properties too, so a shared static object that was invalid failed
+        ///   every model of that type. Since 3.0 the walk asks for public instance properties only.
         /// - A Lazy&lt;T&gt; property is walked through Value, so validation runs the factory.
         /// - A Task&lt;T&gt; property is walked through Result. A completed task is fine.
         ///   See the class summary for a task that has not completed.
@@ -903,13 +906,38 @@ public class Circle : IShape
                 public int Radius { get; set; }
             }
 
+            public class WithStaticList
+            {
+                public static List<Leaf> All { get; } = new List<Leaf> { new Leaf() };
+            }
+
             [Fact]
-            public void Invalid_static_property_fails_every_instance()
+            public void Invalid_static_property_is_not_walked()
             {
                 var (valid, errors) = Run(new WithStatic());
 
-                Assert.False(valid);
-                Assert.Equal(ResultText.Expect("Shared.Name" + NameRequired), errors);
+                Assert.True(valid);
+                Assert.Empty(errors);
+            }
+
+            [Fact]
+            public void Static_collection_property_is_not_enumerated()
+            {
+                var (valid, errors) = Run(new WithStaticList());
+
+                Assert.True(valid);
+                Assert.Empty(errors);
+            }
+
+            // The same for an item. A type whose only object property is static has nothing to
+            // walk, so its items are not even enumerated.
+            [Fact]
+            public void Static_property_of_an_item_is_not_walked()
+            {
+                var (valid, errors) = Run(new Holder<List<WithStatic>> { Value = new List<WithStatic> { new WithStatic() } });
+
+                Assert.True(valid);
+                Assert.Empty(errors);
             }
 
             [Fact]

test/RecursiveDataAnnotationsValidation.Tests/UpgradeCompatibilityTests.cs

@@ -810,8 +810,9 @@ public void Init_only_and_private_setter_properties_are_walked()
             }
 
             /// <summary>
-            /// Only public instance and static properties are walked. Non-public properties,
-            /// write-only properties and indexers are not.
+            /// Only public instance properties are walked. Non-public properties, write-only
+            /// properties and indexers are not. Up to 2.3.3, public static properties were walked
+            /// too (see OddShapeTests.MembersWithSideEffects).
             /// See: https://learn.microsoft.com/dotnet/api/system.type.getproperties
             /// </summary>
             [Fact]

test/RecursiveDataAnnotationsValidation.Tests/ValidatorHardeningTests.cs

@@ -552,8 +552,10 @@ public record OriginPoint(int X, int Y)
                 public OriginPoint Origin => new OriginPoint(0, 0);
             }
 
-            // The same pattern as a static property. Type.GetProperties() returns public static
-            // properties as well as instance properties, so the validator walks this one too.
+            // The same pattern as a static property. Type.GetProperties() with no arguments returns
+            // public static properties as well as instance properties, so up to 2.3.3 the validator
+            // walked this one too. Since 3.0 it walks instance properties only, so this guard
+            // passes without the Equals check.
             // See: https://learn.microsoft.com/dotnet/api/system.type.getproperties
             public record StaticOriginPoint(int X, int Y)
             {