Reinitialize tenancy for queued jobs if tenant id has changed

[seanmtaylor]

Hi

I have put in the fix as mentioned in #274.

Previously the code would only initialise tenancy when processing a queued job if tenancy wasn't already initialised.

This will now initialise the tenancy if the previously initialised tenant id is different to the tenant_id in the job payload.

---

Fixes #274

[codecov]

Codecov Report

Merging #276 into 2.x will decrease coverage by 0.19%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##                2.x     #276     +/-   ##
===========================================
- Coverage     87.64%   87.45%   -0.2%     
- Complexity      378      381      +3     
===========================================
  Files            58       58             
  Lines          1101     1108      +7     
===========================================
+ Hits            965      969      +4     
- Misses          136      139      +3

Impacted Files	Coverage Δ	Complexity Δ
src/TenancyServiceProvider.php	85.41% <0%> (-5.7%)	8 <0> (+1)
...rc/TenantDatabaseManagers/MySQLDatabaseManager.php	85.71% <0%> (+2.38%)	6% <0%> (+1%)	⬆️
...nantDatabaseManagers/PostgreSQLDatabaseManager.php	83.33% <0%> (+3.33%)	6% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb46767...e3fe37a. Read the comment docs.

[stancl]

So this makes both queue:work and queue:listen work as expected?

@viezel Can you check the original issue #274 and this PR and see if this fixes your "persistent config between jobs" issue?

[seanmtaylor]

So this makes both queue:work and queue:listen work as expected?

@stancl Yes they are both working for me now. The fix ensures that tenancy is reinitialized if the tenant_id in the payload is different to the id of the initialized tenant.

[viezel]

@stancl @seanmtaylor well I don't think this solves it. Hence, this design does not support the custom implementation developers create. An example would be Feature Flags.
Imagine a developer creating feature flags in a SaaS app that sets feature for each tenant. Now the tenancy()->initById($tenantId); does not reinitialize the custom feature flags because it does not know how to. If we don't reinitialize the feature flags, then the next queue job will have incorrect tenant settings.

If we look into others that have the issue of resetting, then this package has defined a config to able the developer to add classes implementing an Interface to be reset on each reinitialize. https://github.com/swooletw/laravel-swoole/blob/master/config/swoole_http.php#L92-L94s
Therefore, I suggest that the solution will include a way for developers to define what would be reset by this package.

[stancl]

Can you elaborate on feature flags? initById() and initializing using a tenant object are almost the same.

[viezel]

@stancl it was just an example. What i'm trying to say is that whatever a developer chooses to develop on the "master side" of the SaaS needs to be reinitialized on each queue job. Typically tenant-specific config.

In my platform, we have things like: CRM Oauth credentials, Email Templates, extensions enabled, extensions settings etc. All these are different from tenant to tenant and need to be reset before the next queue job gets processed.

[stancl]

whatever a developer chooses to develop on the "master side" of the SaaS needs to be reinitialized on each queue job

I don't see what you mean by this.

In my platform, we have things like: CRM Oauth credentials, Email Templates, extensions enabled, extensions settings etc. All these are different from tenant to tenant and need to be reset before the next queue job gets processed.

Why would you need to reset anything? Those things should be stored in Tenant Storage. Since the Tenant instance that is bound in the container is changed when you initialize tenancy for a different tenant, you don't have to manually reset anything.

[viezel]

I see... Maybe in but fully aware of the possibilities with Tenant Storage in this package.
If that's the case - then, by all means, merge it in :)

[stancl]

Even if you configure tenant storage-to-config bindings, they are reset after tenancy is ended/reinitialized, so there shouldn't be any issues with global state.

[stancl]

This basically initializes tenancy if the job payload has a tenant_id and the tenant is different from the current tenant (to avoid re-initializing tenancy in case of dispatchNow, but to re-initialize tenancy if a new job for a different tenant should be executed), right.

I wonder if this could be refactored somehow. This takes me at least a minute to parse in my head which I don't like. But I can't think of a much better way to write this. Maybe something like this?

$tenantId = $event->job->payload()['tenant_id'] ?? null;

if (! $tenantId) { // The job is not tenant-aware
    return;
}

if (tenancy()->initialized && tenant('id') === $tenantId) { // Tenancy is already initialized for the tenant (e.g. dispatchNow was used)
   return;
}

// Tenancy was either not initialized, or initialized for a different tenant. Therefore, we initialize it for the correct tenant.
tenancy()->initById($tenantId);

[seanmtaylor]

Completely agree @stancl. I've refactored the logic to be much easier to understand what's going on