Add support for IKeyedServiceProvider.

[DominicUllmann]

Tried to implement the keyed service provider support for >= .NET 8.0 without breaking < .NET 8.0

Sadly, there is one complication in this with resultion of IEnumerable. Ninject sadly doesn't consider metadata constraint when resolving an IEnumerable. This seems to be a potential bug in Ninject.

The following code snippet shows what Microsoft DI expects:

		var builder = WebApplication.CreateBuilder();
		builder.Services.AddKeyedTransient<IWarrior, Samurai>("Warrior");
		builder.Services.AddKeyedSingleton<IWarrior>("Warrior", new Ninja("test"));
		
		var app = builder.Build();
		var list = app.Services.GetKeyedServices(typeof(IWarrior),"Warrior") as IList<IWarrior>;
		list.Should().HaveCount(2);
		var list2 = app.Services.GetServices(typeof(IWarrior)) as IList<IWarrior>;
		var list3 = app.Services.GetRequiredService(typeof(IEnumerable<IWarrior>)) as IEnumerable<IWarrior>;
		list2.Should().HaveCount(0);
		list3.Should().HaveCount(0);

We can't resolve all the elements without considering the service key. The Microsoft DI always applies the constraint.

The PR should be extended with conversion to Array and List as well, that's not yet done. The current state is for disussion.

[DominicUllmann]

This is similar to how Ninject does it, but I can't reuse the Ninject version directly as it is internal. Here we should also handle the case if an array or list was requested similar to how Ninject does it.

[DominicUllmann]

As Microsoft DI can only resolve IEnumerable this should be fine.

[DominicUllmann]

Added this one to prevent code duplications in ServiceCollectionAdapter. MS decided to use Keyed* properties on the service descriptor for keyed services.
Note that the implementation factory receives the ServiceKey => that's maybe one reason for the different set of properties for keyed services.

[lord-executor]

Not sure if you saw that, but there is actually a separate set of "specification tests" in the "Microsoft.Extensions.DependencyInjection.Specification" namespace just for testing keyed services. And of course, a lot of them revolve around generics which is why currently 45 out of 54 of these services fail. I think if we're trying to support keyed services, the result should somehow get all of these tests to green. This is why I abandoned that side-project when keyed services first arrived on the scene. I could not figure out a way to satisfy all of those requirements without essentially building two different Kernels, one for keyed services and one for unkeyed services.

[DominicUllmann]

Not sure if you saw that, but there is actually a separate set of "specification tests" in the "Microsoft.Extensions.DependencyInjection.Specification" namespace just for testing keyed services. And of course, a lot of them revolve around generics which is why currently 45 out of 54 of these services fail. I think if we're trying to support keyed services, the result should somehow get all of these tests to green. This is why I abandoned that side-project when keyed services first arrived on the scene. I could not figure out a way to satisfy all of those requirements without essentially building two different Kernels, one for keyed services and one for unkeyed services.

Thanks a lot, I was wondering about that. Thanks for having added these. I have discovered a few features I have not implemented yet and also some implementation problems.
But there are really strange requirements, like e.g. ResolveKeyedServiceSingletonInstanceWithAnyKey or ResolveWithAnyKeyQuery_Constructor.
I will have to check if I can support these.

[lord-executor]

Thanks a lot, I was wondering about that. Thanks for having added these. I have discovered a few features I have not implemented yet and also some implementation problems. But there are really strange requirements, like e.g. ResolveKeyedServiceSingletonInstanceWithAnyKey or ResolveWithAnyKeyQuery_Constructor. I will have to check if I can support these.

I definitely didn't remember all of them, but I do remember that this was the best example I have ever seen that an interface on its own is just a horribly insufficient form of specification 😬. It appears that they simply made some design decisions for what would work best with their particular implementation and that then became the implicit specification.

[lord-executor]

This is also definitely interesting...

public class Service : IService
{
    private readonly string _id;

    public Service() => _id = Guid.NewGuid().ToString();

    public Service([ServiceKey] string id) => _id = id; // oh yeah, btw: the ServiceKey can be injected into the service

    public override string? ToString() => _id;
}

[DominicUllmann]

We have to split the binidngindex by servicekey as otherwise the overriding doesn't work correctly.

[DominicUllmann]

We have to create a ConstructorInjectionDirectiveWithKeyedSupport instead of a ConstructorDirective. Therefore we have to use an own implementation here.

[DominicUllmann]

Important here is to create a ParameterTargetWithKeyedSupport to support FromKeyedService as well as ServiceKey attribute.

[DominicUllmann]

As we have now always constraints (either to exclude keyed bindings or only consider keyed bindings, we can't check for request.Constraint == null. But this should not be a big issue as Microsoft.Extension.DI bindings can't provide custom constraints. For normal ninject bindings we don't have BindingIndex, so we can always apply them here in my opinion. Do you see any reason why we have request.Constraint == null here in the first place @lord-executor ? All the existing tests still work when removing it.
Current state is that 20 of the new tests fail...

[DominicUllmann]

All of the new tests are ok now beside of 4 which are really too stupid and we don't want to support these.

[DominicUllmann]

I managed to fullfill all the compliance tests but 4. I think these 4 are just wrong and stupidly enforce an implemntation detail:

The 4 failing tests enforce that the enumarble returned when querying the same list of instances is the same instead of just equal. Some other tests doing list queries just compare for equality instead of same list instance, which would make much more sense.

So I would say the current code fullfills all the tests.

[DominicUllmann]

Tried to implement the keyed service provider support for >= .NET 8.0 without breaking < .NET 8.0

Sadly, there is one complication in this with resultion of IEnumerable. Ninject sadly doesn't consider metadata constraint when resolving an IEnumerable. This seems to be a potential bug in Ninject.

The following code snippet shows what Microsoft DI expects:

		var builder = WebApplication.CreateBuilder();
		builder.Services.AddKeyedTransient<IWarrior, Samurai>("Warrior");
		builder.Services.AddKeyedSingleton<IWarrior>("Warrior", new Ninja("test"));
		
		var app = builder.Build();
		var list = app.Services.GetKeyedServices(typeof(IWarrior),"Warrior") as IList<IWarrior>;
		list.Should().HaveCount(2);
		var list2 = app.Services.GetServices(typeof(IWarrior)) as IList<IWarrior>;
		var list3 = app.Services.GetRequiredService(typeof(IEnumerable<IWarrior>)) as IEnumerable<IWarrior>;
		list2.Should().HaveCount(0);
		list3.Should().HaveCount(0);

We can't resolve all the elements without considering the service key. The Microsoft DI always applies the constraint.

The PR should be extended with conversion to Array and List as well, that's not yet done. The current state is for disussion.

Another difference here is that Microsoft DI can only resolve lists based on IEnumerable. The others just return null:

		var builder = WebApplication.CreateBuilder();
		builder.Services.AddKeyedTransient<IWarrior, Samurai>("Warrior");
		builder.Services.AddKeyedSingleton<IWarrior>("Warrior", new Ninja("test"));

		var app = builder.Build();
		var list = app.Services.GetKeyedServices(typeof(IWarrior), "Warrior") as IList<IWarrior>;
		var list1 = app.Services.GetKeyedService(typeof(IEnumerable<IWarrior>), "Warrior") as IEnumerable<IWarrior>;
		var list1a = app.Services.GetKeyedService(typeof(IList<IWarrior>), "Warrior");
		var list1b = app.Services.GetKeyedService(typeof(IWarrior[]), "Warrior");

i.e. list1a and list1b return null

[DominicUllmann]

I managed to fullfill all the compliance tests but 4. I think these 4 are just wrong and stupidly enforce an implemntation detail:

The 4 failing tests enforce that the enumarble returned when querying the same list of instances is the same instead of just equal. Some other tests doing list queries just compare for equality instead of same list instance, which would make much more sense.

So I would say the current code fullfills all the tests.

I have put some ignores so that these 4 tests are not run by dotnet test. By this everything is green now.

[DominicUllmann]

The compliance test for non keyed services is really stupid. It checked that ActivatorUtilities.Activate returns InvalidOperationException, but it doesn't call GetRequiredService directly. But checking the implemenation it throws InvalidOperationException. So for consistency, I adapted here as well.

[DominicUllmann]

I need this special request to have the option to get the ServiceKey in the AnyBindingResolver in the end.

[DominicUllmann]

Have to adapt the logic form base.ResolveWithin(parent) to correctly handle fromKeyedService attributes and default values. The implementation itself is quite similar still to the base implementation.

[lord-executor]

Looks like you made it! (🎵 Look how far we've come my baby 🎵 / Shania Twain - You're Still The One)
Quite impressive that you managed to satisfy all of those, at times rather odd, constraints of the keyed service implementation.

Also... Happy New Year 🥳

Fun fact: If you check for KeyedService.AnyKey references on source.dot.net which includes all of ASP.NET Core, there isn't a single actual use case of that. I went to check for that because I wanted to know if this is used for anything other than getting a list of all keyed services of some type... I'm having a hard time coming up with an actual legitimate use case where I would be needing a list of all keyed services but none of the unkeyed ones 🤔.

Let's see if I caught all the big topics that this implementation has to cover:

• actual "keyed services" that can be requested by key
• a fundamental separation of keyed services from unkeyed ones - a separation that is underminded by the fact that using null as the service key essentially means unkeyed services...
• the magical and mystical behavior of KeyedService.AnyKey
• injecting the service key itself into services
• dealing with the FromKeyedServicesAttribute

Most of what I "found" in the code review is how stupid some of the compliance tests are and how an actual specification or documentation would have been nice. There are a whole lot of very awkward and implicit constraints that really should not have made it into the "design" of keyed services. I made a couple of small suggestions, but I couldn't find any actual problems with the implementation 👍 .

At this point, the changes to the standard Ninject behavior are quite extensive which makes me wonder about the future of Ninject overall. It's probably for the better that Ninject isn't being actively developed since the types of interventions that we are now making would otherwise be quite a liability for maintenance.

[lord-executor]

I'm not sure I understand the purpose of this 🤔. The IndexedBindingPrecedenceComparer is not considering the IndexKey as far as I can tell. Is the primary reason for this key so that we don't have null keys? If so, then I would suggest instead of using a string that anybody could "accidentally" use as an actual service key, we do another "Microsoft copy-pasta bit" and define a dedicated private sealed nested type:

private sealed class DefaultIndexKey
{
    public override string? ToString() => nameof(DefaultIndexKey);
}

[DominicUllmann]

We have to also support overriding bindings with servicekeys.
If we have a registration for IWarrior without service key and with some service keys and we override them again and again only the latest binding per service key must be considered when resolving a unique instance.

[DominicUllmann]

But the idea with the class is good, adapted in 299b11f

[DominicUllmann]

Looks like you made it! (🎵 Look how far we've come my baby 🎵 / Shania Twain - You're Still The One) Quite impressive that you managed to satisfy all of those, at times rather odd, constraints of the keyed service implementation.

Also... Happy New Year 🥳

Fun fact: If you check for KeyedService.AnyKey references on source.dot.net which includes all of ASP.NET Core, there isn't a single actual use case of that. I went to check for that because I wanted to know if this is used for anything other than getting a list of all keyed services of some type... I'm having a hard time coming up with an actual legitimate use case where I would be needing a list of all keyed services but none of the unkeyed ones 🤔.

Let's see if I caught all the big topics that this implementation has to cover:

• actual "keyed services" that can be requested by key
• a fundamental separation of keyed services from unkeyed ones - a separation that is underminded by the fact that using null as the service key essentially means unkeyed services...
• the magical and mystical behavior of KeyedService.AnyKey
• injecting the service key itself into services
• dealing with the FromKeyedServicesAttribute

Most of what I "found" in the code review is how stupid some of the compliance tests are and how an actual specification or documentation would have been nice. There are a whole lot of very awkward and implicit constraints that really should not have made it into the "design" of keyed services. I made a couple of small suggestions, but I couldn't find any actual problems with the implementation 👍 .

At this point, the changes to the standard Ninject behavior are quite extensive which makes me wonder about the future of Ninject overall. It's probably for the better that Ninject isn't being actively developed since the types of interventions that we are now making would otherwise be quite a liability for maintenance.

Thanks a lot, it was quite hard work to fullfill all this. But luckily Ninject is quite flexible and has extension points which allow to also fullfill such strange requirements. But yes, especially the FromKeyedServices implementation needed some advanced customizations.

[lord-executor]

Thanks a lot, it was quite hard work to fullfill all this. But luckily Ninject is quite flexible and has extension points which allow to also fullfill such strange requirements. But yes, especially the FromKeyedServices implementation needed some advanced customizations.

Nice cleanup too 👍. It all checks out as far as I can tell and I've test it on some of my projects too.