From 1516844d009123d6e2c91898022e7eca345026a2 Mon Sep 17 00:00:00 2001 From: Andrew Best Date: Tue, 21 Jan 2020 15:06:04 +1030 Subject: [PATCH] Ensures property-based conventions only inspect declared properties. (#69) This behaviour change was forced by the dotnet type system. If you have a type, and you want to specify that it's properties have private setters using PropertiesMustHavePrivateSetters, and your type inherits a base type, the type system provides no access or visibility to the setter on the base class regardless of whether it is private set or has no setter Since we cannot determine this, we cannot enforce this convention on the base type when inspecting via the derived type The better approach is to only inspect declared properties If you want to inspect the base type, ensure it is included in the set of types under inspection --- .../ConventionSpecificationTests.cs | 220 +++++++++++++++++- ...stNotHaveSettersConventionSpecification.cs | 3 +- ...ustBeEagerLoadedConventionSpecification.cs | 3 +- ...eAPropertyOfTypeConventionSpecification.cs | 5 +- ...vePrivateSettersConventionSpecification.cs | 3 +- ...ProtectedSettersConventionSpecification.cs | 3 +- ...avePublicGettersConventionSpecification.cs | 3 +- ...avePublicSettersConventionSpecification.cs | 3 +- ...stNotHaveSettersConventionSpecification.cs | 3 +- ...ustHaveAttributeConventionSpecification.cs | 3 +- .../Conventional/Extensions/TypeExtensions.cs | 6 + 11 files changed, 238 insertions(+), 17 deletions(-) diff --git a/src/Core/Conventional.Tests/Conventional/Conventions/ConventionSpecificationTests.cs b/src/Core/Conventional.Tests/Conventional/Conventions/ConventionSpecificationTests.cs index 9045227..65e1ecf 100644 --- a/src/Core/Conventional.Tests/Conventional/Conventions/ConventionSpecificationTests.cs +++ b/src/Core/Conventional.Tests/Conventional/Conventions/ConventionSpecificationTests.cs @@ -58,6 +58,26 @@ public void PropertiesMustHavePublicGetters_Success() .BeTrue(); } + private class AllPublicGetterMockBase + { + public string Private { private get; set; } + } + + private class AllPublicGetterMockDerived : AllPublicGetterMockBase + { + public string Public { get; set; } + } + + [Test] + public void PropertiesMustHavePublicGetters_IgnoresInheritedProperties() + { + typeof(AllPublicGetterMockDerived) + .MustConformTo(Convention.PropertiesMustHavePublicGetters) + .IsSatisfied + .Should() + .BeTrue(); + } + private class PrivateGetterMock { public string PrivateGet { private get; set; } @@ -88,6 +108,26 @@ public void PropertiesMustHavePublicSetters_Success() .BeTrue(); } + private class AllPublicSetterMockBase + { + public string Private { get; private set; } + } + + private class AllPublicSetterMockDerived : AllPublicSetterMockBase + { + public string Public { get; set; } + } + + [Test] + public void PropertiesMustHavePublicSetters_IgnoresInheritedProperties() + { + typeof(AllPublicSetterMockDerived) + .MustConformTo(Convention.PropertiesMustHavePublicSetters) + .IsSatisfied + .Should() + .BeTrue(); + } + private class AllPrivateSetterMock { public string PrivateSet { get; private set; } @@ -134,6 +174,26 @@ public void PropertiesMustHaveProtectedSetters_Success() .BeTrue(); } + private class AllProtectedSetterMockBase + { + public string Private { get; private set; } + } + + private class AllProtectedSetterMockDerived : AllProtectedSetterMockBase + { + public string Public { get; protected set; } + } + + [Test] + public void PropertiesMustHaveProtectedSetters_IgnoresInheritedProperties() + { + typeof(AllProtectedSetterMockDerived) + .MustConformTo(Convention.PropertiesMustHaveProtectedSetters) + .IsSatisfied + .Should() + .BeTrue(); + } + [Test] public void PropertiesMustHaveProtectedSetters_FailsWhenOtherSetterExists() { @@ -164,6 +224,26 @@ public void PropertiesMustHavePrivateSetters_Success() .BeTrue(); } + private class AllPrivateSetterMockBase + { + public string Public { get; set; } + } + + private class AllPrivateSetterMockDerived : AllPrivateSetterMockBase + { + public string Private { get; private set; } + } + + [Test] + public void PropertiesMustHavePrivateSetters_IgnoresInheritedProperties() + { + typeof(AllPrivateSetterMockDerived) + .MustConformTo(Convention.PropertiesMustHavePrivateSetters) + .IsSatisfied + .Should() + .BeTrue(); + } + [Test] public void PropertiesMustHavePrivateSetters_FailsWhenOtherSetterExists() { @@ -484,6 +564,17 @@ private class SomeGenericInterfaceImplementation : ISomeGeneric Descriptions { get; set; } + } + + private class HasEagerLoadedEnumerablesDerived : HasLazilyLoadedEnumerablesBase + { + public string[] Names { get; set; } + } + + [Test] + public void EnumerablePropertiesMustBeEagerLoadedConventionSpecification_IgnoresInheritedProperties() + { + typeof(HasEagerLoadedEnumerablesDerived) + .MustConformTo(Convention.EnumerablePropertiesMustBeEagerLoaded) + .IsSatisfied + .Should() + .BeTrue(); + } + private class HasLazilyLoadedEnumerables { public IEnumerable Names { get; set; } @@ -608,6 +729,31 @@ public void CollectionPropertiesMustNotHaveSetters_Success() .BeTrue(); } + private class HasMutableCollectionsBase + { + public string[] Colours { get; set; } + } + + private class HasImmutableCollectionsDerived : HasMutableCollectionsBase + { + public HasImmutableCollectionsDerived(string[] names) + { + Names = names; + } + + public string[] Names { get; } + } + + [Test] + public void CollectionPropertiesMustNotHaveSetters_IgnoresInheritedProperties() + { + typeof(HasImmutableCollectionsDerived) + .MustConformTo(Convention.CollectionPropertiesMustNotHaveSetters) + .IsSatisfied + .Should() + .BeTrue(); + } + private class HasMutableCollections { public string[] Names { get; set; } @@ -625,18 +771,15 @@ public void CollectionPropertiesMustNotHaveSetters_FailsWhenAMutableCollectionPr private class HasImmutableProperties { - private readonly string[] _names; - private readonly int _age; - public HasImmutableProperties(string[] names, int age) { - _names = names; - _age = age; + Names = names; + Age = age; } - public string[] Names => _names; + public string[] Names { get; } - public int Age => _age; + public int Age { get; } } [Test] @@ -649,6 +792,34 @@ public void PropertiesMustNotHaveSetters_Success() .BeTrue(); } + private class HasMutablePropertiesBase + { + public int Age { get; set; } + } + + private class HasImmutablePropertiesDerived : HasMutablePropertiesBase + { + public HasImmutablePropertiesDerived(string[] names, int age) + { + Names = names; + Age = age; + } + + public string[] Names { get; } + + public int Age { get; } + } + + [Test] + public void PropertiesMustNotHaveSetters_IgnoresInheritedProperties() + { + typeof(HasImmutablePropertiesDerived) + .MustConformTo(Convention.PropertiesMustNotHaveSetters) + .IsSatisfied + .Should() + .BeTrue(); + } + private class HasMutableProperties { public string[] Names { get; set; } @@ -666,6 +837,41 @@ public void PropertiesMustNotHaveSetters_FailsWhenMutablePropertiesExists() result.Failures.Should().HaveCount(1); } + private class DoesNotHaveEnumerableProperty + { + public long Ticks { get; set; } + } + + [Test] + public void MustNotHaveAPropertyOfType_Success() + { + var result = typeof(DoesNotHaveEnumerableProperty) + .MustConformTo(Convention.MustNotHaveAPropertyOfType(typeof(IEnumerable<>), "reason")) + .IsSatisfied + .Should() + .BeTrue(); + } + + private class HasEnumerablePropertyBase + { + public IEnumerable Numbers { get; set; } + } + + private class DoesNotHaveEnumerablePropertyDerived : HasEnumerablePropertyBase + { + public long Ticks { get; set; } + } + + [Test] + public void MustNotHaveAPropertyOfType_IgnoresInheritedProperties() + { + var result = typeof(DoesNotHaveEnumerablePropertyDerived) + .MustConformTo(Convention.MustNotHaveAPropertyOfType(typeof(IEnumerable<>), "reason")) + .IsSatisfied + .Should() + .BeTrue(); + } + private class HasGenericAndNonGenericProperty { public IEnumerable Names { get; set; } diff --git a/src/Core/Conventional/Conventions/CollectionPropertiesMustNotHaveSettersConventionSpecification.cs b/src/Core/Conventional/Conventions/CollectionPropertiesMustNotHaveSettersConventionSpecification.cs index 8fb6c4e..44a71c2 100644 --- a/src/Core/Conventional/Conventions/CollectionPropertiesMustNotHaveSettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/CollectionPropertiesMustNotHaveSettersConventionSpecification.cs @@ -2,6 +2,7 @@ using System.Collections; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -12,7 +13,7 @@ public class CollectionPropertiesMustNotHaveSettersConventionSpecification : Pro protected override PropertyInfo[] GetNonConformingProperties(Type type) { return type - .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .GetDeclaredProperties() .Where(p => p.PropertyType != typeof (string) && (typeof (IEnumerable).IsAssignableFrom(p.PropertyType))) .Where(p => p.GetSetMethod(true) != null) .ToArray(); diff --git a/src/Core/Conventional/Conventions/EnumerablePropertiesMustBeEagerLoadedConventionSpecification.cs b/src/Core/Conventional/Conventions/EnumerablePropertiesMustBeEagerLoadedConventionSpecification.cs index cb7343a..e5849d9 100644 --- a/src/Core/Conventional/Conventions/EnumerablePropertiesMustBeEagerLoadedConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/EnumerablePropertiesMustBeEagerLoadedConventionSpecification.cs @@ -2,6 +2,7 @@ using System.Collections; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -12,7 +13,7 @@ public class EnumerablePropertiesMustBeEagerLoadedConventionSpecification : Conv public override ConventionResult IsSatisfiedBy(Type type) { var enumerables = type - .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .GetDeclaredProperties() .Select(p => p.PropertyType) .Where(t => t != typeof(string) && (typeof(IEnumerable).IsAssignableFrom(t))) .ToArray(); diff --git a/src/Core/Conventional/Conventions/MustNotHaveAPropertyOfTypeConventionSpecification.cs b/src/Core/Conventional/Conventions/MustNotHaveAPropertyOfTypeConventionSpecification.cs index 22cbfcf..64ea1c2 100644 --- a/src/Core/Conventional/Conventions/MustNotHaveAPropertyOfTypeConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/MustNotHaveAPropertyOfTypeConventionSpecification.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -20,14 +21,14 @@ public override ConventionResult IsSatisfiedBy(Type type) { if (_propertyType.IsGenericTypeDefinition) { - return type.GetProperties() + return type.GetDeclaredProperties() .Any(p => p.PropertyType.IsGenericType && p.PropertyType.GetGenericTypeDefinition() == _propertyType) ? NotSatisfied(type) : ConventionResult.Satisfied(type.FullName); } - return type.GetProperties().Any(p => p.PropertyType == _propertyType) + return type.GetDeclaredProperties().Any(p => p.PropertyType == _propertyType) ? NotSatisfied(type) : ConventionResult.Satisfied(type.FullName); } diff --git a/src/Core/Conventional/Conventions/PropertiesMustHavePrivateSettersConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesMustHavePrivateSettersConventionSpecification.cs index 90fb1e4..8c5c333 100644 --- a/src/Core/Conventional/Conventions/PropertiesMustHavePrivateSettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesMustHavePrivateSettersConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -10,7 +11,7 @@ public class PropertiesMustHavePrivateSettersConventionSpecification : PropertyC protected override PropertyInfo[] GetNonConformingProperties(Type type) { - return type.GetProperties() + return type.GetDeclaredProperties() .Where(subject => subject.CanWrite == false || subject.GetSetMethod(true).IsPrivate == false) .ToArray(); } diff --git a/src/Core/Conventional/Conventions/PropertiesMustHaveProtectedSettersConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesMustHaveProtectedSettersConventionSpecification.cs index f78c6e3..c5dc8e1 100644 --- a/src/Core/Conventional/Conventions/PropertiesMustHaveProtectedSettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesMustHaveProtectedSettersConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -10,7 +11,7 @@ public class PropertiesMustHaveProtectedSettersConventionSpecification : Propert protected override PropertyInfo[] GetNonConformingProperties(Type type) { - return type.GetProperties() + return type.GetDeclaredProperties() .Where(subject => subject.CanWrite == false || subject.GetSetMethod(true).IsFamily == false) .ToArray(); } diff --git a/src/Core/Conventional/Conventions/PropertiesMustHavePublicGettersConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesMustHavePublicGettersConventionSpecification.cs index 98540ee..00bf7ba 100644 --- a/src/Core/Conventional/Conventions/PropertiesMustHavePublicGettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesMustHavePublicGettersConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -10,7 +11,7 @@ public class PropertiesMustHavePublicGettersConventionSpecification : PropertyCo protected override PropertyInfo[] GetNonConformingProperties(Type type) { - return type.GetProperties() + return type.GetDeclaredProperties() .Where(subject => subject.CanRead == false || subject.GetGetMethod(true).IsPublic == false) .ToArray(); } diff --git a/src/Core/Conventional/Conventions/PropertiesMustHavePublicSettersConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesMustHavePublicSettersConventionSpecification.cs index 70028b0..b246582 100644 --- a/src/Core/Conventional/Conventions/PropertiesMustHavePublicSettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesMustHavePublicSettersConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -10,7 +11,7 @@ public class PropertiesMustHavePublicSettersConventionSpecification : PropertyCo protected override PropertyInfo[] GetNonConformingProperties(Type type) { - return type.GetProperties() + return type.GetDeclaredProperties() .Where(subject => subject.CanWrite == false || subject.GetSetMethod(true).IsPublic == false) .ToArray(); } diff --git a/src/Core/Conventional/Conventions/PropertiesMustNotHaveSettersConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesMustNotHaveSettersConventionSpecification.cs index 0f70e2f..b167d49 100644 --- a/src/Core/Conventional/Conventions/PropertiesMustNotHaveSettersConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesMustNotHaveSettersConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -10,7 +11,7 @@ public class PropertiesMustNotHaveSettersConventionSpecification : PropertyConve protected override PropertyInfo[] GetNonConformingProperties(Type type) { - return type.GetProperties() + return type.GetDeclaredProperties() .Where(x => x.GetSetMethod(true) != null) .ToArray(); } diff --git a/src/Core/Conventional/Conventions/PropertiesOfTypeMustHaveAttributeConventionSpecification.cs b/src/Core/Conventional/Conventions/PropertiesOfTypeMustHaveAttributeConventionSpecification.cs index 2a11d1d..7900abb 100644 --- a/src/Core/Conventional/Conventions/PropertiesOfTypeMustHaveAttributeConventionSpecification.cs +++ b/src/Core/Conventional/Conventions/PropertiesOfTypeMustHaveAttributeConventionSpecification.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Reflection; +using Conventional.Extensions; namespace Conventional.Conventions { @@ -22,7 +23,7 @@ public PropertiesOfTypeMustHaveAttributeConventionSpecification(Type propertyTyp public override ConventionResult IsSatisfiedBy(Type type) { var properties = type - .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .GetDeclaredProperties() .Where(p => _propertyType.IsAssignableFrom(p.PropertyType)) .Where(p => _writablePropertiesOnly && p.CanWrite) .ToArray(); diff --git a/src/Core/Conventional/Extensions/TypeExtensions.cs b/src/Core/Conventional/Extensions/TypeExtensions.cs index 6b70e1d..3a98f97 100644 --- a/src/Core/Conventional/Extensions/TypeExtensions.cs +++ b/src/Core/Conventional/Extensions/TypeExtensions.cs @@ -7,6 +7,12 @@ namespace Conventional.Extensions { public static class TypeExtensions { + internal static PropertyInfo[] GetDeclaredProperties(this Type type) + { + return type.GetProperties(BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.Static | + BindingFlags.Public); + } + internal static bool IsGenericImplementation(this Type type) { return (type.BaseType != null && type.BaseType.IsGenericType) ||