search

Finbuckle / Finbuckle.MultiTenant

Finbuckle.MultiTenant is an open-source multitenancy middleware library for .NET. It enables tenant resolution, per-tenant app behavior, and per-tenant data isolation.
https://www.finbuckle.com/multitenant
Apache License 2.0
1.3k stars 265 forks source link

TryUpdateAsync in MultiTenantStoreWrapper doesn't update existing tenants #165

Closed steebwba closed 5 years ago

steebwba commented 5 years ago

Hi,

I have tried implementing a custom store and also tried with the provided stores, but when I try to update existing tenant info using TryUpdateAsync on the store, the return value is false. From what I can see in MultiTenantStoreWrapper, if the result of TryGetAsync isn't null, so a tenant is found, then the update is skipped.

https://github.com/Finbuckle/Finbuckle.MultiTenant/blob/c5877b0ba7377c9010ac381c56f2585c9f0d87e7/src/Finbuckle.MultiTenant.Core/Stores/MultiTenantStoreWrapper.cs#L125-L130

When looking at the EFCoreStore, the opposite occurs and it will update details when the tenant is found. What I don't understand is, how would the code in EFCoreStore get called.

It appears as though the two areas are conflicting, whereas I would imagine that the update should occur only if the tenant does exist, instead of when it doesn't

AndrewTriesToCode commented 5 years ago

Hi @steebwba thanks for posting this issue, I think you might have found a bug and I am taking a closer look.

AndrewTriesToCode commented 5 years ago

Just to double check-- the code snippet above is from TryAdd, but you are seeing the issue in TryUpdate right? (Which is 99% the same as TryAdd, except it should skip the update on an existing item as you noted)

AndrewTriesToCode commented 5 years ago

In TryUpdate I found this var existing = await TryGetAsync(tenantInfo.Identifier);

Yikes! It should be passing tenantInfo.Id not identifier and then the check should be == null after that.

I'll get this fixed (and add unit tests) for the next release.

steebwba commented 5 years ago

Sorry, I did mean to reference TryUpdateAsync, rather than TryAddAsync! I did make adjustments to the method, but it broke the unit tests for the method so then I started to wonder if it was intended after all!