Multi-Tenancy: Implement tenant per Database strategy

[bahusoid]

Fixes #974

Based on hibernate implementation (related commits) with some modifications (omitted unused classes, interface members and modified tenant session configuration)

Example of tenant configuration:

1. Add multi_tenancy.strategy and multi_tenancy.connection_provider NHibernate configuration properties (hibernate-configuration\session-factory):

<property name="multi_tenancy.strategy">Database</property>
<property name="multi_tenancy.connection_provider>AssemblyQualifiedNameForMyMultiTenancyConnectionProvider</property>

Or example with by code configuration:

Configuration configuration = GetNHConfiguration();
configuration.DataBaseIntegration(
	x =>
	{
		x.MultiTenancy = MultiTenancyStrategy.Database;
		x.MultiTenancyConnectionProvider<MyMultiTenancyConnectionProvider>();
	});

Where MyMultiTenancyConnectionProvider must implement IMultiTenancyConnectionProvider. Can be implemented based on provided AbstractMultiTenancyConnectionProvider. Example of implementation:

public class MyMultiTenancyConnectionProvider : AbstractMultiTenancyConnectionProvider
{
	private Dictionary<string, string> _dic = new Dictionary<string, string>
	{
		{"tenant1", "tenantConnectionString1"},
		{"tenant2", "tenantConnectionString2"},
	};

	protected override string GetTenantConnectionString(TenantConfiguration configuration)
	{
		return _dic[configuration.TenantIdentifier];
		//Or something like:
		//return ConfigurationProvider.Current.GetNamedConnectionString("tenantConnectionFor." + configuration.TenantIdentifier);
		//Or
		//return ((MyCustomTenantConfiguration)configuration).TenantConnectionString;
	}
}

2. Provide tenant configuration each time you open session. Something like:

sessionFactory.WithOptions()
.Tenant(new TenantConfiguration(TenantName))
.OpenSession()

You can create subclass of TenantConfiguration and provide some specific details about you tenant configuration and use it to obtain proper tenant connection inside your IMultiTenancyConnectionProvider implementation:

sessionFactory.WithOptions()
.Tenant(new MyCustomTenantConfiguration(TenantName, TenantConnectionString))
.OpenSession()

class MyCustomTenantConfiguration : TenantConfiguration
{
	public string TenantConnectionString { get; }

	public MyCustomTenantConfiguration(string tenantIdentifier, string tenantConnectionString) : base (tenantIdentifier)
	{
		TenantConnectionString = tenantConnectionString;
	}
}

Current tests are not exactly multi-tenant as executed on the same database. For proper multi-tenant tests we need additional db which I'm not sure how to properly add (and I think should be done as separate PR). Still current tests make sure that different connection string is used for different tenants (by injecting AppName on SQL Server) and make sure that second-level/query cache is separated for different tenants. Let me know if I missed something to test.

Things to implement in future:

• Ability to clear tenant specific caches (maybe option to prefix cache region with tenantIdentifier?) (WIP Multi-Tenancy: Separate second-level cache per tenant (prototype for discussion) #2133)

[bahusoid]

Not sure if I added MutiTenancy "Loquacious" configuration to proper place but everything else seems to be ready.

[gliljas]

Sorry for being late here, but I personally think that the Hibernate structure is much cleaner. However, this version can certainly be wrapped away into extension methods or app unique "session providers", and made to look like the Hibernate version.

That's not saying that this is bad or that the Hibernate implementation is good. Like a lot of the Hibernate code it's quite bad.

What it boils down to is that NHibernate's extensibility should allow for something like this, without requiring very specific code changes. Injecting strategies, providers and decorators should do the trick.

[bahusoid]

Could you be more specific. As it's unclear to me which part and why would you want to wrap in hibernate version. So could you describe some scenario that cleaner in hibernate?

[gliljas]

Gladly.

I just think that bringing things like a ConnectionProvider up to the topmost API feels a bit awkward. Multi tenancy is to me a more infrastructural concern. Just "tagging" the session, or even better, use the (I)CurrentTenantIdentifierResolver method that Hibernate does, would be more flexible, I think. It would also allow for different kinds of tenancy strategies, which may not necessarily use different connection providers (just as the Hibernate implementation does).

My objection really isn't with your implementation. It rather stems from a frustration with the old codebase in the configuration and execution pipeline, which is such a feast of bad practices (all inherited from Hibernate). With a properly extensible API, adding something like multi tenancy wouldn't require core changes. A similar extension, NHibernate.Shards, would also be able to inject itself with ease.

Maybe something for 7.0

[bahusoid]

I just think that bringing things like a ConnectionProvider up to the topmost API feels a bit awkward.

MyTenantConnectionProvider from example consists of two things: tenantIdentifier and tenantConnectionString. So I don't see it as something heavy and akward. It's essentialy Database specific configuration and you don't need to provide it for other tenancy strategies.

Just "tagging" the session, or even better, use the (I)CurrentTenantIdentifierResolver method that Hibernate does, would be more flexible,

I don't see it that way. It just means that you split your tenant specific logic across different classes. And it means user needs to configure NHibernate in more places and implement more Nhibernate specific interfaces. That doesn't look cleaner to me. Now all that can be implemented in user code using convinient for user data models and be wrapped in whatever way he likes.

It would also allow for different kinds of tenancy strategies, which may not necessarily use different connection providers

As I already said you don't need to provide tenant connection provider for other tenancy strategies - just add different ctor to TenantConfiguration. But IMHO all other strategies should still support different connection providers (for instance you want to store 100 small tenants per database - means you need both schema/discriminator + database strategies).

[gliljas]

MyTenantConnectionProvider from example consists of two things: tenantIdentifier and tenantConnectionString. So I don't see it as something heavy and akward. It's essentialy Database specific configuration and you don't need to provide it for other tenancy strategies.

"Weight" is not the issue. It's placement of responsibilities. It can be wrapped away. I just think that it should be by default.

I don't see it that way. It just means that you split your tenant specific logic across different classes. And it means user needs to configure NHibernate in more places and implement more Nhibernate specific interfaces. That doesn't look cleaner to me. Now all that can be implemented in user code using convinient for user data models and be wrapped in whatever way he likes.

OK, we're different. To me, it's cleaner.

It would also allow for different kinds of tenancy strategies, which may not necessarily use different connection providers

As I already said you don't need to provide tenant connection provider for other tenancy strategies - just add different ctor to TenantConfiguration. But IMHO all other strategies should still support different connection providers (for instance you want to store 100 small tenants per database - means you need both schema/discriminator + database strategies).

It may need that, but IMHO adding ctors isn't a convenient extensibility model.

It's clearly a matter of taste, but to me:

1. Selecting "who" should be very lightweight or preferably completely transparent.
2. Selecting "how" belongs in a different layer.

[grahambunce]

A bit late to this, but isn’t this handled perfectly well by the NHibernate Shards project? We use that perfectly well already to implement multi-tenancy across databases. Wouldn’t it be better to merge that into the base NHibernate code base, or add additional effort to that project?

[bahusoid]

isn’t this handled perfectly well by the NHibernate Shards project?

All you need for multi-tenancy is to create separate session factory per tenant. Using NHibernate.Shards for this is very strange decision. But well - use whatever makes you happy.

And this PR allows to share the same session factory for all tenants.

[fredericDelaporte]

I have only reviewed the tests for now.

I share at least a part of @gliljas viewpoint about opening session. I think we should only provide the tenant id string for opening a session for a tenant. Supplying the whole tenant configuration at each session opening looks like a code smell to me.
I would rather have to add tenant configurations to the session factory first, which would have a (maybe concurrent) dictionary of them, then start opening session by just specifying their tenant id.

[bahusoid]

I think we should only provide the tenant id string for opening a session for a tenant.

I don't know anything less convenient than single string param for whole configuration. That's some
legacy code smell to me. Even hibernate provides interface now (but again just for tenant id resolution)

Supplying the whole tenant configuration at each session opening looks like a code smell to me.

It's basically tenantIdentifier + tenantConnectionString. And it's wrapped in class to simplify further extensions for us without breaking changes

I would rather have to add tenant configurations to the session factory first, which would have a (maybe concurrent) dictionary of them, then start opening session by just specifying their tenant id.

Introducing yet another dictionary when it can be avoided - again makes no sense to me and looks like code smell to me too.

Update Yeah it's pretty much the same arguments I've already said to gliljas. But my big reluctance on using string key is based on personal experience where such design would lead to some awkward code to "deconstruct" string for retrieving tenant configuration. Also I don't like idea of keeping tenant configurations in session factory dictionary. Sometime it's not something that can be/should be populated beforehand - tenant configuration can be dynamic, obtained on request and there is no reasons to keep it after end of request/session polluting session factory

[fredericDelaporte]

But my big reluctance on using string key is based on personal experience where such design would lead to some awkward code to "deconstruct" string for retrieving tenant configuration.

For me, building some string based convention for supplying settings in a string to be parsed is not to be done. I am speaking about nothing else than a tenant identifier, with each tenant to be configured at the session factory level.

I do not understand the need for supplying all the tenant settings at each session opening. It looks unpractical to me.

Sometime it's not something that can be/should be populated beforehand - tenant configuration can be dynamic, obtained on request and there is no reasons to keep it after end of request/session polluting session factory

Maybe then we should have both possibilities.

[bahusoid]

I am speaking about nothing else than a tenant identifier, with each tenant to be configured at the session factory level.

So OK it's about simple scenario when you have small static list of tenant configurations. It's not very useful for scenarios when tenant configurations are dynamic.

building some string based convention for supplying settings in a string to be parsed is not to be done

And still unless some part of code is bound to be executed for given tenant - you need some code that constructs required tenantIdentifier for session opening. And it means you have all tenant settings in this context. So string parameter or instance of tenant configuration doesn't make much difference for me.

I do not understand the need for supplying all the tenant settings at each session opening. It looks unpractical to me.

It's extensible. You can easily wrap this code to be executed the way you like. Via string identifier or via some custom user class. That's how it supposed to work. And why to add additional dictionary lookup in our code on each session opening if we can avoid it. It can easily be added by user if needed.

Maybe then we should have both possibilities.

As I already said I think it's trivial task that can be done in user code.

[fredericDelaporte]

I remain unconvinced. This "supply the whole configuration at each session opening" pattern allows to supply inconsistent configurations, like supplying the same tenant identifier with different connections.
Dynamically adding tenant configurations to the session factory is possible. Of course this implies using a concurrent dictionary in order to be safe, but this would forbid mistakes like inconsistent configurations, provided we do not allow overriding a tenant configuration.
The session opening could have some GetOrAdd logic about tenant for allowing easier dynamical tenant configuration.

@hazzik, @maca88, may we have your thoughts about this PR in its current state?

[bahusoid]

I believe it's done - IMultiTenancyConnectionProvider is moved to session factory settings. And TenantConfigration is transformed into simple DTO with tenantIdentifier and optional tenantConnectionString. TenantConfiguration might be extended in user code as subclass to provide additional tenant information. And tenant connection information is retrieved via SessionFactory.Settings.MultiTenancyConnectionProvider.GetConnectionAccess(tenantConfiguration) as requested.

Also added default DefaultMultiTenancyConnectionProvider which expects connection string be provided on each session opening (otherwise it throws exception suggesting to implement own provider)

It doesn't have built in ability to specify static tenant configuration - but again it's trivial task. And hibernate also doesn't provide such ability. Example of static configuration :

public class StaticMultiTenantConfigurationConnectionProvider : DefaultMultiTenancyConnectionProvider
{
	private Dictionary<string, string> _dic = new Dictionary<string, string>
	{
		{"tenant1", "tenantConnectionString1"},
		{"tenant2", "tenantConnectionString2"},
	};

	protected override string GetTenantConnectionString(TenantConfiguration configuration)
	{
		return _dic[configuration.TenantIdentifier];
		//Or something like:
		//return ConfigurationProvider.Current.GetNamedConnectionString("tenantConnectionFor." + configuration.TenantIdentifier);
	}
}

[maca88]

I revisited the PR and made some refactoring that I would propose:

• Removed ConnectionString property from TenantConfiguration. IMO we should provide only the tenant identifier and let the user decide whether to extend TenantConfiguration to add ConnectionString or handle it in IMultiTenancyConnectionProvider. Also in case we will support MultiTenancyStrategy.DISCRIMINATOR in the future, the connection string will be the same, so I think it is better to not have it.
• Added null check for tenant identifier in TenantConfiguration constructor. I think we should "fail fast" and not wait until trying to open the session.
• Removed ConnectionString property from IConnectionAccess interface.
• Removed IConnectionProvider parameter from IConnectionAccess.(Get|Close)Connection methods. As the connection provider is always from the session factory, I've added the session factory parameter for IMultiTenancyConnectionProvider.GetConnectionAccess method instead.
• Removed DefaultMultiTenancyConnectionProvider and added AbstractMultiTenancyConnectionProvider instead. As mentioned previously, let the user decide how to implement the connection string logic.
• Renamed configuration setting multiTenancy to multi_tenancy.strategy and multiTenancy.connection_provider to multi_tenancy.connection_provider. I think we should stick with the existing naming convention, rather than coping them from Hibernate.

[bahusoid]

Removed ConnectionString proprety from TenantConfiguration.

IMO it's useful to have it even for Discriminator case and it simplifies multi-tenant setup. But fine by me if others members agree it's better not to have it. Also then it seems it makes sense to add Sfi.WithOptions().Tenant("tenantId") shortcut (and maybe rename existing method too)

Removed ConnectionString proprety from IConnectionAccess interface.

Why? It was added explicitly for convenience. It's been bugging me with ConnectionProvider that I can't check ConnectionString for it.

Agree with other changes.

[bahusoid]

Actually scratch it - let's get rid of TenantConfiguration.ConnectionString - it can easily be added in user code. But please return IConnectionAccess.ConnectionString - it's not needed on NHibernate side, but it's useful information that can be used in user code. And feel free to commit your changes here.

[maca88]

Pushed all changes except for removing ConnectionString property from IConnectionAccess interface.

[gliljas]

Ok, now I'm taking a look... :)

[bahusoid]

Ok, now I'm taking a look... :)

Yeah about time. I'm done polishing it :)

[fredericDelaporte]

I am now overall agree with this PR, excepted for a few minor points.
May you also update the first comment (description of this PR) for reflecting this PR current way of working for the feature?

[bahusoid]

I believe I resolved all review suggestions and made last small change - exposed session TenantConfiguration as it might contain some useful context.

[rodrigo-web-developer]

there is some example of this working? I'm trying to implement MultiTenancyConnectionProvider, but it seems to be more required configuration than just

config.DataBaseIntegration(x =>
            {
                x.MultiTenancy = MultiTenancyStrategy.Database;
                x.MultiTenancyConnectionProvider<MyMultiTenancyConnectionProvider>();
            });

It should be used? Or it is better to create a SessionFactory for each tenant, like some people said?