JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
572 stars 119 forks source link

AssertConfigurationIsValid method throws Lamar.IoC.ContainerValidationException with only ASP.NET Core 2.1 framework registrations #78

Closed seanjmalone closed 6 years ago

seanjmalone commented 6 years ago

I've been trying to use the container.AssertConfigurationIsValid() method during the startup of an ASP.NET Core 2.1 app with just the following two lines:

public void ConfigureContainer(ServiceRegistry services)
{
    var container = new Container(services);
    container.AssertConfigurationIsValid();
}

I also tried using it like this:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    var container = app.ApplicationServices.GetService<IContainer>();
    container.AssertConfigurationIsValid();
}

Even with an empty app and none of my own registrations I get an exception from container.AssertConfigurationIsValid() that looks like this 10x over:

Lamar.IoC.ContainerValidationException
  HResult=0x80131500
  Message=Error in new OptionsManager`1()
System.InvalidOperationException: Compilation failures!

CS1526: A new expression requires (), [], or {} after type
CS1002: ; expected
CS1010: Newline in constant
CS1002: ; expected
CS0305: Using the generic type 'OptionsManager<TOptions>' requires 1 type arguments

Code:

using Lamar.IoC;
using System.Threading.Tasks;

namespace Jasper.Generated
{
    // START: Microsoft_Extensions_Options_IOptions_1_optionsManager
    public class Microsoft_Extensions_Options_IOptions_1_optionsManager : Lamar.IoC.Resolvers.SingletonResolver<object>
    {
        private readonly Lamar.IoC.Scope _topLevelScope;

        public Microsoft_Extensions_Options_IOptions_1_optionsManager(Lamar.IoC.Scope topLevelScope) : base(topLevelScope)
        {
            _topLevelScope = topLevelScope;
        }

        public override object Build(Lamar.IoC.Scope scope)
        {
            return new Microsoft.Extensions.Options.OptionsManager"1();
        }

    }

    // END: Microsoft_Extensions_Options_IOptions_1_optionsManager

}

   at Lamar.Compilation.AssemblyGenerator.Generate(String code)
   at Lamar.Compilation.GeneratedAssembly.CompileAll(ServiceGraph services)
   at Lamar.IoC.Instances.GeneratedInstance.buildResolver(Scope scope)
   at Lamar.IoC.Instances.GeneratedInstance.Resolve(Scope scope)
   at Lamar.Container.buildAndValidateAll(StringWriter writer)

I'm using Lamar.Microsoft.DependencyInjection version 1.1.1 and tried Microsoft.AspNetCore.App versions 2.1.1 to 2.1.3.

CodingGorilla commented 6 years ago

@seanjmalone Which version of Lamar are you using, because I have a project currently running on .NET Core 2.1.3 (and have had it running since .NET Core 2.0) and it works normally. This looks like an older version of Lamar prior to some changes where variable names were "sanitized".

seanjmalone commented 6 years ago

@CodingGorilla you're right, I've just checked and the latest (v1.1.1) Lamar.Microsoft.DependencyInjection NuGet package is dependent on an older minimum version (v1.1.0) of the Lamar NuGet package. However after adding the Lamar package as well to explicitly reference v1.1.1 I still get a similar looking exception. Totally thought that was going to fix it too :(

CodingGorilla commented 6 years ago

You said "similar looking" is it exactly the same? If not can you post the new one?

seanjmalone commented 6 years ago

@CodingGorilla, the exception is 500 lines long, so I thought it was a bit bold to write "the same". However I've just used VS Code's comparison and they're identical between Lamar v1.1.0 and v1.1.1:

ContainerValidationException.txt

I've also had my colleague try it on their machine, and they get the same exception with the following minimal steps:

  1. Create a new Empty ASP.NET Core 2.1 Web Application project (this currently sets me up using Microsoft.AspNetCore.App v2.1.3)
  2. Add the Lamar.Microsoft.DependencyInjection v1.1.1 NuGet package (optionally add the Lamar v1.1.1 NuGet package to try the later version)
  3. Add .UseLamar() in Program.cs.
  4. Rename ConfigureServices(IServiceCollection services) to ConfigureContainer(ServiceRegistry services) in Startup.cs
  5. Update Configure() in Startup.cs with:
public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    var container = app.ApplicationServices.GetService<IContainer>();
    container.AssertConfigurationIsValid();

    // Other stuff...
}
  1. Debug and AssertConfigurationIsValid() should throw the ContainerValidationException

Thanks for your help btw :)

CodingGorilla commented 6 years ago

I don't think you're supposed to rename ConfigureServices() to ConfigureContainer() you're need to add the ConfigureContainer() in addition to ConfigureServices(). The framework (.NET Core) uses ConfigureServices() during start up, whereas Lamar needs ConfigureContainer() to get started. At least, this is how I do it.

Is there documentation somewhere that says to rename it instead?

seanjmalone commented 6 years ago

The only thing I found to go off is the documentation here:

https://jasperfx.github.io/lamar/documentation/ioc/aspnetcore/

The second code sample appears to show ConfigureContainer(ServiceRegistry services) as a replacement for the usual ConfigureServices(IServiceCollection services).

I've just tried the approach you suggested (which I think I prefer to be honest):

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
    }

    public void ConfigureContainer(ServiceRegistry services)
    {
    }

    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        var container = app.ApplicationServices.GetService<IContainer>();
        container.AssertConfigurationIsValid();
    }
}

And all three methods are called from top to bottom. But I still get the exception from AssertConfigurationIsValid().

CodingGorilla commented 6 years ago

Ok, I actually never called AssertConfigurationIsValid() in my project, I added it and I can confirm that it throws [a boat-load] of exceptions. :)

CodingGorilla commented 6 years ago

Ok, I did some digging on this, and if you change your code to do:

container.AssertConfigurationIsValid(AssertMode.ConfigOnly);

Then you won't get any exceptions. I think the failures with AssertMode.Full are to be expected (by design?) because what it actually does is attempt to compile build plans for everything registered in the container. An in several cases the are empty generic registrations, e.g. IOptionsManager<>, and those cannot be instantiated without the generic argument (obviously). During normal execution the system will never ask for an instance IOptionsManager<>, it will always provide a type argument, so it works normally.

Maybe @jeremydmiller can weigh in on this as to whether this is "to be expected" or if there needs to be some work done on AssertMode.Full to eliminate these sorts of issues.

seanjmalone commented 6 years ago

Thanks @CodingGorilla, this makes a lot of sense and definitely makes the exception stop. I've just tested it though and I'm not sure AssertMode.ConfigOnly is the diagnostic I was looking for...

I tried injecting a scoped object into a singleton object to try and throw a validation error. With AssertMode.ConfigOnly I get no error. I checked the Lamar source code and it looks like AssertMode.ConfigOnly iterates over existing errors rather than building anything. I guess the solution I need would be as you suggest- like AssertMode.Full but with the unbound generics handled differently.

jeremydmiller commented 6 years ago

@seanjmalone @CodingGorilla I've got some time tomorrow morning finally to slide back to Lamar. It could be as simple as the validation is unnecessarily trying to compile and build open generic registrations.

CodingGorilla commented 6 years ago

@jeremydmiller You can easily reproduce the issue if you modify the integration_with_aspnetcore.use_in_app() test, adding container.AssertConfigurationIsValid(); just above the container.Bootstrapping.DisplayTimings().Write(writer); around like 202.

jeremydmiller commented 6 years ago

Easy money for the fix, and egg on my face for the underlying problem. Think this'll be in 1.1.2 soon. Got a couple other things from @CodingGorilla to get in here.