This project is read-only.

Which decorator to apply?

Jan 19, 2015 at 12:36 PM
Edited Jan 19, 2015 at 12:58 PM
As discussed here, a change is coming in the next path release of Simple Injector is to improve support for creating types that contain more type arguments than the decorated abstraction. Simple Injector supported this for quite some time already, but a bug prevented Simple Injector from resolving types under certain conditions.

But being able to deduce generic arguments out of generic type constraints give some interesting challenges. For instance when a generic type argument can be deduced back to multiple concrete types. The question is how Simple Injector should handle these cases.

Here's an example:
public interface ICommandHandler<TCommand> { }
public interface IConcurrenyCommand<TEntity> { }
public class UpdateCompanyCommand : 
    IConcurrenyCommand<Asset>, 
    IConcurrenyCommand<Company> { }

public class UpdateCompanyCommandHandler : ICommandHandler<UpdateCompanyCommand> { }

public class ConcurrenyCommandHandlerDecorator<TEntity, TCommand> : ICommandHandler<TCommand>
    where TCommand : IConcurrenyCommand<TEntity>
}

var container = new Container();
container.Register<ICommandHandler<UpdateCompanyCommand>, UpdateCompanyCommandHandler>();
container.RegisterDecorator(typeof(ICommandHandler<>), typeof(ConcurrenyCommandHandlerDecorator<,>));
var handler = container.GetInstance<ICommandHandler<UpdateCompanyCommand>>();
The example above defines an ConcurrenyCommandHandlerDecorator<TEntity, TCommand> where Simple Injector is able to deduce the TEntity type argument. In this case however, we resolve an ICommandHandler<UpdateCompanyCommand> and it implements IConcurrenyCommand<TEntity> twice. Once for Asset and once for Company. So now an interesting question arises: how should Simple Injector handle this?

The current 2.7.1-alpha3 of Simple Injector will select 'the first' usually type (what ever that may be) and will construct the decorator using that type. So one decorator is applied. It's very questionable whether this behavior is correct. I think we have the following options:
  1. Select the 'first' type argument (current behavior) and apply the decorator for that argument.
  2. Don't apply a decorator at all (silently skipping).
  3. Throw an exception explaining the ambiguity.
  4. Apply multiple decorators (in this case two).
Options 1 and 2 are very implicit and are not inline with Simple Injector's Design Pinciples (because we would fail silently and the feature won't be intuitive), so they don't have my preference.

If we pick option 3, in what way would it be possible for the user to make the configuration in such way that both decorators can be applied (if that is what the user needs)?

If we pick option 4. the question still remains in which order those decorators should be applied?

What do you think?
Jan 19, 2015 at 1:56 PM
Edited Jan 19, 2015 at 2:34 PM
2 decorators would seem to be the correct response to the configuration. Is there any benefit to having a diagnostic warning 'multiple instances of the same <xyz> decorator in the object graph'?
Jan 19, 2015 at 9:00 PM
Following the design guidelines option 1 and 2 are out of the question from my point of view.

I can't decide for option 3 or 4. As qujck says, option 4 seems to be correct response, but on the other hand, how to answer the question of dot_net_junkie regarding the order...?

I think the most usefull option would be to use option 4, apply the decorators in order of registration (as would be with 'normal' decorators) and log a warning in the analyzer.

Regarding the diagnostic warnings. What do you think of an option to add the possibility to throw warning(s) as an exception, just like we have the option of the debugger 'treat all warnings as exceptions'. Maybe this option should even be: 'Always on', otherwise it will probably never be used. Or even better 'Always on' only for the more severe warnings such as lifestyles, the decorators as discussed, container registrered objects. Possible SRP violation should be 'Always off'
Jan 19, 2015 at 10:33 PM
Edited Jan 20, 2015 at 9:34 AM
I guess the most logical option is to apply them in the order they match the services they implement: IConcurrenyCommand<Asset> followed by IConcurrenyCommand<Company>.

I suggested something similar regarding the diagnostics last August - below is some code I used to express the idea - I think you are suggesting a similar thing?
public class VerificationOptions
{
     public VerificationOptions()
     {
         this.ServicesThatImplementIDisposableMustRegisterForDisposalThrows = false;
         this.DisallowWrappingContainerUncontrolledCollectionsWithSingletonDecoratorsThrows = false;
     }

     /// <summary>
     /// defaulted to false until v3 at which time this will become a default feature
     /// </summary>
     public bool ServicesThatImplementIDisposableMustRegisterForDisposalThrows { get; set; }

     /// <summary>
     /// defaulted to false until v3 at which time this will become a default feature
     /// </summary>
     public bool DisallowWrappingContainerUncontrolledCollectionsWithSingletonDecoratorsThrows { get; set; }

     public static VerificationOptions StrictestOptionsFactory()
     {
         return new VerificationOptions
         {
             ServicesThatImplementIDisposableMustRegisterForDisposalThrows = true,
             DisallowWrappingContainerUncontrolledCollectionsWithSingletonDecoratorsThrows = true
        }
    }
}
Jan 19, 2015 at 10:59 PM
Edited Jan 19, 2015 at 11:02 PM
Yes this looks very much what I suggest! Just from the opposite point of view, which is an interesting point of view also.

Your code shows configuration to disable some buildin checks which can be strict in some situations. While I'm suggesting to let the user configure to be even more strict. Some of the warnings that the diagnostic service provide will than not only produce a warning but can be enabled for throwing an exception.

Usually I write a unit test to test for misconfigurations that the analyzer will pick up (such as lifestyles). For most of them I will change the configuration to 'satisfy' my test.

So why not just throw this warnings? Tests stay optional and can be ignored, especially when the deadline is coming up.

If you commit to do the configuration correctly you could place yourself in a position where you configure the severity of certain checks at the start of project (the part of the project, where you think you still have time ;-) ). These configuration are less likely to be changed during the project and will be checked, tested if you will, everytime you run the application. Configuration mistakes can be fixed than quickly and easily as they will occur most likely directly after you changed (a part of) the configuration.
Jan 22, 2015 at 7:42 PM
>> I guess the most logical option is to apply them in the order they match the services they implement: IConcurrenyCommand<Asset> followed by IConcurrenyCommand<Company>.

But will C# and the CLR always return those interfaces in the exact same order? We already know that the C# team gives no guarantees in the deterministic behavior of C# and we know that the new Roslyn C# compiler will compile in parallel. Perhaps a recompile can mess this up. So what guarantee will we have that the C# will always keep the interfaces in the same order? And what if one of the interfaces is in a base type? And what about the CLR? I doubt whether the CLR gives any guarantees in the order in which Interfaces are returned. A mere application restart might result in a different order of interfaces.

So if we implement this, we probably need to document that ordering is undeterministic.

After doing a short spike, I realized that detecting all decorators and applying them might be a very difficult task with the current code base. I probably need to take a week of to get this working. I probably wont have time for that soon.
Jan 30, 2015 at 1:49 PM
@dot_NET_Junkie:

I'm interested in your opinion on the secondary discussion about the changes in letting the user tune the diagnostics.
Decrease from (certain) exceptions -> warnings and
Increase from (certain) warnings -> exceptions

Looknig forward to your reply!
Jan 31, 2015 at 11:00 AM
@TheBigRic, @qujck,

I had to think about this for some bit, but throwing exceptions as a 'treat all warnings as exceptions' is very much in line with the Simple Injector design principles, because we want to push developers into best practices, never fail silently and communicate errors clearly and describe how to solve them.

So in other words, the whole philosophy screams on failing fast instead of allowing developers to continue. But there are of course a few challenges in doing this. First of all, this is a severe breaking change. So such feature should be delayed until v3. Or at least, throwing by default should be delayed until bumping the major version number. But furthermore, every new diagnostic warning now becomes a breaking change as well. That is much harder to explain, expect when we bump the major version number for each new diagnostic warning, but I'm not sure that's worth it. Other option is to suppress exceptions for new warnings, but that would make the API less intuitive.

Another thing is that we will need to differentiate between diagnostic warnings and information messages. For instance, the unregistered type warning and the SRP violation are not things we want to throw an exception about. We already talked about this and we already decided to split this in v3.

Another challenge is that we need a solid API that developers can use to suppress certain warning types, but more importantly: suppress a certain warning type on a particular component, because you normally don't want to suppress all warnings, just because you have one single registration where the warning is a false positive.

What might be more challenging is to move these diagnostics into the container and do it when resolving an object graph for the first time. Doing it directly after somebody calls Verify() is not enough, because a developer might not call Verify() at all (and he might have valid reasons for not doing this). So the diagnostic checks should also go off when you call GetInstance on some type for the first time. But not all diagnostic warnings can always be successfully be applied at that point in time. The Torn Lifestyle warning for instance, does not check one single registration, but detects when there are multiple registrations that are in some way related to each other. So in some cases the check can only be performed when the complete container is verified, or at least when another affected part of the configuration has been built.

Nonetheless, I think this is the way to go for Simple Injector's future. As I said, this is in line with the design principles and those design principles is what differentiates Simple Injector from the other containers, and these design principles is what helps are users the most.
Jan 31, 2015 at 11:02 AM
This discussion has been copied to a work item. Click here to go to the work item and continue the discussion.