search

ipjohnson / Grace

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

Named Singletons sub Singletons - Tenancy issue #228

Closed stephenlautier closed 4 years ago

stephenlautier commented 4 years ago

We had hit an issue when using multi-tenancy (see sample below), when being a singleton.

Basically, we have something like the following small sample:

In the above example if we remove Service B (HerozClientSub) it works, so basically when having nested singleton deps it doesn't resolve the tenant.

By changing Service A and Service B to scoped it will also work, but it's not ideal as it will create instances redundantly, and also not sure why it doesn't resolve or if it's intended or a bug.

This is the sample if you want to look at it:

Changes to cause the issue: https://github.com/sketch7/dotnet.multitenancy/compare/feature/initial-imp...issue/nested-singleton-tenant-bug Some interest points

Basically, the implementation of how tenancy works is how you had suggested.

Am I doing something wrong? should this work?

Thanks

ipjohnson commented 4 years ago

Sorry you ran into that let me take a look this weekend.

stephenlautier commented 4 years ago

No problem, thanks for your help as always

ipjohnson commented 4 years ago

So I took a look and I'm unclear when you say it doesn't work. What specifically is the error you are getting.

I will say that Singleton as a lifestyle is restrictive on what it can depend on and will not carry or use the injection context from the call (it starts fresh, no injection context).

stephenlautier commented 4 years ago

So by starting from fresh, I guess I'm using it wrong/unsupported. What we would like to achieve is to have singletons, and singleton dependencies per tenant as following:

Tenant A - IHeroDataClient (A) singleton -> HerozClientSub (A) singleton Tenant B - IHeroDataClient (B) singleton -> HerozClientSub (B) singleton

Perhaps the ideal is to look at the sample

ipjohnson commented 4 years ago

It sounds very much like you need a custom lifestyle SingletonPerTenant. I don’t have a solution for you right at this moment but can you use SingletonPerScope for the next week while I come up with something?

Ultimately I think this is a use case worth supporting I’ve just never had someone ask for it.

stephenlautier commented 4 years ago

Yes, we are not blocked by this we found a way around it by using less DI and create a manual factory. Not the ideal but at least it's working. It would be much easier if it will be supported as a consumer would need to know about these quirks and there are certain scenarios unhandled.

ipjohnson commented 4 years ago

Sorry been swamped, I haven't forgotten about this. I'm hoping this weekend to finish the SingletonPerKey lifestyle.

stephenlautier commented 4 years ago

@ipjohnson I understand, thanks!

ipjohnson commented 4 years ago

I've released a new version with SingletonPerKey lifestlye. Let me know how it works

ipjohnson commented 4 years ago

You can see an example unit test here

stephenlautier commented 4 years ago

Great, I will try to give it a shot.

In which version is it? 7.1.0-beta768?

ipjohnson commented 4 years ago

Correct.

stephenlautier commented 4 years ago

I just tried it out quickly and it seems to be working fine, altho I had to do a change in the usage from your test, I used scope instead of context if i use the context i wont get the tenant

return configuration.Lifestyle.SingletonPerKey((scope, context) =>
{
    var tenant = scope.GetTenantContext();
    return tenant?.Key;
});

image

ipjohnson commented 4 years ago

That makes sense. The one change I would make is to provide a key if one doesn't exist otherwise it's going to throw an exception.

stephenlautier commented 4 years ago

Great, thanks for your help and for this great feature 👍

ipjohnson commented 4 years ago

I’m going to mark this as closed. If you run into any issue let’s open a new ticket to track the changes