autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.44k stars 836 forks source link

Disable required property injection #1376

Closed rcdailey closed 1 year ago

rcdailey commented 1 year ago

Please allow disabling required property injection. I use Spectre Console, which automatically registers CommandSettings types. Subclasses of this type can have required properties which signals to the CLI parser that those values are required on the command line. However, they should not be injected by Autofac.

After reading the release notes and the page it links to for further reading, I do not see an easy way to globally disable required property injection. This leaves me with the inability to upgrade for now.

tillig commented 1 year ago

This is going to be an interesting thing. I don't use Spectre Console but it makes me wonder why it's registering those things for you. I also think it's interesting that it's using required properties and not the RequiredAttribute to indicate what needs to be provided. Seems like the settings that should be provided by the CLI shouldn't be injected like that.

Regardless, I recognize there's a dilemma. Can you mark the constructor on your derived class with SetsRequiredMembersAttribute? Doing that tells Autofac that your constructor will do the work to set the properties (which, in this case, it sounds like is actually happening by Spectre Console, but same idea - something else is taking care of it). Would that get you past the issue?

I don't think we'll turn off required property injection now that it's here. Technically those properties do indicate that the property is required for construction, so if you're delegating construction to Autofac, it means you want Autofac to do the whole job. If Spectre Console has changed the meaning, that's maybe convenient but technically wrong. That's what Data Annotations attributes are for - hinting about what's required, valid, etc.

Let us know how SetsRequiredMembersAttribute works out.

tillig commented 1 year ago

I went Googling for how to make something required in Spectre Console settings and every example has to do with overriding the Validate method in the settings class. Perhaps I misunderstood or didn't find the right thing. Can you point to a live example or somewhere in their docs showing use of required properties? Perhaps it's something new?

rcdailey commented 1 year ago

Sorry, I misspoke about a few facts. It's been a while since I looked at my code surrounding the DI registrations. I apologize for the misinformation.

First correction: Spectre.Console is not auto-registering CommandSettings objects. I have written code to register them via reflection. However, I think the problem domain is the same: I register them because some of the subclasses of this type do indeed have constructors where types are injected. Others, do not have constructors or need DI, but will have required properties.

Second correction: Spectre.Console itself does not utilize required properties, to my knowledge. The property is made required by usage of <param> instead of [param] (It interprets the brackets differently). I use the required property so that I am not required to make the property use a nullable type. Here's a real snippet of one such property in my code:

        [CommandArgument(0, "<service>")]
        [EnumDescription<SupportedServices>("The service to obtain information about.")]
        [UsedImplicitly(ImplicitUseKindFlags.Assign)]
        public required SupportedServices Service { get; init; }

By using required here, I do not need to use type SupportedServices?, which in turn means I do not need unnecessary logic to handle null here. Logically, the property will never be null since the CLI library ensures that for me.

Again, I apologize for the misinformation. I should have properly refreshed my memory before posting initially. I'll admit I created the issue in a bit of a hurry. I do feel, however, that the problem statement is still the same. required, in my view, shouldn't always be interpreted with a specific meaning. Autofac should have the ability to be told to ignore it, in case of situations where it shouldn't assign it some meaning. If there is such a way to do this, I haven't found it yet.

Ideally, it would be great to disable it when registering multiple types via reflection. For example:

builder.RegisterAssemblyTypes(Assembly.GetExecutingAssembly())
    .AssignableTo<CommandSettings>()
    .PropertiesAutowired(PropertyWiringOptions.SkipRequired);

I don't know if that example makes sense. But hopefully it communicates what I'm thinking.

rcdailey commented 1 year ago

I forgot to address this point:

don't think we'll turn off required property injection now that it's here. Technically those properties do indicate that the property is required for construction, so if you're delegating construction to Autofac, it means you want Autofac to do the whole job. If Spectre Console has changed the meaning, that's maybe convenient but technically wrong. That's what Data Annotations attributes are for - hinting about what's required, valid, etc.

To be clear, I wasn't suggesting you undo the change. Or in other words, I wasn't suggesting that you turn off required property injection by default. I think it would simply be enough to disable it at the composition root. Two main reasons come to mind:

  1. I typically avoid DI-specific attributes where I can. This is one of the main appeals for Autofac, IMO. It does a really darn good job at allowing DI behavior to be specialized/customized without requiring tons of annotations.
  2. When I do reflection-based DI registrations, where more than one type is impacted, I want the ability to exclude required property injection from all of them. In my view, by doing this, it means 2 things:
    • The required property has some special meaning, and Autofac shouldn't assume anything or even look at it.
    • I only want to use constructor-based injection.
tillig commented 1 year ago

I should clarify: required has a very specific meaning with respect to the compiler and class instantiation. It isn't part of nullable reference types. You're using it wrong, and not just with respect to DI.

If you want the property to start as null but not make the type nullable, force initialize to null.

public string MyProp { get; set; } = null!

Data Annotations attributes are a common way to mark things required from a validation standpoint. There's a [Required] attribute for that purpose. That's really common in things like data models or settings and is not specific to Autofac or DI. It sounds like you may not need this, but if you do add validation, that's not done with required properties, either.

You might want to look at strategies for NRT is ASP.NET models like this blog article because the concepts overlap: you have a model, the property won't be null in a practical sense, and the framework will be filling it in for you.

But the key bit here is that you're using required wrong, and it's created this problem for you. If you use it as it's intended - to be a compiler hint, not as a nullable reference type fix - you won't have the issue.

We won't be introducing a way to turn off required property injection. We took quite a long time to debate what to do about required properties, and we ended up following the actual intention of the keyword: to tell the compiler that the object is not fully constructed without code initializing those property values. We also concluded, after much discussion, that the nullability of a given required property would not indicate "optional" vs "actually required."

rcdailey commented 1 year ago

I appreciate the clarification and the discussion. required is still really new; I don't have my head quite fully wrapped around it yet. Your links certainly help, but at the moment I still find it confusing why it was introduced into the language. I don't personally see what the offer over constructor parameters other than a different syntax. But alas, that's probably part of the learning I need to do.

Thanks for everything!