Add intercept on decorator

[idiotsky]

Add support for register service with Interceptor and multiple decorator, apply Interceptor to the outermost decorator,

[tillig]

A description on the PR here, possibly explaining it and linking back to the source issue, would be good. That way in a year from now when we come back and look to see what the point of the PR was (say, during supporting an issue) we have that to reference.

(Update the actual PR description; don't respond with the description here as a comment. Thanks!)

[codecov]

Codecov Report

Attention: Patch coverage is 97.01493% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.31%. Comparing base (2115f37) to head (b0cc325).

Files with missing lines	Patch %	Lines
src/Autofac.Extras.DynamicProxy/ProxyHelpers.cs	95.91%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #55      +/-   ##
===========================================
+ Coverage    93.44%   95.31%   +1.86%     
===========================================
  Files            1        3       +2     
  Lines          122      128       +6     
  Branches        22       22              
===========================================
+ Hits           114      122       +8     
+ Misses           4        3       -1     
+ Partials         4        3       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tillig]

From what I gather, this PR needs to go through first to help this one. I probably won't look at this one until that one is done.

[idiotsky]

The new changes is base on Autofac 8.2.0, any feed back let me know, I'll try to fix it.

[tillig]

I think the premise is good but I'm worried about the confusing syntax to enable this to happen. We might need to iterate over that a little to get it right.

[tillig]

Can we use NSubstitute here instead of Moq? I am a fan of supporting open source, but some of the phone-home stuff that was implemented leaves Moq on vulnerability lists for some secure enterprises. In places where we already had Moq, it should be pinned to the version just before that was implemented; for tests that are adding new mocking frameworks, we should go NSubstitute.

[tillig]

I find this syntax to be confusing. We already registered a type to have interceptors, but then there's another line to indicate interception.

I feel like it should be maybe more like...

builder.RegisterDecorator<T, U>().InterceptedBy(typeof(Z));

Something analogous to the current syntax.

[idiotsky]

I find this syntax to be confusing. We already registered a type to have interceptors, but then there's another line to indicate interception.

I feel like it should be maybe more like...

builder.RegisterDecorator<T, U>().InterceptedBy(typeof(Z));

Something analogous to the current syntax.

If I change it to all in one line Is it ok? like this,

builder.RegisterType<Interceptable>().InterceptedBy(typeof(StringMethodInterceptor)).As<IPublicInterface>();

but I saw other file like InterfaceInterceptionWithPropertyInjectionFixture.cs

use

 builder
            .RegisterType<InterceptableWithProperty>()
            .PropertiesAutowired()
            .EnableInterfaceInterceptors()
            .InterceptedBy(typeof(StringMethodInterceptor))
            .As<IPublicInterface>();

should we keep same style?

[tillig]

The thing that makes the new syntax weird is that it's not connected to a RegisterType or RegisterDecorator. I think if it reads like a sentence - "register X as Y intercepted by Z" - that would be consistent and easier to understand.

[tillig]

This is the bit currently pending. I'm not sure we want a syntax where it's some sort of builder level registration. I find the syntax to be challenging because there are now two ways to register things. It seems like if the interception needs to apply to a decorator, it should be attached to the decorator registration.

[tillig]

Sorry, I've only had my phone, away for the holiday season, and I'm trying to answer quickly to help but also haven't been able to really look at anything on a big monitor and provide robust feedback.

In the test highlighted here, I see the new interception syntax like this:

// Register the decorator.
builder.RegisterDecorator<Decorator, IPublicInterface>();

// Register a type that needs to be intercepted.
builder
  .RegisterType<Interceptable>()
  .EnableInterfaceInterceptors()
  .InterceptedBy(typeof(StringMethodInterceptor))
  .As<IPublicInterface>();

// This is the confusing/inconsistent part - this standalone interception line.
builder.EnableInterfaceInterceptors<IPublicInterface>();

The thing that's inconsistent here is that, generally speaking, the syntax for registering things is all built up and attached to one registration.

// Start a registration
builder

  // Register a type `Interceptable`
  .RegisterType<Interceptable>()

  // I want to enable interface interception for this type
  .EnableInterfaceInterceptors()

  // Say what that type is intercepted by
  .InterceptedBy(typeof(StringMethodInterceptor))

  // Expose the type as `IPublicInterface`
  .As<IPublicInterface>();

It's a chained builder style syntax. I know that when I register the interceptor it will be intercepting things for RegisterType. I know the As applies to that registration.

The new line here:

builder.EnableInterfaceInterceptors<IPublicInterface>();

This is inconsistent. It reads like it's enabling some sort of global interception, but if I read that in context of the other syntax...

• Why would I need .EnableInterfaceInterceptors() on the RegisterType<T> call if I'm supposed to use this new standalone builder.EnableInterfaceInterceptors<T>() syntax?
• What happens if I drop .EnableInterfaceInterceptors() on the RegisterType<T> call and only use the new standalone syntax?
• I'm enabling interface interception, but by what? What's intercepting it? How do I control that?

I know that you can probably answer that here in comments, but that's not the point - the point is that someone later reading the code or trying to figure out how to make interception work is going to have these questions. The syntax shouldn't be confusing like that or cause folks to wonder what it means.

That's what I'm stuck on here.

If the goal of this PR is to allow interface interception on decorators, I would expect to see something more like:

[Fact]
public void InterceptsPublicInterfacesUseGenericMethod()
{
  ContainerBuilder builder = new();
  builder.RegisterType<StringMethodInterceptor>();

  // Same support for adding interception to a decorator
  // that we have for existing registrations. Familiar, consistent
  // syntax.
  builder
    .RegisterDecorator<Decorator, IPublicInterface>()
    .EnableInterfaceInterceptors()
    .InterceptedBy(typeof(StringMethodInterceptor);

  // Same syntax as before.
  builder
    .RegisterType<Interceptable>()
    .EnableInterfaceInterceptors()
    .InterceptedBy(typeof(StringMethodInterceptor))
    .As<IPublicInterface>();

  // ... and the rest of the test
}

Perhaps a good test to verify the syntax works is to see if we can get the decorator to be intercepted by a different interceptor than the thing being decorated.

[Fact]
public void DifferentInterceptorsForEachComponent()
{
  ContainerBuilder builder = new();
  builder.RegisterType<StringMethodInterceptor>();
  builder.RegisterType<CallCountInterceptor>();

  // Decorator gets intercepted by one thing
  builder
    .RegisterDecorator<Decorator, IPublicInterface>()
    .EnableInterfaceInterceptors()
    .InterceptedBy(typeof(CallCountInterceptor);

  // Base service gets intercepted by something else
  builder
    .RegisterType<Interceptable>()
    .EnableInterfaceInterceptors()
    .InterceptedBy(typeof(StringMethodInterceptor))
    .As<IPublicInterface>();

  // ... and the rest of the test
}

There are other features, too, that need to work the same if we add this.

• Being able to add proxy generation options when enabling interception for the decorator.
• Being able to mark the decorator with an attribute to select the interceptor.
• Possibly needing to differentiate between class and interface interceptors - can the class being decorated have EnableClassInterceptors but the decorator has EnableInterfaceInterceptors? What about the other way around?

That's the syntax and functionality that I'm stuck on, and it's why the standalone builder.EnableInterfaceInterceptors<T>() doesn't really work in the ecosystem.

[idiotsky]

Although all checks have passed, I still have a few questions about the code.
ProxyHelpers line 61

if (!proxiedInterfaces.Any())
{
    return;
}

Actually, this code lacks code coverage. My question is regarding line 52: after calling EnsureInterfaceInterceptionApplies, can we confirm that proxiedInterfaces will never be empty? if yes, I think we can remove line 61 to 64.
ProxyHelpers line 52

EnsureInterfaceInterceptionApplies(ctx.Registration);

// The instance won't ever _practically_ be null by the time it gets here.
var proxiedInterfaces = ctx.Instance!
    .GetType()
    .GetInterfaces()
    .Where(ProxyUtil.IsAccessible)
    .ToArray();

if (!proxiedInterfaces.Any())
{
    return;
}

ProxyHelpers line 127, this code lacks code coverage too, I just worry about may be this code will never reach too.

if (!implTypeInfo.IsClass)
{
    return Enumerable.Empty<Service>();
}

[tillig]

Re: Line 61 - the only difference I see is that when we check to see if interface interception applies, we're checking the service registration type, while when we check to see if there are any proxied interfaces, we're checking the actual instance type.

If memory serves, there was something with WCF proxy interception where we might end up with a service registration of, say, IWcfService but the object instance was a TransparentProxy so the registration would say, "Yes, the interception applies!" but then an extension like WCF integration would have set up the instance and that instance wouldn't support interception.

It's just my memory, though, I can't point you to a specific test or issue that indicated this. There was a lot with WCF proxies that wasn't awesome. I'm glad we don't have that so much in .NET Core but we still do support .NET Desktop.

If I had to guess, the way to test this would be to try creating a registration that has an event handler that replaces the instance with something like a TransparentProxy:

builder.RegisterType<MyType>()
       .As<IMyInterface>()
       .OnActivating(e => e.ReplaceInstance(new TransparentProxy()));

Something like that - the idea being the instance doesn't actually implement the interfaces the way we expect. I don't remember the exact syntax off the top of my head.

I won't hold up the PR on that. Some of these runtime things can be hard to replicate.

Re: Line 127 - I think you're right, the implementation type will only ever be a class so that line should never practically be hit. Nice catch!

Minor point - we probably want to make that ProxyHelpers class internal. I'm not sure we want to support folks outside the library calling it.

[idiotsky]

I already change ProxyHelpers to internal and remove line 127 and related code.
And also, I try
Add this to InterfaceInterceptorsFixture
'''
[Fact]
public void InterceptsPublicInterfacesWithReplacedInstance()
{
var builder = new ContainerBuilder();
builder.RegisterType();
builder
.RegisterType()
.InterceptedBy(typeof(StringMethodInterceptor))
.EnableInterfaceInterceptors()
.As()
.OnActivating(e => e.ReplaceInstance(new TransparentProxy()));
var container = builder.Build();
var obj = container.Resolve();
Assert.Equal("intercepted-PublicMethod", obj.PublicMethod());
}
public class TransparentProxy
{
}
'''
while debug the ctx.Instance( in ProxyHelpers line 55) still Interceptable not TransparentProxy when call container.Resolve(); it throws excpetion as following
Autofac.Core.DependencyResolutionException: An exception was thrown while activating Autofac.Extras.DynamicProxy.Test.InterfaceInterceptorsFixture+Interceptable.
---> System.InvalidCastException: Unable to cast object of type 'Castle.Proxies.IPublicInterfaceProxy' to type 'Interceptable'.

So may be this is not the case.

[tillig]

Well, like I said, I can't remember the exact syntax for TransparentProxy off the top of my head. I know when this is used in context of WCF, it somehow creates an object that implements the WCF service interface. It's not a super common use case, but it does happen.

Don't worry about coverage on that one line. I'm a little bit afraid of removing it, though, since, again, I have a vague memory from many years ago where we had to add that for something.

[idiotsky]

Well, like I said, I can't remember the exact syntax for TransparentProxy off the top of my head. I know when this is used in context of WCF, it somehow creates an object that implements the WCF service interface. It's not a super common use case, but it does happen.

Don't worry about coverage on that one line. I'm a little bit afraid of removing it, though, since, again, I have a vague memory from many years ago where we had to add that for something.

ok, nice, can you approve the pull request or any comment on it?

[tillig]

The comment on the syntax is what's holding this up.

[idiotsky]

The comment on the syntax is what's holding this up.
if you mean this

builder.RegisterDecorator<T, U>().InterceptedBy(typeof(Z));

the RegisterDecorator mehod is from autofac main project and currently no return value. so it can not be chained.