WIP - Replace IObjectsFactory with IServiceProvider interface

[maca88]

Here is an attempt to replace the IObjectsFactory with IServiceProvider interface based on #1781.
As discussed, with IServiceProvider we can get the service by the base type or interface without worrying whether the service is registered or not as it is designed to return null in case the service is not registered. I've added a default IServiceProvier implementation that has also the ability to register services in case the user does not use any IoC container and wants to pass a service instance. In addition, an additional method PropertiesHelper.GetInstance was added in order to be used by other NHibernate projects when instantiating a service via configuration properties.

[maca88]

This class should not be generated as there is nothing async, it is a bug related to async generator that I need to fix.

[maca88]

I released a new version of the async generator (0.8.2.10) that fixes the issue of generating non async methods.

[hazzik]

Let's hold-on on this for a moment. The PR is a good starting point, but, I think, we need to discuss what we want to achieve and what direction shall we go.

[maca88]

what we want to achieve

The aim of this PR is to achieve the ability to configure services like ICacheProvider by an IoC container without setting any configuration property.

what direction shall we go

With #1781 the IObjectsFactory will contain only one non-obsolete method that has the same functionality as the built-in IServiceProvider.GetService method, so I think the replacement of IObjectsFactory with IServiceProvider is the way to go.

[fredericDelaporte]

I have rebased this on master by the way.

[fredericDelaporte]

Here is a series of fix proposals addressing my review comments.

[fredericDelaporte]

While looking into this, I have spotted a number of cases which will not play nice with dependency injection, due to not using the service provider, or forcing a default concrete class:

• CreateLinqQueryProviderType
• ConstructConverter
• AbstractBytecodeProvider.CollectionTypeFactory
• ConfigureProxyFactoryFactory

We should likely rework those cases too, probably in a dedicated PR.

[maca88]

@fredericDelaporte thanks for fixing the mentioned issues, I am ok with them.

[hazzik]

Hi all. I think the registration/container building logic should not be there.

[maca88]

I've removed the registration logic and renamed the class to ActivatorServiceProvider, as I agree that it is out of scope about what NHibernate as an ORM library should provide.

[hazzik]

Was thinking about this approach and found it might be inconvenient for an end user. For example, using other service provider other than supplied, an IoC/DI container which requires explicit components registration, for example, would require the user to register all NH services. I don't think that this is a desired behaviour.

I think we need to have some fallback mechanism, eg fallback to Activator.CreateInstance if IServiceProvider.GetService returns null. Thoughts?

I don't want to bring additional dependencies (eg Microsoft.Extensions.DependencyInjection) nor provide an ability to register the components (we're not building DI container after all).

[maca88]

an IoC/DI container which requires explicit components registration, for example, would require the user to register all NH services

I didn't known that some IoC containers like Castle.Windsor require explicit registration for concrete types, thanks for pointing this out.

I think we need to have some fallback mechanism, eg fallback to Activator.CreateInstance if IServiceProvider.GetService returns null. Thoughts?

Yes, I also think that we should add a fallback mechanism that should try to instantiate concrete types with Activator.CreateInstance when ServiceProvider.GetService returns null. I will update this PR to reflect that.

[maca88]

I've added the fallback only for NHibernate internal types and leaved the strictness for external types in order to respect the IoC container rules. If you think that we should have a fallback also for external type, I will change that.
Update:
The fallback is added for all types in order to avoid registering also types from NH family projects like NHibernate.Caches.

[fredericDelaporte]

@hazzik, do you agree for merging or do you wish to have another look yourself before?

[hazzik]

I'll take another look.

[fredericDelaporte]

Postponing this to 5.3 is quite unfortunate. This PR changes public interfaces which were not yet released. Delaying it will cause more breaking changes to dodge, and more members to flag as obsolete, instead of just dropping them directly.

[hazzik]

PR changes public interfaces which were not yet released

Which exact interface are you talking about?

[fredericDelaporte]

I am talking about "interface" in its broad meaning, not just the technical C# interface:

• Removal of class HibernateObjectsFactoryException.
• Removal of setting objects-factory, replaced by service-provider.
• Removal of HibernateConfiguration.ObjectsFactoryType property, replaced by ServiceProviderType.
• Removal of Environment.PropertyObjectsFactory constant, replaced by PropertyServiceProvider.
• Removal of Environment.ObjectsFactory property, replaced by ServiceProvider.
• Removal of Environment.BuildObjectsFactory method, replaced by BuildServiceProvider.

[hazzik]

I do not want this to go into an opposite direction of #1781 but rather in the same and add value rather than replace. What it means is that, in my mind, it would be better if IServiceProvider is hidden behind the IObjectsFactory, not replace it.

[fredericDelaporte]

I do not see it as going into an opposite direction. For me, it is instead the same direction: removing specificities hindering dependency injection. I do neither see where is the added value in keeping an IObjectsFactory hiding away IServiceProvider.

I see a value for the case of Microsoft ILogger, because that interface lies in an extension, requiring an additional dependency people might not want. But IServiceProvider is instead in the standard.

The only value, that IObjectsFactory was having, was its obsolete methods, supporting cases not allowed by IServiceProvider. But these cases were removed specifically because they are not supported by many dependency injection frameworks. Sticking to IServiceProvider will avoid re-introducing new features not widely supported, for a thing that is a very basic need and which does not need fancy features.

It will also allow users to directly use any dependency injection framework supporting IServiceProvider, instead of having to code some boilerplate class encapsulating it in NHibernate custom IObjectsFactory. (Or instead of having NHibernate supply such a class, with then two ways of configuring DI: supplying a custom IObjectsFactory or supplying a IServiceProvider to be used by the default objects factory. Having two ways of doing the same thing is always a bit confusing.)

In my view, if IObjectsFactory is to stay while also directly supporting IServiceProvider, that is IObjectsFactory which should be hidden away from the user. It would be only internally used for resolving dependencies, while the user could setup its own IServiceProvider to be used by NHibernate "internal" objects factory.

[fredericDelaporte]

Switched to WIP: since 5.2 has been released, this PR has now binary breaking changes.

[maulik-modi]

@fredericDelaporte , @hazzik and @maca88 , This is something very important to embrace MS DI Abstractions and be conformant