Dependencyinjection: Property injection should fail for incomplete registrations

[seesharper]

In the process of getting LightInject ready for RC2, I ran the tests against the latest bits and discovered that I had one failing test (ClosedServicesPreferredOverOpenGenericServices). This test used to pass and the only thing that has changed was it was using AnotherClass as the generic argument instead of a string in the old version. I soon discovered that it failed because that AnotherClass has a constructor dependency (IFakeService) that is not registered with the container. I understand the loose strategy for property injection, but IMHO it should fail if the property dependency is registered and that dependency in turn has a dependency that is not registered/resolvable. Otherwise this could lead to some scenarios that could be very hard to debug.

Let me illustrate what I mean with a couple of tests.

 public void ShouldHandleMissingPropertyDepenency()
{
    // Arrange 
    var collection = new ServiceCollection();
    collection.AddTransient(typeof(FakeService), typeof(FakeService));
    var provider = CreateServiceProvider(collection);

    // Act 
    var service = provider.GetService<FakeService>();

    // Assert
    Assert.IsType<FakeService>(service);
    Assert.Null(service.Value);                        
}

The fact that the value is null(AnotherClass) is perfectly fine since it was never registered into the service collection. The next test demonstrates the problem.

public void ShouldThrowExceptionForIncompletePropertyDependency()
{           
    // Arrange
    var collection = new ServiceCollection();
    collection.AddTransient(typeof(FakeService));
    collection.AddTransient(typeof(AnotherClass));
    var provider = CreateServiceProvider(collection);

    // THIS SHOULD FAIL 
    var service = provider.GetService<FakeService>();

}

This test should fail because the property dependency (AnotherClass) is registered, but is not actually resolvable because AnotherClass has a unregistered required constructor dependency.

If this (dangerous) behavior is by design, there should at least be tests that verifies this behavior.

[pranavkm]

I think the test can be refactored to not reuse AnotherClass to avoid this failure. It's isn't trying to intentionally produce an incomplete dependency graph.

[dotnetjunkie]

Refactoring the test doesn't resolve the underlying problem. @seesharper is absolutely right about this. If you want to support implicit property injection (which is questionable by itself), you should at least prevent to skip injecting registered types. And there should be unit tests that verify this behavior.

[davidfowl]

Yes, it should fail but @pranavkm 's point is that this specific test wasn't intentionally trying to use an unregistered service. We should have a separate test to make sure the above scenario does fail though.

[seesharper]

I totally agree that we need a test specific for this scenario rather than relying on a test with another intent to cover it. This new test will not pass with the current implementation though. Is there still time to fix this within the RC2 timeframe?

[dotnetjunkie]

Fixing this after RTM is a breaking change, which will also break all the adapter implementations.

[seesharper]

The test used to supply a string as the generic argument, but was rewritten to use AnotherClass in this commit.
https://github.com/aspnet/DependencyInjection/commit/6a7bdda613c0cc526775b38532dedbffd2d2d6ac

Possible solutions are to revert this commit or make sure that the test register IFakeService that is required by the AnotherClass constructor.

But that does not take care of the underlying problem which is that the default implementation allows incomplete graphs to be injected into properties. I assume that this is not the case for constructor dependencies although I have not verified this myself.

The abstraction defines a contract through a well defined set of interfaces, but the behavior of the default implementation is also part of the contract. This can only be verified through unit tests. Every such behavior being depended upon in ASP.NET Core, Entity Framework or any other library must be covered by a unit test so that developers can safely replace the default implementation with another adapter that fulfills the requirements. This means passing the tests. The minute you introduce behavior not covered by a specification test, a third party adapter is most likely to break something. Currently there are 41 tests in the specification package and I got to say that I am a little worried as if to every scenario is accounted for in these tests. Even LightInject, which is a relatively simple DI library has got over 500 unit tests.

That being said, how do we fix this? I can PR the failing test, but I think someone with more knowledge about the default implementation should actually make the test pass.

I can also PR the test in question so that it does not try to inject an incomplete graph.

PS: Congrats on the RC2 release :)

[dotnetjunkie]

but the behavior of the default implementation is also part of the contract

@seesharper is spot on here: The built-in container defines the contract of the adapter.

It seems to me that Microsoft never really realized this, since if they would, there would have been way more tests that verify the adapter’s behavior. With 41 tests, it is currently quite easy to build an adapter that makes all the tests green, but that will fail with real world usage, since it isn't an exact superset of the behavior defined by the built-in container.

And to make things worse, every feature that is added to the built-in DI library after v1 will be a breaking change, since –again- the built-in library is part of the contract. Every added feature will break every existing adapter. So in fact, the existence of the adapter is even stifles improvement and innovation within the built-in container itself; every change is a breaking change. This is exactly why people like @ploeh, @danielmarbach and myself have been warning Microsoft about their adapter.

[Eilon]

@seesharper we would certainly be happy to work with you on a PR to improve the tests.

As an aside:

We realize that we had to make some compromises in the design and implementation of Microsoft.Extensions.DependencyInjection, but that statement is of course true of all software. Ultimately, despite the shortcomings of the system, we have seen generally positive experiences from people using the system. The DI system used here isn't intended to be the end-all, be-all implementation, _and also not_ the end-all, be-all interface. There are undoubtedly many scenarios where the code in this repo doesn't help. And any "user" code can ultimately use any DI container it wishes. It's the components that choose to depend on the contracts and/or implementations here that might have some of the limits described in the various discussions we've had.

[Eilon]

Let's update the specification tests to have better coverage in these areas.