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

Let Verify fail when a decorator exists for a short circuited dependency #248

Closed
dotnetjunkie opened this Issue Jun 6, 2016 · 9 comments

Comments

3 participants
@dotnetjunkie
Collaborator

dotnetjunkie commented Jun 6, 2016

The short circuited dependency check will only go off in case the short-circuited dependency has a different lifestyle as the explicit abstraction-based registration. But this doesn't cover the case where a decorator (or other expression replacing mechanism) is applied to the explicit abstraction-based registration.

Example:

container.Register<ILogger, MyLogger>();
container.RegisterDecorator<ILogger, LoggerDecorator>();
container.Register<HomeController>();

class HomeController { public HomeController(MyLogger logger) { ... } }

Here, the use of MyLogger in HomeController will cause problems, because we can expect users to expect the LoggerDecorator to be applied, while it won't, since it can't be applied to the concrete MyLogger type.

@dotnetjunkie dotnetjunkie added this to the v3.2 milestone Jun 6, 2016

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jun 6, 2016

Alternatively to trying to detect whether a given registration got intercepted, the Short Circuited Dependencies warning could also be made more strict, so that it fails whenever there is an explicit registration, instead of only when the lifestyle of the concrete unregistered type (typically transient) doesn't match with the explicit registration.

Such change is much easier to make and safer to do (less chance of getting false-positives), but is on the other hand a bigger breaking change, since it can cause more configurations to fail, while there is no real problem.

@qujck

This comment has been minimized.

Collaborator

qujck commented Jun 12, 2016

In the past I have relied on the ability to retrieve the undecorated instance (I can't recall the exact purpose at that time but it was related to database activity and the underlying implementation had an extra/hidden method that was available to an event that was triggered elsewhere in the transaction. A change to the container could break that solution.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jun 12, 2016

A change to the container could break that solution.

Yes, a change to this diagnostic rule would case the container to throw more often, and might cause an exception in your particular case as well, but you can suppress this by making the registration explicitly.

@TheBigRic

This comment has been minimized.

Collaborator

TheBigRic commented Jun 12, 2016

The short circuited dependency check will only go off in case the short-circuited dependency has a different lifestyle than the explicit abstraction-based registration

Correct me if I'm wrong but is this (existing) behavior useful at all? I guess if the short circuited dependency diagnostic result is only thrown in this case, it doesn't add anything except a useful message, because this should also throw a ambigious lifestyle mismatch? Is there a scenario where the short circuited dependency diagnostic result is thrown in its own?

That being said I think the diagnostic result should throw in case of an applied decorator and ofcourse @qujck is correct, this is a breaking change. But as you explained to me when v3 went live, every change in the diagnostic subsystem has become a breaking change since we throw these when verifying the container.

Nevertheless this is a bug IMO and thus should be fixed.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jun 13, 2016

it doesn't add anything except a useful message, because this should also throw a ambigious lifestyle mismatch?

Correct. This lifestyle is a mix of an Ambiguous Lifestyle and a Container-Registered Component. I think the Ambiguous Lifestyle warning has been added at a later point in time, and is a superset of the Short Circuited Dependency (I'm not sure I ever realized this). This does make the Short Circuited Dependency warning itself ambiguous :-). It could be removed without breaking any applications (although the warning message could be useful).

So in its current form, this warning isn't useful (except that it more precisely describes the error). So it should either be removed or fixed. There are two possible fixes: we make sure that it warns when decorators get applied, or we also warn in the case of equal lifestyles as well. Both options are breaking changes. The latter might possibly affect more users than the former does; it could cause more false-positives.

@TheBigRic

This comment has been minimized.

Collaborator

TheBigRic commented Jun 13, 2016

we also warn in the case of equal lifestyles

Does this really add something, because isn't this the same as Torn Lifestyle?

So the question is does the Ambiguous and Torn lifestyle warnings incorporate decorators? And if not, is it possible to add this? That would make the Short Circuited dependency obsolete.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jun 13, 2016

So the question is does the Ambiguous and Torn lifestyle warnings incorporate decorators?

The following configuration will cause an Ambiguous Lifestyles error:

container.Register<ILogger, MyLogger>(Lifestyle.Scoped);
container.RegisterDecorator<ILogger, LoggerDecorator>(Lifestyle.Transient);
container.Register<MyLogger>(Lifestyle.Transient);
container.Verify();

The following will cause a Torn Lifestyle warning:

container.Register<ILogger, MyLogger>(Lifestyle.Scoped);
container.RegisterDecorator<ILogger, LoggerDecorator>(Lifestyle.Transient);
container.Register<MyLogger>(Lifestyle.Scoped);
container.Verify();

So to answer your question: yes, they work independently of whether decorators are applied or not.

Does this really add something, because isn't this the same as Torn Lifestyle?

What this adds is that the error will pop up more often, because typically you don't want to have short-circuited dependencies, even if both the registration for the abstraction and the unregistered concrete type are transient. So adding this prevents that possible misconfigurations stay unnoticed, because of any transformations or interception that might take place. For instance, we might explicitly check for decorators, but any type of transformation (using ExpressionBuilt) could have taken place. It might be tricky to detect them and it might be hard to communicate proper error messages.

@TheBigRic

This comment has been minimized.

Collaborator

TheBigRic commented Jun 13, 2016

What this adds is that the error will pop up more often

In that case it seems valuable.

yes, they work independently of whether decorators are applied or not

Yes independently, but what I meant was: Does the check take the decorator into account including it's lifestyle. But it doesn't, because otherwise this config should throw an Ambigious Lifestyle and not a Torn Lifestyle.

container.Register<ILogger, MyLogger>(Lifestyle.Scoped);
container.RegisterDecorator<ILogger, LoggerDecorator>(Lifestyle.Transient);
container.Register<MyLogger>(Lifestyle.Scoped);
container.Verify();

I think however, especially with the possible changes in ExpressionBuilt, catching 100% is impossible.

@dotnetjunkie dotnetjunkie modified the milestones: v4, v3.2 Jun 23, 2016

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Jan 9, 2017

I decided to implement the strict behaviour, i.e. a short circuited dependency is always reported, even though both the unregistered and the registered type have the same lifestyle. This will cause the warning to go off when with Transient registrations and decorators, which gives the user a very clear description of what is going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment