From 62f3be39dda8814e95adb526d50a9f4bd5189751 Mon Sep 17 00:00:00 2001 From: Alex Zaytsev Date: Thu, 2 May 2024 10:52:55 +1000 Subject: [PATCH 1/2] Treat all non-virtual methods of System.Object as not proxiable --- .../NHSpecificTest/ProxyValidator/Fixture.cs | 82 +++++----- .../ProxyValidator/ShouldBeProxiableTests.cs | 140 +++++++++++------- ...aultDynamicProxyMethodCheckerExtensions.cs | 8 +- src/NHibernate/Proxy/DynProxyTypeValidator.cs | 17 +-- 4 files changed, 145 insertions(+), 102 deletions(-) diff --git a/src/NHibernate.Test/NHSpecificTest/ProxyValidator/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/ProxyValidator/Fixture.cs index 2a16a4578a1..c110eb370ba 100644 --- a/src/NHibernate.Test/NHSpecificTest/ProxyValidator/Fixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/ProxyValidator/Fixture.cs @@ -1,7 +1,6 @@ using System; using NHibernate.Proxy; using NUnit.Framework; -using System.Collections.Generic; namespace NHibernate.Test.NHSpecificTest.ProxyValidator { @@ -10,15 +9,6 @@ public class Fixture { private readonly IProxyValidator pv = new DynProxyTypeValidator(); - private void Validate(System.Type type) - { - ICollection errors = pv.ValidateType(type); - if (errors != null) - { - throw new InvalidProxyTypeException(errors); - } - } - public class ValidClass { private int privateField; @@ -64,12 +54,35 @@ protected int NonVirtualPrivateProperty #pragma warning restore 67 } + [Test] + public void ObjectIsValid() + { + var errors = pv.ValidateType(typeof(object)); + Assert.That(errors, Is.Null); + } + [Test] public void ValidClassTest() { - Validate(typeof(ValidClass)); + var errors = pv.ValidateType(typeof(ValidClass)); + Assert.That(errors, Is.Null); } + public class InvalidSealedToString : ValidClass + { + public sealed override string ToString() + { + return base.ToString(); + } + } + + [Test] + public void SealedObjectOverride() + { + var errors = pv.ValidateType(typeof(InvalidSealedToString)); + Assert.That(errors, Has.Count.EqualTo(1)); + } + public class InvalidPrivateConstructor : ValidClass { private InvalidPrivateConstructor() @@ -80,7 +93,8 @@ private InvalidPrivateConstructor() [Test] public void PrivateConstructor() { - Assert.Throws(() => Validate(typeof(InvalidPrivateConstructor))); + var errors = pv.ValidateType(typeof(InvalidPrivateConstructor)); + Assert.That(errors, Has.Count.EqualTo(1)); } public class InvalidNonVirtualProperty : ValidClass @@ -95,7 +109,8 @@ public int NonVirtualProperty [Test] public void NonVirtualProperty() { - Assert.Throws(() => Validate(typeof(InvalidNonVirtualProperty))); + var errors = pv.ValidateType(typeof(InvalidNonVirtualProperty)); + Assert.That(errors, Has.Count.EqualTo(2)); } public class InvalidPublicField : ValidClass @@ -106,7 +121,8 @@ public class InvalidPublicField : ValidClass [Test] public void PublicField() { - Assert.Throws(() => Validate(typeof(InvalidPublicField))); + var errors = pv.ValidateType(typeof(InvalidPublicField)); + Assert.That(errors, Has.Count.EqualTo(1)); } public class InvalidNonVirtualEvent : ValidClass @@ -119,7 +135,8 @@ public class InvalidNonVirtualEvent : ValidClass [Test] public void NonVirtualEvent() { - Assert.Throws(() => Validate(typeof(InvalidNonVirtualEvent))); + var errors = pv.ValidateType(typeof(InvalidNonVirtualEvent)); + Assert.That(errors, Has.Count.EqualTo(2)); } public interface ValidInterface @@ -129,7 +146,8 @@ public interface ValidInterface [Test] public void Interface() { - Validate(typeof(ValidInterface)); + var errors = pv.ValidateType(typeof(ValidInterface)); + Assert.That(errors, Is.Null); } public class MultipleErrors @@ -153,15 +171,8 @@ public int NonVirtualProperty [Test] public void MultipleErrorsReported() { - try - { - Validate(typeof(MultipleErrors)); - Assert.Fail("Should have failed validation"); - } - catch (InvalidProxyTypeException e) - { - Assert.IsTrue(e.Errors.Count > 1); - } + var errors = pv.ValidateType(typeof(MultipleErrors)); + Assert.That(errors, Has.Count.GreaterThan(1)); } public class InvalidNonVirtualInternalProperty : ValidClass @@ -183,16 +194,18 @@ public class InvalidInternalField : ValidClass [Test] public void NonVirtualInternal() { - Assert.Throws(() => Validate(typeof(InvalidNonVirtualInternalProperty))); + var errors = pv.ValidateType(typeof(InvalidNonVirtualInternalProperty)); + Assert.That(errors, Has.Count.EqualTo(2)); } [Test] public void InternalField() { - Assert.Throws(() => Validate(typeof(InvalidInternalField))); + var errors = pv.ValidateType(typeof(InvalidInternalField)); + Assert.That(errors, Has.Count.EqualTo(1)); } - public class InvalidNonVirtualProtectedProperty : ValidClass + public class ValidNonVirtualProtectedProperty : ValidClass { protected int NonVirtualProperty { @@ -204,8 +217,8 @@ protected int NonVirtualProperty [Test] public void NonVirtualProtected() { - Validate(typeof(InvalidNonVirtualProtectedProperty)); - Assert.IsTrue(true, "Always should pass, protected members do not need to be virtual."); + var errors = pv.ValidateType(typeof(ValidNonVirtualProtectedProperty)); + Assert.That(errors, Is.Null); } public class InvalidNonVirtualProtectedInternalProperty : ValidClass @@ -220,7 +233,8 @@ protected internal int NonVirtualProperty [Test] public void NonVirtualProtectedInternal() { - Assert.Throws(() => Validate(typeof(InvalidNonVirtualProtectedInternalProperty))); + var errors = pv.ValidateType(typeof(InvalidNonVirtualProtectedInternalProperty)); + Assert.That(errors, Has.Count.EqualTo(2)); } interface INonVirtualPublicImplementsInterface @@ -239,7 +253,8 @@ public int NonVirtualMethodImplementsInterface [Test] public void VirtualPublicImplementsInterface() { - Assert.Throws(() => Validate(typeof(NonVirtualPublicImplementsInterface))); + var errors = pv.ValidateType(typeof(NonVirtualPublicImplementsInterface)); + Assert.That(errors, Has.Count.EqualTo(1)); } public class InvalidVirtualPrivateAutoProperty : ValidClass @@ -254,7 +269,8 @@ public virtual int NonVirtualSetterProperty [Test] public void PrivateSetterOnVirtualPropertyShouldThrows() { - Assert.Throws(() => Validate(typeof(InvalidVirtualPrivateAutoProperty))); + var errors = pv.ValidateType(typeof(InvalidVirtualPrivateAutoProperty)); + Assert.That(errors, Has.Count.EqualTo(1)); } } } diff --git a/src/NHibernate.Test/NHSpecificTest/ProxyValidator/ShouldBeProxiableTests.cs b/src/NHibernate.Test/NHSpecificTest/ProxyValidator/ShouldBeProxiableTests.cs index 204a0c0997f..bb29e990bb2 100644 --- a/src/NHibernate.Test/NHSpecificTest/ProxyValidator/ShouldBeProxiableTests.cs +++ b/src/NHibernate.Test/NHSpecificTest/ProxyValidator/ShouldBeProxiableTests.cs @@ -46,80 +46,115 @@ protected internal void AProtectedInternal() { } } [Test] - public void GetTypeNotBeProxiable() + public void ObjectToStringShouldBeProxiable() { - var method = typeof(object).GetMethod("GetType"); - Assert.That(method.ShouldBeProxiable(), Is.False); + var method = typeof(object).GetMethod(nameof(ToString)); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.That(method.IsProxiable(), Is.True); + }); } [Test] - public void DisposeNotBeProxiable() + public void ObjectGetHashCodeShouldBeProxiable() { - var method = typeof(MyClass).GetMethod("Dispose"); - Assert.That(method.ShouldBeProxiable(), Is.False); + var method = typeof(object).GetMethod(nameof(GetHashCode)); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.That(method.IsProxiable(), Is.True); + }); } [Test] - public void ObjectDestructorShouldNotBeProxiable() + public void ObjectEqualsShouldBeProxiable() { - var method = typeof(object).GetMethod( - "Finalize", - BindingFlags.NonPublic | BindingFlags.Instance); - - Assert.That(method.ShouldBeProxiable(), Is.False); + var method = typeof(object).GetMethod(nameof(Equals), BindingFlags.Public | BindingFlags.Instance); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.That(method.IsProxiable(), Is.True); + }); } [Test] - public void ObjectDestructorIsNotProxiable() + public void ObjectMemberwiseCloneShouldNotBeProxiable() { var method = typeof(object).GetMethod( - "Finalize", - BindingFlags.NonPublic | BindingFlags.Instance); + nameof(MemberwiseClone), + BindingFlags.Instance | BindingFlags.NonPublic); - Assert.That(method.IsProxiable(), Is.False); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.False); + Assert.That(method.IsProxiable(), Is.False); + }); } - + [Test] - public void MyClassDestructorShouldNotBeProxiable() + public void ObjectGetTypeShouldNotBeProxiable() { - var method = typeof(MyClass).GetMethod( - "Finalize", - BindingFlags.NonPublic | BindingFlags.Instance, - null, - System.Type.EmptyTypes, - null); + var method = typeof(object).GetMethod("GetType"); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.False); + Assert.That(method.IsProxiable(), Is.False); + }); + } - Assert.That(method.ShouldBeProxiable(), Is.False); + [Test] + public void MyClassDisposeNotBeProxiable() + { + var method = typeof(MyClass).GetMethod("Dispose"); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.False); + Assert.That(method.IsProxiable(), Is.False); + }); } [Test] - public void MyClassDestructorIsNotProxiable() + public void ObjectDestructorShouldNotBeProxiable() { - var method = typeof(MyClass).GetMethod( + var method = typeof(object).GetMethod( "Finalize", - BindingFlags.NonPublic | BindingFlags.Instance, - null, - System.Type.EmptyTypes, - null); + BindingFlags.NonPublic | BindingFlags.Instance); - Assert.That(method.IsProxiable(), Is.False); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.False); + Assert.That(method.IsProxiable(), Is.False); + }); } [Test] - public void MyClassLowerCaseFinalizeShouldBeProxiable() + public void MyClassDestructorShouldNotBeProxiable() { var method = typeof(MyClass).GetMethod( - "finalize", - BindingFlags.Public | BindingFlags.Instance, + "Finalize", + BindingFlags.NonPublic | BindingFlags.Instance, null, System.Type.EmptyTypes, null); - Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.False); + Assert.That(method.IsProxiable(), Is.False); + }); } [Test] - public void MyClassLowerCaseFinalizeIsProxiable() + public void MyClassLowerCaseFinalizeShouldBeProxiable() { var method = typeof(MyClass).GetMethod( "finalize", @@ -128,24 +163,16 @@ public void MyClassLowerCaseFinalizeIsProxiable() System.Type.EmptyTypes, null); - Assert.That(method.IsProxiable(), Is.True); - } - - [Test] - public void MyClassFinalizeWithParametersShouldBeProxiable() - { - var method = typeof(MyClass).GetMethod( - "Finalize", - BindingFlags.Public | BindingFlags.Instance, - null, - new[] { typeof(int) }, - null); - - Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.That(method.IsProxiable(), Is.True); + }); } [Test] - public void MyClassFinalizeWithParametersIsProxiable() + public void MyClassFinalizeWithParametersShouldBeProxiable() { var method = typeof(MyClass).GetMethod( "Finalize", @@ -154,14 +181,19 @@ public void MyClassFinalizeWithParametersIsProxiable() new[] { typeof(int) }, null); - Assert.That(method.IsProxiable(), Is.True); + Assert.Multiple( + () => + { + Assert.That(method.ShouldBeProxiable(), Is.True); + Assert.That(method.IsProxiable(), Is.True); + }); } [Test] public void WhenProtectedNoVirtualPropertyThenShouldntBeProxiable() { var prop = typeof(ProtectedNoVirtualProperty).GetProperty("Aprop", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); - Assert.That(prop.ShouldBeProxiable(), Is.False); + Assert.That(prop.ShouldBeProxiable(), Is.False); } [Test] diff --git a/src/NHibernate/Proxy/DefaultDynamicProxyMethodCheckerExtensions.cs b/src/NHibernate/Proxy/DefaultDynamicProxyMethodCheckerExtensions.cs index b25c01c9fea..f57cf3fe287 100644 --- a/src/NHibernate/Proxy/DefaultDynamicProxyMethodCheckerExtensions.cs +++ b/src/NHibernate/Proxy/DefaultDynamicProxyMethodCheckerExtensions.cs @@ -23,12 +23,10 @@ public static bool IsProxiable(this MethodInfo method) public static bool ShouldBeProxiable(this MethodInfo method) { // to use only for real methods (no getter/setter) - return (method.DeclaringType != typeof (MarshalByRefObject)) && - !IsFinalizeMethod(method) && - (!(method.DeclaringType == typeof (object) && "GetType".Equals(method.Name))) && - (!(method.DeclaringType == typeof (object) && "obj_address".Equals(method.Name))) && // Mono-specific method + return method.DeclaringType != typeof(MarshalByRefObject) && + !(method.DeclaringType == typeof(object) && !method.IsVirtual) && !IsDisposeMethod(method) && - (method.IsPublic || method.IsAssembly || method.IsFamilyOrAssembly); + (method.IsPublic || method.IsAssembly || method.IsFamilyOrAssembly); } public static bool ShouldBeProxiable(this PropertyInfo propertyInfo) diff --git a/src/NHibernate/Proxy/DynProxyTypeValidator.cs b/src/NHibernate/Proxy/DynProxyTypeValidator.cs index 2f0370c3628..b53df86c5b6 100644 --- a/src/NHibernate/Proxy/DynProxyTypeValidator.cs +++ b/src/NHibernate/Proxy/DynProxyTypeValidator.cs @@ -50,9 +50,8 @@ protected virtual void CheckAccessibleMembersAreVirtual(System.Type type) foreach (var member in members) { - if (member is PropertyInfo) + if (member is PropertyInfo property) { - var property = (PropertyInfo) member; if(property.ShouldBeProxiable()) { MethodInfo[] accessors = property.GetAccessors(true); @@ -66,21 +65,19 @@ protected virtual void CheckAccessibleMembersAreVirtual(System.Type type) } } } - else if (member is MethodInfo) + else if (member is MethodInfo method) { - var methodInfo = (MethodInfo) member; // avoid the check of properties getter and setter because already checked when the PropertyInfo was found. - if (!IsPropertyMethod(methodInfo) && methodInfo.ShouldBeProxiable()) + if (!IsPropertyMethod(method) && method.ShouldBeProxiable()) { - CheckMethodIsVirtual(type, methodInfo); + CheckMethodIsVirtual(type, method); } } - else if (member is FieldInfo) + else if (member is FieldInfo field) { - var memberField = (FieldInfo) member; - if (memberField.IsPublic || memberField.IsAssembly || memberField.IsFamilyOrAssembly) + if (field.IsPublic || field.IsAssembly || field.IsFamilyOrAssembly) { - EnlistError(type, "field " + member.Name + " should not be public nor internal (ecapsulate it in a property)."); + EnlistError(type, "field " + field.Name + " should not be public nor internal (ecapsulate it in a property)."); } } } From d617a784b17c9415497f4c01c5f7786ac845b8c3 Mon Sep 17 00:00:00 2001 From: Alex Zaytsev Date: Fri, 3 May 2024 16:39:58 +1000 Subject: [PATCH 2/2] Set build 5.5.2-dev --- build-common/NHibernate.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build-common/NHibernate.props b/build-common/NHibernate.props index d56d15ccbc4..3564b80a784 100644 --- a/build-common/NHibernate.props +++ b/build-common/NHibernate.props @@ -3,9 +3,9 @@ 5.5 - 1 + 2 - + dev 9.0 $(NhVersion).$(VersionPatch)