New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto cross-wiring ASP.NET Core components #442

Closed
dotnetjunkie opened this Issue Jul 21, 2017 · 23 comments

Comments

3 participants
@dotnetjunkie
Collaborator

dotnetjunkie commented Jul 21, 2017

The ASP.NET Core integration package for Simple Injector contains an CrossWire<T> extension method that allows adding a registration to Simple Injector that forwards the request to the built-in container.

This might potentially lead to having to do quite some cross-wirings, and when it comes to registrations of generic types (such as IStringLocalizer<T> as seen in this Stackoverflow question), the number of registrations might explode.

So I'm toying around with implementing an ResolveUnregisteredType event that allows falling back to the .NET Core configuration system when such registration is missing from Simple Injector.

It looks like this:

public static void EnableAutoCrossWiring(this Container container, IApplicationBuilder builder)
{
    container.ResolveUnregisteredType += (s, e) =>
    {
        // Skip IEnumerable<T> registrations: they behave rather different in .NET Core, and
        // auto-wiring them would cause us to loose the possibility to tell the user he forgot
        // to register his collection.
        if (IsEnumerable(e.UnregisteredServiceType)) return;

        IServiceCollection services = GetCrossWireContext(container).Services;

        // Locate the ServiceDescriptor for the service type in .NET Core's IServiceCollection
        ServiceDescriptor descriptor = FindDescriptor(e.UnregisteredServiceType, services);

        // If such descriptor exists, we will cross-wire. If not, we skip this registration and 
        // Simple Injector will throw an exception.
        if (descriptor != null)
        {
            // Build a registration with the correct lifestyle based on information from .NET Core
            var registration = CreateRegistration(container, e.UnregisteredServiceType, builder);

            // Apply the registration to Simple Injector
            e.Register(registration);
        }
    };
}

What do you think? Should we add such feature? What could be the risks of doing this?

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Jul 24, 2017

I recently tried out the new documentation for Identity Core with version 4.0.10. There are alot of 'helpful' libraries I use that provide Integration extensions that push their dependencies into the default Microsoft DI container. For some of these it was easy to redirect to SimpleInjector and others just seemed to go on and on and on as you went down the Dependency tree.

So yes it's worth ago. Another option is to have SimpleInjector produce some documentation for what it finds when it goes through the auto-wiring that would enable the developer to cut and paste and manually wire it themselves.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 24, 2017

@tim-elvidge, would you be interested in trying this feature? I'm very interested in getting some feedback on this. If you are interested, I'll post a code snippet that you can use to start this auto-cross-wiring process.

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Jul 24, 2017

@dotnetjunkie yes - project is still in development and would be happy to assist and provide feedback on what glitches/bugs it throws up. I'd like to give it ago on some of the libraries that ended up just being too hard and one in particular that my license gives me access to their source code.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 24, 2017

@tim-elvidge,

Here is a copy-pastable version of this extension method:

using System;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using SimpleInjector;
using SimpleInjector.Diagnostics;

public static void ConfigureAutoCrossWiring(
    this Container container, IApplicationBuilder builder, IServiceCollection services)
{
    var accessor = builder.ApplicationServices.GetRequiredService<IHttpContextAccessor>();
    var scopeFactory = builder.ApplicationServices.GetRequiredService<IServiceScopeFactory>();

    container.Register(scopeFactory.CreateScope, Lifestyle.Scoped);

    container.ResolveUnregisteredType += (s, e) =>
    {
        if (e.Handled) return;

        Type serviceType = e.UnregisteredServiceType;

        ServiceDescriptor descriptor = services.LastOrDefault(d => d.ServiceType == serviceType);

        if (descriptor == null && serviceType.GetTypeInfo().IsGenericType)
        {
            var serviceTypeDefinition = serviceType.GetTypeInfo().GetGenericTypeDefinition();
            descriptor = services.LastOrDefault(d => d.ServiceType == serviceTypeDefinition);
        }

        if (descriptor != null)
        {
            Lifestyle lifestyle =
                descriptor.Lifetime == ServiceLifetime.Singleton ? Lifestyle.Singleton :
                descriptor.Lifetime == ServiceLifetime.Scoped ? Lifestyle.Scoped :
                Lifestyle.Transient;

            Registration registration = lifestyle.CreateRegistration(serviceType, () =>
                {
                    var provider = accessor.HttpContext?.RequestServices
                        ?? container.GetInstance<IServiceScope>().ServiceProvider;
                    return provider.GetRequiredService(serviceType);
                },
                container);

            if (lifestyle == Lifestyle.Transient 
                && typeof(IDisposable).IsAssignableFrom(serviceType))
            {
                registration.SuppressDiagnosticWarning(
                    DiagnosticType.DisposableTransientComponent,
                    justification: 
                        "This is a cross-wired service. ASP.NET will ensure it gets disposed.");
            }

            e.Register(registration);
        }
    };
}

You can use this method as follows:

public void ConfigureServices(IServiceCollection services)
{
    // usual stuff here

    // store IServiceCollection for later use.
    this.services = services;
}

// Private field for IServiceCollection
private IServiceCollection services;

public void Configure(IApplicationBuilder app, IHostingEnvironment env,
    ILoggerFactory loggerFactory)
{
    // Call the extension method and supply the IServiceCollection
    container.ConfigureAutoCrossWiring(app, this.services);

    // Usual stuff here.
}

Let me know how this works out and what problems you encounter.

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Jul 25, 2017

Ok that worked a treat. I just commented out all the explicit cross wiring:

//container.CrossWire<UserManager<ApplicationUser>>(app) ... etc

ran the web site and after finding a few dependencies I hadn't registered in the container was good to go.
Pushed release code to a web server running IIS and that also performed without a glitch.
Makes it alot easier to get up and running quickly.

The only real issue is figuring out which container to use Microsoft's services.Add... or SimpleInjector's container.Register.... IMHO if it's something in Microsoft's framework like Identity and you need to override or extend then it's service.Add... otherwise use container.Register...

If it's the pipepline then

app.Use((c, next) => container.GetInstance<Some....Middleware>().Invoke(c, next)); 

works fine.

For singelton's anyway I found that it's fine to regsister them in both containers:

container.RegisterSingleton<Infrastructure.ICommon, Infrastructure.Common>();

and

services.AddSingleton<Infrastructure.ICommon, Infrastructure.Common>();

The only other bit of the puzzle was working out how to do a Hybrid Lifestyle for when HttpContext is not present:

var hybridLifestyle = Lifestyle.CreateHybrid(
    () => new HttpContextAccessor().HttpContext != null,
    Lifestyle.Scoped,
    Lifestyle.Transient);

did the trick.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 25, 2017

The only real issue is figuring out which container to use Microsoft's services.Add... or SimpleInjector's container.Register....

  • For 3rd party components that already hook in to Microsoft's services.Add... (such as AddIdentity), you should keep using them, because they typically have a dependency on the details of Microsoft's registration API. Trying to get them in Simple Injector would be problematic, because you will end up re-registering all dependencies , while your application only needs to depend on a few at most.
  • For application components you use Simple Injector, because registering application components in the Microsoft configuration system, will cause you to lose all benefits that Simple Injector provides, such as auto-wiring, superior support for generics and all the safety guards that Simple Injector has both through its design and through the Diagnostic Services.

One warning though: be very careful re-registering types as did with the singletons:

container.RegisterSingleton<Infrastructure.ICommon, Infrastructure.Common>();
services.AddSingleton<Infrastructure.ICommon, Infrastructure.Common>();

This can lead to a problem called Torn Lifestyle. This will result in having two instances of the registered component, which might be a problem if that component has some state that depends on being a true singleton.

This is something Simple Injector typically can spot for you, but it can't diagnose the configuration system. That's why cross-wiring is typically safer; Simple Injector will get that instance from the Microsoft configuration system and cache it locally. You are guaranteed to have that same instance.

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Jul 25, 2017

@dotnetjunkie Agreed because I wrote them I knew that they were truelly stateless but have since changed the configuration to best practice. Added some new services to the project thanks to AutoCrossWiring utilising ICompositeViewEngine and ITempDataProvider to render a view with model to a string for emailing and conversion to PDF. As far as I am concerned AutoCrossWiring is a beautiful thing - allows you to just get on and code knowing SimpleInjector will do the heavy lifting.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 25, 2017

The main downside about auto cross-wiring is to me an educational one. Auto cross-wiring promotes having many dependencies from application components to framework and third party components, while in general we should strive to minimize those dependencies, and work with application-tailored abstraction instead.

The other downside is that it might work like a magic want, where developers don't know where services are coming from.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 25, 2017

The only other bit of the puzzle was working out how to do a Hybrid Lifestyle for when HttpContext is not present:

Can you go into details about why you needed to register HttpContext? Is there some 3rd party or framework component that requires an HttpContext instance in its constructor? If so, which component is that?

This is something I really would like to know, because this would be a design flaw. Last year, when ASP.NET Core was still in beta, I reported this as a bug in Identity's SignInmanager and Microsoft admitted this as a design flaw by fixing this problem.

The general issue is that runtime data should not be used to initialized components.

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Jul 25, 2017

@dotnetjunkie HttpContext - I use Dapper for some database requests where I can do some stuff that requires a more complex and tailored SQL than what EF core provides and within those requests the Lifestyle is scoped. I also use Dapper for logging where I want it to run on another thread and in this instance the lifestyle is transient. Hence this code:

container.Register(() => new System.Data.SqlClient.SqlConnection(connString), hybridLifestyle);

I agree that AutoCrossWiring is rather like a magic wand but with ICompositeViewEngine and ITempDataProvider they are both part of Microsoft's framework and AutoCrossWiring could be a feature in Simple Injector that prevents you going down the rabbit hole chasing all their dependencies. I don't need to override any of their features but I do need them in the constructor of my service.

using System.IO;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Rendering;
using Microsoft.AspNetCore.Mvc.ViewEngines;
using Microsoft.AspNetCore.Mvc.ViewFeatures;

   public interface IViewRenderingService
   {
      Task<string> RenderViewToStringAsync(Controller controller, string viewName, object model);
   }

   public class ViewRenderingService : IViewRenderingService
    {
      private readonly ICompositeViewEngine viewEngine;
      private readonly ITempDataProvider tempDataProvider;
      public ViewRenderingService(ICompositeViewEngine viewEngineInject, ITempDataProvider tempDataProviderInject)
      {
         viewEngine = viewEngineInject;
         tempDataProvider = tempDataProviderInject;
      }

      public async Task<string> RenderViewToStringAsync(Controller controller, string viewName, object model)
      {
         controller.ViewData.Model = model;
         using (StringWriter sw = new StringWriter())
         {
            ViewEngineResult viewResult = viewEngine.FindView(controller.ControllerContext, viewName, false);

            ViewContext viewContext = new ViewContext(
                controller.ControllerContext,
                viewResult.View,
                controller.ViewData,
                new TempDataDictionary(
                    controller.ControllerContext.HttpContext,
                    tempDataProvider),
                sw,
                new HtmlHelperOptions()
            );
            await viewResult.View.RenderAsync(viewContext);
            return sw.GetStringBuilder().ToString();
         }
      }

Or am I misunderstanding something?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jul 25, 2017

AutoCrossWiring could be a feature in Simple Injector that prevents you going down the rabbit hole chasing all their dependencies.

I would say that you shouldn't be going down the rabbit hole at all. You will be going down the rabbit hole in case you register some 3rd party component in Simple Injector, because this will force you to register its dependencies as well, all the way down. Instead of registering that whole object graph in Simple Injector, you only cross-wire that dependency that your application code actually depends on. With cross-wire, that dependency is resolved from the Microsoft configuration system, and that system will hold that component with its dependencies.

So you don't need auto cross-wiring to do this. The only thing that auto cross-wiring makes easy is the fact that you don't have to make the cross-wirings one by one, just like batch-registration prevents you to make all registrations one by one.

So it's not so much that I discourage cross-wiring, but rather that I discourage letting application code take a dependency on some 3rd party dependency. Instead the DIP promotes the use of application-tailored abstractions. For those abstractions you'll have an adapter that will can resolve the 3rd party dependency from the Microsoft configuration system.

Trying to prevent dependencies on 3rd party components however, might not make much sense when you are in a part of the application whose job is to integrate between parts of the system. An AccountController might be a good example, because all it does is dispatch incoming web requests to other libraries, such as Identity Framework.

So it's not so much that you should hide 3rd party components for the sake of hiding, but rather that you try to provide application code with a simplified abstraction, that is easier to test and maintain.

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Aug 16, 2017

Just updated my web application to Core 2.0 and as expected everything provided by SimpleInjector just worked.
On Microsoft's docs (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection#replacing-the-default-services-container) where they provide an example of Autofac replacing the default container and returning a IServiceProvider, was wondering why the SimpleInjector integration didn't go down this path?

One more question what do you think of this construct:-
services.AddSingleton(_ => container.GetInstance<SqlConnectionService>());

it is a dumb service that sole responsibility is allowing Dapper to get at the connection string.
I resorted to this approach because in the scenario I was using decrypting a link sent in an email, signing in the user in the AccountController and then a RedirectToAction to another controller it seemed to be using the same SqlConnection but the connection was closed/closing and threw errors??

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Aug 16, 2017

was wondering why the SimpleInjector integration didn't go down this path?

For starters, please read What’s wrong with the ASP.NET Core DI abstraction?. It describes why it is impossible to have a compatible Adapter for Simple Injector.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Aug 16, 2017

seemed to be using the same SqlConnection

Perhaps because you registered your SqlConnectionService as singleton?

@tim-elvidge

This comment has been minimized.

tim-elvidge commented Aug 17, 2017

I am not that stupid as to use it as a singleton and expect it to be different on each call (well most days I am not). I figure out what was the issue in my last question. Using Dapper I have a hybrid lifestyle that is Lifestyle.Scoped when the HttpContext != null and it gets created in the constructor as per usual but some of the calls to these bits of code are wrapped in a Task.Run(() => etc. ); so sometimes the HttpContext exists in the constructor but being scoped it get disposed of in the main thread before the background thread run it's task. And catastophic errors result.

But the question I wanted your opinion on was:
services.AddSingleton(_ => container.GetInstance<SomeClass>());
or
services.AddScoped<SomeClass>(_ => container.GetInstance<SomeClass>());

It seems to work fine.
Basically I prefer SimpleInjector for the reasons specified in your blogs and would like to have everything resolved from it and have only Microsoft Framework stuff resolved by their DI.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Aug 17, 2017

I am not that stupid as to use it as a singleton and expect it to be different on each call

Sorry, but I had to ask. I do don't keep a list of the experience level of each of our users :)

@guy-lud

This comment has been minimized.

guy-lud commented Oct 1, 2017

Hi @dotnetjunkie,
Took some time but i finally looked at the extension method above.
I've been playing with it a bit and the issue with @inject in a view was not solved (with of course is logical since the serviceProvider on the HttpContext is the basic one - i still don't understand why MS did it like so).
So i've used your CrossWire with my code (thus overriding the ServiceProvider on the HttpContext).
With your extension method we still get issues of IEnumerable reegistration (again, logical you handled collection registration differently) and Generics.
In my solution you commented that we'll get issues with scope lifecycle yet i've handled that on the scopeSomethingProvider.
With some more work we might be able to integration the best IOC SimpleInjector with Asp.Net core :)

It's beyond my understanding why MS did it so hard to plug in an IoC to Mvc. Maybe they could have some answers

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 5, 2017

the issue with @Inject in a view was not solved (with of course is logical since the serviceProvider on the HttpContext is the basic one - i still don't understand why MS did it like so)

There was a flaw in their thinking believing that every DI container would be able to adapt to their DI abstraction, which has proven to be incorrect. Although for the most part, MS provided us with the correct interception points / abstractions such as IControllerActivator, such abstraction is missing to intercept the @inject attribute.

It's beyond my understanding why MS did it so hard to plug in an IoC to Mvc. Maybe they could have some answers

Don't expect too much from Microsoft. They have proven to be stubborn and unwillingly to cooperate as can be seen here.

@guy-lud

This comment has been minimized.

guy-lud commented Oct 5, 2017

@dotnetjunkie thank, did you hear from them will they solved this issue ?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 5, 2017

did you hear from them will they solved this issue ?

Nope. They have been silent ever since 14 Nov 2016.

@guy-lud

This comment has been minimized.

guy-lud commented Oct 5, 2017

1.@dotnetjunkie do you think that overriding IServiceScopeFactory and replcaing the IServiceResolver with one of our own is not enough of interception point ?

  1. What is your course of action this days? not using asp.net core ? not using @inject ? using nancyFx ?
    just wondering
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 5, 2017

@dotnetjunkie do you think that overriding IServiceScopeFactory and replcaing the IServiceResolver with one of our own is not enough of interception point?

I don't see how overriding IServiceScopeFactory would help in injecting dependencies into the view using @inject. I'm not familiar with the IServiceResolver interface.

What is your course of action this days? not using asp.net core?

No, I think ASP.NET Core is (for the most part) a great framework. You can absolutely use it if you wish to.

not using @inject ?

That would certainly be my preference, as I expressed here. I certainly wouldn't let normal views have any dependencies. You might have some global 'master page'-like views, that might have a few dependencies thought, but you don't need the @inject attribute for that. At that point there is no harm in using a Service Locator in that part of your web application's infrastructure.

using nancyFx?

I don't know enough of NancyFx to say anything constructive about that.

@guy-lud

This comment has been minimized.

guy-lud commented Oct 5, 2017

@dotnetjunkie , thanks for your input.

I'm not familiar with the IServiceResolver interface.

I meant the IServiceProvider interface - the IServiceResolver is my service Locator pattern in mvc 5. :)

I agree with your take on @inject in any view other than a master view, yet it's my OCD not able to use a feature in a framework due to design limitation.

the IServiceScopeFactory has property of IServiceProvider this is the IServiceProvider used in the HttpContext.RequireService method. if you look in my simple app this way i was able to inject a service into a view

anyway, great talking with you again

dotnetjunkie added a commit that referenced this issue Mar 1, 2018

@dotnetjunkie dotnetjunkie changed the title from Should we implement "auto cross-wiring" in the ASP.NET Core integration? to Auto cross-wiring ASP.NET Core components Mar 22, 2018

dotnetjunkie added a commit to simpleinjector/Documentation that referenced this issue Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment