Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to modify the registration pipeline from a module or other external mechanism #1211

Closed
xumix opened this issue Oct 8, 2020 · 18 comments

Comments

@xumix
Copy link

xumix commented Oct 8, 2020

Problem Statement

It is currently unclear how to add some Aspects to components if the component type is not known at the registration time.
For example some aspects like Logging and Caching are perfect for this.
Is there any way to conditionally enable interception after the registration?

So I want to add caching for my methods in all components if some conditions are met, like Attribute is used on method or type.

Current api for this:

builder.RegisterType<SomeAdapter>().As<ISomeAdapter>().EnableClassInterceptors().InterceptedBy(typeof(CacheInterceptor)).SingleInstance();

Desired Solution

Add ability to modify registration in some pipeline/add interceptors after component registration.
There is no way to enable interception for some particular components in some common core module
The desired api would look like this:

public class AspectSupportModule : Module
    {
       // Not sure if this is the right place, maybe it is already possible to do this in another method?
        protected override void AttachToComponentRegistration(
            IComponentRegistryBuilder componentRegistry,
            IComponentRegistration registration)
        {
             if (someCondition)
{
    registration.EnableClassInterceptors();
}
        }

Alternatives You've Considered

As of now, I added my custom module to intercept object activation but this solution does not look very robust

public class AspectSupportModule : Module
    {
        private static readonly Dictionary<Type, Type> Aspects = new Dictionary<Type, Type>
        {
            {
                typeof(LoggedAttribute), typeof(LoggingInterceptor)
            },
            {
                typeof(CachedAttribute), typeof(CacheInterceptor)
            }
        };

        private readonly ProxyGenerator generator;

        public AspectSupportModule()
        {
            generator = new ProxyGenerator();
        }

        /// <inheritdoc />
        protected override void Load(ContainerBuilder builder)
        {
            base.Load(builder);
            foreach (var aspect in Aspects)
            {
                builder.RegisterType(aspect.Value);
            }
        }

        protected override void AttachToComponentRegistration(
            IComponentRegistryBuilder componentRegistry,
            IComponentRegistration registration)
        {
            var type = registration.Activator.LimitType;

            var aspects = GetInterceptors(type).ToArray();
            if (aspects.Length == 0)
            {
                return;
            }

            var ctors = type.GetConstructors();
            var infos = ctors.First().GetParameters();

            // Attach to the registration's pipeline build.
            registration.PipelineBuilding += (sender, pipeline) =>
            {
                pipeline.Use(PipelinePhase.Activation,
                    MiddlewareInsertionMode.EndOfPhase,
                    (context, next) =>
                    {
                        if (context.Instance is IProxyTargetAccessor)
                        {
                            return;
                        }

                        var param = new List<object>();

                        if (ctors.Length > 0 && infos.Length > 0)
                        {
                            foreach (var info in infos)
                            {
                                param.Add(context.Resolve(info.ParameterType));
                            }
                        }

                        var proxyGenerationOptions = new ProxyGenerationOptions();

                        var interceptors = aspects.Select(context.Resolve).Cast<IInterceptor>().ToArray();

                        var proxy = generator.CreateClassProxy(registration.Activator.LimitType,
                            proxyGenerationOptions,
                            param.ToArray(),
                            interceptors: interceptors);

                        context.Instance = proxy;
                    });
            };
        }
}

Additional Context

It is possible to forcefully add interception(proxy generation) for all components registered for some service, but it is not desired since only some of the registered components may have actual interceptors needed

@tillig
Copy link
Member

tillig commented Oct 8, 2020

It appears this is not specifically about interceptors so much as the ability to modify a registration conditionally after registration.

I feel like we may have another issue around here somewhere with a similar request. Again, not specifically I need to add interceptors conditionally, but more about I want to modify registrations conditionally. Let me see if I can find it.

@tillig
Copy link
Member

tillig commented Oct 8, 2020

Hmmm. I can't find the other issue.

However, it seems like there are a few things being requested here and I don't know that it's "one issue."

  • Better documentation on interceptors: If you ignore the "how do I add them in this custom way?" part of the question, I'm not clear about what sort of docs you think are missing. If the docs that are missing are the docs that explain how to conditionally add interceptors... those docs aren't missing, they don't exist because it's not a feature we have. If it's something else, please be specific and file an issue for it in the Documentation repo.
  • Conditional attachment of interceptors: That is, "attach an interceptor if some condition is true." That sounds like an interesting feature. If you'd like that, [the Autofac.Extras.DynamicProxy repo] is a good idea. I think Use proxy generator options "Selector" for interceptor selection Autofac.Extras.DynamicProxy#29 might actually be similar to this. I also added Conditional registration for interceptors Autofac.Extras.DynamicProxy#42 just now for it because it seems interesting.
  • Ability to affect the registration pipeline: This is the part that I think we've seen before but I can't find it. Maybe @alistairjevans or @alsami remember? Point being, the part where a Module or something outside the registration mechanism can "attach to" the registration and affect it, possibly before the full container/scope is built.

Given the part of the issue we'd look at here is that registration pipeline part; and that the other two parts either need or already have separate issues; I'm going to rename this issue to indicate it's about affecting the registration pipeline.

@tillig tillig changed the title Add more documentation on interceptors and how to use proxying or add more api for interceptor registration Ability to modify the registration pipeline from a module or other external mechanism Oct 8, 2020
@tillig
Copy link
Member

tillig commented Oct 8, 2020

It could be that we just need to better document the pipeline / middleware solution that is shown here as the "workaround" if that's the recommended solution. If that's the case, it might be interesting to see if we can figure out some sort of common usage pattern to remove some of the extra boilerplate. If there is any. Attaching an aspect-oriented interceptor to something conditionally seems like maybe it'd be more than two lines of code and that's OK.

@xumix
Copy link
Author

xumix commented Oct 8, 2020

@tillig Thanks for explanation and decomposition.
My current solution conflicts with the components that are actually registered with Autofac api since I cannot make a proxy for proxy.

At the moment it looks for me like the most generic approach would be smth like IComponentRegistryBuilder having methods to modify existing registrations.
So the api would look like this:

builder.RegisterCallback(crb =>
            {
                foreach (var registration in crb.Registrations)
                {
                    if (conditionSelector(registration.Component))
                    {
                        registration.Component
                                    .EnableClassInterceptors()
                                    .InterceptedBy(Aspects.Select(s => s.Value));
                    }
                }
            });

@alistairjevans
Copy link
Member

The approach to modifying the registration pipeline in a module, where you attach to the PipelineBuilding event, and add your middleware there, is the required approach to adding behaviour to a registration 'after the fact'.

I'm not super keen around exposing the ContainerBuilder on a registration that has already been created; the builder syntax only really works before the registration has been created.

The log4net example is probably the best guide we have for adding to the pipeline in a module.

@xumix
Copy link
Author

xumix commented Oct 9, 2020

@alistairjevans Yes, I looked into this example and used it as a starting point, the problem is, I cannot modify already created registrations or use Autofac's api to fully reimplement the interception functionality

@alistairjevans
Copy link
Member

I suspect this may be better addressed in the DynamicProxy issue (autofac/Autofac.Extras.DynamicProxy#42), by exposing the middleware we have there more directly, to allow custom behaviour to add that middleware (with a selector for example) yourself rather than needing to go via our ContainerBuilder methods. Probably needs more thought to be honest, but I suspect most of the thinking is related to specifically to the interceptor code in DynamicProxy.

@tillig
Copy link
Member

tillig commented Oct 9, 2020

I think part of the challenge is that the way Autofac registers things, it's not a simple collection like IServiceCollection so there's this sort of incorrect assumption that one can "just" iterate through some registrations and update them later. With the various callbacks and things that occur, not to mention things that are handled with registration sources (like assembly scanning), that's really not how it works.

Anyway, if the answer to "how do I modify a registration later?" is "you modify the pipeline" then maybe the answer is that we need an FAQ or something in the docs to dive deeper into it, explain why that's the case (ie, the whole 'it's not simply a collection of registrations' thing), then show one or two examples in detail.

@xumix
Copy link
Author

xumix commented Nov 18, 2020

@tillig that would be great, but at the same time I've tried modifying the pipeline and failed.
Is there any chance to add modification api for pipelines/registry? like here #1211 (comment)

@tillig
Copy link
Member

tillig commented Nov 18, 2020

I wouldn't be against having a simpler API for modifying a registration pipeline if we can figure out some common patterns and use cases that might be made easier with some convenience methods. However, as with any new public API we might provide, we really need to think through how it will be used, how it can be misused, and what the cost of ownership might be on the Autofac team. Once we add something, we own it and the support and the documentation and everything forever, so it really needs to be thought out pretty well.

@alistairjevans
Copy link
Member

I've taken a look at our options here, and I'm going to split my thoughts into two.

Modifying Pipelines in AttachToComponentRegistration

If you want to add to the Resolve Pipeline after a registration has already been constructed, the PipelineBuilding event is the way to go. That being said, I can see how the use of that event is a little confusing, in that it deviates from the standard ConfigurePipeline method used when actually setting up the registration.

So, I'd propose adding a ConfigurePipeline method to IComponentRegistration, which wraps the attach to PipelineBuilding and adds a check that the registration is in the right state.

This makes the module's code look more like this:

protected override void AttachToComponentRegistration(
            IComponentRegistryBuilder componentRegistry,
            IComponentRegistration registration)
{
    // Configure the registration's pipeline.
    registration.ConfigurePipeline(pipeline =>
    {
        pipeline.Use(PipelinePhase.Activation,
            MiddlewareInsertionMode.EndOfPhase,
            (context, next) =>
            {
                // Do the middleware stuff.
                next(context);
            });
    });
}

I reckon that will reduce confusion and make it easier for people to find the way to do this.

Fundamentally though, the internal Autofac methods that add their own middleware and options are not available. Which leads me onto my second point.

Modifying a Registration using Autofac's Own Setup Methods

Fundamentally, it's important to understand how much Autofac's registration customisation methods are tied to the generic type arguments determined by the method on ContainerBuilder you call to get started (i.e. RegisterType, RegisterGeneric, etc). The typed IRegistrationBuilder generic arguments are what define the set of permissible extension methods on each builder.

We do not track that IRegistrationBuilder object; instead the builder just adds the deferred callbacks it needs to configure registrations manually. Even if we did track the builder, we would lose all the generic type arguments as soon as we converted it to a ComponentRegistration, by which point everything has more or less been made 'concrete', and we no longer retain the type arguments we had before.

I can't think of a way to get access to a typed IRegistrationBuilder that can successfully modify a registration inside AttachToComponentRegistration; it's basically too late at that point. We would need a method on IComponentRegistryBuilder that was something like GetBuilder, which returned an IRegistrationBuilder...but we don't know what generic type arguments to use!

If we tried to implement this sort of change internally it would probably involve refactoring most of the registration extensions to be hung off a generic type that implements IResolvePipelineOwner instead of the IRegistrationBuilder directly, but involve ripping up huge swathes of current code, while also introducing a number of potentially confusing and/or foot-gun situations where people try to modify registrations to late, or the methods they want aren't available.

I'd suggest that in situations like the original raised issue, I would place the responsibility on the actual registration builder to support better registration filtering, and particularly on specific integrations like DynamicProxy, where we can add conditional
interception as expressed in autofac/Autofac.Extras.DynamicProxy#42.

Wrapping Up

I'm going to raise a PR to add ConfigurePipeline to IComponentRegistration, which is nice and simple. But I propose not doing anything on the second point in core Autofac, and addressing such needs in the integration library that needs them.

@tillig
Copy link
Member

tillig commented Dec 6, 2020

The conditional pipeline modification mentioned in the second point seems to overlap a bit with #1235 in that we can add more specific support for filtering at registration time rather than trying to modify post-facto. Makes sense to me.

@xumix
Copy link
Author

xumix commented Dec 8, 2020

@alistairjevans Thanks for the deep insight! I can't really grasp what is the purpose of ConfigurePipeline?
How will it simplify my module code for instance?

@alistairjevans
Copy link
Member

@xumix, your module code will get a little simpler, but not much. Fundamentally, the module you've got here is the right way to do things, all we've done is make it slightly easier to modify the pipeline.

Basically, it'll now look like this:

public class AspectSupportModule : Module
{
    private static readonly Dictionary<Type, Type> Aspects = new Dictionary<Type, Type>
    {
        {
            typeof(LoggedAttribute), typeof(LoggingInterceptor)
        },
        {
            typeof(CachedAttribute), typeof(CacheInterceptor)
        }
    };

    private readonly ProxyGenerator generator;

    public AspectSupportModule()
    {
        generator = new ProxyGenerator();
    }

    /// <inheritdoc />
    protected override void Load(ContainerBuilder builder)
    {
        base.Load(builder);
        foreach (var aspect in Aspects)
        {
            builder.RegisterType(aspect.Value);
        }
    }

    protected override void AttachToComponentRegistration(
        IComponentRegistryBuilder componentRegistry,
        IComponentRegistration registration)
    {
        var type = registration.Activator.LimitType;

        var aspects = GetInterceptors(type).ToArray();
        if (aspects.Length == 0)
        {
            return;
        }

        var ctors = type.GetConstructors();
        var infos = ctors.First().GetParameters();

        // Attach to the registration's pipeline build.
        // This is the change here.
        registration.ConfigurePipeline(pipeline =>
        {
            pipeline.Use(PipelinePhase.Activation,
                MiddlewareInsertionMode.EndOfPhase,
                (context, next) =>
                {
                    if (context.Instance is IProxyTargetAccessor)
                    {
                        return;
                    }

                    var param = new List<object>();

                    if (ctors.Length > 0 && infos.Length > 0)
                    {
                        foreach (var info in infos)
                        {
                            param.Add(context.Resolve(info.ParameterType));
                        }
                    }

                    var proxyGenerationOptions = new ProxyGenerationOptions();

                    var interceptors = aspects.Select(context.Resolve).Cast<IInterceptor>().ToArray();

                    var proxy = generator.CreateClassProxy(registration.Activator.LimitType,
                        proxyGenerationOptions,
                        param.ToArray(),
                        interceptors: interceptors);

                    context.Instance = proxy;
                });
        });
    }
}

It's only the way you attach to the pipeline that has simplified slightly.

@iyilm4z
Copy link

iyilm4z commented Apr 10, 2023

Hi @alistairjevans,

Thanks for your solution.
I followed it and it's working with classes that have virtual methods, but not working with interfaces:/
If you have time may I ask you to review my code, please? I just want to be sure whether I'm not missing something.

What I want to do is apply a unit of work interceptor based on some conventions. So, if a class is assignable from the IUnitOfWorkEnabled interface or if it has the UnitOfWork attribute, or if any of its methods has the UnitOfWork attribute, then apply a unit of work interceptor.

Thank you in advance!

public class UnitOfWorkAttribute : Attribute { }

public interface IUnitOfWorkEnabled { }

public abstract class UowServiceBase : IUnitOfWorkEnabled { }

public abstract class UowRepositoryBase<TEntity> : IUnitOfWorkEnabled { }

public class FooService : UowServiceBase, IFooService { }

public class FooRepository : UowRepositoryBase<Foo>, IFooRepository { }

public class FooManager
{
    [UnitOfWork]
    public virtual void DoFoo() { }
}

public static class UnitOfWorkHelper
{
    public static bool IsUnitOfWorkType(TypeInfo typeInfo)
    {
        if (HasUnitOfWorkAttribute(typeInfo) || AnyMethodHasUnitOfWorkAttribute(typeInfo))
        {
            return true;
        }

        return typeof(IUnitOfWorkEnabled).GetTypeInfo().IsAssignableFrom(typeInfo);
    }

    private static bool AnyMethodHasUnitOfWorkAttribute(TypeInfo typeInfo)
    {
        return typeInfo
            .GetMethods(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)
            .Any(HasUnitOfWorkAttribute);
    }

    private static bool HasUnitOfWorkAttribute(MemberInfo methodInfo)
    {
        return methodInfo.IsDefined(typeof(UnitOfWorkAttribute), true);
    }
}

public class UnitOfWorkInterceptor : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        using (var uow = _uowManager.Begin())
        {
            invocation.Proceed();
            uow.Complate();
        }
    }
}

public static class UnitOfWorkRegistrar
{
    public static void ConfigureInterceptor(IComponentRegistration registration)
    {
        var classToIntercept = registration.Activator.LimitType;

        if (!UnitOfWorkHelper.IsUnitOfWorkType(classToIntercept.GetTypeInfo()))
        {
            return;
        }

        var constructors = classToIntercept.GetConstructors();

        var parameters = !constructors.Any()
            ? Array.Empty<ParameterInfo>()
            : constructors.First().GetParameters();

        registration.ConfigurePipeline(
            pipeline =>
            {
                pipeline.Use(
                    PipelinePhase.Activation,
                    MiddlewareInsertionMode.EndOfPhase,
                    (context, next) =>
                    {
                        if (context.Instance is IProxyTargetAccessor)
                        {
                            return;
                        }

                        var arguments = new List<object>();

                        if (constructors.Length > 0 && parameters.Length > 0)
                        {
                            arguments.AddRange(
                                parameters.Select(x => context.Resolve(x.ParameterType))
                            );
                        }

                        var uowInterceptor = (IInterceptor)context.Resolve(
                            typeof(UnitOfWorkInterceptor)
                        );

                        var proxyClass = _generator.CreateClassProxy(
                            classToIntercept,
                            arguments.ToArray(),
                            uowInterceptor
                        );

                        context.Instance = proxyClass;
                    }
                );
            }
        );
    }

    public static void RegisterTypes(ContainerBuilder builder)
    {
        builder.RegisterType<UnitOfWorkInterceptor>();
        builder.RegisterType<FooRepository>().As<IFooRepository>();
        builder.RegisterType<FooService>().As<IFooService>();
        builder.RegisterType<FooManager>();
    }
}

public class MyAppModule : Module
{
    protected override void AttachToComponentRegistration(
        IComponentRegistryBuilder componentRegistryBuilder,
        IComponentRegistration registration
    )
    {
        UnitOfWorkRegistrar.ConfigureInterceptor(registration);
    }

    protected override void Load(ContainerBuilder builder)
    {
        UnitOfWorkRegistrar.RegisterTypes(builder);
    }
}

public class UseCases {
   private readonly IFooService _fooService
   private readonly IFooRepository _fooRepository 
   private readonly FooManager _fooManager

   public UseCases(IFooService fooService, 
      IFooRepository fooRepository, 
      FooManager fooManager){
      _fooService = fooService;
      _fooRepository = fooRepository;
      _fooManager = fooManager;
   }

   public void Case1(){
      _fooService.DoFoo();
   }

   public void Case2(){
      _fooRepository.DoFoo();
   }

   public void Case3(){
      _fooManager.DoFoo();
   }
}

@alistairjevans
Copy link
Member

At a glance I can't see anything here Autofac-related that seems off; you said "not working with interfaces", but you didn't indicate in what way it wasn't working? Is Autfac throwing an exception? Is the pipeline entry not being invoked? More detail required please.

@iyilm4z
Copy link

iyilm4z commented Apr 11, 2023

Hi @alistairjevans,
Sorry for not providing enough detail.
The interceptor method was not called for use cases Case1 and Case2 (the DoFoo() methods of _fooService and _fooRepository were not called in the interceptor). However, it's called for the use of Case3 as expected.

Then I realized I didn't pass the additionalInterfacesToProxy argument. After passing it, all cases worked.
I used RegistrationExtensions.cs#L169 code for getting the additionalInterfacesToProxy list. Do you think it's okay?

I also wonder why we use MiddlewareInsertionMode.EndOfPhase instead of MiddlewareInsertionMode.StartOfPhase and don't call the next(ctxt) as in the RegistrationExtensions.cs.

My final ConfigureInterceptor method code is as below:

public static void ConfigureInterceptor(IComponentRegistration registration)
{
    var classToProxy = registration.Activator.LimitType;
    
    if (!UnitOfWorkHelper.IsUnitOfWorkType(classToProxy.GetTypeInfo()))
    {
        return;
    }

    var constructors = classToProxy.GetConstructors();

    var parameters = !constructors.Any()
        ? Array.Empty<ParameterInfo>()
        : constructors.First().GetParameters();
    
    var additionalInterfacesToProxy = classToProxy
        .GetInterfaces()
        .Where(ProxyUtil.IsAccessible)
        .ToArray();

    registration.ConfigurePipeline(p => p.Use(PipelinePhase.Activation, MiddlewareInsertionMode.EndOfPhase,
        (context, next) =>
        {
            if (context.Instance is IProxyTargetAccessor)
            {
                return;
            }

            var arguments = new List<object>();

            if (constructors.Length > 0 && parameters.Length > 0)
            {
                arguments.AddRange(parameters.Select(x => context.Resolve(x.ParameterType)));
            }

            var unitOfWorkInterceptor = (IInterceptor)context.Resolve(typeof(UnitOfWorkInterceptor));

            context.Instance = _proxyGenerator.CreateClassProxy(classToProxy, additionalInterfacesToProxy,
                ProxyGenerationOptions.Default, arguments.ToArray(), unitOfWorkInterceptor);
        }));
}

@tillig
Copy link
Member

tillig commented Nov 6, 2023

This issue has been open for quite some time without any traction. Much as we understand the challenges presented here, we also get very few substantial contributions and we don't have time to really dig into non-trivial issues like this. As such, we're closing this issue. If someone feels like they really want this, we would love to see a PR where this gets addressed.

@tillig tillig closed this as completed Nov 6, 2023
@tillig tillig added the wontfix label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants