Add support for OnlyIf and IfNotRegistered on module registration

[alistairjevans]

This PR is a partial fix for #1235, by adding OnlyIf support for Module Registrations.

This change is technically breaking, through the addition of a new RegistrarData property on IModuleRegistrar, however if someone has implemented a custom IModuleRegistrar, I will be quite surprised.

This change allows you to do this:

var builder = new ContainerBuilder();

builder.RegisterModule(new MyModule())
       .OnlyIf(regBuilder => someCondition);

or this:

var builder = new ContainerBuilder();

builder.RegisterModule(new MyModule())
       .IfNotRegistered(typeof(string));

You can stack OnlyIf calls in the same way you can do it on normal registrations:

var builder = new ContainerBuilder();

builder.RegisterModule(new MyModule())
       .OnlyIf(regBuilder => someCondition)
       .OnlyIf(regBuilder => somethingElse);

The way I've laid out the evaluation of the conditions and the module registrations is as follows (after some thought):

• Evaluate any predicates (in reverse definition order, same as normal registrations), stopping if one fails.
• If all predicates pass, then run all module registrations.

This means that any OnlyIf statements used against a single IModuleRegistrar instance will be applied to the state of the registry before any modules got registered.

This behaviour seemed like the most predicatable/logical, rather than (for example), evaluating the predicate per-module.

[codecov]

Codecov Report

Merging #1272 (6ce35d4) into develop (bf50d85) will increase coverage by 0.11%.
The diff coverage is 97.50%.

@@             Coverage Diff             @@
##           develop    #1272      +/-   ##
===========================================
+ Coverage    76.42%   76.53%   +0.11%     
===========================================
  Files          187      188       +1     
  Lines         5085     5123      +38     
  Branches      1034     1041       +7     
===========================================
+ Hits          3886     3921      +35     
- Misses         706      707       +1     
- Partials       493      495       +2     

Impacted Files	Coverage Δ
src/Autofac/Core/Registration/ModuleRegistrar.cs	95.23% <93.75%> (-4.77%)	⬇️
...c/Autofac/Core/Registration/ModuleRegistrarData.cs	100.00% <100.00%> (ø)
src/Autofac/ModuleRegistrationExtensions.cs	61.53% <100.00%> (+17.09%)	⬆️
src/Autofac/Util/SequenceGenerator.cs	71.42% <0.00%> (-28.58%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf50d85...6ce35d4. Read the comment docs.

[tillig]

I think this is pretty slick, though I made a note where the actual logic was somewhat confusing for a moment.

I think the "breaking" IModuleRegistrar change is fine. Honestly, if you've implemented IModuleRegistrar on your own, God have mercy on your soul.

[tillig]

It took me a bit to figure out that this is effectively the call to module.Configure for all modules registered via this registrar. The RegistrationData allows wrapping of that chain with predicates, but RegisterModule needs the ability to add modules inside that wrapper.

I wonder if a more descriptive name like _moduleConfigureChain or something might help? I'm not glued to that as the name, but it did take me a few reads through here to go, "OOOHHH, it's predicates wrapped around this callback, and this callback gets sort of lazy-initialized via closure." Like two levels of callbacks, not just one "pipeline." I think _nextModule threw me off because I was expecting one pipeline, like ASP.NET Core _next and my poor zombie brain didn't connect that it's sorta two levels going on here.

I also had a big block of // comments written but was like... maybe it's just me and a more descriptive name would be enough.

[alistairjevans]

The word 'next' does imply pipeline, you're right (in Autofac as well, never mind ASP.NET Core). Will re-word and add some comments.

[tillig]

🖲️