pamidur / aspect-injector

AOP framework for .NET (c#, vb, etc)
Apache License 2.0
745 stars 112 forks source link

Allow inherit Aspect Definition (look for effects in the base classes) #145

Closed StefH closed 2 years ago

StefH commented 3 years ago

Nice project BTW!

I would be very useful if it was very easy to just extend a class from a pre-defined Aspect and just override some virtual methods.

This idea is based on https://doc.postsharp.net/exception-handling

So that it can be used like this:

public class MyLogger : PredefinedAspectAttribute
{
    public override void OnBefore(object instance, string name)
    {
        // Do some logging
    }
}

Or

public class MyGrpcExceptionHandler : PredefinedAspectAttribute
{
    public override OnException(Exception exception, string name)
    {
        switch (exception)
        {
            case UnauthorizedAccessException accessException:
                throw new RpcException(new Status(StatusCode.Unauthenticated, name, accessException), accessException.Message);

            default:
                throw new RpcException(new Status(StatusCode.Unknown, name, exception), exception.Message);
        }
    }
}

I've tried to build a pre-defined Aspect based on https://github.com/pamidur/aspect-injector/tree/master/samples/UniversalWrapper.

However I cannot get this to work in such a way that it can be used + extended correctly.

My code is below (which is just doing 'hardcoded' logic, like , logging and handling + throwing a GrpcException.

[Aspect(Scope.PerInstance)]
[Injection(typeof(GrpcServiceAspectInjectorAttribute))]
public class GrpcServiceAspectInjectorAttribute : Attribute
{
    private static readonly Lazy<ILogger<GrpcServiceAspectInjectorAttribute>> _logger = AspectServiceLocator.GetService<ILogger<GrpcServiceAspectInjectorAttribute>>();

    private static readonly MethodInfo _asyncHandler = typeof(GrpcServiceAspectInjectorAttribute).GetMethod(nameof(GrpcServiceAspectInjectorAttribute.WrapAsync), BindingFlags.NonPublic | BindingFlags.Instance);
    private static readonly MethodInfo _syncHandler = typeof(GrpcServiceAspectInjectorAttribute).GetMethod(nameof(GrpcServiceAspectInjectorAttribute.WrapSync), BindingFlags.NonPublic | BindingFlags.Instance);
    private static readonly Type _voidTaskResult = Type.GetType("System.Threading.Tasks.VoidTaskResult");

    [Advice(Kind.Before, Targets = Target.Public | Target.Method)]
    public void LogBefore([Argument(Source.Instance)] object instance, [Argument(Source.Name)] string name)
    {
        _logger.Value.LogInformation("{instance}:{name}", instance.GetType().Name, name);
    }

    [Advice(Kind.Around, Targets = Target.Public | Target.Method)]
    public object? Handle(
        [Argument(Source.Target)] Func<object[], object> target,
        [Argument(Source.Arguments)] object[] args,
        [Argument(Source.Name)] string name,
        [Argument(Source.ReturnType)] Type returnType)
    {
        if (typeof(Task).IsAssignableFrom(returnType))
        {
            var syncResultType = returnType.IsConstructedGenericType ? returnType.GenericTypeArguments[0] : _voidTaskResult;
            return _asyncHandler.MakeGenericMethod(syncResultType).Invoke(this, new object[] { target, args, name });
        }

        returnType = returnType == typeof(void) ? typeof(object) : returnType;
        return _syncHandler.MakeGenericMethod(returnType).Invoke(this, new object[] { target, args, name });
    }

    private void OnException(Exception exception, string name)
    {
        switch (exception)
        {
            case UnauthorizedAccessException accessException:
                throw new RpcException(new Status(StatusCode.Unauthenticated, name, accessException), accessException.Message);

            default:
                throw new RpcException(new Status(StatusCode.Unknown, name, exception), exception.Message);
        }
    }

    private T? WrapSync<T>(Func<object[], object> target, object[] args, string name)
    {
        try
        {
            return (T)target(args);
        }
        catch (Exception exception)
        {
            OnException(exception, name);
            return default;
        }
    }

    private async Task<T?>? WrapAsync<T>(Func<object[], object> target, object[] args, string name)
    {
        try
        {
            return await (Task<T?>)target(args);
        }
        catch (Exception exception)
        {
            OnException(exception, name);
            return default;
        }
    }
}
pamidur commented 3 years ago

Hi @StefH , thank you for your interest in the project!

What you're suggesting is a good looking feature, which is, I'm afraid, impossible in the moment. Or at least impossible the way you see it. The reason is that currently you one cannot inherit an Aspect without loosing it's magic, because the aspect injector doesn't look into base classes for Advices. It should not be hard to improve. I'll add that as an enhancement.

In the meantime you can use a different (a bit slower and arguably uglier) approach:

[PredefinedAspect(typeof(MyLogger))]
public void MyMethod(){}

and then in the Advice you can instantiate MyLogger and call needed method.

pamidur commented 3 years ago

Nah, I figured better way: Define common aspect like this

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, Inherited = true)]
    [Injection(typeof(UniversalWrapperAspect))]
    public abstract class UniversalWrapperAttribute : Attribute
    {
        public virtual void OnBefore()
        {
        }

        public virtual void OnAfter()
        {
        }

        public virtual void OnException(Exception exception)
        {
        }
    }

    [Aspect(Scope.Global)]
    public class UniversalWrapperAspect
    {
        private static readonly MethodInfo _asyncHandler = typeof(UniversalWrapperAspect).GetMethod(nameof(UniversalWrapperAspect.WrapAsync), BindingFlags.NonPublic | BindingFlags.Instance);
        private static readonly MethodInfo _syncHandler = typeof(UniversalWrapperAspect).GetMethod(nameof(UniversalWrapperAspect.WrapSync), BindingFlags.NonPublic | BindingFlags.Instance);
        private static readonly Type _voidTaskResult = Type.GetType("System.Threading.Tasks.VoidTaskResult");

        [Advice(Kind.Around, Targets = Target.Public | Target.Method)]
        public object? Handle(
            [Argument(Source.Target)] Func<object[], object> target,
            [Argument(Source.Arguments)] object[] args,
            [Argument(Source.Name)] string name,
            [Argument(Source.ReturnType)] Type returnType,
            [Argument(Source.Triggers)] Attribute[] triggers)
        {
            var properTriggers = triggers.OfType<UniversalWrapperAttribute>().ToArray();

            if (typeof(Task).IsAssignableFrom(returnType))
            {
                var syncResultType = returnType.IsConstructedGenericType ? returnType.GenericTypeArguments[0] : _voidTaskResult;
                return _asyncHandler.MakeGenericMethod(syncResultType).Invoke(this, new object[] { target, args, name, properTriggers });
            }

            returnType = returnType == typeof(void) ? typeof(object) : returnType;
            return _syncHandler.MakeGenericMethod(returnType).Invoke(this, new object[] { target, args, name, properTriggers });
        }

        private T WrapSync<T>(Func<object[], object> target, object[] args, string name, UniversalWrapperAttribute[] attributes)
        {
            foreach (var attr in attributes)
                attr.OnBefore();

            try
            {
                var result = (T)target(args);

                foreach (var attr in attributes)
                    attr.OnAfter();

                return result;
            }
            catch (Exception exception)
            {
                foreach (var attr in attributes)
                    attr.OnException(exception);
                return default;
            }
        }

        private async Task<T> WrapAsync<T>(Func<object[], object> target, object[] args, string name, UniversalWrapperAttribute[] attributes)
        {
            foreach (var attr in attributes)
                attr.OnBefore();

            try
            {
                var result =  await (Task<T>)target(args);
                foreach (var attr in attributes)
                    attr.OnAfter();

                return result;
            }
            catch (Exception exception)
            {
                foreach (var attr in attributes)
                    attr.OnException(exception);
                return default;
            }
        }
    }

and then usage

   [Injection(typeof(UniversalWrapperAspect))]
    public class LogStuffAttribute: UniversalWrapperAttribute
    {
        public override void OnBefore()
        {
            Console.WriteLine("Logged!");
        }
    }

    [LogStuff]
    public class TestClass
    {
         ....
    }

The only downside is that you have to mark all your attributes with [Injection(typeof(UniversalWrapperAspect))]

StefH commented 3 years ago

Thanks.

  1. "The only downside is that you have to mark all your attributes with [Injection(typeof(UniversalWrapperAspect))]" --> for me that's no downside, this sounds logical to me.

  2. The warpper does have [Aspect(Scope.Global)]. Is it still possible to overwrite this with "Instance", if yes, then how?

pamidur commented 3 years ago

[Aspect(Scope.Global)] only applies to Aspect itself and it controls whether aspect instance is bound to target instance. [Aspect(Scope.PerInstance)] only makes sense if your aspect class has some instance data that you want to stick with target instance.

In the above example I cannot see how PerInstance changes anything, you can mark aspect as PerInstance but the only difference would be that you'll have as many UniversalWrapperAspect's instances as you have targets (types where something is injected). Example for PerInstance usage would be - 'I want to count method calls for this particular instance'

What's your use-case for [Aspect(Scope.PerInstance)] ?

StefH commented 3 years ago

I think your proposed solution fits my scenario.

Updated code:

using System;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using AspectInjector.Broker;

namespace Skills.Server.Grpc.Aspects
{
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, Inherited = true)]
    [Injection(typeof(PublicMethodWrapperAspect))]
    public abstract class PublicMethodWrapperAttribute : Attribute
    {
        public virtual void OnBefore(object instance, string name)
        {
        }

        public virtual void OnAfter(object instance, string name)
        {
        }

        public virtual void OnException(Exception exception, string name)
        {
        }
    }

    [Aspect(Scope.Global)]
    public class PublicMethodWrapperAspect
    {
        private static readonly MethodInfo _asyncHandler = typeof(PublicMethodWrapperAspect).GetMethod(nameof(PublicMethodWrapperAspect.WrapAsync), BindingFlags.NonPublic | BindingFlags.Instance);
        private static readonly MethodInfo _syncHandler = typeof(PublicMethodWrapperAspect).GetMethod(nameof(PublicMethodWrapperAspect.WrapSync), BindingFlags.NonPublic | BindingFlags.Instance);
        private static readonly Type _voidTaskResult = Type.GetType("System.Threading.Tasks.VoidTaskResult");

        [Advice(Kind.Around, Targets = Target.Public | Target.Method)]
        public object? Handle(
            [Argument(Source.Instance)] object instance,
            [Argument(Source.Target)] Func<object[], object> target,
            [Argument(Source.Arguments)] object[] args,
            [Argument(Source.Name)] string name,
            [Argument(Source.ReturnType)] Type returnType,
            [Argument(Source.Triggers)] Attribute[] triggers)
        {
            var properTriggers = triggers.OfType<PublicMethodWrapperAttribute>().ToArray();

            if (typeof(Task).IsAssignableFrom(returnType))
            {
                var syncResultType = returnType.IsConstructedGenericType ? returnType.GenericTypeArguments[0] : _voidTaskResult;
                return _asyncHandler.MakeGenericMethod(syncResultType).Invoke(this, new object[] { instance, target, args, name, properTriggers });
            }

            returnType = returnType == typeof(void) ? typeof(object) : returnType;
            return _syncHandler.MakeGenericMethod(returnType).Invoke(this, new object[] { instance, target, args, name, properTriggers });
        }

        private T? WrapSync<T>(object instance, Func<object[], object> target, object[] args, string name, PublicMethodWrapperAttribute[] attributes)
        {
            OnBefore(attributes, instance, name);

            try
            {
                var result = (T)target(args);

                OnAfter(attributes, instance, name);

                return result;
            }
            catch (Exception exception)
            {
                OnException(attributes, exception, name);

                return default;
            }
        }

        private async Task<T?>? WrapAsync<T>(object instance, Func<object[], object> target, object[] args, string name, PublicMethodWrapperAttribute[] attributes)
        {
            OnBefore(attributes, instance, name);

            try
            {
                var result = await (Task<T>)target(args);

                OnAfter(attributes, instance, name);

                return result;
            }
            catch (Exception exception)
            {
                OnException(attributes, exception, name);

                return default;
            }
        }

        private void OnBefore(PublicMethodWrapperAttribute[] attributes, object instance, string name)
        {
            foreach (var attr in attributes)
            {
                try
                {
                    attr.OnBefore(instance, name);
                }
                catch
                {
                }
            }
        }

        private void OnAfter(PublicMethodWrapperAttribute[] attributes, object instance, string name)
        {
            foreach (var attr in attributes)
            {
                try
                {
                    attr.OnAfter(instance, name);
                }
                catch
                {
                }
            }
        }

        private void OnException(PublicMethodWrapperAttribute[] attributes, Exception exception, string name)
        {
            foreach (var attr in attributes)
            {
                try
                {
                    attr.OnException(exception, name);
                }
                catch
                {
                }
            }
        }
    }
}

Maybe you can add this functionality to a new NuGet package ? The same as you did with Cache?

pamidur commented 3 years ago

Great idea! We could create a library with this and similar base-aspects! Would you like to participate? PRs are welcome!

StefH commented 3 years ago

(I'll create a PR to indicate what I've in mind....)

StefH commented 3 years ago

I've made work in progress PR which highlights the logic how this generic functionality can be build. https://github.com/pamidur/aspect-injector/pull/148

(Not yet tested if this runs correctly...)

jerviscui commented 3 years ago
_asyncHandler.MakeGenericMethod(syncResultType).Invoke()

Method.Invoke is low performance.

pamidur commented 3 years ago

@jerviscui , we're working on #148 and discussing the same thing about performance there

pamidur commented 2 years ago

I guess this should cover it https://github.com/pamidur/aspect-injector/pull/148 Feel free to reopen if needed