[4.x] Queue logic refactor (#1289)

* simplify QueueTenancyBootstrapper

* wip: add persistent queue bootstrapper, minor testcase refactor

* ci: run persistent queue tests

* simplify persistent queue bootstrapper

* Fix code style (php-cs-fixer)

* phpstan fixes, clarify previousTenant use

* remove false positive regression test

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

.github/workflows/queue.yml

@@ -25,3 +25,5 @@ jobs:
           cd tenancy-queue-tester
           TENANCY_VERSION=${VERSION_PREFIX}#${GITHUB_SHA} ./setup.sh
           ./test.sh
+          ./alternative_config.sh
+          PERSISTENT=1 ./test.sh

src/Bootstrappers/PersistentQueueTenancyBootstrapper.php

@@ -0,0 +1,146 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Stancl\Tenancy\Bootstrappers;
+
+use Illuminate\Config\Repository;
+use Illuminate\Contracts\Events\Dispatcher;
+use Illuminate\Contracts\Foundation\Application;
+use Illuminate\Queue\Events\JobFailed;
+use Illuminate\Queue\Events\JobProcessed;
+use Illuminate\Queue\Events\JobProcessing;
+use Illuminate\Queue\Events\JobRetryRequested;
+use Illuminate\Queue\QueueManager;
+use Illuminate\Support\Testing\Fakes\QueueFake;
+use Stancl\Tenancy\Contracts\TenancyBootstrapper;
+use Stancl\Tenancy\Contracts\Tenant;
+
+class PersistentQueueTenancyBootstrapper implements TenancyBootstrapper
+{
+    /** @var Repository */
+    protected $config;
+
+    /** @var QueueManager */
+    protected $queue;
+
+    /**
+     * The normal constructor is only executed after tenancy is bootstrapped.
+     * However, we're registering a hook to initialize tenancy. Therefore,
+     * we need to register the hook at service provider execution time.
+     */
+    public static function __constructStatic(Application $app): void
+    {
+        static::setUpJobListener($app->make(Dispatcher::class), $app->runningUnitTests());
+    }
+
+    public function __construct(Repository $config, QueueManager $queue)
+    {
+        $this->config = $config;
+        $this->queue = $queue;
+
+        $this->setUpPayloadGenerator();
+    }
+
+    protected static function setUpJobListener(Dispatcher $dispatcher, bool $runningTests): void
+    {
+        $previousTenant = null;
+
+        $dispatcher->listen(JobProcessing::class, function ($event) use (&$previousTenant) {
+            $previousTenant = tenant();
+
+            static::initializeTenancyForQueue($event->job->payload()['tenant_id'] ?? null);
+        });
+
+        $dispatcher->listen(JobRetryRequested::class, function ($event) use (&$previousTenant) {
+            $previousTenant = tenant();
+
+            static::initializeTenancyForQueue($event->payload()['tenant_id'] ?? null);
+        });
+
+        // If we're running tests, we make sure to clean up after any artisan('queue:work') calls
+        $revertToPreviousState = function ($event) use (&$previousTenant, $runningTests) {
+            if ($runningTests) {
+                static::revertToPreviousState($event->job->payload()['tenant_id'] ?? null, $previousTenant);
+
+                // We don't need to reset $previousTenant since the value will be set again when a job is processed.
+            }
+
+            // If we're not running tests, we remain in the tenant's context. This makes other JobProcessed
+            // listeners able to deserialize the job, including with SerializesModels, since the tenant connection
+            // remains open.
+        };
+
+        $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds
+        $dispatcher->listen(JobFailed::class, $revertToPreviousState); // artisan('queue:work') which fails
+    }
+
+    protected static function initializeTenancyForQueue(string|int|null $tenantId): void
+    {
+        if (! $tenantId) {
+            // The job is not tenant-aware
+            if (tenancy()->initialized) {
+                // Tenancy was initialized, so we revert back to the central context
+                tenancy()->end();
+            }
+
+            return;
+        }
+
+        // Re-initialize tenancy between all jobs even if the tenant is the same
+        // so that we don't work with an outdated tenant() instance in case it
+        // was updated outside the queue worker.
+        tenancy()->end();
+
+        /** @var Tenant $tenant */
+        $tenant = tenancy()->find($tenantId);
+        tenancy()->initialize($tenant);
+    }
+
+    protected static function revertToPreviousState(string|int|null $tenantId, ?Tenant $previousTenant): void
+    {
+        // The job was not tenant-aware
+        if (! $tenantId) {
+            return;
+        }
+
+        // Revert back to the previous tenant
+        if (tenant() && $previousTenant && $previousTenant->isNot(tenant())) {
+            tenancy()->initialize($previousTenant);
+        }
+
+        // End tenancy
+        if (tenant() && (! $previousTenant)) {
+            tenancy()->end();
+        }
+    }
+
+    protected function setUpPayloadGenerator(): void
+    {
+        $bootstrapper = &$this;
+
+        if (! $this->queue instanceof QueueFake) {
+            $this->queue->createPayloadUsing(function ($connection) use (&$bootstrapper) {
+                return $bootstrapper->getPayload($connection);
+            });
+        }
+    }
+
+    public function getPayload(string $connection): array
+    {
+        if (! tenancy()->initialized) {
+            return [];
+        }
+
+        if ($this->config["queue.connections.$connection.central"]) {
+            return [];
+        }
+
+        return [
+            'tenant_id' => tenant()->getTenantKey(),
+        ];
+    }
+
+    public function bootstrap(Tenant $tenant): void {}
+    public function revert(): void {}
+}

src/Bootstrappers/QueueTenancyBootstrapper.php

@@ -24,16 +24,6 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
     /** @var QueueManager */
     protected $queue;
 
-    /**
-     * Don't persist the same tenant across multiple jobs even if they have the same tenant ID.
-     *
-     * This is useful when you're changing the tenant's state (e.g. properties in the `data` column) and want the next job to initialize tenancy again
-     * with the new data. Features like the Tenant Config are only executed when tenancy is initialized, so the re-initialization is needed in some cases.
-     *
-     * @var bool
-     */
-    public static $forceRefresh = false;
-
     /**
      * The normal constructor is only executed after tenancy is bootstrapped.
      * However, we're registering a hook to initialize tenancy. Therefore,
@@ -68,9 +58,12 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
             static::initializeTenancyForQueue($event->payload()['tenant_id'] ?? null);
         });
 
-        // If we're running tests, we make sure to clean up after any artisan('queue:work') calls
         $revertToPreviousState = function ($event) use (&$previousTenant) {
-            static::revertToPreviousState($event, $previousTenant);
+            // In queue worker context, this reverts to the central context.
+            // In dispatchSync context, this reverts to the previous tenant's context.
+            // There's no need to reset $previousTenant here since it's always first
+            // set in the above listeners and the app is reverted back to that context.
+            static::revertToPreviousState($event->job->payload()['tenant_id'] ?? null, $previousTenant);
         };
 
         $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds
@@ -79,61 +72,25 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
 
     protected static function initializeTenancyForQueue(string|int|null $tenantId): void
     {
-        if ($tenantId === null) {
+        if (! $tenantId) {
-            // The job is not tenant-aware
-            if (tenancy()->initialized) {
-                // Tenancy was initialized, so we revert back to the central context
-                tenancy()->end();
-            }
-
             return;
         }
 
-        if (static::$forceRefresh) {
-            // Re-initialize tenancy between all jobs
-            if (tenancy()->initialized) {
-                tenancy()->end();
-            }
-
-            /** @var Tenant $tenant */
-            $tenant = tenancy()->find($tenantId);
-            tenancy()->initialize($tenant);
-
-            return;
-        }
-
-        if (tenancy()->initialized) {
-            // Tenancy is already initialized
-            if (tenant()->getTenantKey() === $tenantId) {
-                // It's initialized for the same tenant (e.g. dispatchSync was used, or the previous job also ran for this tenant)
-                return;
-            }
-        }
-
-        // Tenancy was either not initialized, or initialized for a different tenant.
-        // Therefore, we initialize it for the correct tenant.
-
         /** @var Tenant $tenant */
         $tenant = tenancy()->find($tenantId);
         tenancy()->initialize($tenant);
     }
 
-    protected static function revertToPreviousState(JobProcessed|JobFailed $event, ?Tenant &$previousTenant): void
+    protected static function revertToPreviousState(string|int|null $tenantId, ?Tenant $previousTenant): void
     {
-        $tenantId = $event->job->payload()['tenant_id'] ?? null;
+        // The job was not tenant-aware so no context switch was done
-
-        // The job was not tenant-aware
         if (! $tenantId) {
             return;
         }
 
-        // Revert back to the previous tenant
+        // End tenancy when there's no previous tenant
-        if (tenant() && $previousTenant?->isNot(tenant())) {
+        // (= when running in a queue worker, not dispatchSync)
-            tenancy()->initialize($previousTenant);
+        if (tenant() && ! $previousTenant) {
-        }
-
-        // End tenancy
-        if (tenant() && (! $previousTenant)) {
             tenancy()->end();
         }
     }
@@ -149,16 +106,6 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
         }
     }
 
-    public function bootstrap(Tenant $tenant): void
-    {
-        //
-    }
-
-    public function revert(): void
-    {
-        //
-    }
-
     public function getPayload(string $connection): array
     {
         if (! tenancy()->initialized) {
@@ -169,10 +116,11 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
             return [];
         }
 
-        $id = tenant()->getTenantKey();
-
         return [
-            'tenant_id' => $id,
+            'tenant_id' => tenant()->getTenantKey(),
         ];
     }
+
+    public function bootstrap(Tenant $tenant): void {}
+    public function revert(): void {}
 }

src/TenancyServiceProvider.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Stancl\Tenancy;
 
+use Closure;
 use Illuminate\Cache\CacheManager;
 use Illuminate\Database\Console\Migrations\FreshCommand;
 use Illuminate\Routing\Events\RouteMatched;
@@ -18,9 +19,15 @@ use Stancl\Tenancy\Resolvers\DomainTenantResolver;
 
 class TenancyServiceProvider extends ServiceProvider
 {
+    public static Closure|null $configure = null;
+
     /* Register services. */
     public function register(): void
     {
+        if (static::$configure) {
+            (static::$configure)();
+        }
+
         $this->mergeConfigFrom(__DIR__ . '/../assets/config.php', 'tenancy');
 
         $this->app->singleton(Database\DatabaseManager::class);

tests/Features/NoAttachTest.php

@@ -20,12 +20,6 @@ use Stancl\Tenancy\Middleware\InitializeTenancyByPath;
 use Stancl\Tenancy\Tests\Etc\Tenant;
 
 test('sqlite ATTACH statements can be blocked', function (bool $disallow) {
-    try {
-        readlink(base_path('vendor'));
-    } catch (\Throwable) {
-        symlink(base_path('vendor'), '/var/www/html/vendor');
-    }
-
     if (php_uname('m') == 'aarch64') {
         // Escape testbench prison. Can't hardcode /var/www/html/extensions/... here
         // since GHA doesn't mount the filesystem on the container's workdir

tests/QueueTest.php

@@ -22,16 +22,12 @@ use Stancl\Tenancy\Listeners\BootstrapTenancy;
 use Stancl\Tenancy\Listeners\RevertToCentralContext;
 use Stancl\Tenancy\Bootstrappers\QueueTenancyBootstrapper;
 use Stancl\Tenancy\Bootstrappers\DatabaseTenancyBootstrapper;
+use Stancl\Tenancy\Bootstrappers\PersistentQueueTenancyBootstrapper;
 use Stancl\Tenancy\Listeners\QueueableListener;
 
 beforeEach(function () {
-    $this->mockConsoleOutput = false;
-
     config([
-        'tenancy.bootstrappers' => [
+        'tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class],
-            QueueTenancyBootstrapper::class,
-            DatabaseTenancyBootstrapper::class,
-        ],
         'queue.default' => 'redis',
     ]);
 
@@ -45,7 +41,22 @@ afterEach(function () {
     pest()->valuestore->flush();
 });
 
-test('tenant id is passed to tenant queues', function () {
+dataset('queue_bootstrappers', [
+    QueueTenancyBootstrapper::class,
+    PersistentQueueTenancyBootstrapper::class,
+]);
+
+function withQueueBootstrapper(string $class) {
+    config(['tenancy.bootstrappers' => [
+        DatabaseTenancyBootstrapper::class,
+        $class,
+    ]]);
+
+    $class::__constructStatic(app());
+}
+
+test('tenant id is passed to tenant queues', function (string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withTenantDatabases();
 
     config(['queue.default' => 'sync']);
@@ -61,9 +72,10 @@ test('tenant id is passed to tenant queues', function () {
     Event::assertDispatched(JobProcessing::class, function ($event) {
         return $event->job->payload()['tenant_id'] === tenant('id');
     });
-});
+})->with('queue_bootstrappers');
 
-test('tenant id is not passed to central queues', function () {
+test('tenant id is not passed to central queues', function (string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withTenantDatabases();
 
     $tenant = Tenant::create();
@@ -82,9 +94,10 @@ test('tenant id is not passed to central queues', function () {
     Event::assertDispatched(JobProcessing::class, function ($event) {
         return ! isset($event->job->payload()['tenant_id']);
     });
-});
+})->with('queue_bootstrappers');
 
-test('tenancy is initialized inside queues', function (bool $shouldEndTenancy) {
+test('tenancy is initialized inside queues', function (bool $shouldEndTenancy, string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withTenantDatabases();
     withFailedJobs();
 
@@ -117,7 +130,7 @@ test('tenancy is initialized inside queues', function (bool $shouldEndTenancy) {
     $tenant->run(function () use ($user) {
         expect($user->fresh()->name)->toBe('Bar');
     });
-})->with([true, false]);
+})->with([true, false])->with('queue_bootstrappers');
 
 test('changing the shouldQueue static property in parent class affects child classes unless the property is redefined', function () {
     // Parent – $shouldQueue is true
@@ -142,7 +155,8 @@ test('changing the shouldQueue static property in parent class affects child cla
     expect(app(ShouldNotQueueListener::class)->shouldQueue(new stdClass()))->toBeFalse();
 });
 
-test('tenancy is initialized when retrying jobs', function (bool $shouldEndTenancy) {
+test('tenancy is initialized when retrying jobs', function (bool $shouldEndTenancy, string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withFailedJobs();
     withTenantDatabases();
 
@@ -189,9 +203,10 @@ test('tenancy is initialized when retrying jobs', function (bool $shouldEndTenan
     $tenant->run(function () use ($user) {
         expect($user->fresh()->name)->toBe('Bar');
     });
-})->with([true, false]);
+})->with([true, false])->with('queue_bootstrappers');
 
-test('the tenant used by the job doesnt change when the current tenant changes', function () {
+test('the tenant used by the job doesnt change when the current tenant changes', function (string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withTenantDatabases();
 
     $tenant1 = Tenant::create();
@@ -208,26 +223,11 @@ test('the tenant used by the job doesnt change when the current tenant changes',
     pest()->artisan('queue:work --once');
 
     expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant1->getTenantKey());
-});
+})->with('queue_bootstrappers');
-
-test('tenant connections do not persist after tenant jobs get processed', function() {
-    withTenantDatabases();
-
-    $tenant = Tenant::create();
-
-    tenancy()->initialize($tenant);
-
-    dispatch(new TestJob(pest()->valuestore));
-
-    tenancy()->end();
-
-    pest()->artisan('queue:work --once');
-
-    expect(collect(DB::select('SHOW FULL PROCESSLIST'))->pluck('db'))->not()->toContain($tenant->database()->getName());
-});
 
 // Regression test for #1277
-test('dispatching a job from a tenant run arrow function dispatches it immediately', function () {
+test('dispatching a job from a tenant run arrow function dispatches it immediately', function (string $bootstrapper) {
+    withQueueBootstrapper($bootstrapper);
     withTenantDatabases();
 
     $tenant = Tenant::create();
@@ -241,7 +241,7 @@ test('dispatching a job from a tenant run arrow function dispatches it immediate
     expect(tenant())->toBe(null);
 
     expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant->getTenantKey());
-});
+})->with('queue_bootstrappers');
 
 function createValueStore(): void
 {

tests/TestCase.php

@@ -22,6 +22,7 @@ use Stancl\Tenancy\Bootstrappers\RedisTenancyBootstrapper;
 use Stancl\Tenancy\Bootstrappers\UrlGeneratorBootstrapper;
 use Stancl\Tenancy\Bootstrappers\BroadcastingConfigBootstrapper;
 use Stancl\Tenancy\Bootstrappers\BroadcastChannelPrefixBootstrapper;
+use Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper;
 
 abstract class TestCase extends \Orchestra\Testbench\TestCase
 {
@@ -85,11 +86,8 @@ abstract class TestCase extends \Orchestra\Testbench\TestCase
             '--realpath' => true,
         ]);
 
-        // Laravel 6.x support todo@refactor clean up
+        \Illuminate\Testing\TestResponse::macro('assertContent', function ($content) {
-        $testResponse = class_exists('Illuminate\Testing\TestResponse') ? 'Illuminate\Testing\TestResponse' : 'Illuminate\Foundation\Testing\TestResponse';
+            \Illuminate\Testing\Assert::assertSame($content, $this->baseResponse->getContent());
-        $testResponse::macro('assertContent', function ($content) {
-            $assertClass = class_exists('Illuminate\Testing\Assert') ? 'Illuminate\Testing\Assert' : 'Illuminate\Foundation\Testing\Assert';
-            $assertClass::assertSame($content, $this->baseResponse->getContent());
 
             return $this;
         });
@@ -175,18 +173,25 @@ abstract class TestCase extends \Orchestra\Testbench\TestCase
             'tenancy.models.tenant' => Tenant::class, // Use test tenant w/ DBs & domains
         ]);
 
-        $app->singleton(RedisTenancyBootstrapper::class); // todo@samuel use proper approach eg config for singleton registration
+        // Since we run the TSP with no bootstrappers enabled, we need
-        $app->singleton(CacheTenancyBootstrapper::class); // todo@samuel use proper approach eg config for singleton registration
+        // to manually register bootstrappers as singletons here.
+        $app->singleton(RedisTenancyBootstrapper::class);
+        $app->singleton(CacheTenancyBootstrapper::class);
         $app->singleton(BroadcastingConfigBootstrapper::class);
         $app->singleton(BroadcastChannelPrefixBootstrapper::class);
         $app->singleton(PostgresRLSBootstrapper::class);
         $app->singleton(MailConfigBootstrapper::class);
         $app->singleton(RootUrlBootstrapper::class);
         $app->singleton(UrlGeneratorBootstrapper::class);
+        $app->singleton(FilesystemTenancyBootstrapper::class);
     }
 
     protected function getPackageProviders($app)
     {
+        TenancyServiceProvider::$configure = function () {
+            config(['tenancy.bootstrappers' => []]);
+        };
+
         return [
             TenancyServiceProvider::class,
         ];