pamidur / aspect-injector

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

Interface Triggers #143

Closed zejji closed 3 years ago

zejji commented 3 years ago

Consider the following interface:

public interface IRequiresPermissionCheck
{
    public void CheckPermissions();
}

By creating the following aspect, it is possible to automatically call the CheckPermissions() method prior to any method call within a class that implements IRequiresPermissionCheck:

[Aspect(Scope.Global)]
[Injection(typeof(ApplyPermissionCheckAttribute))]
public class ApplyPermissionCheckAttribute : Attribute
{
    [Advice(Kind.Before, Targets = Target.Method)]
    public void BeforeMethod([Argument(Source.Instance)] object instance)
    {
        if (instance is IRequiresPermissionCheck rc)
        {
            rc.CheckPermissions();
        }
        else
        {
            throw new InvalidCastException(
                $"{instance.GetType().Name} must implement {nameof(IRequiresPermissionCheck)} in order to apply the {nameof(ApplyPermissionCheckAttribute)}.");
        }
    }
}

Is it possible to restrict an aspect so that it can only be applied to classes implementing a certain interface, i.e. a via a compile-time check rather than throwing a runtime exception?

Update: While googling I noticed a similar feature referenced in the PostSharp documentation: https://doc.postsharp.net/aspect-validation

pamidur commented 3 years ago

Hi @zejji , as of now there is no such possibility. I will make this issue to be FeatureRequest.

Could you please give some info? Is IRequiresPermissionCheck in your control and you can modify it? Like

[Injection(typeof(ApplyPermissionCheckAttribute))] //won't do anything now, but shows possible implementation approach
public interface IRequiresPermissionCheck
{
    public void CheckPermissions();
}
pamidur commented 3 years ago

with v2.5.0-pre1 you could try something like the following

Please note:

[Aspect(Scope.Global)]
public class ApplyPermissionCheck
{
    [Advice(Kind.Before, Targets = Target.Method)]
    public void BeforeMethod([Argument(Source.Instance)] object instance, [Argument(Source.Metadata)] MethodBase metadata)
    {
        //Alternative to ProragationFilter
        //if (metadata.Name.Contains(nameof(IRequiresPermissionCheck.CheckPermissions)))
        //    return;

        ((IRequiresPermissionCheck)instance).CheckPermissions();
    }
}

[Injection(typeof(ApplyPermissionCheck), PropagationFilter = "^(?=.)((?!" + nameof(CheckPermissions) + ").)*$")]
public interface IRequiresPermissionCheck
{
    void CheckPermissions();
}

public class TestClass : IRequiresPermissionCheck
{
    public void CheckPermissions()
    {
        Console.WriteLine("Allowed");
    }

    public void Do()
    {
        Console.WriteLine("Done");
    }
}

class Program
{
    static void Main(string[] args)
    {
        new TestClass().Do();
    }
}

I'd like to hear feedback. Please let me know how it works, is it convenient or not

zejji commented 3 years ago

@pamidur - Many thanks for getting back to me - much appreciated!

I tried the approach you suggested (using the metadata check on the basis that it is more readable) and can confirm it works successfully.

In terms of design, I wonder whether the CompileTimeValidate approach used by PostSharp might be cleaner? I had four specific thoughts regarding the implementation in your post above, some of which are related to use of AspectInjector in a team:

  1. While I am personally comfortable with regex, I know plenty of junior programmers who aren't and therefore the use of the PropagationFilter attribute combined with a regex might obscure what is going on.
  2. Perhaps one way of dealing with the recursion problem would be to disable it by default?
  3. One worry is that using interface triggers rather than a marker attribute on the implementing class (i.e. TestClass in your example) might hide the intended behaviour for team members not familiar with AspectInjector. In contrast, most people will know e.g. the concept of ASP.NET Core's Authorize attribute, so use of an attribute will likely be instantly recognizable. However, using the interface trigger approach does have the significant advantage that it prevents users from implementing the IRequiresPermissionCheck interface but forgetting to apply the attribute. This was something I realised I had forgotten to do on one class I was in the middle of writing!
  4. The interface trigger approach does not provide an easy way to apply permission checks to some methods in the class, but not others. A marker attribute, on the other hand, can be applied at either class or method level.
pamidur commented 3 years ago

So in fact I understood you wrong from the very beginning. It might be because I was thinking about InterfaceTriggers long before :)

What you need is -> Aspect injection is still triggered by an Attribute. However it is only applied if a target has specific Interface implementation. Why not simply ignore other targets?

        if (instance is IRequiresPermissionCheck rc)
        {
            rc.CheckPermissions();
        }

        // simply ignore everything else

For InterfaceTriggers feature:

  1. Agree
  2. Recursion is only there because Advice method makes it happen
  3. It is as obscure as an Attribute. If think about it neither Attribute nor Interface do any magic in regular c#
  4. Agree. It simply an additional option. One can use Attributes where needed. We might think about extending PropagateTo filter (not the regex one)

CompileTimeValidate is executed in compile time, which I'd like to avoid in AspectInjector because:

zejji commented 3 years ago

@pamidur - Thanks for your reply. The reason I though it preferable not just to ignore other targets that don't implement IRequiresPermissionCheck is that I would prefer to catch any inappropriate usage of the attribute at compile-time rather than runtime if possible. However, I completely understand your issues with the CompileTimeValidate idea.

One final thought - although I haven't had a chance to investigate the source code in details, some validation appears to done currently at compile time. Perhaps there could be an additional parameter to the Injection attribute which specifies a required interface to be implemented by the target class? Just an idea - I don't know whether this is either (i) possible or (ii) would be helpful enough for other users to consider implementing.

pamidur commented 3 years ago

I was thinking about additional Propagation options to allows only certain types, like PropagateToTypes=[typeof(IRequiresPermissionCheck)]. This should work for methods too, restricting by return type. But it won't throw compile time error, it will just ignore incorrect attribute assignment attempts.

So for restricting targets I think it is possible (won't break anything) to add new field to InjectionAttribute - RestrictToTypes=[typeof(IRequiresPermissionCheck)], combining that with your regular AttributeUsage(Class) might fulfill your requirements.

Please feel free to contribute)

pamidur commented 3 years ago

Released in 2.5.0

zejji commented 3 years ago

Great - thanks for the heads-up. Will give this a try!