diff --git a/assets/TenancyServiceProvider.stub.php b/assets/TenancyServiceProvider.stub.php index 46f35515..a1e681d7 100644 --- a/assets/TenancyServiceProvider.stub.php +++ b/assets/TenancyServiceProvider.stub.php @@ -81,6 +81,8 @@ class TenancyServiceProvider extends ServiceProvider ])->send(function (Events\TenantDeleted $event) { return $event->tenant; })->shouldBeQueued(false), + + // ResourceSyncing\Listeners\DeleteAllTenantMappings::class, ], Events\TenantMaintenanceModeEnabled::class => [], @@ -129,6 +131,9 @@ class TenancyServiceProvider extends ServiceProvider ResourceSyncing\Events\SyncedResourceSaved::class => [ ResourceSyncing\Listeners\UpdateOrCreateSyncedResource::class, ], + ResourceSyncing\Events\SyncedResourceDeleted::class => [ + ResourceSyncing\Listeners\DeleteResourceMapping::class, + ], ResourceSyncing\Events\SyncMasterDeleted::class => [ ResourceSyncing\Listeners\DeleteResourcesInTenants::class, ], diff --git a/src/ResourceSyncing/Events/SyncedResourceDeleted.php b/src/ResourceSyncing/Events/SyncedResourceDeleted.php new file mode 100644 index 00000000..941e1841 --- /dev/null +++ b/src/ResourceSyncing/Events/SyncedResourceDeleted.php @@ -0,0 +1,18 @@ +onDelete('cascade') + * on the constraint (otherwise, deleting a tenant will throw a foreign key constraint violation). + * That way, the cleanup will happen on the database level, and this listener will essentially + * just perform an extra 'where' query. + */ +class DeleteAllTenantMappings extends QueueableListener +{ + /** + * Pivot tables to clean up after a tenant is deleted, + * formatted like ['table_name' => 'tenant_key_column']. + * + * Since we cannot automatically detect which pivot tables + * you want to clean up, they have to be specified here. + * + * By default, resource syncing uses the tenant_resources table, and the records are associated + * to tenants by the tenant_id column (thus the ['tenant_resources' => 'tenant_id'] default). + * + * To customize this, set this property, e.g. in TenancyServiceProvider: + * DeleteAllTenantMappings::$pivotTables = [ + * 'tenant_users' => 'tenant_id', + * // You can also add more pivot tables here + * ]; + */ + public static array $pivotTables = ['tenant_resources' => 'tenant_id']; + + public function handle(TenantDeleted $event): void + { + foreach (static::$pivotTables as $table => $tenantKeyColumn) { + DB::table($table)->where($tenantKeyColumn, $event->tenant->getTenantKey())->delete(); + } + } +} diff --git a/src/ResourceSyncing/Listeners/DeleteResourceMapping.php b/src/ResourceSyncing/Listeners/DeleteResourceMapping.php new file mode 100644 index 00000000..319fd043 --- /dev/null +++ b/src/ResourceSyncing/Listeners/DeleteResourceMapping.php @@ -0,0 +1,60 @@ +getCentralResource($event->model); + + if (! $centralResource) { + return; + } + + // Delete pivot records if the central resource doesn't use soft deletes + // or the central resource was deleted using forceDelete() + if ($event->forceDelete || ! in_array(SoftDeletes::class, class_uses_recursive($centralResource::class), true)) { + Pivot::withoutEvents(function () use ($centralResource, $event) { + // $event->tenant is null when the deleted resource is a SyncMaster - all mappings are deleted in that case + // When $event->tenant is not null (= a Syncable was deleted), only delete the mapping for that tenant + $centralResource->tenants()->detach($event->tenant); + }); + } + } + + public function getCentralResource(Syncable&Model $resource): SyncMaster|null + { + if ($resource instanceof SyncMaster) { + return $resource; + } + + $centralResourceClass = $resource->getCentralModelName(); + + /** @var (SyncMaster&Model)|null $centralResource */ + $centralResource = $centralResourceClass::firstWhere( + $resource->getGlobalIdentifierKeyName(), + $resource->getGlobalIdentifierKey() + ); + + return $centralResource; + } +} diff --git a/src/ResourceSyncing/Listeners/DeleteResourcesInTenants.php b/src/ResourceSyncing/Listeners/DeleteResourcesInTenants.php index 6876f476..7b071a27 100644 --- a/src/ResourceSyncing/Listeners/DeleteResourcesInTenants.php +++ b/src/ResourceSyncing/Listeners/DeleteResourcesInTenants.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Stancl\Tenancy\ResourceSyncing\Listeners; -use Illuminate\Database\Eloquent\SoftDeletes; use Stancl\Tenancy\Listeners\QueueableListener; use Stancl\Tenancy\ResourceSyncing\Events\SyncMasterDeleted; @@ -21,12 +20,6 @@ class DeleteResourcesInTenants extends QueueableListener tenancy()->runForMultiple($centralResource->tenants()->cursor(), function () use ($centralResource, $forceDelete) { $this->deleteSyncedResource($centralResource, $forceDelete); - - // Delete pivot records if the central resource doesn't use soft deletes - // or the central resource was deleted using forceDelete() - if ($forceDelete || ! in_array(SoftDeletes::class, class_uses_recursive($centralResource::class), true)) { - $centralResource->tenants()->detach(tenant()); - } }); } } diff --git a/src/ResourceSyncing/ResourceSyncing.php b/src/ResourceSyncing/ResourceSyncing.php index f0d8cc12..824d909c 100644 --- a/src/ResourceSyncing/ResourceSyncing.php +++ b/src/ResourceSyncing/ResourceSyncing.php @@ -11,6 +11,7 @@ use Stancl\Tenancy\Contracts\UniqueIdentifierGenerator; use Stancl\Tenancy\Database\Contracts\TenantWithDatabase; use Stancl\Tenancy\ResourceSyncing\Events\CentralResourceAttachedToTenant; use Stancl\Tenancy\ResourceSyncing\Events\CentralResourceDetachedFromTenant; +use Stancl\Tenancy\ResourceSyncing\Events\SyncedResourceDeleted; use Stancl\Tenancy\ResourceSyncing\Events\SyncedResourceSaved; use Stancl\Tenancy\ResourceSyncing\Events\SyncMasterDeleted; use Stancl\Tenancy\ResourceSyncing\Events\SyncMasterRestored; @@ -25,8 +26,8 @@ trait ResourceSyncing } }); - static::deleting(function (Syncable&Model $model) { - if ($model->shouldSync() && $model instanceof SyncMaster) { + static::deleted(function (Syncable&Model $model) { + if ($model->shouldSync()) { $model->triggerDeleteEvent(); } }); @@ -42,13 +43,13 @@ trait ResourceSyncing if (in_array(SoftDeletes::class, class_uses_recursive(static::class), true)) { static::forceDeleting(function (Syncable&Model $model) { - if ($model->shouldSync() && $model instanceof SyncMaster) { + if ($model->shouldSync()) { $model->triggerDeleteEvent(true); } }); static::restoring(function (Syncable&Model $model) { - if ($model->shouldSync() && $model instanceof SyncMaster) { + if ($model->shouldSync()) { $model->triggerRestoredEvent(); } }); @@ -67,6 +68,8 @@ trait ResourceSyncing /** @var SyncMaster&Model $this */ event(new SyncMasterDeleted($this, $forceDelete)); } + + event(new SyncedResourceDeleted($this, tenant(), $forceDelete)); } public function triggerRestoredEvent(): void diff --git a/src/ResourceSyncing/SyncMaster.php b/src/ResourceSyncing/SyncMaster.php index 882aeb54..710bc097 100644 --- a/src/ResourceSyncing/SyncMaster.php +++ b/src/ResourceSyncing/SyncMaster.php @@ -25,7 +25,5 @@ interface SyncMaster extends Syncable public function triggerAttachEvent(TenantWithDatabase&Model $tenant): void; - public function triggerDeleteEvent(bool $forceDelete = false): void; - public function triggerRestoredEvent(): void; } diff --git a/src/ResourceSyncing/Syncable.php b/src/ResourceSyncing/Syncable.php index 3d5288f1..c38b02ea 100644 --- a/src/ResourceSyncing/Syncable.php +++ b/src/ResourceSyncing/Syncable.php @@ -16,6 +16,8 @@ interface Syncable public function triggerSyncEvent(): void; + public function triggerDeleteEvent(bool $forceDelete = false): void; + /** * Get the attributes used for creating the *other* model (i.e. tenant if this is the central one, and central if this is the tenant one). * diff --git a/tests/Etc/ResourceSyncing/CentralUser.php b/tests/Etc/ResourceSyncing/CentralUser.php index 1533bd21..ece09550 100644 --- a/tests/Etc/ResourceSyncing/CentralUser.php +++ b/tests/Etc/ResourceSyncing/CentralUser.php @@ -12,6 +12,7 @@ use Stancl\Tenancy\ResourceSyncing\SyncMaster; class CentralUser extends Model implements SyncMaster { use ResourceSyncing, CentralConnection; + protected $guarded = []; public $timestamps = false; diff --git a/tests/Etc/synced_resource_migrations/2020_05_11_000002_create_tenant_users_table.php b/tests/Etc/synced_resource_migrations/2020_05_11_000002_create_tenant_users_table.php index 0aafd23c..dcd667a6 100644 --- a/tests/Etc/synced_resource_migrations/2020_05_11_000002_create_tenant_users_table.php +++ b/tests/Etc/synced_resource_migrations/2020_05_11_000002_create_tenant_users_table.php @@ -16,9 +16,6 @@ class CreateTenantUsersTable extends Migration $table->string('global_user_id'); $table->unique(['tenant_id', 'global_user_id']); - - $table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade'); - $table->foreign('global_user_id')->references('global_id')->on('users')->onUpdate('cascade')->onDelete('cascade'); }); } diff --git a/tests/ResourceSyncingTest.php b/tests/ResourceSyncingTest.php index 3250c37a..a4736ce7 100644 --- a/tests/ResourceSyncingTest.php +++ b/tests/ResourceSyncingTest.php @@ -46,6 +46,12 @@ use Stancl\Tenancy\ResourceSyncing\Events\SyncedResourceSavedInForeignDatabase; use Illuminate\Database\Eloquent\Scope; use Illuminate\Database\QueryException; use function Stancl\Tenancy\Tests\pest; +use Illuminate\Support\Facades\Schema; +use Illuminate\Database\Schema\Blueprint; +use Stancl\Tenancy\Events\TenantDeleted; +use Stancl\Tenancy\ResourceSyncing\Events\SyncedResourceDeleted; +use Stancl\Tenancy\ResourceSyncing\Listeners\DeleteAllTenantMappings; +use Stancl\Tenancy\ResourceSyncing\Listeners\DeleteResourceMapping; beforeEach(function () { config(['tenancy.bootstrappers' => [ @@ -69,6 +75,7 @@ beforeEach(function () { CreateTenantResource::$shouldQueue = false; DeleteResourceInTenant::$shouldQueue = false; UpdateOrCreateSyncedResource::$scopeGetModelQuery = null; + DeleteAllTenantMappings::$pivotTables = ['tenant_resources' => 'tenant_id']; // Reset global scopes on models (should happen automatically but to make this more explicit) Model::clearBootedModels(); @@ -92,6 +99,7 @@ beforeEach(function () { CentralUser::$creationAttributes = $creationAttributes; Event::listen(SyncedResourceSaved::class, UpdateOrCreateSyncedResource::class); + Event::listen(SyncedResourceDeleted::class, DeleteResourceMapping::class); Event::listen(SyncMasterDeleted::class, DeleteResourcesInTenants::class); Event::listen(SyncMasterRestored::class, RestoreResourcesInTenants::class); Event::listen(CentralResourceAttachedToTenant::class, CreateTenantResource::class); @@ -890,7 +898,54 @@ test('deleting SyncMaster automatically deletes its Syncables', function (bool $ 'basic pivot' => false, ]); -test('tenant pivot records are deleted along with the tenants to which they belong to', function() { +test('tenant pivot records are deleted along with the tenants to which they belong to', function (bool $dbLevelOnCascadeDelete, bool $morphPivot) { + [$tenant] = createTenantsAndRunMigrations(); + + if ($morphPivot) { + config(['tenancy.models.tenant' => MorphTenant::class]); + $centralUserModel = BaseCentralUser::class; + + // The default pivot table, no need to configure the listener + $pivotTable = 'tenant_resources'; + } else { + $centralUserModel = CentralUser::class; + + // Custom pivot table + $pivotTable = 'tenant_users'; + } + + if ($dbLevelOnCascadeDelete) { + addTenantIdConstraintToPivot($pivotTable); + } else { + // Event-based cleanup + Event::listen(TenantDeleted::class, DeleteAllTenantMappings::class); + + DeleteAllTenantMappings::$pivotTables = [$pivotTable => 'tenant_id']; + } + + $syncMaster = $centralUserModel::create([ + 'global_id' => 'user', + 'name' => 'Central user', + 'email' => 'central@localhost', + 'password' => 'password', + 'role' => 'user', + ]); + + $syncMaster->tenants()->attach($tenant); + + // Pivot records should be deleted along with the tenant + $tenant->delete(); + + expect(DB::select("SELECT * FROM {$pivotTable} WHERE tenant_id = ?", [$tenant->getTenantKey()]))->toHaveCount(0); +})->with([ + 'db level on cascade delete' => true, + 'event-based on cascade delete' => false, +])->with([ + 'polymorphic pivot' => true, + 'basic pivot' => false, +]); + +test('pivot record is automatically deleted with the tenant resource', function() { [$tenant] = createTenantsAndRunMigrations(); $syncMaster = CentralUser::create([ @@ -903,12 +958,32 @@ test('tenant pivot records are deleted along with the tenants to which they belo $syncMaster->tenants()->attach($tenant); - $tenant->delete(); + $tenant->run(function () { + TenantUser::firstWhere('global_id', 'cascade_user')->delete(); + }); - // Deleting tenant deletes its pivot records + // Deleting tenant resource deletes its pivot record expect(DB::select("SELECT * FROM tenant_users WHERE tenant_id = ?", [$tenant->getTenantKey()]))->toHaveCount(0); }); +test('DeleteAllTenantMappings handles incorrect configuration correctly', function() { + Event::listen(TenantDeleted::class, DeleteAllTenantMappings::class); + + [$tenant1, $tenant2] = createTenantsAndRunMigrations(); + + // Existing table, non-existent tenant key column + // The listener should throw an 'unknown column' exception + DeleteAllTenantMappings::$pivotTables = ['tenant_users' => 'non_existent_column']; + + // Should throw an exception when tenant is deleted + expect(fn() => $tenant1->delete())->toThrow(QueryException::class, "Unknown column 'non_existent_column' in 'where clause'"); + + // Non-existent table + DeleteAllTenantMappings::$pivotTables = ['nonexistent_pivot' => 'non_existent_column']; + + expect(fn() => $tenant2->delete())->toThrow(QueryException::class, "Table 'main.nonexistent_pivot' doesn't exist"); +}); + test('trashed resources are synced correctly', function () { [$tenant1, $tenant2] = createTenantsAndRunMigrations(); migrateUsersTableForTenants(); @@ -1265,6 +1340,13 @@ test('global scopes on syncable models can break resource syncing', function () expect($tenant1->run(fn () => TenantUser::first()->name))->toBe('tenant2 user'); }); +function addTenantIdConstraintToPivot(string $pivotTable): void +{ + Schema::table($pivotTable, function (Blueprint $table) { + $table->foreign('tenant_id')->references('id')->on('tenants')->onDelete('cascade'); + }); +} + /** * Create two tenants and run migrations for those tenants. *