Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding constraints on Kernel.Get<IEnumerable<T>> are not applied to the resolution of T #404

Open
lord-executor opened this issue Nov 19, 2023 · 0 comments

Comments

@lord-executor
Copy link
Contributor

The context is Ninject 3.3.6.

I am currently looking into adding support for the newly introduced .NET 8 IKeyedServiceProvider over in Ninject.Web.AspNetCore when I ran into this issue.

I am not entirely sure whether this is a bug or a feature request, but over all it probably doesn't matter anyway since I'm primarily looking for a discussion.

The problem that I have encountered is that resolving enumerables/lists/etc. of services with the GetAll<T> method behaves differently from the Get<IEnumerable<T>> method when it comes to binding constraints. The following test demonstrates this in a way that is hopefully easy to understand:

public class EnumerableResolutionTest
{
    [Fact]
    public void KernelGetEnumerable_WithMultipleNamedBindings_DoesNotPropagateNameConstraint()
    {
        var kernel = new StandardKernel();
        kernel.Bind<IWarrior>().To<Ninja>().Named("one").WithConstructorArgument("name", "Alpha");
        kernel.Bind<IWarrior>().To<Ninja>().Named("one").WithConstructorArgument("name", "Beta");
        kernel.Bind<IWarrior>().To<Ninja>().Named("two").WithConstructorArgument("name", "Gamma");
        kernel.Bind<IWarrior>().To<Ninja>().Named("two").WithConstructorArgument("name", "Delta");

        // GetAll works correctly
        IList<IWarrior> warriors = kernel.GetAll<IWarrior>("one").ToList();
        warriors.Count.Should().Be(2);
        warriors.Select(n => n.Name).Should().BeEquivalentTo(new[] { "Alpha", "Beta" });
		
        // Get<IEnumerable<T>> does NOT work as expected since it returns all the warriors
        warriors = kernel.Get<IList<IWarrior>>("one");
        warriors.Count.Should().Be(4);
        warriors.Select(n => n.Name).Should()
            .BeEquivalentTo(new[] { "Alpha", "Beta", "Gamma", "Delta" });
    }
    
    private interface IWarrior
    {
        string Name { get; }
    }
    
    private class Ninja : IWarrior
    {
        public string Name { get; }
        public Ninja(string name)
        {
            Name = name;
        }
    }
}

As you can see, the Get<IList<IWarrior>> returns all the warriors regardless of the name constraint. This also applies to any other binding constraint (Func<IBindingMetadata, bool> constraint) argument that is passed to the Get method.

This behavior is caused by the KernelBase.Resolve.UpdateRequest method which explicitly passes null to the CreateRequest method when there is no parent request.

void UpdateRequest(Type service)
            {
                if (request.ParentRequest == null)
                {
                    request = this.CreateRequest(service, /* this right here */ null, request.Parameters.Where(p => p.ShouldInherit), true, false);
                }
                else
                {
                    request = request.ParentRequest.CreateChild(service, request.ParentContext, request.Target);
                    request.IsOptional = true;
                }
            }

It means that any binding constraint when resolving collections is effectively ignored which strikes me as weird and is definitely inconsistent with the GetAll behavior.

There doesn't seem to be any test in the Ninject project that actually relies on this behavior which makes me think that it might just be an accident. When I change the UpdateRequest method to pass the request.Constraint instead of null all of the tests still pass and that weird behavior goes away.

Logically, it makes sense to me to pass the constraint along in that case. If there is a parent request, then the updated request gets its constraint from the request.Target which also makes sense.

What do you think @scott-xu ?
I could of course create a PR with some additional tests to cover this behavior if that is something that you are interested in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant