37

I have a base controller in my MVC 5 project which implements some shared functionality. This functionality requires some dependencies. I am using Unity 3 to inject these implementations into my controllers, a pattern which has worked fine until I switched my controllers to inherit from this base controller. Now I am running into the following issue:

public class BaseController : Controller
{
    private readonly IService _service;

    public BaseController(IService service)
    {
        _service = service;
    }
}

public class ChildController : BaseController
{
    private readonly IDifferentService _differentService;

    public ChildController(IDifferentService differentService)
    {
        _differentService = differentService;
    }
}

This is understandably throwing an error of 'BaseController' does not contain a constructor that takes 0 arguments. Unity is not resolving the construction of the BaseController, so it can't inject the dependencies into it. I see two obvious ways of solving this issue:

1.) Explicitly call the BaseController ctor and have each ChildController ctor inject the BaseController's dependencies

public class ChildController : BaseController
{
    private readonly IDifferentService _differentService;

    public ChildController(IDifferentService differentService,
                           IService baseService)
        : base(baseService)
    {
        _differentService = differentService;
    }
}

I don't like this approach for several reasons: one, because the ChildControllers are not making use of the additional dependencies (so it causes constructor bloat in the child controllers for no reason), and more importantly, if I ever change the constructor signature of the Base Controller, I have to change the constructor signatures of each child controller.

2.) Implement the BaseController's dependencies via property injection

public class BaseController : Controller
{
    [Dependency]
    public IService Service { get; set; }

    public BaseController() { }
}

I like this approach better - I'm not using any of the dependencies in the BaseController's constructor code - but it makes the dependency injection strategy of the code inconsistent, which isn't ideal either.

There's probably an even better approach which involves some sort of BaseController dependency resolution function that calls on the Unity container to sort out the ctor's method signature, but before I get into writing anything too involved, I was wondering if anyone had solved this problem before? I found a few solutions floating around on the web, but they were workarounds such as Service Locator which I don't want to use.

Thanks!

NWard
  • 2,016
  • 2
  • 20
  • 24

3 Answers3

35

The first thing you have to understand is that you aren't instantiating the base controller. You're instantiating the child controller, which inherits the base controllers interface and functionality. This is an important distinction. When you say "the ChildControllers are not making use of the additional dependencies", then you're absolutely wrong. Because the ChildController IS the BaseController as well. There aren't two different classes created. Just one class that implements both functionality.

So, since ChildController IS A BaseController, there is nothing wrong or strange about passing parameters in the child controllers constructor that calls the base classes constructor. This is the way it should be done.

If you change your base class, you will likely have to change your child classes anyways. There is no way to use constructor injection to inject base class dependencies that are not included in the child class.

I do not recommend property injection, since this means your objects can be created without proper initialization, and you have to remember to configure them correctly.

BTW, the proper terms are Subclass and Superclass. A "child" is a subclass, the parent is the "superclass".

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
  • 1
    You're right! I added a base controller as a solution to needing shared functionality without fully considering the OO implications. I think what I'll do is refactor a bit to ensure that the base dependencies are nice and tidy, and pass those through the child constructors as you suggest. Thanks Eric! – NWard Feb 21 '14 at 07:39
  • Perhaps this is the right answer here, but it's unfortunately that, while I can enclose the initialization of code within the base class, each derived class has to implement the correct constructor. I have to remember to type in that same constructor for each derived class. And any change the data I'm passing to the constructor requires changing every class that derives from it. It would be far better to be able to maintain this solely from the base class. – Jonathan Wood Dec 01 '14 at 01:35
  • @JonathanWood - That argument is not particularly compelling, since constructors are there for a reason... You want to initialize the object with specific data. If you "forget" to include the right constructor, then the task you want to accomplish won't work because there won't be a constructor that you need and you will quickly notice your mistake. It's like complaining that you have to "remember" to add properties that you need to a class... you won't get very far if you don't remember. – Erik Funkenbusch Dec 01 '14 at 03:34
  • @ErikFunkenbusch: I don't think I was arguing for one thing or another. I was pointing out what I see as some limitations to this approach. One of the big points of OOP programming, is encapsulation. And I maintain that some of this has been lost with this approach, and I've attempted to describe some of the issues that follow as a result. – Jonathan Wood Dec 01 '14 at 03:42
  • 2
    @JonathanWood - what you call an "approach" is the way all object oriented languages that i'm aware of work, and since this is.. by definition "the object oriented approach", I don't quite understand what you mean. To my knowledge, all object oriented languages that implement inheritance are implemented in a manner in which the derived class is the only instance of the object, and thus there are no alternate instances that something like Unity or any other DI container could inject a value in if the derived class itself does not implement that constructor. – Erik Funkenbusch Dec 01 '14 at 05:19
  • 1
    @JonathanWood - I think what you seem to be suggesting is that base class constructors should be exposed by default, which is really a very different thing from what the original poster was asking for. If that is what you mean, then I suggest you read any of the many articles out there about why constructors are not inherited in C++, Java, and other langauges (including C#). FWIW, C++-11 has added a feature that allows the compiler to automatically create matching constructors in the base classes, but this is not quite the same thing. – Erik Funkenbusch Dec 01 '14 at 05:32
20

With ASP.Net 5 and it's built in DI

public class BaseController : Controller
{
    protected ISomeType SomeMember { get; set; }

    public BaseController(IServiceProvider serviceProvider)
    {
        //Init all properties you need
        SomeMember = (SomeMember)serviceProvider.GetService(typeof(ISomeMember));
    }
}

public class MyController : BaseController  
{
public MyController(/*Any other injections goes here*/, 
                      IServiceProvider serviceProvider) : 
 base(serviceProvider)
{}
}

UPDATE

There is also an extension method in Microsoft.Extensions.DependencyInjection to make it shorter

SomeMember = serviceProvider.GetRequiredService<ISomeMember>();
Vitaly
  • 2,064
  • 19
  • 23
  • This is a great addition. Is it possible to implement a base controller without DI in ASP.NET 4.5? – JoshYates1980 Jan 25 '16 at 19:11
  • Not sure what you mean. You could use Factory+Singleton instead of DI in BaseController. – Vitaly Jan 26 '16 at 16:02
  • yea, disregard. I left off Namespace Controllers. This project I'm working on does not have DI implimented. – JoshYates1980 Jan 26 '16 at 17:27
  • Good one, goes well with MVC 6 – Hari Gillala Apr 27 '16 at 13:39
  • could you provide an additional example? I've faced *controller must have parameterless public ctor* error. – anatol Jan 12 '18 at 05:52
  • @anatol, you either use different version of MVC (MVC 4\5, WebAPI, etc), or use non-default DI container. The example is for ASP.Net Core MVC 6, which was called ASP.Net 5 at the moment I added my answer. In ASP.Net Core you rarely have parameterless constructor and there is no such restriction exists – Vitaly Jan 12 '18 at 08:28
  • 1
    MVC 5 is used and MS DI. I've solved my issue by adding `public HomeController() : this(DependencyInjectionResolver.ServiceProvider) { }` etc – anatol Jan 12 '18 at 10:52
  • @anatol, instead you could just use DependencyInjectionResolver.ServiceProvider all the way or create a property in base controller that returns it directly. There is not much sense passing static property into base class constructor – Vitaly Jan 12 '18 at 15:58
  • You still have to inject the serviceProder into the base controller's constructor. Back to original same problem. – Sam Aug 11 '20 at 12:48
  • @Sam, the idea is that you have to inject only IServiceProvider, Say you have 50 controllers, and you want all of them to have access to file storage. You can change all 50 or only one base controller. What you choose depends on how you get paid: per job done or per line :-D – Vitaly Aug 18 '20 at 13:44
  • That makes sense. – Sam Aug 18 '20 at 17:27
0

I've taken a somewhat different (but to me, fairly obvious and probably common) approach. It works for me, but there may be pitfalls I'm not aware of.

I have created common service properties in my BaseController class that most of my controllers use. They get instantiated when they are needed/referenced.

If there's a service a particular controller needs that is less used, I inject it into that controller's constructor as normal.

using JIS.Context;
using JIS.Managers;
using JIS.Models;
using JIS.Services;
using System;
using System.Collections.Generic;
using System.Web;
using System.Web.Mvc;

namespace JIS.Controllers
{
    public class BaseController : Controller
    {
        public BaseController()
        {
            ViewBag.User = UserManager?.User;
        }

        private IUserManager userManager;
        public IUserManager UserManager
        {
            get
            {
                if (userManager == null)
                {
                    userManager = DependencyResolver.Current.GetService<IUserManager>();
                }
                return userManager;
            }
            set
            {
                userManager = value;
            }
        }

        private ILoggingService loggingService;
        public ILoggingService LoggingService
        {
            get
            {
                if (loggingService == null)
                {
                    loggingService = DependencyResolver.Current.GetService<ILoggingService>();
                }
                return loggingService;
            }
            set { loggingService = value; }
        }

        private IUserDirectory userDirectory;
        public IUserDirectory UserDirectory
        {
            get
            {
                if (userDirectory == null)
                {
                    userDirectory = DependencyResolver.Current.GetService<IUserDirectory>();
                }
                return userDirectory;
            }
            set { userDirectory = value; }
        }

        private ApplicationDbContext appDb;
        public ApplicationDbContext AppDb
        {
            get
            {
                if (appDb == null)
                {
                    appDb = new ApplicationDbContext();
                }
                return appDb;
            }
            set
            {
                appDb = value;
            }
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing && appDb != null)
            {
                appDb.Dispose();
            }
            base.Dispose(disposing);
        }
    }
}

In my controller I simply subclass from this BaseController:

public class UsersController : BaseController
{
    // and any other services I need are here:
    private readonly IAnotherService svc;
    public UsersController(IAnotherService svc)
    {
        this.svc = svc;
    }
...
}

This way, common services are generated on the fly when they're needed, and are available to my controllers without a lot of boilerplate.

DBatesX
  • 11
  • 3