Change WindsorServiceProviderFactory to follow SOLID behaviour

[generik0]

• Removed ServiceCollectionExtensions as code was related only to logic utilized by ServiceProviderFactory
• Added ServiceProviderFactory Base with small methods all virtual to allow override
• Rename to ServiceDescriptorExtensions for CreateWindsorRegistration

#535

[generik0]

@ltines @jonorossi please consider this change. I have made the IServiceProviderFactory follow more a SOLID approach and contained relevant logic fo the IServiceProviderFactory in a baes class.

I think this change is very important for the openness of the package. I had the same discussion with Gary when making aspnetcore2.x. It was also closed initially but was opened by demand later. Maybe this time best to allow substitution from the start?

[generik0]

@jonorossi @ltines After converting one of my largest apsnetcore apps. That uses a multitude of special features from aspnectore. I have added extra methods in HostBuilderExtensions to allow the openness needed to control the container and factory.
Any thoughts?

Missing tests. Waiting for dialogue about changes. However without this for of an update, i cannot see that the castle implementation will allow migration to 3.x of larger aspnetcore applications.
This openness and control of pre-serviceprovider-build registrations will also give the castle implementation the competitors cannot... I.e. you can use the container for all the service collection registrations.

[generik0]

@ltines any thoughts on this PR.
Personally I need the container before ConfigureContainer is called. I also need it when ConfigureServices are called.
This fixes this issue

[generik0]

@jonorossi @ltines
Please, should we have a dialogue about these changes?

I know the current implementation will give many issues, as we need access to the container before the service collections are made.

Some service collections additions use options that require usage of class implementations (eg modelbinder)
For this reason it is optimal to already have implematons in the container, so we have control already of what is being used.
Also, eg the cookie options can need implementations (eg for a factory).

Thoughts please?
In sort, I don't see any reason why we shouldn't open up the implementation for flexibility to allow access to the container initially...

Thought please, I know I will not be able to use the full DI without this change (so far I have forked and created own package)

[jonorossi]

@generik0 I'm happy to facilitate changes to Windsor that the community wants, however I don't have time to be involved in the specific direction of this extension project, I won't be commenting on the specific changes. If @ltines is no longer interested or doesn't have time to be involved with this extension library, then I recommend getting at least one other person involved, there appeared to be interest from many users in the issue.

[AntonioDrusin]

At my company we are at a crossroads now with Castle Windsor and Asp.Net Core. Due to how the two libraries are no longer working well together, we have to either support CastleWindsor or migrate our IoC to MS.
Looking at https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-3.1#request-services it seems that Microsoft does not even list Castle Windsor as a supported IoC for ASP.NET Core.

[generik0]

Hi @AntonioDrusin it does support aspnetcore 2.x and now 3.x
I have been working with it the last weeks, and this new extension is very very good (especially after my changes ;- )

Everything is working. NodeService, Authentication, SignalR, swagger, rest api, it all!

The package is in beta because we still need people like you and you company to try the beta package...

If you need any help, just let me know ;-)

PS, with these changes I have made, the container is usable before the service collections are built. Meaning more versatility than any other DI that can only be "conforming"
This package can now be a "conforming" DI and a little more ;-)

[AntonioDrusin]

@generik0 ok, let me pull this branch and give it a test. One of my main issues was the lifetime of transient services. I'll see what it looks like in this branch. Thank you.

[generik0]

@AntonioDrusin the prerelease is also on nuget.org, so you can install the current beta from there. However, this does not give access to the container in the servicecollection registration in startup.

What is your worry about the transient? For me it works as standard...

[AntonioDrusin]

I filed #530 about that. In a simple proof of concept it does not look like transient dependencies of controllers get released when the controller gets released.

[generik0]

Very interesting @AntonioDrusin i did not catch this before
When I get a chance I will need to look at it....
I agree with you that the transients should be release when the controller/scope is release...

[AntonioDrusin]

I agree with your changes @generik0. There are a lot of important choices made in the WindsorServiceProviderFactoryBase class. I feel it is important to allow clients of this library to adjust subresolver behavior and the scope behaviors that are determined by that class.
It is a good change to put all the windsor container levers in one place.

[AntonioDrusin]

With this change we probably want to update docs/net-dependency-extension.md to cover the way the new configuration is open to change.

[AntonioDrusin]

I am happy to volunteer to provide an update to the md file if you want.

[AntonioDrusin]

@jonorossi I am looking at this extension to use within my company's microservices. While I can probably use the vanilla version of this extension it would be preferable to have the more extensible version that this PR provides. Are there any steps we should follow to get this PR merged? Thank you.

[generik0]

@AntonioDrusin please Update .md If you have time.
I am Justine my b@&$$s right now to make a release (That is still on 2.2)
Any help would be Great!!!!

[generik0]

@jonorossi please note. I have tested the facility on a rather large web solution with aspnetcore 3.1 + angular (newest).
With signlar, special cookie authentication, gzip, binders, special rest controllers and routning (for spa and mvc controllers)

I need to open the facility up like this to get it to work.
However the facility and these changes are tested to be Working.
(Except for the release issue @AntonioDrusin has talked about That still needs Looking into).

If @AntonioDrusin (or I) improve the readme, can we please get it into master. We need this change! :-/
@jonorossi ? :-)

[jonorossi]

@AntonioDrusin no idea really. As I commented earlier in this thread I'm happy to do a final code review and get the changes released, however don't have time for the back and forth to work out what it should actually do and fit into the Microsoft Extensions project. Since there appeared to be some disagreement on how this facility should work I asked in that comment above for one other person who is actually planning to use this be involved to review these changes and then we can move forward.

If it isn't clear, what I'm asking for is for people who actually want to use this stuff to actually review it and see if it works with their project, I'm no one special, don't underestimate your ability to contribute, nothing is perfect but without contributions (in the form of code, code reviews, bug reports, documentation, etc) things don't move forward. Everyone should have GitHub permissions to comment on the code, I just want to see a seconder that is happy with the changes.

[jonorossi]

@generik0 there is still a broken unit test with these changes:

Castle.Windsor.Extensions.DependencyInjection.Tests.WindsorScopedServiceProviderTests.DisposesInReverseOrderOfCreation [30ms]
  Error Message:
   System.ArgumentOutOfRangeException : Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
  Stack Trace:
     at Microsoft.Extensions.DependencyInjection.Specification.DependencyInjectionSpecificationTests.DisposesInReverseOrderOfCreation()
Test Run Failed.
Total tests: 146
     Passed: 145
     Failed: 1
 Total time: 4.5018 Seconds

[generik0]

@jonorossi i found the issue. I had added an extra set off UTests. Using the DependencyInjectionSpecificationTests. Unfortunately looks like the static "stuff" in implementation (original) does not allow for 2 tests. something must be common for the test assembly. Beyond my control. Not an issue with code. Prob issue with MS Tests

[generik0]

@jonorossi apart from doco Update, is there anything missing before we can completed this merge?

Please note. I have NOT changed any of the original logic. Just refactored away from using extensions, and adding opened up to allow users to use there own controller or get the container created by the facility.

[jonorossi]

@generik0 sorry for being so slack responding to this, a bit going on this end. Looks great, I assume you'll submit another pull request for documentation and changelog updates.