Closed commonsensesoftware closed 1 year ago
@commonsensesoftware hi, are you ok if I edit the first post to point to my proposal?
Paging people to review this proposal
DI council: @alexmg @tillig @pakrym @ENikS @ipjohnson @dadhi @seesharper @jeremydmiller @alistairjevans @halter73 @DamianEdwards @steveharter
A couple of questions:
Is the intended behaviour that when resolving a service registered only as a keyed service (for example AddKeyedSingleton(new MyClass(), "key1")
), calling ResolveService
without the key would throw with a missing service? In other words keyed services do not overlap with non-keyed services in any way?
Is it expected that IServiceProviderIsService.IsService
get an overload that accepts a key? i.e.
bool IsService(Type serviceType);
bool IsKeyedService(Type serviceType, object key);
If it isn't going to change, what should IsService
do?
In Autofac world, an IsService
call without a key on a type registered only as a keyed service would return false (as in the first point).
Similar question for ISupportRequiredService
; does GetRequiredKeyedService
go in there, or in the new ISupportKeyedService
?
What will happen to the ImplementationFactory
property in ServiceDescriptor
? Will a new property be added that reflects a delegate Func<IServiceProvider, object, object>
to allow a key to be provided? Or will the runtime wrap the registered delegate in its own that provides the key value itself?
Asking because Autofac accesses this property for delegate activation, wonder if we would need to do anything special or if the runtime would just handle it.
I'm sure something else will occur to me when I look at this again, but those are all my questions for now.
- Is the intended behaviour that when resolving a service registered only as a keyed service (for example
ddKeyedSingleton(new MyClass(), "key1"))
, callingResolveService
without the key would throw with a missing service? In other words keyed services do not overlap with non-keyed services in any way?
That's correct. And GetServices
will also not return all keyed registrations.
- Is it expected that IServiceProviderIsService.IsService get an overload that accepts a key? i.e. [...] In Autofac world, an IsService call without a key on a type registered only as a keyed service would return false (as in the first point).
I have to admit that I forgot the IsService
case. We will need a new IsKeyedService
too, and IsService
should return false if there is no non-keyed registration.
- Similar question for ISupportRequiredService; does GetRequiredKeyedService go in there, or in the new ISupportKeyedService?
I think it's cleaner in another interface like ISupportKeyedService
but that may be overkill (that's what I have in my poc)
- What will happen to the
ImplementationFactory
property inServiceDescriptor
? Will a new property be added that reflects a delegateFunc<IServiceProvider, object, object>
to allow a key to be provided? Or will the runtime wrap the registered delegate in its own that provides the key value itself?Asking because Autofac accesses this property for delegate activation, wonder if we would need to do > anything special or if the runtime would just handle it.
Good question. For now I wrap the delegate with a new one, but that might not be the most effective solution. But adding a new one will break compatibility.
@alistairjevans Keyed and non-keyed should never overlap.
I considered all of these different scenarios because I realized there would be a ton of different friction points and churn. In my original proposal, zero changes are required to these interfaces, delegates, nor the ServiceDescriptor
. It all fits neatly within the confines of what already exists. The things that are added are new service registration methods and some type that can infer a convention from the call site. I should point out that it would still be possible to use a keyed type mapping via explicit configuration in the same manner that a key parameter can be configured without some surrogate type. I have yet to see any significant counter arguments to the original proposal, but I do feel we are moving on to alternate proposals before covering all of the requirements.
@benjaminpetit you have yet to define or describe how call site injection can work with a keyed service without restricting everything to explicit container configuration or attributes. @davidfowl has previously stated that he did not want to use an attribute. I have demonstrated how that is possible without it. An attribute will make source code generation more complex (but not impossible).
... An attribute will make source code generation more complex (but not impossible).
It will be considerably slower as well.
@alistairjevans Keyed and non-keyed should never overlap.
I considered all of these different scenarios because I realized there would be a ton of different friction points and churn. In my original proposal, zero changes are required to these interfaces, delegates, nor the
ServiceDescriptor
. It all fits neatly within the confines of what already exists. The things that are added are new service registration methods and some type that can infer a convention from the call site. I should point out that it would still be possible to use a keyed type mapping via explicit configuration in the same manner that a key parameter can be configured without some surrogate type. I have yet to see any significant counter arguments to the original proposal, but I do feel we are moving on to alternate proposals before covering all of the requirements.
Your proposal definitely works (I used a similar approach to my proof-of-concept wrapper), but only supports Type
as the key. You can use a similar approach to use an object
instead (again , that's what I did in my poc), but the implementation starts to get ugly imo.
While adding a field to ServiceDescriptor
might not be ideal, it's a far easier and cleaner solution that adding a bunch of new types.
@benjaminpetit you have yet to define or describe how call site injection can work with a keyed service without restricting everything to explicit container configuration or attributes. @davidfowl has previously stated that he did not want to use an attribute. I have demonstrated how that is possible without it. An attribute will make source code generation more complex (but not impossible).
Not sure to fully understand your question here.
I don't think service registration should be doable via attributes, but I am not totally ruling out service lookup by attributes.
While adding a field to ServiceDescriptor might not be ideal, it's a far easier and cleaner solution that adding a bunch of new types.
This is a new, major feature, we should be looking to do this in as native a way as possible. Adding a field to the service descriptor seems like the right thing to do.
but I am not totally ruling out service lookup by attributes.
We need to support lookup via attributes. At least the most basic kind of lookup (which will be string based). We'll need to support this in a few subsystems in the stack (like parts of ASP.NET Core) manually.
Is AddControllersAsServices
still a thing? If so, how do attribute-based filters/lookups hook into IServiceProvider
conforming container implementations? Does that IServiceProvider
attribute now leak into every conforming container who now has to support a new attribute? Seems like it'd be a pretty big impact if that's the case.
Is AddControllersAsServices still a thing? If so, how do attribute-based filters/lookups hook into IServiceProvider conforming container implementations? Does that IServiceProvider attribute now leak into every conforming container who now has to support a new attribute?
Yes, other container implementations would need to understand the attribute natively. If we don't do this, the feature is only usable at the top of the dependency graph:
interface IA { }
interface IB { }
interface IC { }
public class A(IB b) : IA { }
public class B([ServiceKey("something")]IC b) : IB { }
public class C() : IC { }
var services = new ServiceCollection();
services.AddSingleton<IA, A>();
services.AddSingleton<IB, B>();
services.AddKeyedSingleton<IC, C>("something");
var sp = services.BuildServiceProvider();
var a = sp.GetRequiredService<IA>();
Without understanding of the attribute, it's unclear how the above would work.
I get it, I just wanted to make sure this is raised and visible. Previous implementations of the conforming container were non-intrusive to the core system. Other than the [I still think incorrect] assumption that IEnumerable<T>
must return everything in the order of registration, which did impact the core DI system, everything else has been external: IsService
, etc. I could keep all of the Microsoft DI abstractions in a separate sort of adapter package.
This addition means that the core backing container now likely has to actually refer directly to Microsoft abstractions because it needs to know about the attribute. It also means that features like attribute filtering, which are actually kind of expensive if it's on by default, now get switched to on by default for everything registered, though admittedly some work may be possible to look at every type for every registration and "automatically opt in" for any of them that have any constructor with that attribute, regardless of whether that constructor is used.
Basically, "that little part" of this feature becomes no longer a feature of the adapter for every container out here, it becomes a hard tie to the MS DI framework, with some perf implications.
And, again, I get it, that attribute thing is helpful. Autofac has that. But it's opt-in, and it's Autofac attributes.
To be clear, a different option might be to inject something like Dictionary<KeyType, Lazy<DependencyType>>
and let the thing pick the one from the list. It's obviously not as pretty, but it also doesn't tie any core container to MS.
public class B(Dictionary<string, Lazy<IC>> deps)
{
public void DoWork()
{
var thing = deps["the-key"].Value;
thing.DoWork();
}
}
Pick the key out as you need it, and don't resolve it until you need it.
And, again, I get it's not as pretty, but it reverses the coupling.
I'm just... I was with the keyed thing up until "also must have attribute and it must be the Microsoft attribute and now every DI container out there is going to be affected by the tail wagging the dog." I have a challenge with that.
Your proposal definitely works (I used a similar approach to my proof-of-concept wrapper), but only supports
Type
as the key. You can use a similar approach to use anobject
instead (again , that's what I did in my poc), but the implementation starts to get ugly imo.
Type
as a key only represents the surface area of the API. There is no requirement that Type
itself has to be used as the key and I've already demonstrated how that might be achieved. If we're universally settling on Object
as the key that can be used by anyone, then the original proposal can very easily be extended to formally expose any key you choose of type Object
from Type
where Type
itself is never used as the key in an implementation. First, we need to define a way to get the key for any give type T
.
public interface IServiceKey
{
static abstract object Key { get; }
}
It is now possible to assign keys of any type to the keyed Type
. Let's consider the two most common scenarios of Enum
and String
.
Given the following example enumeration:
public enum Services
{
Foo,
Bar,
}
We can now create keys a la:
public static class Keys
{
public sealed class Foo : IServiceKey
{
public static object Key => "Foo";
}
public sealed class Bar : IServiceKey
{
public static object Key => Services.Bar;
}
}
While this might be slightly different from what developers do today, it should be pretty close. The majority of implementations I've see use enumerations or a class containing a bunch of string constants. Arguably using an enumeration as a key can be supplanted by using the Type
directly as the key, but that is a developer decision and is required for backward compatibility.
We can now easily get the key a la:
static object? GetKey<T>() where T : IServiceKey => T.Key;
static object? GetKey(Type type)
{
var methods = type.GetInterfaceMap(typeof(IServiceKey)).TargetMethods;
return methods.Length == 0 ? default : methods[0].Invoke(null, null);
}
The surface area, therefore, continues to use Type
without any attributes, but the container implementation can use IServiceKey.Key
associated to the type like so:
// registers IFoo with IServiceKey.Key = "Foo"
services.AddTransient<Keys.Foo, IFoo, FooImpl>();
// registers IBar with IServiceKey.Key = "Bar"
services.AddTransient<Keys.Bar, IBar, BarImpl>();
var provider = services.BuildServiceProvider();
// resolves IFoo with the string key "Foo"
var foo = provider.GetRequiredService<Keys.Foo, IFoo>();
// resolves IBar with the enumeration key Services.Bar
var bar = provider.GetRequiredService<Keys.Bar, IBar>();
Again, this variation does not require any changes to the interfaces, delegates, or even ServiceDescriptor
. This approach also would not require an attribute.
As for how ugly the implementation gets, that is of little consequence IMHO. As long as the API and usage to developers is clean and rational, how difficult or ugly the implementation is less important and is why we're paid the big bucks. 😄
I'm just... I was with the keyed thing up until "also must have attribute and it must be the Microsoft attribute and now every DI container out there is going to be affected by the tail wagging the dog." I have a challenge with that.
I hear ya, but maybe its worth discussing how systems would use this feature, and that would help drive some of this discussion around attribute or not (btw the coupling could be decoupled further with a contract, but I'll ignore that for now). Consider that today there are several subsystems that have worked around not having this ability (named options and all of the other subsystems that rely on it). I'd love to see what those would look like given this feature.
... Other than the [I still think incorrect] assumption that
IEnumerable<T>
must return everything in the order of registration, which did impact the core DI system, everything else has been external:IsService
, etc.
@tillig Is there an issue open discussing this problem? Unity has the same problem and unless this "assumption" is changed will never be compatible with the DI. (Sorry for the OT)
Is there an issue open discussing [the ordering] problem?
The discussion about the ordering started in https://github.com/aspnet/DependencyInjection/pull/416 and continued in https://github.com/aspnet/DependencyInjection/issues/433. A lot of code depends on this behavior now, so I don't see this requirement realistically changing.
As for this issue, I think we need change to the ServiceDescriptor type and add a new attribute to make this usable. Do we still want this for .NET 8 though? Would we cut the feature if there aren't enough third-party containers supporting this? How do we prevent people using outdated third-party containers if they're relying on keyed services? This could even be an issue if people hoist Microsoft.Extensions.DependencyInjection.Abstractions from a nuget package but not Microsoft.Extensions.DependencyInjection.
@ENikS I'm not sure what your question is. My statement there is in reference to another issue from long ago where the MS DI container imply assumed that all registrations would be returned in the order registered. At the time, that wasn't the case with Autofac and possibly with others. IEnumerable<T>
would return all the registrations, just not in registration order. What it meant was in order to be "compatible" with an interface, we had to change container internals. (And there's still weirdness about registration order when it comes to assembly scanning, etc.)
It's not a secret, I'm not afraid of discussing it, it's just the gritty details aren't relevant. (I'm also not looking to walk that particular decision back. That wasn't the intent of the comment.)
The similarity is that we still have this interface to implement, but now there's the potential we need to "know" about an external attribute. I recognize the value of the desired feature, it just seems that particular aspect goes - again - beyond interface implementation and again into container internals.
I don't want to play into a "slippery slope" argument, but it seems like it's not impossible we'll end up at a point where every container effectively has to be the same, like everyone is a forked version of the MS container, because instead of implementing an adapter to conform it'll be a series of implicit behaviors.
And maybe that's where the road leads. It'd be nice to know what the thoughts are on maintaining the concept of a conforming container interface or whether we're going to slowly just all end up with custom implementations of literally the same features. It could save me a lot of time and effort maintaining Autofac if we're all just heading toward Spring Boot here.
But, again, I recognize the value of the feature. I don't need to be sold that it's interesting, or that it can save having to work around the lack of it, or whatever. Autofac has had it for years. This is just the... second?... step on the road past confirming container. It'll be what it's going to be. 🤷♂️
As a variant of implementation, would you consider reusing System.ComponentModel.Composition
namespace's Import
attribute?
It is a part of NET and would be better suited for integration compared to a new attribute. It would also be a lot friendlier to static DI if export/import attributes were used for registrations. I understand it is a bit far-fetched at the moment, but so were keyed imports just a while back.
We shouldn't reuse any attributes. Lets keep this focused on the minimal feature set related to MS.DI and the abstractions.
The similarity is that we still have this interface to implement, but now there's the potential we need to "know" about an external attribute. I recognize the value of the desired feature, it just seems that particular aspect goes - again - beyond interface implementation and again into container internals.
I believe that I understand your position, as an author of a famous DI container library, you feel many of the discussions about changing .NET DI may somehow infringe, or encroach on your territory. Specifically, when you raise the concern of interface implementations yielding a specific structure of a DI container. Yet however, I find that argument invalid! The current interface contract is currently under revisement - we want to add keyed services - what follows must be accounted for in the aftermath, not now. That includes the unknown attribute, if any will exist.
I don't want to play into a "slippery slope" argument, but it seems like it's not impossible we'll end up at a point where every container effectively has to be the same, like everyone is a forked version of the MS container, because instead of implementing an adapter to conform it'll be a series of implicit behaviors.
I'm not sure that I can't agree with this, I am just not so knowledgeable on the topic - who knows what the future brings?
And maybe that's where the road leads. It'd be nice to know what the thoughts are on maintaining the concept of a conforming container interface or whether we're going to slowly just all end up with custom implementations of literally the same features. It could save me a lot of time and effort maintaining Autofac if we're all just heading toward Spring Boot here.
Maybe it does, I frankly don't know, but I don't see dependency injection as the final frontier of cross-cutting application concerns - just the start of a well organized codebase.
Maybe it does, I frankly don't know, but I don't see dependency injection as the final frontier of cross-cutting application concerns - just the start of a well organized codebase.
May I remind you that this is Microsoft's fifth attempt at Dependency Injection?
Perhaps, instead of 'revolutionizing' DI and starting everything from scratch, we could stick to something that works for everyone? As I understand it, there is a consensus from community that attribute will not work, or did I get it incorrectly? Should you put it to vote?
We’re moving ahead with an implementation with the attribute so we can get a better understanding of the impact. While there’s feedback both ways it shouldn’t stop forward progress on this issue. This isn’t a new issue and we’re not revolutionizing DI, just trying to understand what adding a highly requested feature that we think is more important now than it was before.
May I remind you that this is Microsoft's fifth attempt at Dependency Injection?
- Unity
- MEF (System.ComponentModel.Composition)
- MEF2 (System.Composition)
- MEF Visual Studio (vs-mef)
- and now this one.
Perhaps, instead of 'revolutionizing' DI and starting everything from scratch, we could stick to something that works for everyone?
Thank you for clarifying the context, I for one, wasn't aware of the previous attempts.
Hey all, we have an update and a PR for this change https://github.com/dotnet/runtime/pull/87183. Feedback welcome (the issue should have the updated API proposal).
@benjaminpetit are we planning to take this to the design review soon as it looks almost everything ready to go?
ISupportKeyedService
should be IKeyedServiceProvider
and derive from IServiceProvider
IServiceProviderIsServiceKeyed
should be IKeyedServiceProviderIsService
IKeyedServiceProviderIsService.IsService
should be called IsKeyedService
namespace Microsoft.Extensions.DependencyInjection;
public partial class ServiceDescriptor
{
public object? ServiceKey { get; }
public object? KeyedImplementationInstance { get; }
public Type? KeyedImplementationType { get; }
public Func<IServiceProvider, object, object>? KeyedImplementationFactory { get; }
public bool IsKeyedService { get; }
}
public static partial class ServiceCollectionServiceExtensions
{
public static IServiceCollection AddKeyedScoped(this IServiceCollection services, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type serviceType, object serviceKey);
public static IServiceCollection AddKeyedScoped(this IServiceCollection services, Type serviceType, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static IServiceCollection AddKeyedScoped(this IServiceCollection services, Type serviceType, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static IServiceCollection AddKeyedScoped<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection services, object serviceKey) where TService : class;
public static IServiceCollection AddKeyedScoped<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static IServiceCollection AddKeyedScoped<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection services, object serviceKey) where TService : class where TImplementation : class, TService;
public static IServiceCollection AddKeyedScoped<TService, TImplementation>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TImplementation> implementationFactory) where TService : class where TImplementation : class, TService;
public static IServiceCollection AddKeyedSingleton(this IServiceCollection services, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type serviceType, object serviceKey);
public static IServiceCollection AddKeyedSingleton(this IServiceCollection services, Type serviceType, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static IServiceCollection AddKeyedSingleton(this IServiceCollection services, Type serviceType, object serviceKey, object implementationInstance);
public static IServiceCollection AddKeyedSingleton(this IServiceCollection services, Type serviceType, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static IServiceCollection AddKeyedSingleton<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection services, object serviceKey) where TService : class;
public static IServiceCollection AddKeyedSingleton<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static IServiceCollection AddKeyedSingleton<TService>(this IServiceCollection services, object serviceKey, TService implementationInstance) where TService : class;
public static IServiceCollection AddKeyedSingleton<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection services, object serviceKey) where TService : class where TImplementation : class, TService;
public static IServiceCollection AddKeyedSingleton<TService, TImplementation>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TImplementation> implementationFactory) where TService : class where TImplementation : class, TService;
public static IServiceCollection AddKeyedTransient(this IServiceCollection services, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type serviceType, object serviceKey);
public static IServiceCollection AddKeyedTransient(this IServiceCollection services, Type serviceType, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static IServiceCollection AddKeyedTransient(this IServiceCollection services, Type serviceType, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static IServiceCollection AddKeyedTransient<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection services, object serviceKey) where TService : class;
public static IServiceCollection AddKeyedTransient<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static IServiceCollection AddKeyedTransient<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection services, object serviceKey) where TService : class where TImplementation : class, TService;
public static IServiceCollection AddKeyedTransient<TService, TImplementation>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TImplementation> implementationFactory) where TService : class where TImplementation : class, TService;
public static void TryAddKeyedScoped(this IServiceCollection collection, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type service, object serviceKey);
public static void TryAddKeyedScoped(this IServiceCollection collection, Type service, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static void TryAddKeyedScoped(this IServiceCollection collection, Type service, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static void TryAddKeyedScoped<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection collection, object serviceKey) where TService : class;
public static void TryAddKeyedScoped<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static void TryAddKeyedScoped<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection collection, object serviceKey) where TService : class where TImplementation : class, TService;
public static void TryAddKeyedSingleton(this IServiceCollection collection, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type service, object serviceKey);
public static void TryAddKeyedSingleton(this IServiceCollection collection, Type service, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static void TryAddKeyedSingleton(this IServiceCollection collection, Type service, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static void TryAddKeyedSingleton<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection collection, object serviceKey) where TService : class;
public static void TryAddKeyedSingleton<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static void TryAddKeyedSingleton<TService>(this IServiceCollection collection, object serviceKey, TService instance) where TService : class;
public static void TryAddKeyedSingleton<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection collection, object serviceKey) where TService : class where TImplementation : class, TService;
public static void TryAddKeyedTransient(this IServiceCollection collection, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type service, object serviceKey);
public static void TryAddKeyedTransient(this IServiceCollection collection, Type service, object serviceKey, Func<IServiceProvider, object, object> implementationFactory);
public static void TryAddKeyedTransient(this IServiceCollection collection, Type service, object serviceKey, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType);
public static void TryAddKeyedTransient<[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TService>(this IServiceCollection collection, object serviceKey) where TService : class;
public static void TryAddKeyedTransient<TService>(this IServiceCollection services, object serviceKey, Func<IServiceProvider, object, TService> implementationFactory) where TService : class;
public static void TryAddKeyedTransient<TService, [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(this IServiceCollection collection, object serviceKey) where TService : class where TImplementation : class, TService;
public static IServiceCollection RemoveAllKeyed(this IServiceCollection collection, Type serviceType, object serviceKey);
public static IServiceCollection RemoveAllKeyed<T>(this IServiceCollection collection, object serviceKey);
}
public interface IKeyedServiceProvider : IServiceProvider
{
object? GetKeyedService(Type serviceType, object serviceKey);
object GetRequiredKeyedService(Type serviceType, object serviceKey);
}
public interface IKeyedServiceProviderIsService
{
bool IsKeyedService(Type serviceType, object serviceKey);
}
[AttributeUsageAttribute(AttributeTargets.Parameter)]
public class ServiceKeyAttribute : Attribute
{
public ServiceKeyAttribute();
}
[AttributeUsageAttribute(AttributeTargets.Parameter)]
public class FromKeyedServicesAttribute : Attribute
{
public FromKeyedServicesAttribute(object key);
public object Key { get; }
}
@steveharter @benjaminpetit with https://github.com/dotnet/runtime/pull/87183 merged, can this be closed?
Yup, closing.
Hey everyone :) I saw that this issue just got closed. However, I still want to raise some questions/concerns.
I like the idea of named services, but I dislike 2 things about this proposal:
When I read the docs about the Options Pattern, you can choose to use services.Configure<TType>(config)
OR services.Configure<TType>(string, config)
.
This proposal adds different methods (AddKeyed*
) to do the same thing with service registrations, which I find inconsistent. I think this can cause a lot of confusion. I'd much prefer to get new overloads on existing AddSingleton/AddScoped/AddTransient
methods so the approach is consistent with the Options Pattern.
I see there is a new interface to retrieve keyed services from. I don't like this approach for the following reasons:
Right now, IServiceProvider
is the 1 interface you can use as a Service Locator to retrieve services from. But instead of just using IServiceProvider
to get access to all the services you might need, you'll now also need to inject the IKeyedServiceProvider
. So you need 2 interfaces. This doesn't feel very SOLID; what if in the future we get another way to register services, do we then also get a 3rd interface to retrieve items from?
In the proposal it's established that ServiceDescriptor
gets extended with some new properties. So why can't IServiceProvider
be extended with some new overloads on GetRequiredService()
with a key
parameter?
I think the new interface will make things much more complex than necessary. In .NET Framework, we used libraries like Autofac, Ninject, etc.. With .NET Core we got Microsoft's own DI framework and I found this very easy to work with. This new interface increases the complexity unnecessarily, in my opinion!
Curious for your thoughts :)
@sander1095 Extending IServiceProvider
with a new overload will create breaking changes.
You could watch the API Proposal video to understand the specific background.
Hi @deeprobin, I had a quick look at the video but couldn't find the source, but I'll believe you ;).
I'm not sure if these next thoughts are addressed in the video, so I'll go ahead and ask:
Instead of adding a new overload, could we not insert these new Keyed
methods on IServiceProvider
like GetKeyedService()
andGetRequiredKeyedService()
? I think this makes the API a lot more friendly to use.
If it is part of another interface, it will be much more difficult to discover the existence of keyed services. What are your thoughts on this? I feel like that introducing a whole new interface is a pretty big deal considering everyone "knows" you only need IServiceProvider
for all things DI, and now that won't be true anymore.
One last suggestion would be to rename the AddKeyedTransient()
(and others) to AddTransientKeyed()
. This will benefit intellisense in IDE's and also make it easier to discover this new API.
@sander1095 we didn't want to put the new method on IServiceProvider
to not break backward compatibility. Note that all extensions methods are made with IServiceProvider
, the user doesn't have to use IKeyedServiceProvider
directly.
@commonsensesoftware What about using the Generic Attributes ? I mean, the feature looks pretty coupled to the use of strings as keys.
As an example:
public static IServiceCollection AddKeyedScoped<TService, TKey>(this IServiceCollection services, TKey serviceKey, Func<IServiceProvider, TKey, TService> implementationFactory) where TService : class;
public class FromKeyedServicesAttribute<TKey> : Attribute
{
public FromKeyedServicesAttribute(TKey key);
public TKey Key { get; }
}
@deinok
I think that's a good idea in principle. It would probably make sense to create a new API proposal for it, since this one is already closed.
The service key is an object, it can be anything. The generic overload is syntax sugar for keys of a certain type. As for the generic attribute, what would you imagine would go in there besides types and constant strings? C# doesn't support any values declared as attribute values.
@deinok While you can use an attribute, generic or otherwise, it requires inspecting the attribute and allows breaking the contract. For example:
public class Bar
{
private readonly IFoo foo;
public Bar([FromKeyedServices(typeof(IPityTheFoo))] IFoo foo) => this.foo = foo;
}
Allows passing any IFoo
. There is also no guarantee that IPityTheFoo
exists. It is possible for a DI framework to impose validation if it knows to look for it, but it is possible to construct Bar
in a way not intended by the author; certainly not the way the self-describing way it was written.
The counter-argument that I proposed requires using the Type
in code and allowing any implementor to choose to use the Type
, string
, enumeration, or any other choice for the actual key. For example:
public class Bar
{
private readonly IFoo foo;
public Bar(Keyed<IPityTheFoo, IFoo> foo) => this.foo = foo.value;
}
The main difference is formalizing the type such that you must pass a keyed type and it's explicitly expressed. No attribute required. It also meant no change to IServiceProvider
or ServiceDescriptor
. You'd ultimately have serviceProvider.GetService(typeof(Keyed<IPityTheFoo, IFoo>))
, which can be further simplified and dressed up with extension methods.
Despite showing working implementations, my proposal was rejected. I guess people didn't like the way it looked 🤔. Using an attribute is just as verbose, if not more so, and lacks the benefits of explicitly specifying the parameter with a key.
Regardless, we're getting keyed service support - finally. It may not have been what I would have done, but we now have something to work with. 😄
@commonsensesoftware I prefer your version insted of using object
as a generic and attributes. I feel this API a little bit weird
Maybe this is not the best place to ask, but we're trying to onboard to this new API in https://github.com/dotnet/extensions/pull/4224 and didn't find a good way of having a chain of keyed dependencies to be easily registered/resolved.
Consider this hierarchy: MyRootType
-> IDep1
-> IDep2
-> ...
, so that each one resolves the next dependency using the same service key. We can't use [FromKeyedServices(...)]
however, because serviceKey
is unknown at compile-time.
We don't want to use service locator antipattern, i.e.:
public MyRootType(
IServiceProvider serviceProvider,
[ServiceKey] string? serviceKey = null)
{
var myDep1 = serviceProvider.GetRequiredKeyedService<IDep1>(serviceKey);
// ...
}
Are we missing something here and there's a better way of doing that?
cc @klauco @geeknoid
Just a suggestion, and it might not be best suited,
but, @xakep139 couldn't you subclass the FromKeyedServices attribute to somehow reflect on the service key of the root?
@xakep139 Unity used to have something where you could register a type TService
to be resolved and could instruct its resolvers that for one or more TDependency
at any level in the hierarchy of dependencies down, it should substitute a preconfigured instance or type.
But something like that is incredibly brittle. Modified Peter Parker Principle applies: "with great power; come great f--k ups."
Unfortunately, we can't rely on Unity or any other 3P IoC container. Hopefully this use-case have been discussed during design reviews and there's a clear guidance.
We don't want to use service locator antipattern
Given your scenario, I think the service locator pattern your only real option. I don't think it's that bad. I do try to avoid it when possible for easier reviewing, testing and so forth, but sometimes there's no choice.
It might be interesting if someone made an API proposal for a [FromCurrentKeyedServices]
or something like that for a future version of .NET. Of course there's a challenge on how you would deal with containers that support the base set of keyed service features in .NET 8, but not the new [FromCurrentKeyedServices]
or similar attribute.
@xakep139 Unity used to have something where you could register a type
TService
to be resolved and could instruct its resolvers that for one or moreTDependency
at any level in the hierarchy of dependencies down, it should substitute a preconfigured instance or type.But something like that is incredibly brittle. Modified Peter Parker Principle applies: "with great power; come great f--k ups."
@rjgotten why used to have?
This feature is still available and works for any TService dependency or only dependency on the specified type.
As for being incredibly brittle, I totally disagree, it works every single time, as designed.
@ENikS Because with all due respect to your tenure and stewardship on Unity - it's become outmoded and a has-been.
As for being incredibly brittle, I totally disagree, it works every single time, as designed.
If you are resolving a nested dependency through such a custom resolver which happens to be singleton, it can make the singleton take the override type rather than the normal type. It makes the result of type resolution non-deterministic and dependent on actual resolve order. First build-up chain wins.
I experienced that problem myself a few times trying to use it for registering decorator patterns.
Then I stopped using it and wrote a dedicated container-agnostic implementation for it at the IServiceCollection level with factory resolvers and ActivatorUtilities.CreateFactory
. It's slower, but it's stable.
This is why I brought up Unity's thing in the first place. As a warning that this type of pattern where you attempt to push specialized context down the resolve chain, is generally a bad idea. Unless you make it 100% bulletproof and guarantee that everything resolved along that chain is isolated from the norm. Which may have yet again its own unforeseen consequences such as singletons suddenly becoming multiton.
I maybe too late, but can we have generic version of IKeyedServiceProvider
?
For anyone still following this thread and was interested in the original proposal, I've taken the liberty to fork the original POC repo and created an official Keyed Services repo. I've published a NuGet package for each of the 9 well-known containers that require (albeit minor) adapter changes. Since the proposal wasn't accepted, you'll still need one package for keyed services and possibly a second if your container also required adapter changes.
There's no need to wait for .NET 8. This will work today over the existing APIs and containers without IKeyedServiceProvider
, FromKeyedServicesAttribute
, or changes to ServiceDescriptor
. I've published TFMs for net6.0
and net7.0
in case you're LTS bound. There's the added bonus that all DI registrations that go through the IServiceCollection
are transparently portable to all other supported container implementations. This won't be true of IKeyedServiceProvider
unfortunately. As an example, if your current container supports any object
as a key, it may not work in a container that only accepts string
key if you didn't happen to use a string
. Perhaps there is a known or preconceived notion that people don't/won't switch container frameworks. I might have said the same thing about mocking frameworks - until this week 😬.
The updated README.md
has links to all of the packages as well as links to example projects for each container. As you may notice, the setup for each container is virtually identical regardless of the backing type used by the container. I've released the first round under preview.1
in case there may be some additional thoughts or feedback. I'm curious what people think about it in practice - love it or hate it. Regardless, you now have something you can pull into your own experiments to play with, but without the need to clone, fork, or build. Enjoy!
Thanks @commonsensesoftware for the original proposal. I edited this post to show the current proposition.
Original proposal from @commonsensesoftware
### Background and Motivation ~~I'm fairly certain this has been asked or proposed before. I did my due diligence, but I couldn't find an existing, similar issue. It may be lost to time from merging issues across repos over the years.~~ >A similar question was asked in [Issue 2937](https://github.com/dotnet/extensions/issues/2937) The main reason this has not been supported is that `IServiceProvider.GetService(Type type)` does not afford a way to retrieve a service by key. `IServiceProvider` has been the staple interface for service location since .NET 1.0 and changing or ignoring its well-established place in history is a nonstarter. However... what if we could have our cake _and_ eat it to? 🤔 A keyed service is a concept that comes up often in the IoC world. All, if not almost all, DI frameworks support registering and retrieving one or more services by a combination of type and key. There _are_ ways to make keyed services work in the existing design, but they are clunky to use (ex: via `FuncAPI Proposal
The API is optional
The API is optional, and will not break binary compatibility. If the service provider doesn't support the new methods, the user will get an exception at runtime.
The key type
The service key can be any
object
. It is important thatEquals
andGetHashCode
have a proper implementation.Service registration
ServiceDescriptor
will be modified to include theServiceKey
.KeyedImplementationInstance
,KeyedImplementationType
andKeyedImplementationFactory
will be added, matching their non-keyed equivalent.When accessing a non-keyed property (like
ImplementationInstance
) on a keyedServiceDescriptor
will throw an exception: this way, if the developer added a keyed service and is using a non-compatible container, an error will be thrown during container build.ServiceKey
will staynull
in non-keyed services.Extension methods for
IServiceCollection
are added to support keyed services:I think it's important that all new methods supporting Keyed service have a different name from the non-keyed equivalent, to avoid ambiguity.
"Any key" registration
It is possible to register a "catch all" key with
KeyedService.AnyKey
:Resolving service
Basic keyed resolution
Two new optional interfaces will be introduced:
This new interface will be accessible via the following extension methods:
These methods will throw an
InvalidOperationException
if the provider doesn't supportISupportKeyedService
.Resolving services via attributes
We introduce two attributes:
ServiceKeyAttribute
andFromKeyedServicesAttribute
.ServiceKeyAttribute
ServiceKeyAttribute
is used to inject the key that was used for registration/resolution in the constructor:This attribute can be very useful when registering a service with
KeyedService.AnyKey
.FromKeyedServicesAttribute
This attribute is used in a service constructor to mark parameters speficying which keyed service should be used:
Open generics
Open generics are supported:
Enumeration
This kind of enumeration is possible:
Note that enumeration will not mix keyed and non keyed registrations:
But we do not support: