From 78698b01194f1ed040c24d2bf7726be17481ca54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 9 Jan 2025 12:38:23 +0100 Subject: [PATCH 1/7] simplify QueueTenancyBootstrapper --- .../QueueTenancyBootstrapper.php | 80 +++---------------- 1 file changed, 13 insertions(+), 67 deletions(-) diff --git a/src/Bootstrappers/QueueTenancyBootstrapper.php b/src/Bootstrappers/QueueTenancyBootstrapper.php index 16ae043b..06fdb6a7 100644 --- a/src/Bootstrappers/QueueTenancyBootstrapper.php +++ b/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,10 @@ protected static function setUpJobListener(Dispatcher $dispatcher): void 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); + static::revertToPreviousState($event->job->payload()['tenant_id'] ?? null, $previousTenant); + + $previousTenant = null; }; $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds @@ -79,61 +70,25 @@ protected static function setUpJobListener(Dispatcher $dispatcher): void protected static function initializeTenancyForQueue(string|int|null $tenantId): void { - if ($tenantId === null) { - // 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); - + if (! $tenantId) { 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 + // The job was not tenant-aware so no context switch was done if (! $tenantId) { return; } - // Revert back to the previous tenant - if (tenant() && $previousTenant?->isNot(tenant())) { - tenancy()->initialize($previousTenant); - } - - // End tenancy - if (tenant() && (! $previousTenant)) { + // End tenancy when there's no previous tenant + // (= when running in a queue worker, not dispatchSync) + if (tenant() && ! $previousTenant) { tenancy()->end(); } } @@ -149,16 +104,6 @@ protected function setUpPayloadGenerator(): void } } - public function bootstrap(Tenant $tenant): void - { - // - } - - public function revert(): void - { - // - } - public function getPayload(string $connection): array { if (! tenancy()->initialized) { @@ -169,10 +114,11 @@ public function getPayload(string $connection): array return []; } - $id = tenant()->getTenantKey(); - return [ - 'tenant_id' => $id, + 'tenant_id' => tenant()->getTenantKey(), ]; } + + public function bootstrap(Tenant $tenant): void {} + public function revert(): void {} } From 279909f8557059ab9179d813d90b1945b2b9ef3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 9 Jan 2025 16:39:54 +0100 Subject: [PATCH 2/7] wip: add persistent queue bootstrapper, minor testcase refactor --- .../PersistentQueueTenancyBootstrapper.php | 165 ++++++++++++++++++ src/TenancyServiceProvider.php | 7 + tests/Features/NoAttachTest.php | 6 - tests/QueueTest.php | 58 +++--- tests/TestCase.php | 19 +- 5 files changed, 222 insertions(+), 33 deletions(-) create mode 100644 src/Bootstrappers/PersistentQueueTenancyBootstrapper.php diff --git a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php new file mode 100644 index 00000000..2bdc1e03 --- /dev/null +++ b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php @@ -0,0 +1,165 @@ +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, $runningTests) + { + $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, $previousTenant); + } + }; + + $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds + $dispatcher->listen(JobFailed::class, $revertToPreviousState); // artisan('queue:work') which fails + } + + protected static function initializeTenancyForQueue($tenantId) + { + 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 (true) { // todo0 refactor + // Re-initialize tenancy between all jobs + if (tenancy()->initialized) { + tenancy()->end(); + } + + tenancy()->initialize(tenancy()->find($tenantId)); + + return; + } + + if (tenancy()->initialized) { + // Tenancy is already initialized + if (tenant()->getTenantKey() === $tenantId) { + // It's initialized for the same tenant (e.g. dispatchNow 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. + tenancy()->initialize(tenancy()->find($tenantId)); + } + + protected static function revertToPreviousState($event, &$previousTenant) + { + $tenantId = $event->job->payload()['tenant_id'] ?? null; + + // 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() + { + $bootstrapper = &$this; + + if (! $this->queue instanceof QueueFake) { + $this->queue->createPayloadUsing(function ($connection) use (&$bootstrapper) { + return $bootstrapper->getPayload($connection); + }); + } + } + + public function bootstrap(Tenant $tenant): void + { + // + } + + public function revert(): void + { + // + } + + public function getPayload(string $connection) + { + if (! tenancy()->initialized) { + return []; + } + + if ($this->config["queue.connections.$connection.central"]) { + return []; + } + + $id = tenant()->getTenantKey(); + + return [ + 'tenant_id' => $id, + ]; + } +} diff --git a/src/TenancyServiceProvider.php b/src/TenancyServiceProvider.php index 29caf7a7..4059479e 100644 --- a/src/TenancyServiceProvider.php +++ b/src/TenancyServiceProvider.php @@ -4,6 +4,7 @@ namespace Stancl\Tenancy; +use Closure; use Illuminate\Cache\CacheManager; use Illuminate\Database\Console\Migrations\FreshCommand; use Illuminate\Routing\Events\RouteMatched; @@ -18,9 +19,15 @@ 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); diff --git a/tests/Features/NoAttachTest.php b/tests/Features/NoAttachTest.php index 9ba6079d..e82f3eb4 100644 --- a/tests/Features/NoAttachTest.php +++ b/tests/Features/NoAttachTest.php @@ -20,12 +20,6 @@ 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 diff --git a/tests/QueueTest.php b/tests/QueueTest.php index 4d3f5ce0..0688acc1 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -22,16 +22,12 @@ 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' => [ - QueueTenancyBootstrapper::class, - DatabaseTenancyBootstrapper::class, - ], + 'tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class], 'queue.default' => 'redis', ]); @@ -45,7 +41,22 @@ 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 @@ 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 @@ 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 @@ $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 @@ 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 @@ $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,9 +223,11 @@ 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() { +// todo0 confirm if this should be passing with the persistent bootstrapper +test('tenant connections do not persist after tenant jobs get processed', function (string $bootstrapper) { + withQueueBootstrapper($bootstrapper); withTenantDatabases(); $tenant = Tenant::create(); @@ -224,10 +241,11 @@ pest()->artisan('queue:work --once'); expect(collect(DB::select('SHOW FULL PROCESSLIST'))->pluck('db'))->not()->toContain($tenant->database()->getName()); -}); +})->with('queue_bootstrappers'); // 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 +259,7 @@ expect(tenant())->toBe(null); expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant->getTenantKey()); -}); +})->with('queue_bootstrappers'); function createValueStore(): void { diff --git a/tests/TestCase.php b/tests/TestCase.php index f9a2d357..7fc1813b 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -22,6 +22,7 @@ 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 @@ protected function setUp(): void '--realpath' => true, ]); - // Laravel 6.x support todo@refactor clean up - $testResponse = class_exists('Illuminate\Testing\TestResponse') ? 'Illuminate\Testing\TestResponse' : 'Illuminate\Foundation\Testing\TestResponse'; - $testResponse::macro('assertContent', function ($content) { - $assertClass = class_exists('Illuminate\Testing\Assert') ? 'Illuminate\Testing\Assert' : 'Illuminate\Foundation\Testing\Assert'; - $assertClass::assertSame($content, $this->baseResponse->getContent()); + \Illuminate\Testing\TestResponse::macro('assertContent', function ($content) { + \Illuminate\Testing\Assert::assertSame($content, $this->baseResponse->getContent()); return $this; }); @@ -175,18 +173,25 @@ protected function getEnvironmentSetUp($app) '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 - $app->singleton(CacheTenancyBootstrapper::class); // todo@samuel use proper approach eg config for singleton registration + // Since we run the TSP with no bootstrappers enabled, we need + // 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, ]; From 8f4cd186f9176f8cb0c2bb95e02c7ad294526224 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 9 Jan 2025 16:41:07 +0100 Subject: [PATCH 3/7] ci: run persistent queue tests --- .github/workflows/queue.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/queue.yml b/.github/workflows/queue.yml index 0f3ec82e..4bd30f02 100644 --- a/.github/workflows/queue.yml +++ b/.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 From e6f564c47a293fe04589c7b678bc045f17bdf6c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 9 Jan 2025 18:02:44 +0100 Subject: [PATCH 4/7] simplify persistent queue bootstrapper --- .../PersistentQueueTenancyBootstrapper.php | 46 +++++-------------- tests/QueueTest.php | 2 +- 2 files changed, 13 insertions(+), 35 deletions(-) diff --git a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php index 2bdc1e03..a51a24ee 100644 --- a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php +++ b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php @@ -63,6 +63,10 @@ protected static function setUpJobListener($dispatcher, $runningTests) if ($runningTests) { static::revertToPreviousState($event, $previousTenant); } + + // 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 @@ -81,27 +85,10 @@ protected static function initializeTenancyForQueue($tenantId) return; } - if (true) { // todo0 refactor - // Re-initialize tenancy between all jobs - if (tenancy()->initialized) { - tenancy()->end(); - } - - tenancy()->initialize(tenancy()->find($tenantId)); - - return; - } - - if (tenancy()->initialized) { - // Tenancy is already initialized - if (tenant()->getTenantKey() === $tenantId) { - // It's initialized for the same tenant (e.g. dispatchNow 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. + // 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(); tenancy()->initialize(tenancy()->find($tenantId)); } @@ -136,16 +123,6 @@ protected function setUpPayloadGenerator() } } - public function bootstrap(Tenant $tenant): void - { - // - } - - public function revert(): void - { - // - } - public function getPayload(string $connection) { if (! tenancy()->initialized) { @@ -156,10 +133,11 @@ public function getPayload(string $connection) return []; } - $id = tenant()->getTenantKey(); - return [ - 'tenant_id' => $id, + 'tenant_id' => tenant()->getTenantKey(), ]; } + + public function bootstrap(Tenant $tenant): void {} + public function revert(): void {} } diff --git a/tests/QueueTest.php b/tests/QueueTest.php index 0688acc1..af13cc69 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -225,7 +225,7 @@ function withQueueBootstrapper(string $class) { expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant1->getTenantKey()); })->with('queue_bootstrappers'); -// todo0 confirm if this should be passing with the persistent bootstrapper +// todo0 false positive -- remove after reproducing original issue test('tenant connections do not persist after tenant jobs get processed', function (string $bootstrapper) { withQueueBootstrapper($bootstrapper); withTenantDatabases(); From 1f8db1ce6af64e0ea58713e5702d8c10b8bc11a4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 9 Jan 2025 17:03:11 +0000 Subject: [PATCH 5/7] Fix code style (php-cs-fixer) --- src/Bootstrappers/PersistentQueueTenancyBootstrapper.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php index a51a24ee..3258769a 100644 --- a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php +++ b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php @@ -5,16 +5,16 @@ namespace Stancl\Tenancy\Bootstrappers; use Illuminate\Config\Repository; -use Illuminate\Queue\QueueManager; -use Stancl\Tenancy\Contracts\Tenant; +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\Contracts\Events\Dispatcher; use Illuminate\Queue\Events\JobRetryRequested; +use Illuminate\Queue\QueueManager; use Illuminate\Support\Testing\Fakes\QueueFake; -use Illuminate\Contracts\Foundation\Application; use Stancl\Tenancy\Contracts\TenancyBootstrapper; +use Stancl\Tenancy\Contracts\Tenant; class PersistentQueueTenancyBootstrapper implements TenancyBootstrapper { From 743de4dddbe57fe92bddab474b57af9d4c074644 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sat, 11 Jan 2025 11:47:57 +0100 Subject: [PATCH 6/7] phpstan fixes, clarify previousTenant use --- .../PersistentQueueTenancyBootstrapper.php | 23 +++++++++++-------- .../QueueTenancyBootstrapper.php | 6 +++-- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php index 3258769a..90bb3d08 100644 --- a/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php +++ b/src/Bootstrappers/PersistentQueueTenancyBootstrapper.php @@ -29,7 +29,7 @@ class PersistentQueueTenancyBootstrapper implements TenancyBootstrapper * 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) + public static function __constructStatic(Application $app): void { static::setUpJobListener($app->make(Dispatcher::class), $app->runningUnitTests()); } @@ -42,7 +42,7 @@ public function __construct(Repository $config, QueueManager $queue) $this->setUpPayloadGenerator(); } - protected static function setUpJobListener($dispatcher, $runningTests) + protected static function setUpJobListener(Dispatcher $dispatcher, bool $runningTests): void { $previousTenant = null; @@ -61,7 +61,9 @@ protected static function setUpJobListener($dispatcher, $runningTests) // 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, $previousTenant); + 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 @@ -73,7 +75,7 @@ protected static function setUpJobListener($dispatcher, $runningTests) $dispatcher->listen(JobFailed::class, $revertToPreviousState); // artisan('queue:work') which fails } - protected static function initializeTenancyForQueue($tenantId) + protected static function initializeTenancyForQueue(string|int|null $tenantId): void { if (! $tenantId) { // The job is not tenant-aware @@ -89,13 +91,14 @@ protected static function initializeTenancyForQueue($tenantId) // so that we don't work with an outdated tenant() instance in case it // was updated outside the queue worker. tenancy()->end(); - tenancy()->initialize(tenancy()->find($tenantId)); + + /** @var Tenant $tenant */ + $tenant = tenancy()->find($tenantId); + tenancy()->initialize($tenant); } - protected static function revertToPreviousState($event, &$previousTenant) + protected static function revertToPreviousState(string|int|null $tenantId, ?Tenant $previousTenant): void { - $tenantId = $event->job->payload()['tenant_id'] ?? null; - // The job was not tenant-aware if (! $tenantId) { return; @@ -112,7 +115,7 @@ protected static function revertToPreviousState($event, &$previousTenant) } } - protected function setUpPayloadGenerator() + protected function setUpPayloadGenerator(): void { $bootstrapper = &$this; @@ -123,7 +126,7 @@ protected function setUpPayloadGenerator() } } - public function getPayload(string $connection) + public function getPayload(string $connection): array { if (! tenancy()->initialized) { return []; diff --git a/src/Bootstrappers/QueueTenancyBootstrapper.php b/src/Bootstrappers/QueueTenancyBootstrapper.php index 06fdb6a7..2ed6b7f1 100644 --- a/src/Bootstrappers/QueueTenancyBootstrapper.php +++ b/src/Bootstrappers/QueueTenancyBootstrapper.php @@ -59,9 +59,11 @@ protected static function setUpJobListener(Dispatcher $dispatcher): void }); $revertToPreviousState = function ($event) use (&$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); - - $previousTenant = null; }; $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds From 639ffeec650ba12a43b85e4aad486cffef38b11e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sat, 11 Jan 2025 11:51:44 +0100 Subject: [PATCH 7/7] remove false positive regression test --- tests/QueueTest.php | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/tests/QueueTest.php b/tests/QueueTest.php index af13cc69..2095cc84 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -225,24 +225,6 @@ function withQueueBootstrapper(string $class) { expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant1->getTenantKey()); })->with('queue_bootstrappers'); -// todo0 false positive -- remove after reproducing original issue -test('tenant connections do not persist after tenant jobs get processed', function (string $bootstrapper) { - withQueueBootstrapper($bootstrapper); - 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()); -})->with('queue_bootstrappers'); - // Regression test for #1277 test('dispatching a job from a tenant run arrow function dispatches it immediately', function (string $bootstrapper) { withQueueBootstrapper($bootstrapper);