Generic vs non-generic ResolutionExtension.TryGet overloads are inconsistent

[lord-executor]

Run the following code in RoslynPad or in a simple .NET console application with minor adaptations.

#r "nuget:Ninject/3.3.4"

using Ninject;

interface IWarrior {}

class Paladin : IWarrior {}

var kernel = new StandardKernel();
kernel.Bind<IWarrior>().To<Paladin>();
kernel.Bind<IWarrior>().To<Paladin>();

kernel.TryGet<IWarrior>(meta => true).Dump(); // works; returns null
kernel.TryGet(typeof(IWarrior), meta => true).Dump(); // fails; throws InvalidOperationException

To be precise, the exception is:

InvalidOperationException: Sequence contains more than one element
   at System.Linq.ThrowHelper.ThrowMoreThanOneElementException()
   at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source)
   at Ninject.ResolutionExtensions.TryGet[T](Func`1 iterator)
   at Ninject.ResolutionExtensions.TryGet(IResolutionRoot root, Type service, Func`2 constraint, IParameter[] parameters)

The only difference in the two calls is that one uses the generic version TryGet<T> while the other does not.

The problem lies in the different parameters to GetResolutionIterator in

• Generic: https://github.com/ninject/Ninject/blob/3.3.4/src/Ninject/Syntax/ResolutionExtensions.cs#L111 which passes true for isUnique
• Non-Generic: https://github.com/ninject/Ninject/blob/3.3.4/src/Ninject/Syntax/ResolutionExtensions.cs#L263 which passes false for isUnique

To me, this seems very obviously incorrect and from the code in TryGet which explicitly catches an ActivationException, the case seems quite clear: all overloads of TryGet should pass true for isUnique. That way, the method behaves as expected and just returns null if it cannot resolve the request to a uniquely defined service.

I have not actually tested this, but from looking at the source code of the current master branch, I would say the problem exists there as well.

I can easily provide a PR for a fix, but it seems that there is really not a lot of activity on this project at the moment. If any one of the maintainers of this project is at least reading this, please give a sign.

[scott-xu]

Thanks for reporting. I'll take a look.

[lord-executor]

Hmm.... There are some interesting tests in Ninject.Tests.Integration.ReadOnlyKernelTests with [Fact(Skip ="Unique?")] that seem pretty relevant to my problem here. Why are those tests skipped and why do they seem to assert pretty much the opposite of what the corresponding TryGet<T> do?

[scott-xu]

From my understanding, the "TryGet" seems very "kind" about the resolution process.
What if we change SingleOrDefault to FirstOrDefault in https://github.com/ninject/Ninject/blob/3.3.4/src/Ninject/Syntax/ResolutionExtensions.cs#L415?

[scott-xu]

As you can see, the name of the tests contains "ResolvesUsingFirstMatchingBinding"

[lord-executor]

Whether or not FirstOrDefault would be better seems like it is missing the point. The problem that I am experiencing is not one of the "logic" of TryGet, it is one with inconsistent implementations of TryGet vs. TryGet<T>.

I simply cannot conceive a reasonable explanation why kernel.TryGet<IWarrior>(meta => true) should behave differently from kernel.TryGet(typeof(IWarrior), meta => true). If I had been the one to write the implementations of the TryGet variants, then the generic version would simply be calling the non-generic version to reduce the potential for errors.

All versions of Get and TryGet except for the two that I changed in #380 use true as the value for the GetResolutionIterator isUnique parameter. Using true makes sense since both Get and TryGet return (at most) a single instance. This means that if there are multiple matching bindings, the resolution process will throw an ActivationException which is then caught in the internal private static T TryGet<T>(Func<IEnumerable<T>> iterator) method (https://github.com/ninject/Ninject/blob/3.3.4/src/Ninject/Syntax/ResolutionExtensions.cs#L419) which then returns default(T).
The two TryGet variants that use false instead of true quite frankly look like a simple coding mistake to me; the call to SingleOrDefault in a process that allows non-unique bindings is what causes the LINQ exception which is not caught.

Using FirstOrDefault would just return the instance of the first matching binding, but the expected behavior of TryGet is that it only returns an instance in the cases where Get would also return an instance. The whole point of TryGet is that it simply does not throw an exception in the cases where Get does throw an exception and returns null instead.

As for the tests: They are currently disabled / skipped which (to me) tells me that somebody has realized this inconsistency before but never got around to actually addressing it. The tests literally show excactly how / where the different TryGet implementations are inconsistent.

[scott-xu]

So it really depends on the definition of "TryGet". Literally, returning the first matched means we tried our best and we think the first one is the most probable candidate, while returning null is more consistent with "Get" but it hides two cases. One case is no binding found and not self-bindable, another case is multiple bindings found.

[scott-xu]

Let me check with Microsoft Dependency Injection library and see what's their solution about GetService when there are multiple registrations.

[scott-xu]

It looks like the Microsoft Dependency Injection library uses last descriptor when call GetService/GetRequiredService.

[scott-xu]

There are three options here for Get

1. throw exception if there are multiple matched bindings
2. resolve the instance using the first matched binding
3. resolve the instance using the last matched binding

The relevant TryGet would be

1. return default(T) if there are multiple matched bindings
2. resolve the instance using the first matched binding
3. resolve the instance using the last matched binding

We can provide a setting for this behavior to let user select what behavior they want.

Anyway, I agree that we should make it consistent for all TryGet overloads.

[lord-executor]

According to my understanding of the Ninject conventions, you should only use TryGet in cases where you expect to encounter a single matching binding. In cases where you don't want to deal with the exceptions that Get throws. One use case for this is for an optional service which may or may not have a binding, but to assume that that is the only reason somebody out there would be using TryGet is a pretty big assumption. I thnink adding a separate GetOptional would be more helpful since I think the fact that Ninject actually tries to prevent this type of ambiguity is one of its core strengths.

If you simply do not know if there are zero or more matching bindings, you should use GetAll instead and sort things our for yourself. TryGet doesn't guess which service you should receive; just like the rest of Ninject, it is pretty deterministic. If there is a unique service to resolve, then it will return that, otherwise it will return null without trying to guess what you may have been trying to do.

Also... after (successfully, but painfully) implementing a Ninject integration for ASP.NET Core (https://github.com/lord-executor/Ninject.Web.AspNetCore/tree/bugfix/service-provider-resolve-latest/src/Ninject.Web.AspNetCore) and going through the implementation details of the Microsoft dependency injection code, I have come to the following conclusions:

• It is overly simplistic and overall pretty bad conceptually
• It was built to work for exactly that one use case (ASP.NET Core) and anything else is going to have a hard time
• The fact that duplicate bindings are allowed and resolve to the "latest" descriptor is undocumented and looks much more like an accidental side-effect (or "emergent behavior" if you feel generous) of the implementation than a deliberate decision
• This "just return the latest" approach only works because descriptors are added in an imperative cascade that is very sensitive to even minor changes in the execution order (temporal coupling) which requires any developer to effectively understand the exact call tree of the entire ASP.NET Core DI configuration if he/she wants to do anything non-trivial.

I would even go so far as to argue that this temporal coupling aspect of extension methods calling each other all over the place is the worst feature of that DI system and consequently looking to that implementation for guidance would be unwise.

Btw: Sorry.... I have what I like to refer to as "strong opinions" on topics where I have spent a lot of time analyzing and comparing different designs and Ninject vs. Microsoft.Extensions.DependencyInjection is one such topic.