Adding support for constrained open generics

[jbogard]

(again)

See #471 for details. Figured I'd give this another shot.

The general scenario is:

Suppose I have some interface, IRepository<T>. This interface allows me to save an entity to a database, perhaps wrapping EFCore DbContext. As part of saving, I want to validate so I have IValidator<T>.

The implementation DbContextRepository<T> uses an enumerable of IValidator<T> to validate:

class DbContextRepository<T> {
    public DbContextRepository(
        DbContext db, 
        IEnumerable<IValidator<T>> validators) {
        // etc
    }
}

I want to be able to support:

class IFooValidator<T> : IValidator<T> where T : IFoo { }

instead of

class IFooValidator : IValidator<IFoo> {}

For the reasons in #471.

In case anything has changed, just wanted to throw this out there. I modified the approach to do what Autofac does, which is actually check the constraints if they exist (instead of the previous try-catch-swallow approach).

[Eilon]

@muratg / @davidfowl - can you take a look?

[jbogard]

For posterity of "how do other containers handle this?":

• Autofac
• Castle Windsor
• StructureMap
• LightInject
• DryIoc
• JasperFx-Lamar
• SimpleInjector

SimpleInjector is the odd one out here but @dotnetjunkie would know more I suppose about the approach he took.

Unity is the only ASP.NET DI-supporting container I could find that does not support this feature. I'm not sure the impact, that package has 2k NuGet downloads and Autofac has >1M.

[dadhi]

What about this case?

public interface I<T> {}
public interface X<A, B> where A : I<B> {}
public class Y<A> : X<A, String> where A : I<String> {}

collection.AddTransient(typeof(X<,>), typeof(Y<>);

[jbogard]

Would existing containers even be affected? If they didn't support it before, they blew up with exceptions. Unity for example just blows up with that ArgumentException. They'd only see the blow up during service resolution, not configuration, so for those containers that don't support filtering services based on generic constraints, they'd continue to blow up today.

From my understanding of the extensions, they either replace IServiceCollection, or dump its contents into their own container configuration. So AFAIK, containers that supply their own IServiceProviderEngine won't ever hit the code in this PR. Unity will continue to break :)

[IDisposable]

Perfect then... just wasn't wrapping my head around the full use case.

[pakrym]

Would existing containers even be affected? If they didn't support it before, they blew up with exceptions. Uni

It doesn't exactly work like that.

As soon as a feature is added to DI.Specification AspNetCore might take a dependency on it and would start exploding with containers that used to run fine on.

@jbogard did you validate that all mentioned containers actually pass new spec test?

@ENikS what are your thought on this feature? Do you think it's worth adding? Would you be willing/able to support it in unity?

[pakrym]

How does this change affect time to first service benchmark (https://github.com/aspnet/DependencyInjection/blob/dev/benchmarks/DI.Performance/TimeToFirstServiceBenchmark.cs)?

[jbogard]

What's the implication of moving it out of the DI.Specification? I just want the feature in this container, I'm less concerned if every container does. Though I wasn't sure if features are added to this container just for ASP.NET Core, or other libraries that also want to use it.

I'll update with benchmark #'s.

I've also opened an issue with Unity here: https://github.com/unitycontainer/container/issues/81

I can also open a PR with them.

[jbogard]

Benchmarks after: https://gist.github.com/jbogard/20f026885a79fb5f0227f577d16397f0

Benchmarks on dev: https://gist.github.com/jbogard/82db983fe5d1518bc8bd230211422db7

[ENikS]

I would like to implement this feature but I am not sure what is correct behavior. Is there a concise description of how it should behave?

Correct me if I am wrong. I am assuming it applies to either resolving collection or single service. When resolving collection it should return only these that satisfy constraint and when resolving single service it should blow?

[jbogard]

@ENikS it mainly concerns collection behavior. If I'm resolving IEnumerable<IService<...>> then only the IService implementations that can close the generic parameters should be returned. Then based on the generic parameters and what fits constraints I may have a different list of services returned.

It really isn't for single services, but most containers I've checked just let it apply to both, and let the parent service's needs dictate the behavior (if I require a service, and none exists, blow up).

The real-world scenarios are in cases where I still need the generic parameter for a further dependency. I want the generic parameter to "flow" down, instead of terminating with a base type. Keeping IFooService<T> : IService<T> where T: Foo instead of subclassing to IFooService : IService<Foo>. If the implementer then has dependencies, it's lost the T parameter to then get other dependencies that may depend on that T parameter.

In terms of what it means to support this behavior, all containers just put a try-catch around closing an open generic type (some try to catch the simple cases without just catch-first) since the methods to actually test a closed type aren't public in the BCL. See the links above for how other containers do it. It can be pretty simple to support, just as simple as try-catch-don't match.

[ENikS]

I will add this to next patch release of Unity.

[dadhi]

Correction regarding DryIoc.

It does the filtering when registering by walking down the constraints.

The try-catch in the link above is for resolution phase to wrap the unlikely failed match in container exception.

[jbogard]

Autofac does the same, but there are wonky edge-cases that basically only the compiler catches right now. Tries to do its best and catches as the last resort. Other containers weren't so brave as to try to mimic the compiler ;)

On Thu, Apr 12, 2018 at 8:37 AM, Maksim Volkau notifications@github.com wrote:

Correction regarding DryIoc.

It does the filtering when registering by walking down the constraints https://bitbucket.org/dadhi/dryioc/src/12245e67f615873a89482eed7cfcf5dd751eaac2/DryIoc/Container.cs?at=dev&fileviewer=file-view-default#Container.cs-9357 .

The try-catch in the link above https://bitbucket.org/dadhi/dryioc/src/e788ffce78b727b35fda5355a2297d40f0e7731b/DryIoc/Container.cs?at=default&fileviewer=file-view-default#Container.cs-7465 is for resolution phase to wrap the unlikely failed match in container exception.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aspnet/DependencyInjection/pull/635#issuecomment-380807881, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMrNxb-gjgduSSHA_wJBuK9C42pH1ks5tn1i2gaJpZM4TNU2C .

[ENikS]

I see no point in doing checks. Try/Catch is 'cheaper' to execute. Occasional fail is better than extra delay on every resolve.

[jbogard]

@ENikS that's what about half the containers do, just allow the first to fail and store the result. I opted for the more complete check, mostly because my first PR was rejected because of first chance exceptions. So I made it do more checks.

[dadhi]

Throwing exceptions is bad, because it is not the exceptional case. I don't want to break on where clause in LINQ for the same reason. Yes, the code is a PITA, but you may peek / steal an existing solution.

[ENikS]

I'll have more intelligent opinion after I dig dipper.

[jbogard]

I agree in general that it's bad, but there's not too much of a choice because the API isn't public. Jon Skeet recommended https://stackoverflow.com/a/4864565/58508 because even the "simple" constraints are easy to get wrong (i.e. the behavior of the System.Reflection API is slightly different on different platforms). You can do your best but still understand that whatever you write isn't as robust as the private API inside the CLR.

[pakrym]

What's the implication of moving it out of the DI.Specification? I just want the feature in this container, I'm less concerned if every container does. Though I wasn't sure if features are added to this container just for ASP.NET Core, or other libraries that also want to use it.

If it's not in specification neither AspNetCore nor other libraries that target M.E.DependencyInjection.Abstractions can use the feature.

We avoided adding such features to M.E.DependencyInjection in the past for couple reasons:

1. To avoid AspNetCore depending on it by accident (we dev and test against M.E.DependencyInjection implementation)
2. It makes switching containers harder - currently, other containers support all features of M.E.DependencyInjection so customers can switch without having to change anything. Adding feature only supported in M.E.DI would break that.

[pakrym]

@jbogard how does the error message look like for a single service being resolved that doesn't satisfy the constraint?

[jbogard]

How do you track how other containers support the features in M.E.DI? Or is this something each container does by itself? I'd like to at least run the specification tests against the containers listed but I'm not sure how you know if a new behavior would break some container.

[davidfowl]

How do you track how other containers support the features in M.E.DI? Or is this something each container does by itself? I'd like to at least run the specification tests against the containers listed but I'm not sure how you know if a new behavior would break some container.

We don't track this, we just have no plans to add any more features to this container for the reasons that @pakrym stated. The bar is extremely high for adding new container features. The work you've done is pretty admirable but any feature that drops any existing container support (that we're aware of) is a no go.

That's just where we are with DI abstractions today.

[ENikS]

Perhaps you could implement the feature but leave it out of the specifications tests?

[davidfowl]

Perhaps you could implement the feature but leave it out of the specifications tests?

Nope. See https://github.com/aspnet/DependencyInjection/pull/635#issuecomment-380853231.

We're not currently setup to make sure we (ASP.NET and EF) doesn't take dependencies on behaviors that may exist outside of the specification. That approach is frozen until we do more work to ensure that.

This problem spreads to any library taking a dependency this library to provide a DI abstraction really.

[ENikS]

I see, makes sense.

[dadhi]

@jbogard,

May be the workaround can be done externally, e.g. on scanning side, before adding into service collection. May be as a part of Scrutor /cc @khellang

[jbogard]

@dadhi It cannot. This is a resolution problem and I'd have to "guess" every possible generic parameter that might be attempted to be resolved. Today I can't get around anything because it just blows up with an ArgumentException.

My original PR was just "stop blowing up".

[ENikS]

@jbogard You could always iterate by yourself, one item at the time. Get the list of generics and try to resolve each one 'manually'. You'll need this 'adapter' for only one or two containers.

[jbogard]

@ENikS I still cannot, the resolution logic still scans all types and will blow up with an exception. Ultimately, the issue is that there is a possibility of an uncaught exception of trying to close a generic type blindly. There is no "hook" for me to prevent that from happening, no matter what I explicitly register.

[ENikS]

Well, I guess the only option left is negotiating conditions how to add it to the spec than.

[jbogard]

There's another option too - nearly all the stuff that I'd need to extend is internal. I thought this was the simpler approach

[jbogard]

And that leaves the larger question - how would features get added to the spec? Do you wait until you have some way of testing the "blessed" containers? Or is there some threshold of support? For comparison, here are some download counts of the ASP.NET Core-supporting DI containers:

• Autofac - 1,159,451
• Castle Windsor - 0 (no official package), 134,192 unofficial package
• StructureMap - 0 (no official package), 250,648 unofficial package
• LightInject - 20,478
• DryIoc - 33,281
• Lamar/BlueMilk (StructureMap vNext) - 460
• SimpleInjector - 176,363
• Unity - 2,013

Of this list Unity is the only non-conforming container for this behavior. Based purely on download count, that's 0.1% non-conformers and 99.9% conformers. And from @ENikS's comment above, Unity with a little try-catch magic, will go out of the 0.1%.

If this container is the least-common-denominator, when do then approve a new feature? I'd like to develop libraries that have features that target the conforming container, but it'll be a bit tough if this one doesn't attempt to keep up with the features in all the other ones.

I'm also happy to discuss what it would look like to be set up some kind of testing matrix to validate conformity, and, therefore provide some vehicle to validate new features.

[jbogard]

@pakrym to your original question, this test passes:

[Fact]
public void CheckingMessage()
{
    var serviceCollection = new ServiceCollection();
    serviceCollection.AddTransient(typeof(IFakeOpenGenericService<>), typeof(ConstrainedFakeOpenGenericService<>));
    serviceCollection.AddSingleton(new PocoClass());
    var serviceProvider = CreateServiceProvider(serviceCollection);

    var service = serviceProvider.GetService<IFakeOpenGenericService<int>>();
    Assert.Null(service);

    var otherService = serviceProvider.GetService<IFakeOpenGenericService<PocoClass>>();

    Assert.NotNull(otherService);

    Assert.Throws<InvalidOperationException>(() => serviceProvider.GetRequiredService<IFakeOpenGenericService<int>>());
}

Because a non-matching service is the same as no service, it behaves the same as "no service".

[pakrym]

@jbogard I didn't doubt that it would pass, I'm worried about the exact message. Because having "no service" message instead of "generics constraint violation" is confusing.

[pakrym]

If Unity supports this feature we are willing to consider it for 2.2.

The biggest concern I have is false negatives of constraint evaluation that would silently break existing usages.

[jbogard]

@pakrym for the false negatives, the safest route would be to just do try-catch around MakeGenericType, since the API to test a generic type can be closed isn't public.

I don't know what to do about the error message, since the API it's in is designed to return null for no match. That'd be a bigger change, to return something that signified a failed match so that the higher up API could then manage it.

[ENikS]

If we could agree on a set of unit test verifying the behavior I can start implementing it.

[jbogard]

I'll open a PR with some failing tests, would that work?

On Mon, Apr 16, 2018 at 7:46 AM, Eugene Sadovoi notifications@github.com wrote:

If we could agree on a set of unit test verifying the behavior I can start implementing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aspnet/DependencyInjection/pull/635#issuecomment-381586767, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmQ_6h4MN8SgxZZZ-5GcO4zHutXoks5tpJKYgaJpZM4TNU2C .

[ENikS]

Yes, that would be very helpful

[jbogard]

Done!

On Mon, Apr 16, 2018 at 8:09 AM Eugene Sadovoi notifications@github.com wrote:

Yes, that would be very helpful

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aspnet/DependencyInjection/pull/635#issuecomment-381593048, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMgehXD6adS6R7juKmboCiha7wPIiks5tpJf7gaJpZM4TNU2C .

[ENikS]

As of v5.8.6 Unity supports constrained generics

[issafram]

@jbogard Wanted to make sure to ask this. If you are adding support for generics, my assumption is that the following example would work out using the Lazy<T>class:

public interface ILogger
{
    void Log(string message);
}

public class ConsoleLogger : ILogger
{
    public void Log(string message)
    {
        Console.WriteLine(message);
    }
}

public class TestClass
{
    private readonly Lazy<ILogger> lazyLogger;
    private ILogger Logger => this.lazyLogger.Value;

    public SomeClass(Lazy<ILogger> logger)
    {
        this.lazyLogger = logger;
    }

    public void SomeMethod()
    {
        var logMessage = "Test Message to log";
        this.Logger.Log(logMessage);
    }
}

And I can just register my type as below: .AddTransient<ILogger, ConsoleLogger>()

I apologize as I haven't read the entire conversation. I was about to start work on a PR for that use case. Let me know if my assumption is correct or not. Thanks.

[ENikS]

Not sure who you asking but in Unity it is supported scenario.

[issafram]

@ENikS I was actually asking @jbogard but that doesn't matter.

I have used Unity in all of my ASP.NET apps. So I am familiar with it.
I use Lazyas much as possible for objects that may or may not be used in a service. No need to resolve if it isn't used in an instance.

I was trying to go with the new built-in container included in ASP.NET Core for any new work.
That is when I recognized that my use case wouldn't work.

But if sticking to Unity is the only viable solution, then I will stick with it using the Unity.Microsoft.DependencyInjection package.

[khellang]

Resolving Lazy<T> out of the box won't work, no. You'd have to register it using something like services.AddTransient<Lazy<ILogger>>(p => new Lazy<ILogger>(() => p.GetService<ILogger>())); 😕

[davidfowl]

@muratg lets put this into the 2.2 milestone for consideration.

[jbogard]

@muratg So we're in the 2.2 window now...thoughts?

[jbogard]

bump

[muratg]

@davidfowl @pakrym please take a look.