From 3e78410d8a5d79dbbe0c4e225b1dcdaa8e44216b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Thu, 17 Oct 2019 13:55:10 +0200 Subject: [PATCH] [2.1.0] Fix queue race conditions (#178) * Add TenantDoesNotExist exception, fix queued migrator serialization * wip * Apply fixes from StyleCI * Use only one config for queueing * Apply fixes from StyleCI * rename test * rename test --- assets/config.php | 5 ++-- src/DatabaseManager.php | 8 ++++-- .../TenantDoesNotExistException.php | 15 +++++++++++ src/Jobs/QueuedTenantDatabaseMigrator.php | 8 +++--- .../Database/DatabaseStorageDriver.php | 9 ++++++- src/StorageDrivers/RedisStorageDriver.php | 9 ++++++- src/TenantManager.php | 26 ++++++++++++------- tests/TenantManagerTest.php | 26 ++++++++++++++----- 8 files changed, 78 insertions(+), 28 deletions(-) create mode 100644 src/Exceptions/TenantDoesNotExistException.php diff --git a/assets/config.php b/assets/config.php index b36bdbba..a6b84e68 100644 --- a/assets/config.php +++ b/assets/config.php @@ -89,10 +89,9 @@ return [ // 'paypal_api_key' => 'services.paypal.api_key', ], 'home_url' => '/app', - 'migrate_after_creation' => false, // run migrations after creating a tenant - 'queue_automatic_migration' => false, // queue the automatic post-tenant-creation migrations - 'delete_database_after_tenant_deletion' => false, // delete the tenant's database after deleting the tenant 'queue_database_creation' => false, + 'migrate_after_creation' => false, // run migrations after creating a tenant 'queue_database_deletion' => false, + 'delete_database_after_tenant_deletion' => false, // delete the tenant's database after deleting the tenant 'unique_id_generator' => Stancl\Tenancy\UniqueIDGenerators\UUIDGenerator::class, ]; diff --git a/src/DatabaseManager.php b/src/DatabaseManager.php index c36b51fa..f480e4b7 100644 --- a/src/DatabaseManager.php +++ b/src/DatabaseManager.php @@ -126,17 +126,21 @@ class DatabaseManager * Create a database for a tenant. * * @param Tenant $tenant + * @param \Illuminate\Contracts\Queue\ShouldQueue[]|callable[] $afterCreating * @return void */ - public function createDatabase(Tenant $tenant) + public function createDatabase(Tenant $tenant, array $afterCreating = []) { $database = $tenant->getDatabaseName(); $manager = $this->getTenantDatabaseManager($tenant); if ($this->app['config']['tenancy.queue_database_creation'] ?? false) { - QueuedTenantDatabaseCreator::dispatch($manager, $database); + QueuedTenantDatabaseCreator::withChain($afterCreating)->dispatch($manager, $database); } else { $manager->createDatabase($database); + foreach ($afterCreating as $callback) { + $callback(); + } } } diff --git a/src/Exceptions/TenantDoesNotExistException.php b/src/Exceptions/TenantDoesNotExistException.php new file mode 100644 index 00000000..eb87701f --- /dev/null +++ b/src/Exceptions/TenantDoesNotExistException.php @@ -0,0 +1,15 @@ +message = "Tenant with this id does not exist: $id"; + } +} diff --git a/src/Jobs/QueuedTenantDatabaseMigrator.php b/src/Jobs/QueuedTenantDatabaseMigrator.php index 84acca66..5ea12656 100644 --- a/src/Jobs/QueuedTenantDatabaseMigrator.php +++ b/src/Jobs/QueuedTenantDatabaseMigrator.php @@ -16,12 +16,12 @@ class QueuedTenantDatabaseMigrator implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; - /** @var Tenant */ - protected $tenant; + /** @var string */ + protected $tenantId; public function __construct(Tenant $tenant) { - $this->tenant = $tenant; + $this->tenantId = $tenant->id; } /** @@ -32,7 +32,7 @@ class QueuedTenantDatabaseMigrator implements ShouldQueue public function handle() { Artisan::call('tenants:migrate', [ - '--tenants' => [$this->tenant->id], + '--tenants' => [$this->tenantId], ]); } } diff --git a/src/StorageDrivers/Database/DatabaseStorageDriver.php b/src/StorageDrivers/Database/DatabaseStorageDriver.php index 4f5c64bb..95df006e 100644 --- a/src/StorageDrivers/Database/DatabaseStorageDriver.php +++ b/src/StorageDrivers/Database/DatabaseStorageDriver.php @@ -10,6 +10,7 @@ use Stancl\Tenancy\Contracts\StorageDriver; use Stancl\Tenancy\DatabaseManager; use Stancl\Tenancy\Exceptions\DomainsOccupiedByOtherTenantException; use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedException; +use Stancl\Tenancy\Exceptions\TenantDoesNotExistException; use Stancl\Tenancy\Exceptions\TenantWithThisIdAlreadyExistsException; use Stancl\Tenancy\StorageDrivers\Database\DomainModel as Domains; use Stancl\Tenancy\StorageDrivers\Database\TenantModel as Tenants; @@ -59,7 +60,13 @@ class DatabaseStorageDriver implements StorageDriver public function findById(string $id): Tenant { - return Tenant::fromStorage(Tenants::find($id)->decoded()) + $tenant = Tenants::find($id); + + if (! $tenant) { + throw new TenantDoesNotExistException($id); + } + + return Tenant::fromStorage($tenant->decoded()) ->withDomains($this->getTenantDomains($id)); } diff --git a/src/StorageDrivers/RedisStorageDriver.php b/src/StorageDrivers/RedisStorageDriver.php index 4e35834f..862f5069 100644 --- a/src/StorageDrivers/RedisStorageDriver.php +++ b/src/StorageDrivers/RedisStorageDriver.php @@ -9,6 +9,7 @@ use Illuminate\Foundation\Application; use Stancl\Tenancy\Contracts\StorageDriver; use Stancl\Tenancy\Exceptions\DomainsOccupiedByOtherTenantException; use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedException; +use Stancl\Tenancy\Exceptions\TenantDoesNotExistException; use Stancl\Tenancy\Exceptions\TenantWithThisIdAlreadyExistsException; use Stancl\Tenancy\Tenant; @@ -73,7 +74,13 @@ class RedisStorageDriver implements StorageDriver public function findById(string $id): Tenant { - return $this->makeTenant($this->redis->hgetall("tenants:$id")); + $data = $this->redis->hgetall("tenants:$id"); + + if (! $data) { + throw new TenantDoesNotExistException($id); + } + + return $this->makeTenant($data); } public function getTenantIdByDomain(string $domain): ?string diff --git a/src/TenantManager.php b/src/TenantManager.php index 9e2f2c6d..16030245 100644 --- a/src/TenantManager.php +++ b/src/TenantManager.php @@ -62,18 +62,24 @@ class TenantManager $this->ensureTenantCanBeCreated($tenant); $this->storage->createTenant($tenant); - $this->database->createDatabase($tenant); + + /** @var \Illuminate\Contracts\Queue\ShouldQueue[]|callable[] $afterCreating */ + $afterCreating = []; if ($this->shouldMigrateAfterCreation()) { - if ($this->shouldQueueMigration()) { - QueuedTenantDatabaseMigrator::dispatch($tenant); - } else { - $this->artisan->call('tenants:migrate', [ - '--tenants' => [$tenant['id']], - ]); - } + $afterCreating += $this->databaseCreationQueued() ? [ + new QueuedTenantDatabaseMigrator($tenant), + ] : [ + function () use ($tenant) { + $this->artisan->call('tenants:migrate', [ + '--tenants' => [$tenant['id']], + ]); + }, + ]; } + $this->database->createDatabase($tenant, $afterCreating); + return $this; } @@ -315,9 +321,9 @@ class TenantManager return $this->app['config']['tenancy.migrate_after_creation'] ?? false; } - public function shouldQueueMigration(): bool + public function databaseCreationQueued(): bool { - return $this->app['config']['tenancy.queue_automatic_migration'] ?? false; + return $this->app['config']['tenancy.queue_database_creation'] ?? false; } public function shouldDeleteDatabase(): bool diff --git a/tests/TenantManagerTest.php b/tests/TenantManagerTest.php index 7f8aae0d..64a7ac5a 100644 --- a/tests/TenantManagerTest.php +++ b/tests/TenantManagerTest.php @@ -8,7 +8,9 @@ use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; use Illuminate\Support\Facades\Storage; use Stancl\Tenancy\Exceptions\DomainsOccupiedByOtherTenantException; +use Stancl\Tenancy\Exceptions\TenantDoesNotExistException; use Stancl\Tenancy\Exceptions\TenantWithThisIdAlreadyExistsException; +use Stancl\Tenancy\Jobs\QueuedTenantDatabaseCreator; use Stancl\Tenancy\Jobs\QueuedTenantDatabaseMigrator; use Stancl\Tenancy\Tenant; use Stancl\Tenancy\TenantManager; @@ -248,22 +250,32 @@ class TenantManagerTest extends TestCase } /** @test */ - public function automigration_can_be_queued() + public function automigration_is_queued_when_db_creation_is_queued() { Queue::fake(); config([ + 'tenancy.queue_database_creation' => true, 'tenancy.migrate_after_creation' => true, - 'tenancy.queue_automatic_migration' => true, ]); $tenant = Tenant::new()->save(); - tenancy()->initialize($tenant); - Queue::assertPushed(QueuedTenantDatabaseMigrator::class); + Queue::assertPushedWithChain(QueuedTenantDatabaseCreator::class, [ + QueuedTenantDatabaseMigrator::class, + ]); - $this->assertFalse(\Schema::hasTable('users')); - (new QueuedTenantDatabaseMigrator($tenant))->handle(); - $this->assertTrue(\Schema::hasTable('users')); + // foreach (Queue::pushedJobs() as $job) { + // $job[0]['job']->handle(); // this doesn't execute the chained job + // } + // tenancy()->initialize($tenant); + // $this->assertTrue(\Schema::hasTable('users')); + } + + /** @test */ + public function TenantDoesNotExistException_is_thrown_when_find_is_called_on_an_id_that_does_not_belong_to_any_tenant() + { + $this->expectException(TenantDoesNotExistException::class); + tenancy()->find('gjnfdgf'); } }