autofac
Support for init-only properties
Problem Statement
AutoFac uses Constructor injection.
I just added a new parameter to a base class's constructor and had to refactor ~50 derived classes. That really sucks.
Desired Solution
C# now has init properties. It would be nice if AutoFac would also populate init properties after construction. This would give the benefits of property injection without the associated problems.
Additional Context
C# is also considering adding required properties. Perhaps this should only apply to required properties?
This is an interesting idea. (For reference: dotnet/csharplang#3630 outlines the proposed C# syntax.)
Examples:
public class Item
{
public string Property { get; init; }
}
// Allowed:
var i = new Item { Property = "value" };
// Not allowed; must be set at construction:
var i = new Item();
i.Property = "value";
I think the required thing is the key. If it's only init, it's still optional. If there was a notion of required I think that'd be where we could do something. I think without that, init is just a property.
However... I'm curious how this works with reflection. I haven't dived in. When we call a constructor, it's basically ConstructorInfo.Invoke(). I don't know how that changes here, like if the required init properties would end up being some sort of syntactic sugar over a generated constructor or what. I'm not sure even if we want to populate init properties what that means.
The day job here is taking the majority of my time at the moment. If someone else has some time to dig in and figure out what all this means (in the form of more detailed explanations, say, in actual reflection/execution code or a PR) that'd be sweet.
I actually spent some time looking at ways init-only properties could be used by Autofac, back when .NET5 was released, but never got round to formally raising anything.
Fundamentally init-only properties are just regular setters, with an IsExternalInit return parameter modifier set to hint the compiler that it cannot be set after construction. These properties can still easily be set via reflection.
I actually decided to blend them with Nullable Reference Type detection to create an init-only property injector that when resolving would:
- Look at all the properties on the type.
- Try and resolve Init-Only properties that were null after activation (i.e. not set by the constructor)
- Validate that all non-nullable (i.e. mandatory) properties have been populated.
In this way, I sort of implement 'required' init-only properties by using nullable reference types to indicate nullability.
So, given the following services + components:
public interface IService1 { }
public interface IService2 { }
public class Component1 : IService1 { }
public class Component2 : IService2 { }
public class ExampleComponent
{
public IService1 Service1 { get; init; }
public IService2 Service2 { get; init; }
public IService1 NotInitProperty { get; set; }
}
I can write a test that only injects init-only properties:
[Fact]
public void InitOnlyPropertyInjection()
{
var builder = new ContainerBuilder();
// Attach our magic to make it work.
builder.RegisterModule(new GlobalMiddlewareModule<InjectInitOnlyPropertiesMiddleware>());
builder.RegisterType<Component1>().As<IService1>();
builder.RegisterType<Component2>().As<IService2>();
builder.RegisterType<ExampleComponent>();
var container = builder.Build();
var component = container.Resolve<ExampleComponent>();
// Injects the two init-only properties.
Assert.NotNull(component.Service1);
Assert.NotNull(component.Service2);
// Non-init-only property not populated.
Assert.Null(component.NotInitProperty);
}
On top of that, leaning on Nullable Reference Types, if I fail to register one of the mandatory init-only properties, I get an exception when resolving:
[Fact]
public void NullableInjectionEnforcement()
{
var builder = new ContainerBuilder();
// Attach our middleware to every registration.
builder.RegisterModule(new GlobalMiddlewareModule<InjectInitOnlyPropertiesMiddleware>());
builder.RegisterType<Component1>().As<IService1>();
// Fail to register IService2.
// builder.RegisterType<Component2>().As<IService2>();
builder.RegisterType<ExampleComponent>();
var container = builder.Build();
// Message is: Property injection could not resolve mandatory init property Service2.
Assert.Throws<DependencyResolutionException>(() => container.Resolve<ExampleComponent>());
}
But if I specify one of the properties as nullable, this isn't a problem:
public class ExampleComponentAllowsNullable
{
public IService1 Service1 { get; init; }
public IService2? Service2 { get; init; }
}
[Fact]
public void NullableInitOnlyPropertiesAllowed()
{
var builder = new ContainerBuilder();
// Attach our middleware to every registration.
builder.RegisterModule(new GlobalMiddlewareModule<InjectInitOnlyPropertiesMiddleware>());
builder.RegisterType<Component1>().As<IService1>();
// Fail to register this.
// builder.RegisterType<Component2>().As<IService2>();
builder.RegisterType<ExampleComponentAllowsNullable>();
var container = builder.Build();
// Not error, because Service2 is nullable.
Assert.NotNull(container.Resolve<ExampleComponentAllowsNullable>());
}
This code uses a custom piece of middleware, combined with a custom property selector, to bring this all together, combined with the now "standard" stackoverflow code (🙄) for doing the unpleasant logic to detect whether a property is nullable or not.
class InjectInitOnlyPropertiesMiddleware : IResolveMiddleware
{
private readonly ConcurrentDictionary<Type, TrackingInitOnlyPropertySelector> _propSelectorCache = new();
public PipelinePhase Phase => PipelinePhase.Activation;
public void Execute(ResolveRequestContext context, Action<ResolveRequestContext> next)
{
// Let the pipeline continue.
next(context);
if (context.Instance is null)
{
return;
}
var instanceType = context.Instance.GetType();
var selector = _propSelectorCache.GetOrAdd(instanceType, t => new TrackingInitOnlyPropertySelector());
// Inject any unset properties.
context.InjectProperties(context.Instance, selector, context.Parameters);
// Now validate that there are no unset non-nullable properties left on the instance.
foreach (var mandatoryProperty in selector.GetMandatoryProperties())
{
if (mandatoryProperty.GetValue(context.Instance) is null)
{
throw new DependencyResolutionException(
$"Property injection could not resolve mandatory init property {mandatoryProperty.Name}.");
}
}
}
private class TrackingInitOnlyPropertySelector : DefaultPropertySelector
{
private Dictionary<PropertyInfo, bool> _isMandatoryProperty = new();
public TrackingInitOnlyPropertySelector()
: base(preserveSetValues: true)
{
}
public IEnumerable<PropertyInfo> GetMandatoryProperties()
{
foreach (var item in _isMandatoryProperty)
{
if (item.Value)
{
// Property is mandatory.
yield return item.Key;
}
}
}
public override bool InjectProperty(PropertyInfo propertyInfo, object instance)
{
var shouldInject = base.InjectProperty(propertyInfo, instance);
if (shouldInject && IsInitOnly(propertyInfo))
{
_isMandatoryProperty[propertyInfo] = !IsNullable(propertyInfo);
return true;
}
return false;
}
private static bool IsInitOnly(PropertyInfo property)
{
if (!property.CanWrite)
{
return false;
}
var setMethod = property.SetMethod;
var setMethodReturnParameterModifiers = setMethod!.ReturnParameter.GetRequiredCustomModifiers();
// Init-only properties are marked with the IsExternalInit attribute on the set method.
return setMethodReturnParameterModifiers.Contains(typeof(System.Runtime.CompilerServices.IsExternalInit));
}
// https://stackoverflow.com/a/58454489/114400
private static bool IsNullable(PropertyInfo property)
{
if (property.PropertyType.IsValueType)
{
throw new ArgumentException("Property must be a reference type", nameof(property));
}
var nullable = property.CustomAttributes
.FirstOrDefault(x => x.AttributeType.FullName == "System.Runtime.CompilerServices.NullableAttribute");
if (nullable != null && nullable.ConstructorArguments.Count == 1)
{
var attributeArgument = nullable.ConstructorArguments[0];
if (attributeArgument.ArgumentType == typeof(byte[]))
{
var args = (ReadOnlyCollection<CustomAttributeTypedArgument>)attributeArgument.Value!;
if (args.Count > 0 && args[0].ArgumentType == typeof(byte))
{
return (byte)args[0].Value! == 2;
}
}
else if (attributeArgument.ArgumentType == typeof(byte))
{
return (byte)attributeArgument.Value! == 2;
}
}
var context = property.DeclaringType!.CustomAttributes
.FirstOrDefault(x => x.AttributeType.FullName == "System.Runtime.CompilerServices.NullableContextAttribute");
if (context != null &&
context.ConstructorArguments.Count == 1 &&
context.ConstructorArguments[0].ArgumentType == typeof(byte))
{
return (byte)context.ConstructorArguments[0].Value! == 2;
}
// Couldn't find a suitable attribute
return false;
}
}
}
One reason I think I didn't raise it is because this doesn't really prevent Nullable Reference Type warnings on the class in question:
But, this is a problem in so many other places already, that I currently resolve with:
public class ExampleComponent
{
public IService1 Service1 { get; init; } = default!;
public IService2 Service2 { get; init; } = default!;
}
This workaround is actually in the .NET docs, so is generally considered acceptable; Entity Framework has to do this loads.
If people are interested on perhaps adding this middleware to Autofac, perhaps via a global InjectInitOnlyProperties method or something, I'm happy to pursue it.
I would absolutely love it if this could be combined with constructor injecting such that the object is created with constructor injection and then init-only properties are filled in with your code.
Any chance that would make it into an official release?
Possibly, but I should point out that the code above doesn't depend on any Autofac internals I believe, so you should just be able to bring that middleware code into your app to get this behaviour yourself.
Check our docs on custom middleware for some context, and how to attach it to every registration.
I feel like the nullable thing, while it works, feels... not great. The multi-purposing of the nullable annotation makes me feel like someone could pretty easily get into a situation where they're getting dependency resolution exceptions in places they weren't before.
I think if we do something here, I'd vote for waiting a bit to see if some required keyword makes it in.
In the meantime, it seems like, if I'm reading this right, regular property injection might "just work" on unset init properties...?
Yeah, I think the reason I didn't pursue it further was that it was indeed a bit...janky.
But yes, init-only properties "just work" in Autofac if you set PropertiesAutowired.
I added a unit test for this last year actually just to prove it out.
Does PropertiesAutowired wire up any other properties beside init-only ones? Does it look at public, protected, or private?
Right now it doesn't inherently care about whether they're init or not. (That's kind of what this issue is about, right? Adding some special handling?) IIRC today it's public, settable properties that can be supplied by the lifetime scope. You can configure that by providing your own property selector when specifying property injection. I'm on my phone right now or I'd be more specific with links and examples, but you could go spelunking in the unit tests to get some ideas.
I just went back and checked on https://github.com/dotnet/csharplang/issues/3630 and it's still under discussion. The actual proposal for required members shows that it'll likely mean we have to search for some sort of attribute on the property that indicates it's a required member.
I'm not sure how long we should leave this open. PropertiesAutowired works on init properties, and without something to indicate they're required, simply being init doesn't necessarily mean they should always be filled in. If the proposal for required members isn't moving forward, I guess we have a couple of options:
- Close this issue for now and then reopen it if
requiredever gets approved. - Leave it hanging open for as long as the
requiredproposal is open. - Look at supporting
initalong withSystem.ComponentModel.DataAnnotations.RequiredAttributesince it sounds like the actual implementation ofrequiredwould also be an attribute. If we go this route, we could add the check for the new compiler-added attribute alongside the check forRequiredAttribute. Perhaps this is as simple as a new out-of-the-boxIPropertySelectorthat only findsinitproperties with that attribute?
If we do end up adding something for this, I think the feature definitely needs to be opt-in like PropertiesAutowired because the reflection to check all the properties is not free.
Pulling on the thread of that third option, I wonder if it would be interesting to add the ability for an IPropertySelector to somehow indicate "if you can't inject this property, fail the resolve." That is, the ability for the selector to indicate the property is required. At the moment we basically just try to set the property and ignore it if the value isn't there.
We could add something on like:
public interface IPropertyRequiredSelector
{
bool IsRequired(PropertyInfo propertyInfo, object instance);
}
Now if you have a property selector, you can optionally implement this interface and we can check it during AutowiringPropertyInjector.InjectProperties, something like:
if (context.TryResolveService(propertyService, new Parameter[] { instanceTypeParameter }, out var propertyValue))
{
var setter = PropertySetters.GetOrAdd(property, MakeFastPropertySetter);
setter(instance, propertyValue);
}
else if (
propertySelector is IPropertyRequiredSelector requiredSelector &&
requiredSelector.IsRequired(property, instance))
{
throw new DependencyResolutionException();
}
The existing implementations of IPropertySelector could remain as-is - no breaking changes - and some future-looking InitPropertySelector could handle looking at the attributes.
I have a few thoughts to share.
First on closing the issue: In my view if we plan to address something, it should remain open. Github's UX is such that closed issues lose visibility, and in practice most of the time closed issues get forgotten or assumed to be resolved.
I do want to say that one of the reasons I love Autofac is that it usually avoids requiring specific implementations and interfaces to change to support DI. For the most part, I can completely delete Autofac and refactor my composition root and still get DI without modifying any of my classes. There's a TON of value in this.
Having said that, I don't like the idea of property injection needing attributes. Even if that's a reasonable workaround, if I want it, now I have to add those attributes and then remove them later when the language builds in support for it. That's just a lot of code churn I'd personally like to avoid. Not to mention it sort of defeats the benefit I mentioned before.
Usually if I'm using property injection it means two things:
- I need a property to be injected as "optional"
- I need two-phase initialization semantics and the property may be either optional or required. In general, special construction/init semantics are needed.
The author of this issue has the use case of adding a parameter to a base class constructor which causes cascading changes. To me, that's a symptom of needing inherited constructor support in the language. There are proposals for that, such as this one, where the same author of this issue has replied to and expressed a dislike for the idea even though it would be a perfect solution to this problem. I don't see why Autofac has to solve this problem.
In my view, constructor parameters inherently mean "Give me this thing, it is not optional". That's consistent in many languages, such as C++. Required properties make sense for DTOs and other domain-specific ways of modeling required data, but for DI I do not agree it is appropriate. It seems counter-intuitive to add the same semantics to required properties when I can't think of how they're different from constructor parameters (with exception to my bullet point, especially about construction semantics).
To summarize my opposition here, I feel like this is not an Autofac problem. The author of this issue is experiencing a symptom of a lack of inherited constructor support, which I assert is the real issue here. If and once inherited constructor support is implemented, Autofac has nothing to do (or rather, has less incentive to support required property injection unless it is for the reasons I mentioned in my bullet points above).
If we do go down the path of implementing this, I'd like to see a more "fluent" way of supporting it in the composition root. A big benefit of using a composition root is that it serves as documentation. Viewing properties that need to be injected from there has a lot of value: Namely, it lets me keep an eye on potential code smells where property injection is being used where it shouldn't be. I would want something more akin to:
builder.RegisterType<MyClassWithInjectableProperties>()
.InjectProperty(x => x.MyProperty, DependencyResolution.Optional)
.InjectProperty(x => x.OtherProperty, DependencyResolution.Required);
Where x is the instance of MyClassWithInjectableProperties (or some proxy type). The point is: Use a property selector delegate (similar to Rx-style of doing things) to specify properties that need explicit injection by Autofac.
My challenge with leaving issues open for years (which happens) is that, honestly, no one actually ends up really following up on them or fixing them. They become just long-running known issues, which probably means the answer is documentation and not just "keeping it open for visibility."
In my view if we plan to address something, it should remain open.
This is going to sound pointed, and I really, really don't mean it to be: In this statement, who is the "we" that's addressing something? History shows that chances are it's going to be one of the (currently just two) active Autofac owners. If I close it, it's because no one has provided a PR and neither of the two of us are planning on addressing it.
And, of course, if we did close it, it'd be up to the people that want this feature to continue to check back on the status of the language proposal and chime in here so we can reopen it. Closing the issue would indicate we - Autofac - are not going to keep polling and checking back because... we just don't have that kind of bandwidth.
The proposal I made for having support for the Required attribute has two purposes:
- It addresses the immediate need. For folks who have these
initproperties and somehow need to mark them as required, barring the language update (which, itself, will likely end up as an attribute, albeit placed by the compiler), this would address that immediate need. It wouldn't require you to change your code later. If you're one of the people who really wants this, you add the[Required]attribute, things work; later you can add the compiler keyword (if that ever actually happens) and... whether you remove the[Required]attribute or not, it should still work. - It would prove out the larger feature of allowing property selectors also indicate that a particular property is required. That's potentially applicable elsewhere, like you could have properties on ASP.NET view pages (where the constructor isn't really accessible) and mark them as required. It'd have a wider applicability than just init properties.
Stepping back a bit, I don't really have a horse in this race. I've never quite understood init-only properties and why a required property wouldn't be a constructor parameter. I get that it's a pain to have to add that new constructor to all bajillion derived classes, but if you have a bajillion derived classes, maybe that's a problem, too. Or maybe it's something a refactoring tool can take care of. Or a source generator. I do like that proposal for base constructor propagation. I wish someone from the .NET language team would say something in there.
Anyway, maybe having required properties at all isn't a good idea, and maybe that's why we haven't seen required init properties making any headway. I'm fine with that.
What I'm challenged with is that this has been sitting for a year and there's this sort of expectation that, as it's open, someone on the Autofac team is "looking into it," which isn't true. I happened to get some time, thought I'd see if I could close out some of these long running bad-boys, and... here we are.
I guess I'll leave it open for now. I think if it's still open with no headway in, oh, six months? then I'll probably close it.
If folks think having the ability to allow a property selector also indicate required properties is interesting, let me know. I don't think it'd be that hard to implement and it may be enough that folks who want to implement this themselves or add other similar functionality could do that - another extension point.
This is going to sound pointed, and I really, really don't mean it to be: In this statement, who is the "we" that's addressing something? History shows that chances are it's going to be one of the (currently just two) active Autofac owners. If I close it, it's because no one has provided a PR and neither of the two of us are planning on addressing it.
I guess I shouldn't have used the word "we" here, as it sounds like I'm counting myself as a contributor when I'm really not. What I intended to refer to was the community at large. Not only existing contributors but potential contributors in the future. I only intended to share my world view here; not necessarily assert facts. Just how I interpret open/closed status.
At the end of the day, it's your project, so I definitely respect your decision either way on this. I think it's fantastic that you're so open to community feedback here, even if nothing changes.
Thanks for spending so much time on this!
Hi All, I just wanted to mention that required properties now exist as of the latest preview of C#.
I would love to see the above code enhanced so that it works with required properties.
With .NET7 and required launching soon, can someone make AutoFac set required properties?
I would like to simplify code from:
public class BaseClass1 {
protected object Arg1 { get; }
public BaseClass1(object Arg1) {
this.Arg1 = Arg1;
}
}
public class DerivedClass1 : BaseClass1 {
protected object Arg2 { get; }
public DerivedClass1(object Arg1, object Arg2) : base(Arg1) {
this.Arg2 = Arg2;
}
}
to simply:
public class BaseClass2 {
public required object Arg1 { protected get; init; }
}
public class DerivedClass2 : BaseClass2 {
public required object Arg2 { protected get; init; }
}
@TonyValenti "Someone" could be you! We'd love to look at a PR for this. Keep in mind there isn't a big team of folks on the project - there are maybe two or three unpaid folks doing all the work in their spare time (ie, instead of spending time with family or doing other hobbies). If there's a feature you really, really want, you'll get it faster by contributing. Otherwise, you'll just have to be patient.
I'm OK being patient or providing funding if anyone wants to do the work.
It's honestly less an issue of money than of time and motivation.
There's a very finite amount of time to allocate between core Autofac and all the ~20 integration libraries, and documentation, and examples, and answering SO questions, and answering issues. Then once all that gets cut up, the question becomes priority.
I won't lie, in many cases, all things being equal, I'll work on issues that are interesting to me, personally or on things that I could see using in my projects. Again, that's all things being equal. If there's a major bug or if there's a huge amount of momentum behind a feature, cool, raise the priority there. Or if I see something small that I can knock out quickly, I might jump on it so I feel accomplished.
Hence we rely on the community to contribute where possible, especially if it's a feature near and dear to their hearts.
I mean, unless you want to fund at the level of competitive-yearly-salary-with-benefits so I can quit my day job and just do Autofac, in which case, drop me an email with a proposal. 😆
Hi all, I'm closing this issue and opening #1355 with a revised scope and additional details. I really hope someone is willing to pick it up.