seesharper / LightInject.Interception

LightInject.Interception supports Aspect Oriented Programming through proxy-based method interceptors.
11 stars 7 forks source link

Interception of base class protected virtual methods broken since version 2.0.0 #26

Open VladimirSmall opened 5 years ago

VladimirSmall commented 5 years ago

The following test fails.

    public class LightInjectInterceptionTests
    {
        [Fact]
        public void ShouldAbleInterceptBaseClassProtectedVirtualMethods()
        {
            var container = new ServiceContainer();

            container.Register<Derived>();

            var interceptor = new ProtectedVirtualMethodInterceptor();
            container.Intercept(
                sr => typeof(Derived).IsAssignableFrom(sr.ServiceType),
                (r, d) => d.Implement(() => interceptor, m => m.Name == "ProtectedVirtualMethod"));

            var instance = container.GetInstance<Derived>();
            instance.Call();

            Assert.True(interceptor.IsInvoked);
        }

        public class Base
        {
            public void Call()
            {
                ProtectedVirtualMethod();
            }

            protected virtual void ProtectedVirtualMethod()
            {
            }
        }

        public class Derived : Base
        {
        }

        internal class ProtectedVirtualMethodInterceptor : IInterceptor
        {
            public bool IsInvoked { get; set; }

            public object Invoke(IInvocationInfo invocationInfo)
            {
                IsInvoked = true;
                return invocationInfo.Proceed();
            }
        }
    }

It's interesting that the same test with Base class instead of Derived succeeds.

Little investigation showed that test works with version 1.2.1, but does not wrok with version 2.0.0 of LightInject.Interception.

The reason in method MethodSelector.Execute code. It was

        public MethodInfo[] Execute(Type targetType, Type[] additionalInterfaces)
        {
            MethodInfo[] interceptableMethods;

            if (targetType.GetTypeInfo().IsInterface)
            {
                interceptableMethods = targetType.GetMethods()
                                          .Where(m => !m.IsSpecialName)
                                          .Concat(typeof(object).GetMethods().Where(m => m.IsVirtual))
                                          .Concat(additionalInterfaces.SelectMany(i => i.GetMethods()))
                                          .Distinct()
                                          .ToArray();
            }
            else
            {
                interceptableMethods = targetType.GetMethods(BindingFlags.Public | BindingFlags.Instance)
                                          .Concat(targetType.GetMethods(BindingFlags.NonPublic | BindingFlags.Instance).Where(m => m.IsFamily && !m.IsDeclaredBy<object>()))
                                          .Where(m => m.IsVirtual && !m.IsFinal)
                                          .Concat(additionalInterfaces.SelectMany(i => i.GetMethods()))
                                          .Distinct()
                                          .ToArray();
            }

            return interceptableMethods;
        }

But in revision 544c1c7c7600d46402195a022073653b2cfd2958 it was changed to.

        public MethodInfo[] Execute(Type targetType, Type[] additionalInterfaces)
        {
            MethodInfo[] interceptableMethods;

            if (targetType.GetTypeInfo().IsInterface)
            {
                interceptableMethods = targetType.GetTypeInfo().DeclaredMethods
                                          .Where(m => !m.IsSpecialName)
                                          .Concat(typeof(object).GetTypeInfo().DeclaredMethods.Where(m => m.IsVirtual && !m.IsFamily))
                                          .Concat(additionalInterfaces.SelectMany(i => i.GetTypeInfo().DeclaredMethods))
                                          .Distinct()
                                          .ToArray();                
            }
            else
            {                
                interceptableMethods = targetType.GetRuntimeMethods().Where(m => m.IsPublic)
                    .Concat(
                        targetType.GetTypeInfo()
                            .DeclaredMethods.Where(
                                m => !m.IsPublic && m.IsFamily && !m.IsDeclaredBy<object>()))
                    .Where(m => m.IsVirtual && !m.IsFinal && !m.IsStatic)
                    .Concat(additionalInterfaces.SelectMany(i => i.GetTypeInfo().DeclaredMethods))
                    .Distinct().ToArray();                
            }

            return interceptableMethods;
        }

targetType.GetTypeInfo().DeclaredMethods does not include base class methods. This brokes LightInject.SignalR support - it based on interception of virtual protected method Hub.Dispose(bool), which belongs to HubBase class.

cnshenj commented 5 years ago

Sorry I was wrong in my previous comment. In your case, it looks the change is not a bug, but by design. The interceptor only wants to intercept all public methods, and own family (protected) methods. You should add an interception registration for Base class' ProtectedVirtualMethod.

VladimirSmall commented 4 years ago

Was closed by my mistake

VladimirSmall commented 3 years ago

You should add an interception registration for Base class' ProtectedVirtualMethod.

Thank you for your advice, but it doesn't work. This test fails.

        [Fact]
        public void ShouldAbleInterceptBaseClassProtectedVirtualMethods()
        {
            var container = new ServiceContainer();

            container.Register<Derived>();

            var interceptor = new ProtectedVirtualMethodInterceptor();
            container.Intercept(
                sr => typeof(Derived).IsAssignableFrom(sr.ServiceType),
                (r, d) => d.Implement(() => interceptor, m => m.Name == "ProtectedVirtualMethod"));
            container.Intercept(
                sr => typeof(Base).IsAssignableFrom(sr.ServiceType),
                (r, d) => d.Implement(() => interceptor, m => m.Name == "ProtectedVirtualMethod"));

            var instance = container.GetInstance<Derived>();
            instance.Call();

            Assert.True(interceptor.IsInvoked);
        }

The interceptor only wants to intercept all public methods, and own family (protected) methods.

Why we should be limited to own protected methods only? Why we couldn't intercept base class protected virtual methods? Your own library LightInject.SignalR based on interception of base class virtual protected method HubBase.Dispose(bool) too. If you will port LightInject.SignalR to support new LightInject and LightInject.Interception you will see that LightInject.SignalR support was broken - interceptor for base class Dispose(bool) method is never called.

You could fix this issue simply by replacing in method MethodSelector.Execute

targetType.GetRuntimeMethods().Where(m => m.IsPublic).

with

targetType.GetRuntimeMethods().Where(m => m.IsPublic || (m.IsFamily && !m.IsDeclaredBy<object>())).

If needed i can create pull request to fix the issue.