ipjohnson / Grace

Grace is a feature rich dependency injection container library
MIT License
336 stars 33 forks source link

Multi tenant support? #157

Closed stephenlautier closed 6 years ago

stephenlautier commented 6 years ago

I'm currently looking into multi-tenant DI for .NET core (aspnet and console e.g. Microsoft Orleans), and I'm currently seeing options, and found out Grace which seems promising so I'm trying it. Does this support multi-tenancy? My guess is yes since it supports the child containers; if so do you have any examples or suggestions how multi-tenancy can be achieved with this library? Thanks!

stephenlautier commented 6 years ago

I'm trying to implement this myself however im running into some issues.

Basically this is what I would like to have:

public async Task Invoke(
    HttpContext httpContext
)
{
    var host = httpContext.Request.Host.Host;
    var tenant = Tenants.ResolveByDomain(host);

    var locatorScope = httpContext.RequestServices.GetService<IExportLocatorScope>();

    var r1 = locatorScope.Locate<RequestContext>(); // works
    var r2 = locatorScope.Locate<RequestContext>(); // works
    var injectionScope = locatorScope.GetInjectionScope();
    //injectionScope.Configure(c => c.ExportInstance(tenant).As<ITenant>().Lifestyle.SingletonPerRequest());

    using (var scope = injectionScope.CreateChildScope(c =>
    {
        // todo: get tenant container services + register
        c.ExportInstance(tenant).As<ITenant>();
    }, "tenant"))
    {
        var t = scope.Locate<ITenant>(); // works
        var r3 = locatorScope.Locate<RequestContext>(); // this is the original scope - works
        var r4 = scope.Locate<RequestContext>(); // new object - empty

        httpContext.RequestServices = scope.Locate<IServiceProvider>(); // replace aspnet request service
        await _next(httpContext);
    }
}

So basically my current issue is im loosing the current scope, since im creating a new child scope

Test proj https://github.com/stephenlautier/.netcore-labs/blob/master/Slabs.AspnetCore/Tenant/MultiTenantMiddleware.cs#L70

stephenlautier commented 6 years ago

Actually after writing the above, i realized i simpley had to move the middleware order to first create tenant scope and then set request context.

Next is to configure and set tenant services

ipjohnson commented 6 years ago

Hi @stephenlautier

I have an idea of how to accomplish what you want to do but I want to test it before I post it. I'll see about putting together a sample this evening or tomorrow morning and send it along.

stephenlautier commented 6 years ago

Thanks @ipjohnson! I had implement a working example to be completely honest, if you want to look at it the source is here https://github.com/stephenlautier/.netcore-labs/tree/master/Slabs.AspnetCore/Infrastructure/Tenency (i took some ideas from saaskit). Its actually quite resusable as well, so can be extracted in a library easily.

Usage is as following

// startup.cs
public void ConfigureContainer(IInjectionScope scope)
{
    scope.Configure(c =>
    {
        c.Export<HeroService>().As<IHeroService>();
    });

    scope.ConfigureTenants<AppTenant>((tenant, c) =>
    {
        if (tenant.Key == "sketch7")
            c.Export<SampleHeroService>().As<IHeroService>();
    });
}

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    ...
    app.UseMultiTenant<AppTenant>(); // register multi tenency
    app.UseRequestContext();
    app.UseMvc();
}

// implement a custom resolver
public class AppTenantResolver<TTenant> : ITenantResolver<TTenant> 
    where TTenant : class
{
    private static readonly Dictionary<string, AppTenant> _domainMap = new Dictionary<string, AppTenant>
    {
        ["local.cerberus.io"] = Tenants.Cerberus,
        ["local.sketch7.io"] = Tenants.Sketch7,
    };

    public TTenant Resolve(string key)
    {
        _domainMap.TryGetValue(key, out var tenant);
        return tenant as TTenant;
    }

    public TTenant Resolve(HttpContext httpContext)
    {
        var host = httpContext.Request.Host.Host;
        return Resolve(host);
    }
}

Now my next step is to try and getting it working within a console (which should be easy) but within Micorsoft Orleans (not 100% sure if its possible)

Also, nice library, I had never heard of it before and I quite liked it so far 👍

ipjohnson commented 6 years ago

Very cool. I put together a quick sample of what I was thinking for doing multi tenant. PerRequestDependency.zip

The big difference is child container vs. using the request scope that is already created. I think you'll find the performance for child scopes is ~100x slower than using the pre-existing scope (which may or may not be a problem).

If you run into any issues with Orleans let me know as I'd be interested in making this work.

stephenlautier commented 6 years ago

Thanks for the sample! Yes, in fact, I had tried using the same scope container but didn't manage to get it working.

Looking at the sample I pretty much understand how its working, what I'm not sure is how will I register brand specific services, as in I know but not sure if theres a better way of doing it

// a
c.Export<EvenService>().AsKeyed<ISomeAService>("TenantA");
c.Export<OddService>().AsKeyed<ISomeAService>("TenantB");
c.ExportFactory<IExportLocatorScope, IRequestInfo, ISomeAService>((scope, info) =>
                scope.Locate<ISomeAService>(withKey: info.Key));
// b
c.Export<EvenService>().AsKeyed<ISomeBService>("TenantA");
c.Export<OddService>().AsKeyed<ISomeBService>("TenantB");
c.ExportFactory<IExportLocatorScope, IRequestInfo, ISomeBService>((scope, info) =>
                scope.Locate<ISomeBService>(withKey: info.Key));

So my question is, is there a nicer way to register a "bulk" with this way, rather than handling each registration.

RE Orleans: I created an issue with theirs https://github.com/dotnet/orleans/issues/4580, I havent tried yet but will try to today. Thanks again!

ipjohnson commented 6 years ago

@stephenlautier I don't have anything specific created for this because there is no standard way to define what a tenant is or how it's separated (i.e. the IRequestInfo is not standard). That said you could probably wrap that logic up into a C# extension.

public static void ExportTenantService<TServiceA,TServiceB,TExport>(this IExportBlockConfiguration block, object keyA, object keyB)
{
    c.Export<TServiceA>().AsKeyed<TExport>(keyA);
    c.Export<TServiceB>().AsKeyed<TExport>(keyB);
    c.ExportFactory<IExportLocatorScope, IRequestInfo, TExport>((scope, info) =>
                scope.Locate<TExport>(withKey: info.Key)); 
}

Do you have a lot of these services?

stephenlautier commented 6 years ago

Yes, I understand it's not standard, I'm just thinking of how best to do it. Not entirely sure how many we will have but I do think we have quite some, but to be honest we will have more on Orleans rather than in the web, in web probably we won't have much.

Thanks for the extension, something like that might suffice 👍

ipjohnson commented 6 years ago

If there was some way to identify them programmatically you could use the bulk registration methods but you'd need to separate the types into different namespace or mark them with attributes (something to distinguish them).

stephenlautier commented 6 years ago

I'm trying to use this for Orleans and currently no luck, there seems to be an issue when using it with Grace (see screenshot below).

image

I had tried the same with StructureMap because it was mentioned in an issue in Orleans which now should work (even tho at the time of writing it doesnt) but im not getting that far with Grace

This is the issue with StructureMap (which was a known issue and should be fixed)

image.

This is how im creating my container and populating it.

private static IServiceProvider ConfigureServices(IServiceCollection services)
{
    // Grace
    var container = new DependencyInjectionContainer();
    container.Configure(c =>
    {
        c.Export<MockHeroDataClient>().As<IHeroDataClient>();
    });
    var provider = container.Populate(services);
    return provider;

    // StructureMap
    //var container = new Container();
    //container.Populate(services);
    //container.Configure(c =>
    //{
    //  c.For<IHeroDataClient>().Use<MockHeroDataClient>();
    //});
    //return container.GetInstance<IServiceProvider>();
}
stephenlautier commented 6 years ago

What i've noticed is that by using the microsoft DI it returns null, so basically Grace is not supporting null, perhaps any way to configure it? or I had misconfigured something?

image

ipjohnson commented 6 years ago

@stephenlautier by default Grace checks to make sure null isn't returned from factories. You can turn it off when you construct the container.

var container = new DependencyInjectionContainer(c => c.Behaviors.AllowInstanceAndFactoryToReturnNull = true);
stephenlautier commented 6 years ago

perfect it worked! (and also it didnt had the issue which structuremap had)

stephenlautier commented 6 years ago

I'm trying to do some extensions methods for Tenants container registration, and currently im trying to do something like:

                                                             V
config.ExportFactory<IExportLocatorScope, ITenantContext, object>((scope, tenantContext) =>
{
    var tenant = tenantContext?.Key;

    if (tenant == null) throw new ArgumentNullException("tenant", "Tenant must be defined");
    return scope.Locate(withKey: tenant, type: interfaceType);
});
return config;

Basically a little bit more dynamic ExportFactory is what i need - the problem is since im passing object, its not resolving for the type correctly, any other way i can do this?

ipjohnson commented 6 years ago

@stephenlautier I'm not sure I follow 100%. Are you attempting to true and intercept the request for interfaceType and then add tenant key in when locating interfaceType again?

stephenlautier commented 6 years ago

Yes pretty much what you said. I want to register an ExportFactory as your suggestion (above per scope), but now what im doing is register "bulk" were i wont have the luxury to use generic types.

Sorry i didnt give you full implemetenation, I have the following

// generic - this works
public static IExportRegistrationBlock ExportPerTenantFactory<T>(this IExportRegistrationBlock config)
{
    config.ExportFactory<IExportLocatorScope, ITenantContext, T>((scope, tenantContext) =>
    {
        var tenant = tenantContext?.Key;

        if (tenant == null) throw new ArgumentNullException("tenant", "Tenant must be defined");
        return scope.Locate<T>(withKey: tenant);
    });
    return config;
}

// non generic - this doesnt work
public static IExportRegistrationBlock ExportPerTenantFactory(this IExportRegistrationBlock config, Type interfaceType)
{
    config.ExportFactory<IExportLocatorScope, ITenantContext, object>((scope, tenantContext) =>
    {
        var tenant = tenantContext?.Key;

        if (tenant == null) throw new ArgumentNullException("tenant", "Tenant must be defined");
        return scope.Locate(withKey: tenant, type: interfaceType);
    });
    return config;
}

I have those methods, the generic one works but the non generic doesnt due to the object since im not registering the interface type correctly.

Then im using that method from:

public static IExportRegistrationBlock ExportForAllTenants(this IExportRegistrationBlock config, IEnumerable<ITenantContext> tenants, Type interfaceType, Type implementationType, Action<IFluentExportStrategyConfiguration> configure = null)
{
    foreach (var tenant in tenants)
    {
        var exportConfig = config.Export(implementationType).AsKeyed(interfaceType, tenant.Key);
        configure?.Invoke(exportConfig);
    }

    config.ExportPerTenantFactory(interfaceType);

    return config;
}

So pretty much i need similar as what im doing above config.Export(implementationType).AsKeyed(interfaceType, tenant.Key); (non generic) but for a factory

Im trying to break the problem a bit down, because what i would like to achieve is different ways how i can register tenants services in simpler ways, and with these goals

container.Configure(c =>
     // register all library services for tenant X (im trying to implement this ) - `x is IServiceCollection`
    c.ForTenant(Tenants.HeroesOfTheStorm.Key).PopulateFrom(x => x.AddHeroesHotsServices());
    c.ForTenant(Tenants.LeageOfLegends.Key).PopulateFrom(x => x.AddHeroesLoLServices());

    // register service per Tenant - this already works but ideally i be able to specify implemention type per tenant
    c.ExportForAllTenants<IHeroDataClient, MockLoLHeroDataClient>(Tenants.All, x => x.Lifestyle.Singleton());
);
ipjohnson commented 6 years ago

@stephenlautier ok let me think about it a bit. While I understand what you want to do it's not exactly straight forward.

There is another cross cutting feature that might be very helpful for this. Essentially there is this interface that allows you to plug linq expressions in when dependencies are being discovered. I'll see about putting together a sample where you can register everything normally and when you have a dependency for string tenant than the container will query the IInjectionValueProvider for how to resolve it.

stephenlautier commented 6 years ago

@ipjohnson great thanks! hmm thats interesting, sounds kind of interception for the resolver

For my immediate issue i did an ugly workaround via reflection, basically invoking my other generic method via reflection as below e.g.

public static IExportRegistrationBlock ExportPerTenantFactoryWeak(this IExportRegistrationBlock config, Type interfaceType)
{
    MethodInfo openMethod = typeof(GraceExtensions).GetMethod(nameof(ExportPerTenantFactory));
    MethodInfo typedMethod = openMethod.MakeGenericMethod(interfaceType);
    typedMethod.Invoke(null, new object[] { config });
    return config;
}
ipjohnson commented 6 years ago

@stephenlautier that's a good way of describing it. Here is an example of how I use it in the MVC extensions where it inspects each dependency for binding attributes ([FromQueryString], etc)

Your work around seems reasonable to me I think you could probably do it with object like below but your generic solution seems good as well.

ExportFactory<IExportLocatorScope, ITenantContext, object>(...).As(interfaceType)
stephenlautier commented 6 years ago

Yes thats what I was looking for, much cleaner :). I knew for the Export you could do that not sure why i didnt try to do the same for the ExportFactory 🤦‍♂️

ipjohnson commented 6 years ago

@stephenlautier how did things turn out? Were you able to get things working?

stephenlautier commented 6 years ago

I have most of the stuff working now pretty much.

I do have a question tho, if I have a context which I'm able to iterate all over all tenants, is it possible to get the instances per tenant (see example below)

// pseudo
ctor(IExportLocatorScope exportLocatorScope)

foreach (ITenant tenant in tenantsRegistry.GetAll())
{
    var service = _exportLocatorScope.Locate<ISpecificTenantService>(withKey: tenant.Id, extraData: new { tenant = tenant });
}

public class SpecificTenantService: ISpecificTenantService
{
    public SpecificTenantService(ITenant tenant)
    {
    }
}

I'm using the exact implementation as you had suggested, with the export factory and set in the context.

I had tried something as above but still it didnt work.

ipjohnson commented 6 years ago

Hopefully I’ll have some time this evening to take a look. The suedo code seem like it should be right so I’ll need to do a little research

ipjohnson commented 6 years ago

Ok after looking at it I think I know what is happening the IInjectionContext that contains the ITenant is not passed through the factory. I think you'll need to pass it by hand.

config.ExportFactory<IExportLocatorScope, IInjectionContext, ITenantContext, object>((scope, injectionContext, tenantContext) =>
{
    var tenant = tenantContext?.Key;

    if (tenant == null) throw new ArgumentNullException("tenant", "Tenant must be defined");
    return scope.Locate(withKey: tenant, type: interfaceType, extraData: injectionContext);
});
stephenlautier commented 6 years ago

Cool i will give that a shot!

This might also fix another issue i was having before i left work today. I was switching an existing service to multi-tenant and for this specific case in the ExportFactory i did have the tenant but after it was being resolved in the service, the tenant in the service was being null

stephenlautier commented 6 years ago

Still its not working with this one. When i see the stack tho its not calling the factory I implemented which is named ExportPerTenantFactory (that contains the code as you pasted)

This when using _exportLocatorScope.Locate<IContentManagementResolver>(withKey: brand.Id, extraData: new { tenant = brand });

image

This is when its being injected via constructor

image

Even tho its being injected via the ExportPerTenantFactory, and within the function i see the Tenant set and with data, once its resolved its being resolved with the Tenant null. Another point i see its using the LocateFromParent, which would be the root scope and that one doesnt have the tenant.

ipjohnson commented 6 years ago

@stephenlautier Looking at the stack trace I see that a Singleton Lifestyle is in the resolution chain. When going through most lifestyles the injection context is dropped. The theory being if something is registered as a singleton is shouldn't depend on anything in the context as it could be different depending on the order of when the singleton is resolved.

I had someone else ask a similar question a little while back and I wrote a custom singleton lifestyle as an example here of how to pass the injection context through.

That said your statement of "When i see the stack tho its not calling the factory I implemented which is named ExportPerTenantFactory (that contains the code as you pasted)" is rather confusing. Do you have child containers in the mix?

I know it's not ideal but I've enabled SourceLink for Grace so if you're on the latest VS you should be able to F11 into the code (if you have Debug just my code turned off). Alternatively if you can create a small sample I can debug it.

stephenlautier commented 6 years ago

Thanks for your reply.

Regarding child containers; No im not using any, just factory + ExtraData extra as per your suggestion.

Yes, it is set as a singleton to be honest, what i was a bit confused is during my initial sample tests I had tried both Singleton/Scoped, and with singleton, it was creating an instance per tenant as a singleton which is very efficient (not 100% sure if it was creating an instance per request for Scoped to be fair)

I had an example project in here https://github.com/sketch7/orleans-heroes/tree/feature/multi-tenancy/Heroes.SiloHost.ConsoleApp - but currently I was implementing stuff in another project (which is private) which is slightly evolved but very similar. In that project as it was I didnt encounter any issues.

For now i try to avoid switching this specific service as mutli tenant, but I will try to get to it soon, if the issue still persist i will try to update the sample project and try to replicate the issue for you.

In grace the Export default lifescope is Scoped or Singleton?

ipjohnson commented 6 years ago

The default lifestyle is actually transient. I have an example of how to set a default lifestyle to something else but out of the box there is no default lifestyle.

I wish I had some more time to setup an example for this stuff but things have been exceptionally busy.

ipjohnson commented 6 years ago

@stephenlautier any more issue or can I close this out?

stephenlautier commented 6 years ago

@ipjohnson so far its working great, you can close it. Thanks for your help! 👍