Report generic constraint mismatches via the Diagnostics API

[fsateler]

Suppose I have the following decorator:

public class InstanceCachingDecorator<TQuery, TModel> : IModelQueryHandler<TQuery, TModel>
    where TQuery : IModelQuery<TModel>, IEquatable<TQuery>
{
    private readonly IModelQueryHandler<TQuery, TModel> Decoratee;

    private readonly Dictionary<TQuery, TModel> Cache = new Dictionary<TQuery, TModel>();

    public InstanceCachingDecorator(IModelQueryHandler<TQuery, TModel> decoratee)
    {
        Decoratee = decoratee;
    }

    public TModel Query(TQuery query)
    {
        TModel result;
        if (!Cache.TryGetValue(query, out result)) {
            result = Cache[query] = Decoratee.Query(query);
        }
        return result;
    }
}

The intent is to cache the result, but obviously this (or at least this implementation) is not meaningful if TQuery does not implement IEquatable, as otherwise the keys will (almost) never match.

I currently do this:

container.RegisterDecorator(typeof(IModelQueryHandler<,>), typeof(InstanceCachingDecorator<,>),
    Lifestyle.Scoped, 
    ctx => ctx.ServiceType.GenericTypeArguments[0].IsEquatableSelf());

Where IsEquatableSelf is a method that determines whether the type T implements IEquatable<T>. However, I recently realized it is not necessary: SimpleInjector appears to silently skip types that do not match the generic type constraints. The current behavior is mentioned in a passing reference in the Advanced Scenarios documentation, but it is not prominent.

I would have expected instead to receive an exception from Verify, noting that there are mismatches, in the spirit of Never Fail Silently. Applying a decorator that does not match the generic constraints represents either a composition error (which currently SimpleInjector is 'fixing' on my behalf), or maybe a very serious error (an interface implementation was missing).

Because due to the way the registration of open generic types works we have given up the compiler type constraints checks at compile time, SimpleInjector should offer this check at verification time.

[dotnetjunkie]

The philosophy here is that generic type constraints are used as filters. So by defining a generic type constraint on a decorator you explicitly state the sub set that the decorator is valid for. This is much more powerful than doing this using a predicate during registration and it adds compile time support inside the decorator.

For that reason I don't consider the decorator to be 'skipped silently' but rather skipped explicitly because of metadata specified in the C# language.

Because of this, there is no failure when doing so. The majority of users of Simple Injector that have decorators with type constraints actually depend on the behaviour of filtering. Conditionally applying decorators based on type constraints is actually a unique feature that up to today no other DI container has. Many Simple Injector users make heavy use of this feature.

Letting verify fail when a 'skipped' implementation is detected, would be a severe breaking change, simply because this is how the feature is used by most.

In case you require this verification, my advice is to remove the type constraint from the decorator and do a type check inside the ctor or cctor that verifies whether all implementations implement your expected interface.

Or even better, if IEquatable is expected for all TQuerys, mark the IQuery<TResult> with IEquatable. This would remove the problem altogether and solves the problem at the design of the application.

[fsateler]

Thanks for your considered reply.

I agree this is a breaking change, and if users are relying on this functionality, a blanket behavior change is not possible. But maybe this can be enabled as an option?

In case you require this verification, my advice is to remove the type constraint from the decorator and do a type check inside the ctor or cctor that verifies whether all implementations implement your expected interface.

But then I would have to invoke the interface methods via reflection or a cast. If the decorator is transient and frequently invoked this may imply a performance cost. (granted, this is not a problem for my sample scenario).

Moreover, this adds code to my decorators (that may not necessarily be part of the composition root) that is specific to the injection container, thereby enlarging the root.

Or even better, if IEquatable is expected for all TQuerys, mark the IQuery<TResult> with IEquatable. This would remove the problem altogether and solves the problem at the design of the application.

The point is that not all TQuerys are cacheable, otherwise your suggestion would be the best solution. The problem is that there are two decisions being made, both of which are not necessarily made at the same time or by the same person.

Let's expand the sample case a bit for a more realistic usage. With the same decorator, we now have an CacheableQueryAttribute. This is the application-specific way to document that a decorator is cacheable.

Then I'd register the decorator as follows:

container.RegisterDecorator(typeof(IModelQueryHandler<,>), typeof(InstanceCachingDecorator<,>),
    Lifestyle.Scoped, 
    ctx => ctx.ImplementationType.GetCustomAttributes(typeof(CacheableQueryAttribute)).Any());

In this case, registering the decorator for a TQuery that does not implement IEquatable is an error, either in the composition or the service implementation. SimpleInjector does not have the necessary knowledge to determine which of the two it is, but it is nevertheless making the choice for me. I would like for it to complain loudly to me that there is an error.

[dotnetjunkie]

But then I would have to invoke the interface methods via reflection or a cast.

Not exactly. The only thing you have to do is to do:

if (!typeof(IEquatable<TQuery>).IsAssignableFrom(typeof(TQuery))
    throw new InvalidOperationException("...");

If the decorator is transient and frequently invoked this may imply a performance cost.

If you place this code in the static constructor, you pay the cost just once per closed type:

static InstanceCachingDecorator()
{
    if (!typeof(IEquatable<TQuery>).IsAssignableFrom(typeof(TQuery))
        throw new InvalidOperationException("...");
}

The point is that not all TQuerys are cacheable, otherwise your suggestion would be the best solution

So what you are saying is that your decorator should actually be applied conditionally, based on its generic type constraints. As I see it, Simple Injector's feature is the perfect match for you. Simple Injector allows you to define this constraint exactly once, in the place it is most obvious, the decorator itself, and in the syntax you are used to the most, c# syntax in the form of type constraints.

Do note though that although you were able to define a predicate that did the check for you, some type constraints are really (and I mean really really) hard to express using the .NET reflection API. This would be an enormous pain for developers to duplicate that very elegant where statement into the reflection equivalent.

In this case, registering the decorator for a TQuery that does not implement IEquatable is an error, either in the composition or the service implementation.

I get your point. In your case, it would be nice to have a safety net in place. There is no way we will break many users with such an important feature, but it is possible to have some sort of configuration switch in place. Question is however whether you want the switch to hold for the complete container, or just for one decorator. I'm not sure this would actually make Simple Injector better; more options means more complication. I will think about this.

For now, I would advise to write the proper unit tests that verify whether things are in sync or not.

To simplify this task, think about moving your attribute to the query definition. Not all metadata belongs on the query (for instance an attribute that defines a certain transaction is an implementation details and probably belongs with the handler), I think a [Cache] attribute is something that suits well on a query class; this is often something the client likes to know about.

In case you move the [Cache] attribute to the query, it becomes trivial to scan all application's queries and verify whether [Cache]d queries are also IEquatable.

[fsateler]

There is no way we will break many users with such an important feature, but it is possible to have some sort of configuration switch in place. Question is however whether you want the switch to hold for the complete container, or just for one decorator. I'm not sure this would actually make Simple Injector better; more options means more complication. I will think about this.

I am slowly internalizing the fact that this is a deliberate feature (initially I thought it was an unintended behaviour, so +1 to #48). I now agree that changing this behavior is a no-no. Users likely rely on this.

Adding an option means more complexity. If nobody else asked for this, then maybe (almost) everyone prefers the current behavior over the one I propose. This means that an option may not be worth it.

However, there is a third option. Perhaps it is possible to expose this information somewhere via the diagnostics API? This way it should be possible to automate a test that verifies that no decorators are skipped.

[dotnetjunkie]

Perhaps it is possible to expose this information somewhere via the diagnostics API?

That's actually not a bad idea. Since v2 the diagnostic already had the notion of warning messages and info messages. This could be an info message.

However, since nobody ever payed any attention to the warnings, we decided to let Verify throw an exception when such warning was detected. Although it is still possible to request all other messages (the info messages) from the container, I'm pretty sure nobody ever looks at this information. I'm pretty sure you never looked at those. That's why I'm so glad we pushed the breaking change in v3 and throw exceptions.

If however we sttart focusing again more on these informational messages, they will have to be promoted more. Problem is however that the docs are pretty clear about their existence. I don't know how be more clear.

[fsateler]

However, since nobody ever payed any attention to the warnings, we decided to let Verify throw an exception when such warning was detected.

This is an awesome feature, and I'm very glad it exists.

Although it is still possible to request all other messages (the info messages) from the container, I'm pretty sure nobody ever looks at this information. I'm pretty sure you never looked at those

Touche. 😉

If however we start focusing again more on these informational messages, they will have to be promoted more. Problem is however that the docs are pretty clear about their existence. I don't know how be more clear.

In my defense, the diagnostics section of the manual is pretty focused on the Warning messages. I missed the fact that there are some diagnostics that are not warnings.

/me goes to play with the Diagnostics api to see what messages I'm missing...

[fsateler]

Oops, I pressed Comment too early.

To give more prominence to the informational messages, I would:

1. Add a short paragraph in the Diagnostic Services introduction documenting this. Something like:

SimpleInjector knows about several common issues that might arise when composing the dependency tree, and can expose this information to the developer via its diagnostics API. Currently, SimpleInjector classifies the issues in two severities: Warnings and Informational.

1. Amend the fist note to mention that DiagnosticsVerificationException is thrown only for diagnostics of level Warning (or equivalently, that informational messages are not considered).
2. Add a section listing the available informational messages (akin to the "Supported Warnings") section.

[dotnetjunkie]

I made some improvements to the documentation based on this discussion and your feedback:

• The AOP chapter now contains clear information about applying decorators conditionally using type constraints.
• I made clarifications to the Diagnostics chapter and moved the list of warnings and informational messages to the top.

Thanks for your help.

[fsateler]

@dotnetjunkie Thanks!. As per the discussion, this has become a feature request and I have retitled the issue accordingly.

[dotnetjunkie]

Moved to new feature request issue.