Updated DefaultPropertyResolver.IsIgnored to return true for a proper…

…ty value that references the owning instance
roryprimrose · Aug 11, 2024 · 83663a1 · 83663a1
1 parent 76ff9eb
commit 83663a1
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 0 deletions.
diff --git a/ModelBuilder.UnitTests/DefaultPropertyResolverTests.cs b/ModelBuilder.UnitTests/DefaultPropertyResolverTests.cs
@@ -1,6 +1,7 @@
 namespace ModelBuilder.UnitTests
 {
     using System;
+    using System.Collections;
     using System.Collections.Generic;
     using System.Collections.ObjectModel;
     using System.Diagnostics.CodeAnalysis;
@@ -870,6 +871,22 @@ public void IsIgnoredReturnsTrueWhenParametersMatchPropertyValueType()
             actual.Should().BeTrue();
         }
 
+        [Fact]
+        public void IsIgnoredReturnsTrueWhenPropertyValueMatchesOwningInstance()
+        {
+            var configuration = Model.UsingDefaultConfiguration();
+            var instance = new Hashtable();
+            var args = Array.Empty<object?>();
+
+            var propertyInfo = instance.GetType().GetProperty(nameof(Hashtable.SyncRoot))!;
+
+            var sut = new DefaultPropertyResolver(CacheLevel.PerInstance);
+
+            var actual = sut.IsIgnored(configuration, instance, propertyInfo, args);
+
+            actual.Should().BeTrue();
+        }
+
         [Fact]
         public void IsIgnoredThrowsExceptionWhenConstructorNotFound()
         {
@@ -931,6 +948,22 @@ public void IsIgnoredThrowsExceptionWithNullPropertyInfo()
             action.Should().Throw<ArgumentNullException>();
         }
 
+        [Fact]
+        public void IsIgnoredTreatsFailedConstructorForDefaultValueDetectionAsNull()
+        {
+            var configuration = Model.UsingDefaultConfiguration();
+            var instance = new PropertyModel<StructConstructorException>();
+            var args = Array.Empty<object?>();
+
+            var propertyInfo = instance.GetType().GetProperty(nameof(PropertyModel<StructConstructorException>.Value))!;
+
+            var sut = new DefaultPropertyResolver(CacheLevel.PerInstance);
+
+            var actual = sut.IsIgnored(configuration, instance, propertyInfo, args);
+
+            actual.Should().BeFalse();
+        }
+
         private class ClassMatchingNameWrapper<T> where T : class
         {
             public ClassMatchingNameWrapper(T value)

diff --git a/ModelBuilder.UnitTests/Models/SelfReferenceInstance.cs b/ModelBuilder.UnitTests/Models/SelfReferenceInstance.cs
@@ -0,0 +1,9 @@
+namespace ModelBuilder.UnitTests.Models
+{
+    internal class SelfReferenceInstance
+    {
+        private SelfReferenceInstance? _value;
+
+        public SelfReferenceInstance Instance { get => _value ?? this; set => _value = value; }
+    }
+}
diff --git a/ModelBuilder.UnitTests/Models/StructConstructorException.cs b/ModelBuilder.UnitTests/Models/StructConstructorException.cs
@@ -0,0 +1,12 @@
+namespace ModelBuilder.UnitTests.Models
+{
+    using System;
+
+    internal struct StructConstructorException
+    {
+        public StructConstructorException()
+        {
+            throw new InvalidOperationException("This is an exception from the constructor.");
+        }
+    }
+}
diff --git a/ModelBuilder.UnitTests/Scenarios/RegressionTests.cs b/ModelBuilder.UnitTests/Scenarios/RegressionTests.cs
@@ -0,0 +1,28 @@
+namespace ModelBuilder.UnitTests.Scenarios
+{
+    using FluentAssertions;
+    using ModelBuilder.UnitTests.Models;
+    using Xunit;
+    using Xunit.Abstractions;
+
+    public class RegressionTests
+    {
+        private readonly ITestOutputHelper _output;
+
+        public RegressionTests(ITestOutputHelper output)
+        {
+            _output = output;
+        }
+
+        [Fact]
+        public void CreateShouldSkipSelfReferencingPropertiesOnPopulateException()
+        {
+            // Fixes https://github.com/roryprimrose/ModelBuilder/issues/347
+            var action = () => Model.WriteLog<SelfReferenceInstance>(_output.WriteLine).Create();
+
+            var actual = action.Should().NotThrow().Subject;
+
+            actual.Instance.Should().BeSameAs(actual);
+        }
+    }
+}
diff --git a/ModelBuilder.sln.DotSettings b/ModelBuilder.sln.DotSettings
@@ -397,6 +397,10 @@
 	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateInstanceFields/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;</s:String>
 	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateStaticFields/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;</s:String>
 	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateStaticReadonly/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;</s:String>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=15b5b1f1_002D457c_002D4ca6_002Db278_002D5615aedc07d3/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static readonly fields (private)"&gt;&lt;ElementKinds&gt;&lt;Kind Name="READONLY_FIELD" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;&lt;/Policy&gt;</s:String>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=4a98fdf6_002D7d98_002D4f5a_002Dafeb_002Dea44ad98c70c/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Instance" AccessRightKinds="Private" Description="Instance fields (private)"&gt;&lt;ElementKinds&gt;&lt;Kind Name="FIELD" /&gt;&lt;Kind Name="READONLY_FIELD" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;&lt;/Policy&gt;</s:String>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=8b8504e3_002Df0be_002D4c14_002D9103_002Dc732f2bddc15/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Enum members"&gt;&lt;ElementKinds&gt;&lt;Kind Name="ENUM_MEMBER" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AaBb" /&gt;&lt;/Policy&gt;</s:String>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=f9fce829_002De6f4_002D4cb2_002D80f1_002D5497c44f51df/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static fields (private)"&gt;&lt;ElementKinds&gt;&lt;Kind Name="FIELD" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /&gt;&lt;/Policy&gt;</s:String>
 	<s:String x:Key="/Default/CodeStyle/Naming/JavaScriptNaming/UserRules/=JS_005FBLOCK_005FSCOPE_005FCONSTANT/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;</s:String>
 	<s:String x:Key="/Default/CodeStyle/Naming/JavaScriptNaming/UserRules/=JS_005FBLOCK_005FSCOPE_005FVARIABLE/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;</s:String>
 	<s:String x:Key="/Default/CodeStyle/Naming/JavaScriptNaming/UserRules/=JS_005FCONSTRUCTOR/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AaBb" /&gt;</s:String>
@@ -438,6 +442,7 @@
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002ECSharpPlaceAttributeOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateThisQualifierSettings/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EPredefinedNamingRulesToUserRulesUpgrade/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002EJavaScript_002ECodeStyle_002ESettingsUpgrade_002EJsParsFormattingSettingsUpgrader/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002EJavaScript_002ECodeStyle_002ESettingsUpgrade_002EJsWrapperSettingsUpgrader/@EntryIndexedValue">True</s:Boolean>
 	<s:String x:Key="/Default/FilterSettingsManager/AttributeFilterXml/@EntryValue">&lt;data /&gt;</s:String>

diff --git a/ModelBuilder/DefaultPropertyResolver.cs b/ModelBuilder/DefaultPropertyResolver.cs
@@ -88,6 +88,14 @@ public virtual bool IsIgnored(
             }
 
             var propertyValue = propertyInfo.GetValue(instance, null);
+
+            if (AreEqual(instance, propertyValue))
+            {
+                // This property is a self reference to the owning instance
+                // For example, Hashtable.SyncRoot is a reference to Hashtable itself
+                return true;
+            }
+
             var defaultValue = GetDefaultValue(propertyInfo.PropertyType);
 
             if (AreEqual(propertyValue, defaultValue))
@@ -202,6 +210,12 @@ private static bool AreEqual(object? first, object? second)
                 return false;
             }
 
+            if (ReferenceEquals(first, second))
+            {
+                // These reference the same instance in memory
+                return true;
+            }
+
             if (first.Equals(second))
             {
                 return true;