Private properties on base class should be probed and exception thrown when marked for injection

[dotnetjunkie]

Code to demonstrate the problem:

public abstract class BaseComponent
{
    [Dependency] private ILogger logger { get; set; }
    public bool HasLogger => this.logger != null;
}

public class InheritedComponent : BaseComponent { }

public sealed class DependencyAttribute : Attribute { }

class DependencyAttributePropertySelectionBehavior : IPropertySelectionBehavior
{
    public bool SelectProperty(Type type, PropertyInfo prop) =>
        prop.GetCustomAttributes(typeof(DependencyAttribute)).Any();
}

[TestMethod]
public void Ctor_Always_Succeeds()
{
    // Act
    var container = new Container();
    container.Options.PropertySelectionBehavior = new DependencyAttributePropertySelectionBehavior();
    container.Register<InheritedComponent>();
    container.Register<ILogger, NullLogger>();

    Assert.IsTrue(container.GetInstance<InheritedComponent>().HasLogger);
}

The test fails, while the expected behavior is for the property to get injected, because:

• Private properties on the derived class are injected
• This worked in the past and is likely a regression that was introduced in v4.

Fixing this issue, however, should be considered a breaking change, because Simple Injector will start calling SelectProperty on properties that were previously skipped.

[dotnetjunkie]

Created bug-893 branch.

[dotnetjunkie]

Unfortunately, there doesn't seem an easy fix for this issue, because the Reflection functionality required to implement this, are not available in .NET Standard 1.0 and 1.3.

In order to explain why this is difficult, I first have to set the requirements:
Requirements:

1. Private properties of the registered component should be eligible for injection (current behavior)
2. Private properties of the component's base type (both direct and indirect) should be eligible for injection (change in behavior)
3. Overridden properties should be injected at most once. This is important for performance, but a user might have some check in place that protects the property from being initialized more than once. (current behavior)
4. When a virtual property is not overridden, but newed, both the base property and the "new" property should be eligible for injection. (current behavior)

There are two ways to retrieve a type's properties:

• Type.GetRuntimeProperties() returns all properties that are accessible to the given type. So it returns a flattered list that includes properties defined on a base type. Although it includes private properties of the type itself, it excludes private properties of the base type. This also holds for internal properties when the base class is defined in a different assembly.
• Type.GetTypeInfo().DeclaredProperties returns all properties defined on the type, but not those of a base type, even if they are accessible to the given type. It also returns all properties that are overridden by the current type.

Get all properties using Type.GetTypeInfo().DeclaredProperties requires it to be called on the complete type hierarchy (so type + base types) in order to get the properties. It then requires filtering out base class properties that were overridden (to adhere to requirement 3).

With Type.GetRuntimeProperties(), it's a little better, because it returns the complete hierarchy and automatically de-duplicates those overridden properties. But still, anything that is not accessible to the type is not returned, so in order to adhere to requirement 2, we still have to go through the base types to get the private and internal properties that are not accessible to on of the sub types.

The problem now starts with finding out which base-type properties should be filtered out, because they are already included in the list. The only reliable way of doing this is by calling PropertyInfo.Accessors(true)[0].GetBaseDefinition(). GetBaseDefinition() returns the method definition for the type that defined this method. But GetBaseDefinition() is not available in .NET Standard 1.0 and 1.3. This makes it much harder to determine whether a property was overridden or that the property is a new property.

Perhaps it's still possible to find out, but I'm currently clueless. Have no idea how to do this.

There are several workarounds, but I'm not happy with either of them:

• Don't filter out the virtual base properties and let Simple Injector inject the properties multiple times (breaks 3)
• Only implement this feature under .NET Standard 2.0 and up. Very dangerous, because Simple Injector starts to behave very different from build to build.
• Only implement filtering in .NET Standard 2.0 and up. Same problem as before. It breaks requirement 3 in the lower .NET Standard builds. Very confusing for users.

[dotnetjunkie]

After a long discussion with @TheBigRic, we came to the conclusion that not injecting private properties is actually good thing, because:

• Properties on base classes are code smells, because they lead to base classes containing (cross-cutting) behavior, which leads to SRP violations; decoration and interception are better approaches to this.
• Private properties on base classes could even be considered an anti-pattern, because besides the previous argument, it makes it impossible to unit test the base class and the derivatives, because private properties are inaccessible.

Unfortunately, compared to static and read-only properties, Simple Injector currently skips those properties when probing the custom IPropertyInjectionBehavior implementation. This is bad, because violates the Never fail silently design guideline.

Instead, Simple Injector should even prope the IPropertyInjectionBehavior implementation for private base class properties but, when the implementation returns true on such property` throw an exception, explaining that Simple Injector will not inject that property, why, and how to fix this (by referring to the documentation with a workaround).

This last, however, still seems impossible to achieve while we depend on .NET Standard 1.3.

I, therefore, move this issue to the Simple Injector v6 milestone. In v6 we might remove (not sure yet) the .NET Standard 1.0 and 1.3 dependency.

[thepigeonfighter]

While I would normally agree that private properties would be better replaced by decoration (I am not familiar with the interception pattern). I feel like Blazor would be an exception that proves the rule here. I could be missing something, but it seems like inheritance is all you have in Blazor components. You have no other way to inject dependencies into a class except property injection and it seems reasonable that sometimes you would want those dependencies to be hidden away from inheriting classes? I can see this being the right approach for Simple Injector as a whole, but for Blazor in particular I would need some convincing on this.

[dotnetjunkie]

@thepigeonfighter,

It certainly seems that there are currently some limitations on Blazor that make interception (either through middleware, decorators, or dynamic interception) hard (if not impossible). I had discussions with the Blazor team about this.

I can see this being the right approach for Simple Injector as a whole, but for Blazor in particular I would need some convincing on this.

In order to be able convince you (or agree with you), I'm interested to learn what dependencies you have on the base class and how you would intend to use them.

[thepigeonfighter]

I'll give two examples, both in Blazor because that is what I have been working most in as of late.

1. There are times when a action is done so many times, in different places on a website that it becomes convenient to wrap it in an easy API. This particular example is using Blazored.Modal to show a popup window that prompts the user to confirm an action.

private IModalService _modal { get; set; }
protected async Task<bool> ShowConfirmBox(
    string title, string message, string ok = "Ok", string cancel = "Cancel")
{
    ModalParameters parameters = new ModalParameters();
    parameters.Add(nameof(ConfirmBox.Message), message);
    parameters.Add(nameof(ConfirmBox.OK), ok);
    parameters.Add(nameof(ConfirmBox.Cancel), cancel);
    var modal = _modal.Show<ConfirmBox>(title, parameters);
    var choice = await modal.Result;
    return !choice.Cancelled;
}

While I can imagine one might argue that this could be pulled out into a separate class and injected into the child class via a private property, to me it is so often used that I prefer not to have to inject it every time I need it.
2. Wrapping commonly used methods makes them more readable in base classes and controls dependency access points. Here is an example with AutoMapper. While this is closer to code style issue vs a design I have actually found it to be very handy and does seem to make child classes easier to understand.

private IMapper _mapper { get; set; }
protected T Map<T>(object source)
{
    return _mapper.Map<T>(source);
}

The inheriting classes don't need access to the IMapper instance so why not make it private?

[dotnetjunkie]

The first example can be rewritten to an extension method on IModalService. This allows allows IModalService to be injected into the Blazor component, which makes the dependency explicit and discoverable.

And I would argue that it's better to inject IMapper in the final Blazor component as well, because it makes the dependency clear.

In case you your Blazor components get too many dependencies, it might get cumbersome, but in that case I would argue that those components become too big. I see no justification for big components, especially because Blazor makes it easy to compose Blazor components from other components.

[thepigeonfighter]

I didn't think of the extension method. That is a better solution for that scenario. Fair enough for the second example. Ok, carry on 🐱‍👤I can't think of any other examples at this point.

[dotnetjunkie]

@thepigeonfighter, thank you for your response.

Do note that, as I noted in the above analysis, even if I wished to add support for private properties, it's not possible ATM (or -at least- not in the core library). I added a documentation example, though, that demonstrates how to work around this issue.

It might be possible to add this behavior in a future Blazor-integration package. I bring this up, because I just noticed that the property-injection behavior of Blazor itself, actually does inject into private properties of base classes. Not implementing this might, therefore, cause incompatibility issues when developers migrate from MS.DI to Simple Injector (even though I still think base classes shouldn't have dependencies).

This behavior is implemented inside the GetPropertiesIncludingInherited of the Microsoft.AspNetCore.Components.Reflection.MemberAssignment, which is part of the Microsoft.AspNetCore.Component. This method also makes use of .GetBaseDefinition() (which is missing from .NET Standard 1.3.)