Is it necessary to check null values with constructor injection?

Question

I'm using .NET Core constructor injection. In a code review from a colleague, he raised the question if I should check for null values on injected dependencies in controllers.

Since the framework is responsible for creating an instance of the service, it seems to me that it would take care of any errors and never have a null value dependency passed to a constructor. I don't have any factual evidence for this though, so I'd like to know if it's possible that a null check may be necessary.

For example, should I check if 'myService' is null in the following code? (Assuming the code is configured to use DI)

public class MyController
{
    private readonly IMyService _myService;

    public MyController(IMyService myService) 
    {
        _myService = myService;
    }
}

Moonfaced

That's what I suspect as well, but can you link to documentation or another source that validates this? – Moonfaced

@CamiloTerevinto that does not mean a null instance could never be passed in though right?. If not from the IOC container, a manual instantiation could potentially be null. I have always just checked for null and threw a ArgumentNullException to be safe – Odle

@CamiloTerevinto it is anti-DI, and I dont ever manually instantiate anything. However there is no way to enforce that programatically. I dont think checking just to be safe hurts or takes any significant processing power – Odle

@CamiloTerevinto: Let us asume that not everyone is doing everything perfect. That only failed with Cooperative Multitasking, Naked Pointers, unchecked Array bounds, expecting a dealloc to follow every alloc, etc. ;) – Lobar

@CamiloTerevinto Which is fair, I am only playing devils advocate (I know that wasnt directed at me) – Odle

@CamiloTerevinto: "I do not need to make a check because the framework will prevent a faulty state" is exactly the kind of asumptions that got us all those examples :) I follow this simple rule for all checks: codeproject.com/Articles/9538/… – Lobar

The assumption here is that manual instantiation would never make it through code reviews and common sense. But I do agree that you can never be safe enough. – Moonfaced

... manual instantiation would never make it through code reviews and common sense even in unit tests? – Preteritive

@Preteritive I don't know, tell me. I'm new to both programming and this company. Wouldn't we mock the service or something? Is that basically manual instantiation, or can we use a DI container in unit tests? I don't know. – Moonfaced

Answer 1

Is it necessary to check null values with constructor injection?

It depends.

• Is this internal code, used by you and (possibly) some teammates in a (fortunately) code-review environment?

Don't. It's not necessary. The framework doesn't allow this.

• Is this code in a public library, used by multiple people or not actually following Dependency Injection?

Then do it. A manual instantation would result in a NullReferenceException somewhere and those are hard to track down.

That said, using something like this:

public MyController(IMyService myService) 
{
    if (myService == null)
    {
        throw new ArgumentNullException(nameof(myService));
    }

    _myService = myService;
}

Is an extremely cheap check and it's much easier to track down if someone passes null for some reason.

---

Even better, as @ScottFraley mentions, with newer C# versions, the above can be even more readable:

public MyController(IMyService myService) 
{
    _myService = myService ?? throw new ArgumentNullException(nameof(myService));
}

Nadya

Also, with newer C# language options, you could just do; (just inside the ctor of course) _httpContextAccessor = httpContextAccessor ?? throw new ArgumentNullException(nameof(httpContextAccessor)); kinda thing. – Buchan

Thanks @ScottFraley - added to answer – Nadya

Answer 2

To get the obvious out of the way, there is no harm in doing a null check in your constructor.

---

There are two methods for obtaining DI services from the framework.

The first is GetService<T>(). This will return a null value if such a service has not been registered.

The second is GetRequiredService<T>(). This will throw an exception if the service can't be found.

Both of these methods throw exceptions if they can't fully instantiate the service you are asking for.

class Program
{
    static void Main(string[] args)
    {
        var services = new ServiceCollection()
            .AddTransient<IServiceB, ServiceB>()
            .BuildServiceProvider();

        var servB = services.GetService<IServiceB>();
    }
}

public interface IServiceA { }
public interface IServiceB { }

public class ServiceA : IServiceA { }
public class ServiceB : IServiceB { public ServiceB(IServiceA a) { } }

In this example, ServiceB requires an IServiceA, but A is not added to the dependency graph. The last method will throw an exception:

System.InvalidOperationException: 'Unable to resolve service for type 'DITests.IServiceA' while attempting to activate 'DITests.ServiceB'.'

If I instead did services.GetService<IServiceA>(), I'd get a null value.

---

You can see this for yourself by looking at the GitHub source code. When calling either method, it will eventually make it's way to the CreateConstructorCallSite method. This throws an exception if it's unable to resolve the dependencies of your type.

---

As for ASP.Net Core MVC, it uses GetRequiredService<>() for getting your controllers from the DI graph.

---

In conclusion, no, you do not need to perform null checks for DI objects in your constructor if you're using the pure Microsoft DI framework. As Camilo Terevinto said, the framework does not allow it.

As noted in your post, I have not seen a written Microsoft document that explicitly says you do not need to

If you are passing in an IServiceProvider as a constructor argument and you're resolving services from within the constructor, then you'll need to do null checks.

Beka

Answer 3

Either of these answers from Camilo Terevinto or gunr2171 will do any good for you.

Generally, we can discuss if trading one exception to another (NullReferenceException to ArgumentNullException) is a good deal or if it is a deal at all. Ask yourself if the latter brings any new information to you. My humble opinion in this particular case is No.

If any of your controller's dependency is null, you immediately know where is the problem.

One way or another, you know immediately what and where is the problem in your particular case. A null check in a constructor of a controller is just overhead. That is where the answer from Camilo Terevinto fails. You will never initiate it by yourself. The build-in DI service in .NET Core is designed to never inject a null into your constructors unless you instruct it to do so.

The answer from gunr2171 is talking about completely different aspect of DI. It is a valid answer but for a different question.

---

More than 80 % of DI use is a constructor injection. Most of these injection are related to MVC controllers and view models in Razor pages. MVC is a deprecated design pattern in ASP.NET Core 3.x and replaced by Razor Pages but the logic is pretty much same.

Any usable controller needs at least three dependencies:

1. logger
2. mapper
3. repository

Thrust me here, you don't want to type:

_logger = logger ?? throw new ArgumentNullException(...)
_mapper = mapper ?? throw new ArgumentNullException(...)
_repository = repository ?? throw new ArgumentNullException(...)

to every single controller/view model you have.

There are definitely better ways how to do it. My personal recommendation is this answer in a duplicate question.

---

.NET 6 and beyond

There is a new method in .NET API ArgumentNullException.ThrowIfNull(someParameter).

This method can save you some typing if you insist on null checks.

Benitobenjamen

What is an injected repository doing in a Controller in the first place? – Destination

Well, it it quite common practice. See this. – Benitobenjamen

"Ask yourself if the latter brings any new information to you" - I would argue that the benefit of throwing an ArgumentNullException over a NullReferenceException is that you'd be failing early, throwing on instantiation rather than at some point (potentially quite a while) later, obscuring the real issue. If this is a library class or will see any use other than purely by DI, I'd put the check in, the overhead of doing so is truly miniscule. – Indubitable

@Indubitable Fail early is a valid approach, but not in the context of this question. The .NET Core/.NET 5 DI will not inject null into constructors. If the injected dependencies become null, it will happen outside the scope of these checks, so once again - there is no real value to having them there. – Benitobenjamen

The key point is The build-in DI service in .NET Core is designed to never inject a null into your constructors.. In case the dependency registration is not done, we get an error in HostBuilder's run itself which is usually declared in the Program file. This means, unlike in the past where we get NullReferenceException only when the dependency is used, here in .NetCore we get the error in initial stage itself. – Perrin

I'm just reading this, but this answer makes no sense. You're claiming that there's no difference between a NullReferenceException (with absolutely no debug information) and an ArgumentNullException (which tells you which exact parameter is null). Your humble opinion couldn't be more wrong, honestly. If you are using .NET Core's DI, you don't ever need a null check, so your second part of the answer makes no sense. Also, MVC and Razor Pages are different technologies for different purposes. This answer makes some really bad statements – Nadya

@CamiloTerevinto Please, read my answer carefully. There is nothing about NullReferenceException == ArgumentNullException. My second part answer provides generic reusable solution that can be used anywhere. Same story about MVC and Razor pages. Please, do not reinterpret some my words into something else. Thank you. – Benitobenjamen

"Ask yourself if the latter brings any new information to you. My humble opinion in this particular case is No." -> completely wrong, as my comment above should inform you. "A null check in a constructor of a controller is just overhead. That is where the answer from Camilo Terevinto fails" -> I begin my answer stating WHY it's not needed. I'm not sure how much English you understand, honestly. – Nadya

You claim that doing the check in the constructor "is not needed" but you go on to suggest a terrible method to do the check in the constructor but hidden away using Reflection - illogical from any point of view. – Nadya

1) Hiding details is called encapsulation. 2) The reflection is used only when needed. 3) You solution introducing a lot of redundant code with no value. My claim was only about controller and their decencies. If a dependency is null and you receive NRE, it is pretty much obvious where the problem is. | Camilo, you have your opinion, I have mine. You downvoted my answer, I did your. I think we are done here. – Benitobenjamen

Answer 4

I'll like to propose a different point of view. This...

private readonly IMyService _myService;

public MyController(IMyService myService) 
{
    _myService = myService ?? throw new ArgumentNullException(nameof(myService));
}

...is not only a run-time check for the execution environment, it's also a class invariant for future developers (cf. "Design by Contract").

Since _myService is readonly and every constructor contains a null check, a developer modifying/extending this class in the future will know for sure that the field _myService can never be null, even without knowing anything about how the class is used.

Another way to specify this invariant would be to add a comment to _myService...

// will always be provided by DI and, thus, will never be null
private readonly IMyService _myService;

...but adding a null-check to the constructor is the more idiomatic way to do that in C#.

Spumescent

Nowadays, if the service can be null you do IMyService? _myService. You don't even need the comment. – Vesuvian

@DanielBoteroCorrea: True, nullable reference types made things a lot easier. – Spumescent