Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WithProperty by expression #1123

Merged
merged 3 commits into from
May 19, 2020
Merged

Conversation

mashbrno
Copy link
Contributor

I'm migrating from Spring.Net and I am using a lot of property assignment. I know it's not the best practice for Autofac, but good start. Cleanup can come later. I've found useful method checking that the assigned value can be actually assigned to the particular type in compile time. I'm probably sacrificing some startup performance during container build.

I have no idea, how to fix error that propertyValue might be null. Thanks for advice.

@tillig
Copy link
Member

tillig commented May 17, 2020

It doesn't look like there are any unit tests here to exercise the function. Can you add some? Thanks!

@mashbrno
Copy link
Contributor Author

@tillig I will try. But please tell me if the extension method is wanted in code base. If so please can you check what's wrong with the build?

@tillig
Copy link
Member

tillig commented May 17, 2020

I think this is probably a reasonable extension. We have something similar for MVC controllers and using expressions for properties instead of strings can help with compilation and refactoring issues.

The build appears to be failing due to nullable reference type annotations. A quick Google of the error message CS8604: Possible null reference argument can get you some additional info. Basically, you have to say "it's OK to pass null here" if you want to allow it. There's some info here.

I'm sort of in the middle of some stuff so I can't dig in and decide yet whether this is an issue where we should add a type constraint on TProperty to ensure it can be nulled out... or maybe whether we just need a ! null forgiving operator in there.

@alistairjevans
Copy link
Member

Regarding the null reference error, the TProperty generic parameter should have a notnull constraint on it to prevent the error.

I can't see a situation where a named property parameter would need to have a null value.

@mashbrno
Copy link
Contributor Author

Added ! as I think setting a null value can be useful in cases where class default is different.

@alistairjevans
Copy link
Member

Hmm, I'm not sure if Autofac's property injection should be used to set things to null. Can you give me an example?

If we want to make nullable properties a thing, rather than use null-forgiving, we should change NamedParameter to accept object?, and handle the downstream consequences.

The null-forgiving operator should really only ever be used if you know that the value cannot be null, but the compiler doesn't; it shouldn't really be used to handle situations where you just want to pass null to a type that doesn't accept it.

@tillig
Copy link
Member

tillig commented May 18, 2020

Ah, yeah, that's the stuff I couldn't look up at the time. I'd agree, it's likely we don't want to allow folks to inject null as a value. It would be inconsistent behavior with the rest of Autofac. For example, you can't do this:

builder.Register(c => (MyType)null).As<IMyType>();

You'll end up getting an exception if the lambda returns null; and you can't register null as an instance, either.

I think we missed this in ConstantParameter and it didn't find its way into derived classes like NamedPropertyParameter but I think that's an oversight rather than an intentional "allow null to be injected via parameter" situation.

@mashbrno
Copy link
Contributor Author

Understand. Removed the support for nullable propertyValue.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks!

@tillig tillig merged commit fd74e30 into autofac:develop May 19, 2020
@mashbrno mashbrno deleted the feature-propertyExpression branch May 19, 2020 20:08
@alistairjevans alistairjevans added this to the v6.0 milestone Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants