Seal ContainerBuilder

[tillig]

The ContainerBuilder has no virtual methods and is intended to be extended using custom extension methods rather than derive-and-override (or derive-and-new, as the case may be). Some recent conversations indicate there may be a small number of folks who have incorrectly created their own ContainerBuilder, which is sort of an unforeseen and not-really-supported method of extension.

I'm thinking if we seal ContainerBuilder we can keep folks on the path to success.

Any reason we shouldn't do that?

If so, this should probably be a 6.0 change, but... I dunno, I could be convinced it's small enough impact it could be a 0.1.0 increment in 5.x.

[alistairjevans]

It's got no virtual methods, and has no protected members, so not sure why anyone would have overrode it.

I'm going to be putting plenty of changes in 6.0 for pipelines (I've started, there's a lot), so we could just do it then.

[johnnyreilly]

Could I make an appeal for this to be unsealed please @tillig?

Tragically there's an issue with .NET Core that means that, by default ConfigureTestContainer doesn't work. The unsealed ContainerBuilder is useful for working around this shortcoming in ASP.NET Core.

You can see more details in the linked issue and of the workaround in my blog post here:

https://blog.johnnyreilly.com/2020/05/autofac-webapplicationfactory-and.html

My fear is that without this workaround being available, and without knowing when (or indeed if) it will be fixed in .NET this may end up being a blocker to upgrade from .NET Core 3.1 to .NET 5.

As I see it, myself and others than write integration tests and use AutoFac are going to be unable to upgrade to .NET 5 without first unpicking AutoFac. Either that or deactivate a whole suite of tests (and that's not something that's likely going be possible).

cc @davidfowl - I realise you're a very busy fellow, but it would be tremendous if this could be considered for fixing in .NET generally so such workarounds wouldn't be necessary.

[tillig]

I'm somewhat against unsealing it because it's not supposed to be an extension point. It's sort of like an interface-that's-only-got-one-implementation. I'd much rather focus on figuring out how we can address the issue without requiring unsealing the class. I have not had any opportunity to figure out if there's a better way as yet, so I don't have any suggestions. This is literally the first I'm hearing of it.

Honestly, we're in the middle of the v6 rollout right now so I don't think this is going to show up soon.

To get better attention on it, I'd recommend filing a separate enhancement issue to explain the problem and link to the aspnetcore issue from there. That way it's not buried down here and we can consider other alternatives for solving rather than putting on the "unseal it is the only way" blinders. If it turns out that's the only solution, it's something to consider; if there are other things we could do, it'd be nice to know what the options are.

[johnnyreilly]

Thanks @tillig - I'll file another issue now.