autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.48k stars 836 forks source link

Make IDecoratorContext extend IComponentContext. #1352

Closed alistairjevans closed 2 years ago

alistairjevans commented 2 years ago

Fairly simple change this one; added a test to prove resolve from decorator conditional is possible.

Also added a test to prove that if a decorator resolves itself from inside the conditional, it throws a circular dependency exception (rather than stack-overflowing or something).

Closes #1338.

codecov[bot] commented 2 years ago

Codecov Report

Base: 77.93% // Head: 77.98% // Increases project coverage by +0.05% :tada:

Coverage data is based on head (ed66591) compared to base (3465490). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1352 +/- ## =========================================== + Coverage 77.93% 77.98% +0.05% =========================================== Files 195 195 Lines 5583 5587 +4 Branches 1119 1119 =========================================== + Hits 4351 4357 +6 + Misses 717 716 -1 + Partials 515 514 -1 ``` | [Impacted Files](https://codecov.io/gh/autofac/Autofac/pull/1352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac) | Coverage Δ | | |---|---|---| | [...rc/Autofac/Features/Decorators/DecoratorContext.cs](https://codecov.io/gh/autofac/Autofac/pull/1352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRmVhdHVyZXMvRGVjb3JhdG9ycy9EZWNvcmF0b3JDb250ZXh0LmNz) | `100.00% <100.00%> (ø)` | | | [...Autofac/Features/Decorators/DecoratorMiddleware.cs](https://codecov.io/gh/autofac/Autofac/pull/1352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRmVhdHVyZXMvRGVjb3JhdG9ycy9EZWNvcmF0b3JNaWRkbGV3YXJlLmNz) | `90.47% <100.00%> (ø)` | | | [src/Autofac/Util/SequenceGenerator.cs](https://codecov.io/gh/autofac/Autofac/pull/1352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvVXRpbC9TZXF1ZW5jZUdlbmVyYXRvci5jcw==) | `100.00% <0.00%> (+28.57%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

tillig commented 2 years ago

The only question I have on this one is whether it's actually interesting to have IDecoratorContext implement IComponentContext or if it's enough to just have a new property that exposes the component context.

That is, instead of IDecoratorContext : IComponentContext it's public IComponentContext ComponentContext { get; private set; }

I don't really feel strongly one way or the other, just thought it reasonable to ask the question. Having it derive does provide a shorter syntax for resolution, but it also sort of breaks down the "composability" notion - instead of composing two things together, the two things are sort of merged. Reminds me a little of how ASP.NET has things like ControllerContext where there are convenience properties to access HttpContext things, but ControllerContext is not, itself, an HttpContext.

alistairjevans commented 2 years ago

I do understand your point; I was trying to match the form of IComponentContext being passed directly to the delegate activator. It feels like users may "expect" (citation needed) that a "context" object passed to a mid-resolve method have direct Resolve methods. To be honest, the reason I agreed with the original issue fairly quickly is that it felt "natural" that IDecoratorContext should be able to resolve, and I was sort of surprised that it couldn't.

I think there are two differences between this and HttpContext composability.

First is perhaps about the complexity/size of the HttpContext contract? In Autofac, the IComponentContext contract is actually pretty tiny; just one property and one method. HttpContext is 12 total? I don't have a firm line for "this is too big a contract to add to a type", but I guess I know it when I see it?

Second, HttpContext is an abstract class, so deriving from it limits classes from deriving from a different, more appropriate, base type. Not the case with interfaces. I think I'd lean towards composability if our IComponentContext was actually an abstract class.

tillig commented 2 years ago

Sold. Seems like a reasonable way to go.

tillig commented 2 years ago

I'll let you delete the feature branch so it doesn't disappear out from under you.