Dependency injection duplication in Controller and BaseController in .Net Core 2.0

Question

If I create a BaseController in my Asp.Net Core 2.0 web application that capsulizes some of the common dependencies are they still necessary in the actual controllers.

For Example, the standard Account and Manage controllers in a default MVC 6 web application.

public class AccountController : Controller
{
    private readonly UserManager<ApplicationUser> _userManager;
    private readonly SignInManager<ApplicationUser> _signInManager;
    private readonly IEmailSender _emailSender;
    private readonly ILogger _logger;

    public AccountController(
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<AccountController> logger)
    {
        _userManager = userManager;
        _signInManager = signInManager;
        _emailSender = emailSender;
        _logger = logger;
    }
   //rest of code removed
}

public class ManageController : Controller
{
    private readonly UserManager<ApplicationUser> _userManager;
    private readonly SignInManager<ApplicationUser> _signInManager;
    private readonly IEmailSender _emailSender;
    private readonly ILogger _logger;
    private readonly UrlEncoder _urlEncoder;

    private const string AuthenicatorUriFormat = "otpauth://totp/{0}:{1}?secret={2}&issuer={0}&digits=6";

    public ManageController(
      UserManager<ApplicationUser> userManager,
      SignInManager<ApplicationUser> signInManager,
      IEmailSender emailSender,
      ILogger<ManageController> logger,
      UrlEncoder urlEncoder)
    {
        _userManager = userManager;
        _signInManager = signInManager;
        _emailSender = emailSender;
        _logger = logger;
        _urlEncoder = urlEncoder;
    }
    // rest of code removed
}

In the custom web application template I am building I refactor the Account Controller to three different Controllers, RegisterController(Which handles everything regarding a user registration), LoginController(which handles login and logout), and the balance into a third. I split the Manage Controller in two, a ManagePasswordController(everything related to passwords) and a UserManageController(everything else).

There is a lot of commonality in the DI requirements for each and I want to put them in a BaseController. To look something like this?

public abstract class BaseController : Controller
{
    private readonly IConfiguration _config;
    private readonly IEmailSender _emailSender;
    private readonly ILogger _logger;
    private readonly SignInManager<ApplicationUser> _signInManager;
    private readonly UserManager<ApplicationUser> _userManager;

     protected BaseController(IConfiguration iconfiguration,
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<ManageController> logger)
    {
        _config = iconfiguration;
        _userManager = userManager;
        _signInManager = signInManager;
        _emailSender = emailSender;
        _logger = logger;
    }
    //rest of code removed
}

But it seems like that accomplishes nothing? because it seems to me I still have to inject everything. I can't be right (I'm new to DI so clearly have no clue) but the BaseController should allow me to do NO DI that's common between BaseController and RegisterController. Am I wrong? How do I accomplish what I am trying to do?

public class RegisterController : BaseController
{
    private const string ConfirmedRegistration = "User created a new account with password.";

    private readonly UserManager<ApplicationUser> _userManager;
    private readonly SignInManager<ApplicationUser> _signInManager;
    private readonly IEmailSender _emailSender;
    private readonly ILogger _logger;
    private readonly IConfiguration _config;

     public RegisterController(
        IConfiguration config,
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<AccountController> logger) : base(config, userManager, signInManager, emailSender, logger)

    {
        _userManager = userManager;
        _signInManager = signInManager;
        _emailSender = emailSender;
        _logger = logger;
        _config = config;
    }
    //rest of code removed
}

Update

Per Sir Rufo's suggestion

public abstract class BaseController : Controller
{
    protected UserManager<ApplicationUser> UserManager { get; }
    protected SignInManager<ApplicationUser> SignInManager { get; }
    protected IConfiguration Config { get; }
    protected IEmailSender EmailSender { get; }
    protected ILogger AppLogger { get; }

    protected BaseController(IConfiguration iconfiguration,
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<ManageController> logger)
    {
        AppLogger = logger;
        EmailSender = emailSender;
        Config = iconfiguration;
        SignInManager = signInManager;
        UserManager = userManager; 
    }
}

And the inheriting controller

public class TestBaseController : BaseController
{

    public TestBaseController() : base()
    {

    }
}

This doesn't work. Resharper is telling me I have to add the parameters to the base constructor call in the TestBaseController constructor.

Also should BaseController be inheriting from Controller or ControllerBase in .Net Core 2.0?

If the `BaseController` requires you to provide all of them in the constructor call, then yes. But you could refactor the `BaseController` to not need all of those. Also, why save references in both the `BaseController` and it's subclasses? — fredrik, Nov 18 '17 at 22:03
@fredrik..Well the 5 controllers I mention in the post, all need those DI's. Is that all I need to do in the actual controllers? Just eliminate the private references and use the parameters passed in? i.e signInManager.IssignedIn, instead of _signInManager.IsSignedIn? — dinotom, Nov 18 '17 at 22:20
Publish the injected references via protected properties in the BaseController to have access to them from any derived class — Sir Rufo, Nov 18 '17 at 22:25
@Sir Rufo..not sure what Publish means in this context. Example? — dinotom, Nov 18 '17 at 22:26
Yes, so you are suggesting making a ControllerBase class with the 5 DI's that are common as protected properties of the class (essentially just adding 5 protected properties to my current BaseController class) so I can call them in the other created controllers like this.Logger, this.Configuration? — dinotom, Nov 18 '17 at 22:33
So essentially just setting the properties = to the private fields in a parameterless constructor? — dinotom, Nov 18 '17 at 22:37
Declare in BaseController: *protected ILogger Logger { get; }* and inside constructor *Logger = logger;*. Just exchange the *private fields* with *protected properties* and you can access them from each derived class. The derived class now will have an empty constructor (**not** parameterless) — Sir Rufo, Nov 18 '17 at 22:58
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/159312/discussion-between-dinotom-and-sir-rufo). — dinotom, Nov 18 '17 at 23:08
You don't need the private members in TestBaseController. They are already accessible from the base — SpruceMoose, Nov 18 '17 at 23:43
The only thing to really understand this, is to try to do what you want by hand.... DI is no magic wand. The rules of the CLR still apply! — Ric .Net, Nov 18 '17 at 23:56
@Calc...and the constructor in TestBaseController would then pass in the parameters and set them equal to the properties in the base? — dinotom, Nov 18 '17 at 23:56
Your initial BaseController was fine, you only needed to make the private members protected instead. Then in your inheriting controllers there is no need for any private members or assignments, just call the base constructor and use the protected (base) members. — SpruceMoose, Nov 19 '17 at 00:13
@Calc...see update part of post for latest iteration that I still can't get to work. — dinotom, Nov 19 '17 at 02:57
To me you need to inject the parameters into the TestController and pass them to the base class (:base()...). I'm curious to see if you can skip this part but I don't think so — , Nov 19 '17 at 10:00

Francisco Vilches · Answer 1 · 2018-05-24T22:26:16.243

The Microsoft.AspNetCore.MVC.Controller class comes with the extension method

HttpContext.RequestServices.GetService<T>

Which can be used whenever the HttpContext is available in the pipeline (e.g. The HttpContext property will be Null if called from the controller's constructor)

Try this pattern

Note: make sure you include this directive using Microsoft.Extensions.DependencyInjection;

Base Controller

public abstract class BaseController<T> : Controller where T: BaseController<T>
{

    private ILogger<T> _logger;

    protected ILogger<T> Logger => _logger ?? (_logger = HttpContext.RequestServices.GetService<ILogger<T>>());

Child Controller

[Route("api/authors")]
public class AuthorsController : BaseController<AuthorsController>
{

    public AuthorsController(IAuthorRepository authorRepository)
    {
        _authorRepository = authorRepository;
    }

    [HttpGet("LogMessage")]
    public IActionResult LogMessage(string message)
    {
        Logger.LogInformation(message);

        return Ok($"The following message has been logged: '{message}'");
    }

Needless to say, remember to register your services in the Startup.cs --> ConfingureServices method

I had to write `using Microsoft.Extensions.DependencyInjection;` to be able to use the generic `GetService` method. — Leniel Maccaferri, May 18 '18 at 20:26
This is the real code i need in base and derived controller! The feature is that the code doesn't pollute derived controller's ctor and stuff like dbContext, logger and other things put in BaseController. — Lapenkov Vladimir, Sep 08 '18 at 05:51

score 7 · Accepted Answer · answered Nov 19 '17 at 11:56

There are very few good reasons to use a BaseController in MVC. A base controller in this scenario only adds more code to maintain, with no real benefit.

For true cross-cutting concerns, the most common way to handle them in MVC is to use global filters, although there are a few new options worth considering in MVC core.

However, your problem doesn't look like a cross-cutting concern so much as a violation of the Single Responsibility Principle. That is, having more than 3 injected dependencies is a code smell that your controller is doing too much. The most practical solution would be to Refactor to Aggregate Services.

In this case, I would argue that you have at least 1 implicit service that you need to make explicit - namely, UserManager and SignInManager should be wrapped into a service of its own. From there, you could potentially inject your other 3 dependencies into that service (depending on how they are used, of course). So, you could potentially whittle this down to a single dependency for both the AccountController and ManageController.

Some signs that a controller is doing too much:

There are a lot of "helper" methods that contain business logic that are shared between actions.
Action methods are doing more than simple HTTP request/response stuff. An action method should generally do nothing but call services that process input and/or produce output and return views and response codes.

In cases such as these, it is worth a look to see if you can move that logic into a service of their own and any shared logic into dependencies of that service, etc.

@NightOwl888...and yet the default web application for MVC 6 comes with an AccountController that has 4 injected dependencies and ManageController with 5 injected dependencies. This was the whole purpose of this exercise, to reduce the injected dependencies that were common, to one base class so the newly created controllers could just take them from the base class. I am not a professional programmer, just trying to learn Core 2.0 to build my own web app template as a learning mechanism. An example for " you have at least 1 implicit service that you need to make explicit ", would be helpful — dinotom, Nov 20 '17 at 00:05

dinotom · Answer 3 · 2017-11-19T11:05:37.507

Per suggestions from both Calc and Sir Rufo, this works.

 public abstract class BaseController : Controller
{
    protected UserManager<ApplicationUser> UserManager { get; }
    protected SignInManager<ApplicationUser> SignInManager { get; }
    protected IConfiguration Config { get; }
    protected IEmailSender EmailSender { get; }
    protected ILogger AppLogger { get; }

    protected BaseController(IConfiguration iconfiguration,
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<ManageController> logger)
    {
        AppLogger = logger;
        EmailSender = emailSender;
        Config = iconfiguration;
        SignInManager = signInManager;
        UserManager = userManager; 
    }

    protected BaseController()
    {
    }
}

The parameters still have to be injected into the inherited controller and passed to the base constructor

public class TestBaseController : BaseController
{
    public static IConfigurationRoot Configuration { get; set; }

    public TestBaseController(IConfiguration config,
        UserManager<ApplicationUser> userManager,
        SignInManager<ApplicationUser> signInManager,
        IEmailSender emailSender,
        ILogger<ManageController> logger) : base(config,userManager,signInManager,emailSender,logger)
    {
    }

    public string TestConfigGetter()
    {

        var t = Config["ConnectionStrings:DefaultConnection"];
        return t;
    }

    public class TestViewModel
    {
        public string ConnString { get; set; }
    }
    public IActionResult Index()
    {
        var tm = new TestViewModel { ConnString = TestConfigGetter() };
        return View(tm);
    }
}

So now all the injected objects will have instances.

Was hoping the final solution would not require injecting the commonly needed instances into each inherited controller, only any additional instance objects required for that specific controller. All I really solved from a code repeating aspect was the removal of the private fields in each Controller.

Still wondering if the BaseController should inherit from Controller or ControllerBase?

This is what we are trying to avoid. – Sam Aug 11 '20 at 12:36 — Sam, Aug 11 '20 at 12:36

Dependency injection duplication in Controller and BaseController in .Net Core 2.0

3 Answers3

Linked