Assembly scanning registers private classes by default

[reservedwords]

According to the documentation, assembly scanning will by default register all public, concrete classes in an assembly. In fact, assembly scanning goes further than that and registers internal and nested private classes:

interface IPrivate
{
}

interface IInternal
{
}

internal class InternalClass : IInternal
{
}

internal class OuterClass
{
	private class PrivateClass : IPrivate
	{
	}
}

[Test]
public void AutofacAssemblyScanningRegistersInternalAndPrivateClasses()
{
	ContainerBuilder cb = new ContainerBuilder();
	cb.RegisterAssemblyTypes(typeof(InternalClass).Assembly)
		.AsImplementedInterfaces()
		.InstancePerLifetimeScope();

	var container = cb.Build();

	using (var scope = container.BeginLifetimeScope())
	{
		Assert.That(scope.Resolve<IInternal>(), Is.InstanceOf<InternalClass>());
		Assert.That(scope.Resolve<IPrivate>().GetType().Name, Is.EqualTo("PrivateClass"));
	}
}

The behaviour with internal classes seems sensible, and I'm happy to update the docs to reflect this, but registering nested private classes seems to violate the principle of least surprise. Assuming that private classes wouldn't be registered led to unexpected registrations and bugs in an application I was working on.

I think the default behaviour should exclude nested private classes. I'm happy to cook up a PR, but realise this is potentially a breaking change and wanted discussion before diving in.

I've observed this behaviour on Autofac 3.5.2 and 4.6.2, on full .NET Framework

[tillig]

Interesting find. We have some tests around assembly scanning. I should add some internal/private items and see about reproducing this.

[tillig]

Looks like we had just never tested this. I've updated things so it only locates public/nested public classes. Fix will be out in 4.7.1.

[Greg-Smulko]

Hi @tillig , the change in registering internal classes is a breaking change IMO, and it took me a while to figure out why the product started to fail in some weird places. :(

I'd be in favour to revert to the previous behaviour as @reservedwords suggested ("The behaviour with internal classes seems sensible, and I'm happy to update the docs to reflect this").

If you think that's not a good idea for any reason, I think that this change deserves a minor version bump, and not only a patch one, to follow the semver specification.

Also, I find registering the internal classes by RegisterAssemblyTypes() really useful, so if we could have some replacement for this, that would be great - assuming, that you really think that reverting this change is not a good idea.

To let you understand my scenario, I have a bunch of classes that implement an interface, and are registered as:

	ContainerBuilder cb = new ContainerBuilder();
	cb..RegisterAssemblyTypes(ThisAssembly)
                   .Where(t => typeof(IInternal).IsAssignableFrom(t))
                   .InstancePerDependency()
                   .As<IInternal>();

These classes have a lot of dependencies:

    internal class InternalClass : IInternal
    {
        public InternalClass (// here a lot of dependencies, that are internal
        )
        {
        }
    }

In a scenario that all these InternalClasses become public, all their dependencies (injected by a constructor) need to become public too. It creates a chain of changes from internal to public, which I don't really like to make.

Do you see any workaround for this, other than resigning from using RegisterAssemblyTypes and painfully register them one by one?

[tillig]

The behavior was really only intended to support public types and we only had tests actually validating the public type scanning so it's sort of hard to say from a long-term standpoint (e.g., before I was on the project) what the original intent was. However, I'm pretty confident internal and private wasn't the intent, because consider a different scenario:

	cb.RegisterAssemblyTypes(NotThisAssembly)
                   .Where(t => typeof(IInternal).IsAssignableFrom(t))
                   .InstancePerDependency()
                   .As<IInternal>();

The only time internal types make sense is when scanning the current assembly, but really no other assembly... unless there's also InternalsVisibleTo marked and we start behaving by that.

Further, there are a lot of other Autofac behaviors that only work with public types, like type and interface interceptors. Working with your exposed/public API by default makes sense.

Whether or not this is fixing a bug in behavior or breaking is debatable since the intended behavior only ever was to use the public API, so if folks were taking advantage of the incorrect behavior... it breaks that, sure, but it wasn't behavior that was intentional anyway.

But it's out there now, so it's a little moot. Folks have taken it and unlisting it really isn't going to happen at this point.

However, I do understand the desire to enable scanning of internal types so I'd like to offer a compromise:

What if we added an assembly scanning override that allows you to specify a flag/parameter that would enable scanning for non-public types? Something like:

	cb.RegisterAssemblyTypes(ThisAssembly, includeNonPublic: true)
                   .Where(t => typeof(IInternal).IsAssignableFrom(t))
                   .InstancePerDependency()
                   .As<IInternal>();

Would that get you what you need?

[tillig]

I suppose from a back-compat standpoint, I'd also be willing to invert that:

	cb.RegisterAssemblyTypes(ThisAssembly, publicOnly: true)
                   .Where(t => typeof(IInternal).IsAssignableFrom(t))
                   .InstancePerDependency()
                   .As<IInternal>();

[tillig]

@alexmg Before I make a change here, do you have any insight as to the original intent of the feature and a preference on how you think we should go?

[Greg-Smulko]

@tillig, thanks for answering so promptly.

A few random thoughts from my side:

• I really appreciate that you "understand the desire to enable scanning of internal types". I think it's a really valid scenario to wire up all dependencies in your assembly without a need to expose them publicly. Especially that you can do it manually (just by using builder.RegisterType<InternalClass>()), so I don't think that the more generic way of doing it (RegisterAssemblyTypes) should be prohibited.
• Regarding type and interface interceptors, according to docs, it's possible to proxy internal interfaces too - unless I misunderstood something.
• If you'd like to have a consistent behaviour between public and internal, maybe Autofac should rather support registration of both, rather than only public ones everywhere?
• I'd be in favour of the RegisterAssemblyTypes(ThisAssembly, publicOnly: true) option because of a back-compat, or maybe even instead of bool publicOnly use an enum/flags, that gives more flexibility for future extensions? Something like RegisterAssemblyTypes(ThisAssembly, Access.Public | Access.Internal | Access.ProtectedInternalWhatever)

[tillig]

@Greg-Smulko It is possible to proxy internal interfaces, but note the docs also say:

Use Public Interfaces
Interface interception requires the interface be public (or, at least, visible to the dynamically generated proxy assembly). Non-public interface types can’t be intercepted.

If you want to proxy internal interfaces, you must mark the assembly containing the interface with [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")].

That's why I mentioned earlier

The only time internal types make sense is when scanning the current assembly, but really no other assembly... unless there's also InternalsVisibleTo marked and we start behaving by that.

But, I thought of a better compromise to this that will help:

• Revert to the original behavior. I think back compat probably wins out here, though I still think the behavior is incorrect.
• Add a LINQ extension RegisterAssemblyTypes(x).PublicOnly() - where PublicOnly() could handle the filtering as an out-of-the-box Where() clause that people can opt into. This also means we don't have to deal with Boolean parameters or extensibility concerns - if folks don't like what's in PublicOnly() they can write their own Where() clause.
• Update the docs to explain that all types found in the assembly will be registered unless you opt in to the public only behavior.
• Increment to 4.8 since PublicOnly will be a new feature in the public API for Autofac.

Does that sound reasonable? I hope that will work for folks.

I'd still be interested in what @alexmg has to say, but I'll get working on this.

[tillig]

OK, folks, check this out and see what you think. I added the PublicOnly() opt in filter and reverted to back compat behavior. The unit tests demonstrate usage.

If that looks good, I'll cut a quick 4.8 to avoid too many people getting bitten with this.

[Greg-Smulko]

@tillig , it looks good to me. I'm happy that backwards compatibility is not broken anymore. :)

[exiton3]

Hello guys, I've updated nuget of Autofac from 4.6.2 to 4.7.1 and now assemblyscanner ignores internal classes what is not good because I don't want to make them public, the implementation of public interfaces it should be hidden by internal modifier. How can I fix this?? I see the solution from @tillig how can I get it via nuget it is not available

[nicolaj-hartmann]

@exiton3 think you just have to wait a few hours :) tillig is based in the USA so he is probably at work or getting to work now.

@tillig Really nice job and nice of you guys to fix it right away.

[tillig]

US west coast, so UTC-7 right now with daylight saving. Also, Autofac isn't my job, so sometimes it gets to wait a bit as I actually do things that pay the bills. ;) (We're looking for help if folks are interested...)

I was hoping to hear from @alexmg before cutting the 4.8 but it seems like it'd be better to just roll forward and call it good.

[tillig]

I just pushed 4.8.0 with the revert in behavior and a new PublicOnly() filter. It will take a bit to be indexed by NuGet so give it an hour or so. I'll update the docs shortly. Sorry for the confusion on this one, folks.