To better-integrate with DI, developers should be able to use an IServiceProvider from a Configuration

[craigfowler]

I recently discovered that NHibernate is more-capable-than-I-had-thought of supporting dependency injection (to a degree as far back as NH4).

Background

The static Environment object contains an object resolver of sorts: IObjectsFactory. That objects factory is used to resolve a number of services used by NHibernate, such as connection providers. Looking at the API of that interface, it is almost identical to System.IServiceProvider, particularly once the obsolete members are removed. I found that - in my own app/usage of NHibernate, I was able to substitute the objects factory with my own custom object which wraps a service provider. If the service provider is able to resolve the desired type then return it, and if not, fall back to Activator.CreateInstance.

How I'd like to improve upon this

Whilst it would be great in the long-term if NHibernate fully supported DI in all its glory, that's a bit unrealistic to ask for all at once. I think a good first step would be:

• Replace all usages of the IObjectsFactory interface with System.IServiceProvider
• Move the property for the objects factory from the static Environment object to the Configuration object
  • From that point, it will become a part of the immutable session factory

Use-case example

This is just to help show how this is actually helpful in the real world. In my specific use-case, I develop for a multi-tenant application. Each tenant has its own database/connection string, but every tenant/DB has the exact same schema. The running application has just one session factory, but in the configuration we set-up a custom IConnectionProvider. That connection provider needs to be able to determine which tenant is currently active in order to return a connection to the appropriate database.

The app has services available via DI to determine which tenant is currently active but so far the NHibernate connection provider has needed to achieve all of this statically. Using the default IObjectsFactory impl, the connection provider can only be instantiated from a public parameterless ctor. In fact with an objects factory which supports DI, the connection provider may have dependencies injected and the static logic can be removed.

Whilst this can already be done (by replacing the objects factory on the Environment), this would be easier to work with if:

• The interface were the more recognisable service provider
• It were part of the configuration and not a static/singleton environment object

I appreciate that this is a breaking change

Obviously-enough, this change involves alterations to the public API, which would mean it would need to be held for a major version increment.

[fredericDelaporte]

This is a work in progress, a bit stale for now. See #1793.

[fredericDelaporte]

And maybe we should consider this issue as a duplicate of #919.

[craigfowler]

Ah thanks for that. It seems that #1793 stalled because there wasn't agreement about the dev approach at the time. There's certainly no point my creating a PR of my own (or working on that stale one) if it's going to be rejected on the fundamentals, so let's have a chat about the options first?

Ideas to do this

I can see a few ways of going-about this, covering a quite wide range of options and scope/commitment to change. This probably also touches on the future plans for the shape of NHibernate, so it's probably quite an important discussion to have. Here's the possibilities I have in-mind. Sorry this is a bit long; I hope you like reading.

Do nothing

This is the baseline; we leave IObjectsFactory alone and go for an early lunch.

Switch for service provider but leave functionality as-is

Here, we essentially just replace IObjectsFactory with IServiceProvider. We don't change how any of that works though. We do a straight-rename of ActivatorObjectsFactory to ActivatorServiceProvider and leave its functionality untouched. We then drop the IObjectsFactory interface because everywhere it was previously-used has been replaced.

• Pro: It's a step in the right direction - devs using NH are more likely to recognise IServiceProvider and see that they can substitute it with their own
• Pro: It's a cheap change to make
• Con: Devs using NH won't want to (or might not be able to) register all of NH's internal services (created via that factory/service provider) in their own container. This means they'll have to add some boilerplate of their own (a decorator class for IServiceProvider?) to fall-back to Activator.CreateInstance.
• Con: The .NET ecosystem isn't really moving in this direction

Switch for service provider and create our own fall-back to Activator

This is almost identical to the suggestion above, but NH is responsible for providing that decorator class so that if the provided IServiceProvider fails to resolve an object, NH falls back to Activator.CreateInstance.

Inherits all pros/cons from the idea above, except:

• Pro: Devs don't need to worry about registering NH internals with their DI container or providing extra boilerplate.
• Con: We are altering the overall functionality of what we were given.
  • That's not exactly terrible but it might mean that we end up suppressing/altering DI resolution exceptions if they occur in a user-created component.
  • If there's a DI error whilst resolving something of their own (which was resolved as a dependency of something consumed by NH), we would end up throwing the Activator.CreateInstance exception about not having a public parameterless ctor which would be misleading.

Register all of our own dependencies and stop using Activator

This is like the scenarios above except rather than falling-back to Activator for things which aren't registered, NHibernate becomes responsible for registering its own types with a container. The standard way that kind of thing is done is via an extension method for IServiceCollection used like: serviceCollection.AddNHibernate();. That's called during app startup when DI is being configured.

If NH were dependent upon a DI container being provided by the consuming app (a lot of modern libraries are now) then we would only need to depend upon Microsoft.Extensions.DependencyInjection.Abstractions. However, that would tie NH to this approach and might make it difficult for older apps (which don't use these techniques) to upgrade. If we want NH to always be able to bootstrap itself (when the consuming app does not provide a container), then if we're "going full DI" then we need to create a container of our own. The logical choice would be the simple ServiceContainer from the full Microsoft.Extensions.DependencyInjection package. IE: If the consuming app provided a container then use it, if not then we create our own container instance.

Some libraries are mitigating the dependency-bloat by splitting themselves up into multiple NuGet packages. One package contains everything needed to bootstrap the library/framework in a DI environment (thus many dependencies). That package is referenced only by the consuming application project (which sets-up DI). For actual usage in the application's library projects, a smaller/slimline consumption-only package would be installed/referenced, which has minimal dependencies. This is in fact a little like the differences between MS Extensions DI and the related Abstractions package. In a similar way, NH could make the ability to bootstrap itself without a provided container elective, moving this into a separate package. Legacy apps which can't provide a container of their own install that "bootstrap" package in order to get a self-hosting NHibernate? Otherwise there's no need to depend on anything but the MS Extensions DI Abstractions package.

It's perhaps something to consider, although I'd fully accept an answer like "No, this doesn't fit with NHibernate's design principles".

• Pro: This is becoming (has become?) the standard approach in the ecosystem, along with techniques like .NET Generic Host
• Con: NH would require new dependencies
• Con: It's quite a significant architectural change which impacts the future of the library

Optional extra: Move the object resolver to Configuration

This is a more expensive change and could probably be combined with any of the ideas above (I suppose it's kind of assumed with the last one). Currently, the objects factory is used via a static reference and so it's more like a service locator than a service provider. Moving it to the configuration (and ultimately into the session factory) seems better for passing it forward to the things which need it.

Doing that might also give NHibernate a transition path to "going full DI" if that's desired in the longer term future?

Overall

It seems like we have a bit of a triangle of priorities:

• Avoiding extra dependencies
• Supporting DI
• Maintaining simplicity

So far the Supporting DI priority has lost out in favour of the other two. I personally think it deserves a little bit more love in 2021, but can we find a balance between those 3 which is good for NH?

Also, @fredericDelaporte - is there anyone else who should be brought into this discussion? I'm not entirely sure who's current and who has 'retired' on the NH team.

[fredericDelaporte]

@maca88, @hazzik, @bahusoid, @gliljas, @oskarb

[maca88]

Replace all usages of the IObjectsFactory interface with System.IServiceProvider

This is a tough one, which I am not really sure what should be done. Despite IServiceProvider being a very minimalistc interface it has a flaw, which in my opinion is not small. As you mentioned, IServiceProvider will suppress any resolution exception that may be caused by an invalid registration, which would end up throwing a misleading exception. Such mechanism essentially removes the diagnostic and verification abilities that some DI containers have, which can very precisely explain what went wrong and possible solutions for fixing the issue, to avoid the developer from spending time debugging. By having a default fallback which was done in #1793, could make things even worse as in case the DI would fail to resolve the type due to an invalid registration, a default implementation would be returned which can cause unexpected behaviors that may be hard to debug. I agree with this post and I am leaning towards providing only an optional Func<Type, object> ServiceProvider setting, which would have to be able to resolve all NHibernate services. By doing so, the developer would be able to enjoy all features of the choosen DI container, but at a cost of providing the registration of every NHiberante service. To solve this issue, we could create a code generator that would generate registrations for the most popular DI containers and optionally publish them on Nuget. In case the ServiceProvider setting would not be provided, an internal DI container could be used to resolve the dependencies to preserve the current functionality.

Move the property for the objects factory from the static Environment object to the Configuration object

I completely agree on this one, having a static service provider is considered to be an anti-pattern, which I do agree. Not only it hides class dependencies, it also prevents us from having different configurations for each session factory in case having multiple databases. This is the second thing that I don't like about #1793, which forced me use this pattern in order to avoid breaking changes and it is probably for the better that it hasn't been merged.

The standard way that kind of thing is done is via an extension method for IServiceCollection used like: serviceCollection.AddNHibernate();

I am in favor of having such method, but we need to think about how such method will work in case of having multiple session factories. Registering ISession, ISessionFactory or Configuration multiple times will not work, so we will have to find a way to solve this issue. In case having only one session factory the configuration is pretty straightforward (example), but in case of multiple databases we would need to create custom classes (e.g. FooSession, BarSession) that we could register (example).

If NH were dependent upon a DI container being provided by the consuming app (a lot of modern libraries are now) then we would only need to depend upon Microsoft.Extensions.DependencyInjection.Abstractions.

Due to the issues described in the post linked earlier, I am not in favor of being dependent on it.

[hazzik]

Switch for service provider but leave functionality as-is

I'm not sure what exactly this switch is going to achieve, can you elaborate? It is super easy to wrap IServiceProvider into an instance of IObjectsFactory. And there are several projects who has done it.

Optional extra: Move the object resolver to Configuration

That is a gigantic task which probably could not be performed in our lifetime.

One huge caveat in the DI/SP task is that noone knows what classes NHibernate instantiates. Some of them are fairly random. That is because when IObjectsFactory was introduced it just replaced all usages of Activator.CreateInstance with objects factory call. That is also part of a reason of having static access to it.

IMO, first step would be to identify all usages of IObjectsFactory.CreateInstance. Then, probably, group them into some groups and start gradual refactoring into the desired state. Some of these CreateInstance should probably be moved back to using Activator as it does not make sense to use DI for them.

Then, there are also some classes which could be provided by user at the mapping stage. What do we do with them? Howdo we handle them?

[craigfowler]

Yikes, this reply got longer than I expected! Apologies again for the Wall of Text™.

IServiceProvider isn't as weak as we thought!

@maca88 wrote:

As you mentioned, IServiceProvider will suppress any resolution exception that may be caused by an invalid registration, which would end up throwing a misleading exception.

You know, until 5 minutes ago I would have agreed with you 100%. Stated in black-and-white like this I couldn't believe that the IServiceProvider abstraction could be that bad. On a hunch I did some digging around in the source code for the GetRequiredService<T> extension method and I discovered the ISupportsRequiredService interface. As noted in the linked docs, this has existed all the way back to version 1 of Microsoft.Extensions.DependencyInjection.Abstractions - we just didn't know about it. This interface is exactly for the scenarios in which we have a container which supports the "resolve or throw" behaviour.

In a very lazy and un-scientific look around, I also discovered that Autofac (my container of choice) does indeed provide an IServiceProvider impl which also implements ISupportsRequiredService, via an elective package which brings in the dependency on MS.DI.Abstractions. I haven't checked any other popular containers but I wouldn't be surprised if they did similar. So - some of this might not be as much of a problem as we had thought, so long as we are using the GetRequiredService extension method. That guarantees it will resolve or throw; the exception will be either a native exception from the container impl or in the worst case an InvalidOperationException with a reasonable message.

Also, I'm very pleased you linked that article. I'm not familiar with it but I am very familiar with Mark Seeman's "Conforming Container" article which that goes on to link. If only I could stumble across an ISupportsNamedScopes interface which I didn't know about. 😠

Code generation of DI registrations versus the abstraction

Again @maca88:

but at a cost of providing the registration of every NHiberante service. To solve this issue, we could create a code generator that would generate registrations for the most popular DI containers and optionally publish them on Nuget.

... and also a little later, about depending upon the MS DI Abstractions package:

Due to the issues described in the post linked earlier, I am not in favor of being dependent on it.

I'm sure code generation would work, but it seems quite a bit of effort and higher maintenance overhead. I do think that (if we can make it work acceptably) then it would be more desirable to stick to just the abstraction. That is a very big "if" though. Resorting to code generation & extra container-specific packages seems like the kind of solution I'd advocate holding in reserve.

In general where it comes to using the abstraction, I can see the concerns and to a degree agree with them, particularly for advanced projects. I frequently grumble that the built-in service provider & scope-factory does not allow for named/tagged scopes (like Autofac does). However, for what NHibernate has, we know that a "lowest common denominator" DI approach is suitable. We know this because right now it doesn't do anything which Activator.CreateInstance couldn't do.

There's also nothing stopping apps from taking a hybrid approach to DI. My day job has me working on an app where we do just this. We use Autofac-native DI registration for all of our own app services, but then a few other third-party libs that we have, we use their built-in registration functionality to add them to the IServiceCollection. Indeed, if we wanted (say we needed something more specific) we could choose not to use those libs' built-in registrations, we could do it via Autofac native registrations instead. We'd have to roll our own of course.

Providing functionality to register NHibernate's internals via an IServiceCollection (thus depending upon MS DI abstractions) is a convenience, but it doesn't have to become a diktat. The motivation for supporting it is that it gets us good coverage of all of the popular DI frameworks in one shot. We could independently choose to support other DI frameworks directly (perhaps with code-generation). If we pick & support individual containers though, we're not going to get anything like the coverage of potential consumers (for the same amount of effort) as if we support the abstraction.

What we could do would be to put MS DI Abstractions support into a different project/package. That would make the dependency fully elective. Alternatively we could add the dependency to NH core but still make it optional to actually switch it on or not. The consumer is welcome to use an Activator-powered IServiceProvider. if they want. They just need to understand that if they do then they can't use DI, but then they have already chosen not to!

There's a downside to supporting an Activator-powered IServiceProvider of course, in that it cuts us off from the benefits of DI. If we have to be compatible with Activator.CreateInstance then everything we wish to resolve must continue to have public parameterless constructors and as long as we support it, we can't go all-in with DI. IMO it would be a shame to limit ourselves that way forever. If we were to go for this, I'd suggest it being a transitional solution.

Just swapping for IServiceProvider without changing anything else is a weak change

@hazzik notes that:

I'm not sure what exactly this switch is going to achieve, can you elaborate?

Well it won't achieve very much at all. It's a very small step and the only real benefit is that there is a chance that more developers will recognise IServiceProvider and immediately know "Hey! This is the extension point where I could add DI!"

It's not a spectacular change to make; the only thing going for it is that it's cheap. I'm happy to take that idea off the table because I agree - it's not enough benefit to be worth doing.

Avoiding service locator anti-pattern

I agree with @maca88 on this. Also, @hazzik adds:

That is a gigantic task which probably could not be performed in our lifetime. // noone knows what classes NHibernate instantiates. Some of them are fairly random. That is because when IObjectsFactory was introduced it just replaced all usages of Activator.CreateInstance with objects factory call.

That's a totally reasonable assessment, and not an unreasonable history of how it came into being. Swapping hardcoded Activator for a factory interface is/was a good first step toward integrating DI. I've not yet done a find-references on the IObjectsFactory interface to know exactly how big the job is.

I'm not sure we need to worry about "knowing which classes NHibernate instantiates" though, since we can find out by using a find-references on that factory interface. I think the biggest hit is going to come with those who upgrade to the version of NH which includes these kinds of proposed changes. If they have substituted some of NH's classes with their own impls then they will need to begin registering those in their DI container from now-on.

We could mitigate that & provide like-for-like functionality by including a default IServiceProvider impl which wraps Activator.CreateInstance. I've already mentioned that, above. Actually I'm not sure this is a good enough justification to do that. Who would activate DI but then not want to register some of their injectable components with their DI container?

Dev approach to replace service locator

IMO, first step would be to identify all usages of IObjectsFactory.CreateInstance ...

99% agree with that dev approach, only sane way is to break it up into chunks of work that don't last forever.

The only thing where I'm not in total agreement is that my instincts put me more in favour of using DI for everything where we can. I'd only fall back to Activator if there's really no other workable solution. But - it's not really worth the disagreement, especially as I'm not super-familiar with the codebase; you're probably right. If it's too hard to migrate a service to DI, and there is little to gain from using DI with that particular service then sure - Activator it is.

Supporting multiple session factories with DI

@maca88 wrote:

I am in favor of having such method, but we need to think about how such method will work in case of having multiple session factories.

Yes, I don't think that there are any deal-breakers in the details of handling that. What really strikes me about apps that use multiple session factories is that it's hard for us to predict the details of their use-case, since it's already a fairly advanced scenario. It's probably best just to give them the tools to implement what they want. I've seen that approach of creating derived configuration/session factory/session objects before.

Possible generalised approach: Registry of session factories

An alternative to the "derived config/SF/sess types" approach, which definitely is easy to generalise, would be to introduce a registry for session factories. This would be registered in DI as a singleton. Something like:

public interface ISessionFactoryRegistry
{ ISessionFactory GetSessionFactory(string key); }

Ship with an impl that is just backed by an in-memory IReadonlyDictionary<string,ISessionFactory>. Then provide registration methods which allow a consuming developer to register delegates that create session factories, with a key:

public static IRegistrationBuilder AddKeyedSessionFactory(
  this IRegistrationBuilder services,
  Func<IServiceProvider,ISessionFactory> factoryDelegate,
  string key) { /* ... */ }

The type IRegistrationBuilder is a hypothetical type which might be used (as builder) in an example like the following (where services is an IServiceCollection):

services.AddNHibernate(builder => {
  builder
    .AddKeyedSessionFactory(provider => { /* Logic to create factory Foo */ }, "Foo")
    .AddKeyedSessionFactory(provider => { /* Logic to create factory Bar */ }, "Bar");
});

Then if the consuming app wants a session factory, they resolve ISessionFactoryRegistry and choose which one they want by key. The consuming app is more than welcome to wrap that registry (using DI!) in a service of their own which provides the API that their app wants. The consumer is also welcome to provide their own impl of ISessionFactoryRegistry if they didn't like the simplistic one.

Dealing with injection at mapping-stage

@hazzik asked:

there are also some classes which could be provided by user at the mapping stage. What do we do with them? Howdo we handle them?

Assuming I understood your question correctly, the traditional approach here is to provide a resolution delegate at registration-time, along the lines of the following. I'm going to use Configuration as an example, but the approach is fairly generalised.

container.AddSingleton(serviceProvider => {
  var config = new Configuration();
  // Obviously I'm making this up, but you get the idea
  config.AMappingService = serviceProvider.GetRequiredService<IDoesAMappingJob>();
  return config;
});

In the same manner, you could resolve a service which contains the impl for actually adding your mappings. If you are actually wanting to set an AQN for a type (or even just a System.Type) into the config then that's fine because it would be resolved later.

For the technique above, we could perhaps provide a fluent-builder way to conveniently specify these things. I'm not completely certain that this would be worthwhile though. The API of a Configuration object is already good and not more complex than it needs to be. Probably just stick with the delegate.

Phew! I think that's everything answered! Further feedback/questions welcome 😁

[bahusoid]

The running application has just one session factory, but in the configuration we set-up a custom IConnectionProvider.

Are you aware there is IMultiTenancyConnectionProvider for your needs since NHibernate 5.3? See #2108

[gliljas]

Thanks for bringing this topic up. It's very interesting and there are certainly things that can be done better. IObjectsFactory vs IServiceProvider is not (yet) a point of importance to me, and I'm a bit ambivalent whether the common IServiceProvider abstraction is good or bad for .NET. However, the work required to make NHibernate a better "DI player" could most certainly be very beneficial to the overall code quality, where by "quality" I mean maintainability, customisability and probably testability too. Examples of this would be:

• Reducing/eliminating the use of static access to things. NHibernate should not assume that it is run it as a singleton. For the most part, it doesn't assume that, but every once in a while something static bleeds through.
• Something like the IMultiTenancyConnectionProvider mentioned above wouldn't have to be built into NHibernate at all. It offers good functionalty (I use it in production, but not for its named purpose, which is sort of my point), but there could be more abstract extension points doing the same thing, without the word "tenant" ever entering into the code base. A DI approach could be what orchestrates the parts needed to provide multitenancy, but the implementation would live in a separate code base.

[craigfowler]

@bahusoid I didn't know about that actually. We're in the process of upgrading to 5.3 right now. I'll be sure to let the dev doing it know about that. Thanks!

That said, it's likely that without being able to constructor-inject into an IMultiTenancyConnectionProvider impl, it might not be that useful to us. At least, no more useful than a custom connection provider, which we already wrote some years ago (back in the NH4 days).

We have a non-trivial set of services required in order to determine "which tenant is active" and to then get that connection string. Those are available via DI in our app but for NH we were previously getting them from a static factory.

[bahusoid]

We have a non-trivial set of services required in order to determine "which tenant is active" and to then get that connection string.

Well you don't really need to inject services to IMultiTenancyConnectionProvider. Instead you can supply already obtained connection string to TenantConfiguration subclass on each session open (something like sessionFactory.WithOptions().Tenant(new CustomTenantConfiguration(TenantName, TenantConnectionString)).OpenSession())

[craigfowler]

Huh, that is quite interesting then. I'll be sure to have a look at it tomorrow. Thanks again.

[maulik-modi]

@craigfowler , how can we take this forward?

[craigfowler]

Hi again everyone. Sorry, I took a bit of a break from OSS and never got around to continuing this conversation. I have had some ideas swirling around in my head over the last few months though, just not written them down until now.

Overview

Having a little think about DI support, there are lots of DI frameworks and also there's the MS Extensions DI package. I don't think it's unreasonable to treat the MS Extensions DI package as "just another framework", with equal treatment to the others. The only real advantage that it has going for it is that it is capable of abstracting many other frameworks, and that it's likely to be compatible with other packages which are DI-friendly.

So, how about developing specific support for specific frameworks/containers in separate elective libraries/packages? NH will continue to use its own abstractions which are written in NH core? Of those elective DI framework-specific packages, Microsoft.Extensions.DependencyInjection is treated as just another one, like the others. It might be worth developing support for MS Extensions DI before any other containers because its inherent ability to integrate with other containers will likely make it popular.

I'm assuming that NH Core should not depend upon MS DI Abstractions

The Microsoft.Extensions.DependencyInjection.Abstractions NuGet package is relatively small and does not contain the full ServiceCollection container implementation. What it does contain are a few types such as ISupportsRequiredService which we'd need if we wanted to viably replace IObjectsFactory with IServiceProvider.

I get the feeling that the overwhelming opinion is is that taking a new dependency (from NH core) upon MS DI Abstractions is not acceptable.

In that case, I will concede that it's worth continuing to use the IObjectsFactory interface and not switching to IServiceProvider. NH has created a well-understood API for that and already defines the behaviour that it will either return an instance or throw, never return null.

Could we improve IObjectsFactory's exception?

Currently, if IObjectsFactory fails to resolve an instance then the precise exception it throws is not defined by the interface. Right now the default impl will throw "whatever Activator.CreateInstance throws" and a non-default impl could throw any exception at all. That makes it quite difficult to catch because developers don't know what's coming their way.

Could we tighten that up by creating something like ObjectsFactoryException in NH core? Then we would alter the default implementation to wrap Activator's exception within. That would certainly encourage consistency by providing a precedent for other impls to follow.

Implementations of IObjectsFactory

So, in vanilla NH core, the current implementation of IObjectsFactory would be largely unchanged (except perhaps wrapping Activator's exception as above).

In the elective container-specific packages, the implementation of IObjectsFactory simply wraps the container-specific resolver (perhaps wrapping its exceptions where applicable, as above). Moreover, we treat IServiceProvider just like it were any other container.

Also in those elective container-specific packages, we would provide "whatever convenience logic is appropriate" to get NHibernate registered up in that container. For example for Autofac we would provide one or more implementations of Autofac.IModule. For MS Extensions DI we would provide one or more extension methods for Microsoft.Extensions.DependencyInjection.IServiceCollection and so on. Consumers don't have to use those if they don't want to; they are welcome to write their own. Even if a consumer does write their own registrations, the stock ones would be useful examples upon which to base customised ones.

Weakness/limitation of this approach

There is a weakness to this approach; whilst obvious after a moment's thought I would still like to point it out. As long as NH core does not require a DI container, NH core logic cannot be written in a manner that realises the benefits of DI. So, implementations of anything which would be resolved by an IObjectsFactory in NH Core must retain public parameterless constructors and may not constructor-inject dependencies. In other words, they must be constructable by object Activator.CreateInstance(Type). The only way in which NH Core could rely on havning DI available would be to make DI mandatory in some way.

Despite that, I don't think anything proposed here either forces or prevents leaving DI as an optional thing, nor does it force or prevent NH Core from revisiting that decision in the future. At any future date (from a technical perspective), NH Core could either keep things as they are or "go full DI" and make a container mandatory.

I think making NH Core "play nicer with DI" for consumers which already committed to DI is a step forward. Whether NH Core goes full DI itself or not, the two things don't have to be linked. It doesn't prevent consumers of NH from writing impl classes (of NH's interfaces) which rely upon DI either. If the consuming app has chosen to require a DI container (and they use a DI-powered IObjectsFactory) then they are welcome to write custom logic which requires a container.

Making the Objects Factory non-static

The biggest challenge (as already pointed-out) will be pushing the objects factory into an instance-based API instead of a static API. Technically this doesn't have to be conflated with the above, and should probably be developed as a separate piece of work/pull request. Keeping the objects factory static is basically service locator and not dependency injection.

This is a case of removing it from Environment and (unless there are objections) adding it to the Configuration class instead. Also, I noticed that there is no way to access the Configuration (or even an immutable copy of the config) from a session factory. We would need to add a get-only property to the session factory to make the objects factory available. Likewise, the same property would be needed on both of ISession and IStatelessSession. I noticed that you can easily traverse from the session (but not stateless session) back to its session factory but I suggest new properties for consistency and to be explicit.

Then, every place which currently gets the objects factory from Environment needs to get it instead from either the config or the session factory or the session/stateless session instead. That won't be fun work, but I don't believe it's impossible (awwww, I just volunteered, didn't I?). I'd certainly propose analysing it first (just by finding what existing code has references to Environment.ObjectsFactory) to get a scale of the work required. There will probably be some easy wins and I bet there will be some horrible/difficult cases too.

Some of this is non-breaking

If we stick with IObjectsFactory in core and just add extra elective packages for specific DI containers (of which MS Extensions DI is treated as just another container) then that's not a breaking change and doesn't require us to go to NH v6.x.

Some of it is breaking

The proposed improvement to the IObjectsFactory interface to throw a more consistent exception is technically breaking. Whilst there's no explicit documentation on IObjectsFactory to indicate that it only throws the same exceptions as Activator.CreateInstance can throw, it's plausible that consumers have written code that catches those specific exceptions. I wouldn't object to pushing that change into NH6+ if it's made.

The migration of IObjectsFactory away from Environment and onto Configuration and then ISessionFactory, ISession and IStatelessSession is totally breaking and would need to go into NH6+. Even adding the properties to the interfaces (and keeping both way of getting the factory as an intermediate state) would be breaking, because we'd break any other implementors of those interfaces. I also had a little think about this along the lines of "Is there anything worthwhile that could be done for this without breaking changes" and I came up empty.

What's the current thinking toward a future NH 6.x? Is there any other work in-flight which is slated for 6.x? If not, is this the right time to begin such work, are the main contributors OK maintaining both a 5.x and a "vnext"? If not, when would you suggest revisiting breaking changes?

[craigfowler]

An update/expansion of that limitation which just struck me (again, it's obvious if you think about it but worth pointing out all the same).

That's that as long as a DI container is not mandatory, the objects factory will always have to be used with concrete types and not simply interfaces.

A usual payoff of going to DI is that service types are resolved by interface and no longer by concrete implementation type. That wouldn't be available to NH core if a container is not mandatory.