Skip to content
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

decoration Issue#91 #169

Merged
merged 17 commits into from
May 23, 2022
Merged

Conversation

DanHarltey
Copy link
Contributor

@DanHarltey DanHarltey commented May 20, 2022

Hi,

I realise this is a fairly large pull request, sorry. It does represent a couple of days' work. It is all around the decoration functionality, I hope this change improves the test coverage, code maintainability, allows better support for open generics and fixes issue #91.

The first three commits are just adding more unit tests. I felt I needed a few more tests to make sure I didn’t break anything and to help me understand the code.

The commit 412186f is where the bigger changes start. This commit refactors ServiceCollectionExtensions.Decoration.cs. Separating out the logic into more composable classes. This allows removal of the repeated logic. It does not change any behaviour, all tests remain the same.

In 6224128 a small code change now allows more of the existing methods to support open generics. The below four methods now support open generics:
IServiceCollection Decorate(this IServiceCollection services, Type serviceType, Func<object, IServiceProvider, object> decorator)
bool TryDecorate(this IServiceCollection services, Type serviceType, Func<object, IServiceProvider, object> decorator)
IServiceCollection Decorate(this IServiceCollection services, Type serviceType, Func<object, object> decorator)
bool TryDecorate(this IServiceCollection services, Type serviceType, Func<object, object> decorator).

Fix for issue #91

I was surprised when I learnt of this issue. In a previous role we found it when we noticed our redis connections were not getting disposed as expected giving us far more connections than needed.

The last commit is a fix for issue #91. It is a large change altering the strategy of how the classes are decorated. To allow the dependency injection to be in charge of disposing the decorated class at the correct time they remain in the ServiceCollection instead of getting removed from it.

To avoid both the decoratored and decorator (and nested decorators) from matching the same type in the service collection, the type kept within the ServiceCollection is altered to a different type (DecoratedType.cs). This DecoratedType implements object reference equality to only match the same object reference. The object reference is kept within the decorators ServiceDescriptor factory allowing it to request the object from the DI directly ensuring that the DI is responsible for disposal.

I will try to get back to you as quickly as possible for any questions or alterations. If my response time is a bit slow, I am still interested, but just finding spare time can be hard sometimes.


namespace Scrutor.Decoration
{
internal class DecoratedType : Type
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a crazy hack. I love it! 😍

@khellang
Copy link
Owner

khellang commented May 23, 2022

Hey @DanHarltey! 👋🏻

Thanks for the PR. Normally I would shoot unannounced PRs at this size down pretty quickly, but this one looks very clean and I like the concepts and the long-overdue decoration cleanup. The added (and passing existing) tests also helps my confidence in this change 😅

I had time for a quick first pass. It looks very good, but I've left a few comments, mostly on naming and layout of the APIs. I'll probably go through an OCD stylistic pass after merging, so I won't bother you with those details.

@khellang
Copy link
Owner

We should also add a test showing that service descriptor order is preserved. It looks like the new code might shuffle the order around a bit?

@khellang
Copy link
Owner

Ah, I guess it doesn't matter as the services added to the end of the collection has a different ServiceType (wrapped in DecoratedType) and won't be resolved anyway?

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #169 (85c030e) into master (c869d54) will increase coverage by 1.56%.
The diff coverage is 61.94%.

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   63.36%   64.93%   +1.56%     
==========================================
  Files          19       25       +6     
  Lines        1182     1289     +107     
==========================================
+ Hits          749      837      +88     
- Misses        433      452      +19     
Impacted Files Coverage Δ
src/Scrutor/Decoration/DecoratedType.cs 8.79% <8.79%> (ø)
src/Scrutor/ServiceDescriptorExtensions.cs 85.71% <85.71%> (ø)
.../Scrutor/Decoration/ClosedTypeDecoratorStrategy.cs 94.73% <94.73%> (ø)
...Scrutor/Decoration/OpenGenericDecoratorStrategy.cs 97.77% <97.77%> (ø)
src/Scrutor/Decoration/Decorator.cs 100.00% <100.00%> (ø)
src/Scrutor/Decoration/DecoratorInstanceFactory.cs 100.00% <100.00%> (ø)
.../Scrutor/ServiceCollectionExtensions.Decoration.cs 100.00% <100.00%> (+39.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c869d54...85c030e. Read the comment docs.

@khellang
Copy link
Owner

I'll just go ahead and merge the changes and do some cleanup on master. I have some bigger changes to the codebase coming up, like moving to file-scoped namespaces etc., so I want to get this in first. Thank you so much for this change! 😘

@khellang khellang merged commit 5067e34 into khellang:master May 23, 2022
@khellang
Copy link
Owner

Thanks again for the PR @DanHarltey! This has been released to NuGet as part of v4.2.0. I ended up refactoring your code slightly to introduce DecorationStrategy as a first-class extensibility point, which does the majority of the decoration work. As a result, the code is much cleaner and easier to read 😍 Hopefully there aren't any regressions 😅 🤞🏻

@DanHarltey
Copy link
Contributor Author

Ah, I guess it doesn't matter as the services added to the end of the collection has a different ServiceType (wrapped in DecoratedType) and won't be resolved anyway?

Hi, yes that is correct. The ServiceDescriptors added at the end of the collection are wrapped in DecoratedType and should never be resolved. The order is maintained by replacing the original service with the decorator like it used to do.

@DanHarltey
Copy link
Contributor Author

Hi @khellang, Super happy this got merged. It was really quick as well. No worries about refactoring, make it your own. Hopefully no issues come up with the new release. I will keep an eye on the GitHub issues and try to help in any popup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants