From a756b9b3bc2678f666248a081184dc0ec9d3a6aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Wed, 13 May 2020 00:31:30 +0200 Subject: [PATCH] Implement changes from review on GH --- src/Contracts/UniqueIdentifierGenerator.php | 4 +- src/Database/Models/TenantPivot.php | 7 +- src/Events/Listeners/UpdateSyncedResource.php | 116 ++++++++++------- ...SyncedResourceChangedInForeignDatabase.php | 21 +++ src/Events/SyncedResourceSaved.php | 6 +- src/UniqueIDGenerators/UUIDGenerator.php | 3 +- ...05_11_000002_create_tenant_users_table.php | 2 + tests/v3/ResourceSyncingTest.php | 123 +++++++++++++++++- 8 files changed, 224 insertions(+), 58 deletions(-) create mode 100644 src/Events/SyncedResourceChangedInForeignDatabase.php diff --git a/src/Contracts/UniqueIdentifierGenerator.php b/src/Contracts/UniqueIdentifierGenerator.php index e4c2a1d0..b21d6028 100644 --- a/src/Contracts/UniqueIdentifierGenerator.php +++ b/src/Contracts/UniqueIdentifierGenerator.php @@ -4,12 +4,10 @@ declare(strict_types=1); namespace Stancl\Tenancy\Contracts; -use Illuminate\Database\Eloquent\Model; - interface UniqueIdentifierGenerator { /** * Generate a unique identifier. */ - public static function generate(Model $model): string; + public static function generate($resource): string; } diff --git a/src/Database/Models/TenantPivot.php b/src/Database/Models/TenantPivot.php index 8a112e48..b28c03fa 100644 --- a/src/Database/Models/TenantPivot.php +++ b/src/Database/Models/TenantPivot.php @@ -3,13 +3,18 @@ namespace Stancl\Tenancy\Database\Models; use Illuminate\Database\Eloquent\Relations\Pivot; +use Stancl\Tenancy\Contracts\Syncable; class TenantPivot extends Pivot { public static function booted() { static::saved(function (self $pivot) { - $pivot->pivotParent->triggerSyncEvent(); + $parent = $pivot->pivotParent; + + if ($parent instanceof Syncable) { + $parent->triggerSyncEvent(); + } }); } } \ No newline at end of file diff --git a/src/Events/Listeners/UpdateSyncedResource.php b/src/Events/Listeners/UpdateSyncedResource.php index 7ae8a40c..6bae909a 100644 --- a/src/Events/Listeners/UpdateSyncedResource.php +++ b/src/Events/Listeners/UpdateSyncedResource.php @@ -5,6 +5,7 @@ namespace Stancl\Tenancy\Events\Listeners; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\Pivot; use Stancl\Tenancy\Contracts\SyncMaster; +use Stancl\Tenancy\Events\SyncedResourceChangedInForeignDatabase; use Stancl\Tenancy\Events\SyncedResourceSaved; use Stancl\Tenancy\Exceptions\ModelNotSyncMaster; @@ -18,53 +19,74 @@ class UpdateSyncedResource extends QueueableListener // We update the central record only if the event comes from tenant context. if ($event->tenant) { - /** @var Model|SyncMaster $centralModel */ - $centralModel = $event->model->getCentralModelName() - ::where($event->model->getGlobalIdentifierKeyName(), $event->model->getGlobalIdentifierKey()) - ->first(); - - // We disable events for this call, to avoid triggering this event & listener again. - $event->model->getCentralModelName()::withoutEvents(function () use (&$centralModel, $syncedAttributes, $event) { - if ($centralModel) { - $centralModel->update($syncedAttributes); - } else { - // If the resource doesn't exist at all in the central DB,we create - // the record with all attributes, not just the synced ones. - $centralModel = $event->model->getCentralModelName()::create($event->model->getAttributes()); - } - }); - - // If the model was just created, the mapping of the tenant to the user likely doesn't exist, so we create it. - $mappingExists = $centralModel->tenants->contains(function ($model) use ($event) { - return $model->tenant_id === $event->tenant->getTenantKey(); - }); - - if (! $mappingExists) { - // Here we should call TenantPivot, but we call general Pivot, so that this works - // even if people use their own pivot model that is not based on our TenantPivot - Pivot::withoutEvents(function () use ($centralModel, $event) { - $centralModel->tenants()->attach($event->tenant->getTenantKey()); - }); - } - - $tenants = $centralModel->tenants->except($event->tenant->getTenantKey()); + $tenants = $this->updateResourceInCentralDatabaseAndGetTenants($event, $syncedAttributes); } else { - $centralModel = $event->model; - - if (! $centralModel instanceof SyncMaster) { - // If we're trying to use a tenant User model instead of the central User model, for example. - throw new ModelNotSyncMaster(get_class($centralModel)); - } - - /** @var SyncMaster|Model $centralModel */ - - // Since this model is "dirty" (taken by reference from the event), it might have the tenants - // relationship already loaded and cached. For this reason, we refresh the relationship. - $centralModel->load('tenants'); - $tenants = $centralModel->tenants; + $tenants = $this->getTenantsForCentralModel($event->model); } - tenancy()->runForMultiple($tenants, function () use ($event, $syncedAttributes) { + $this->updateResourceInTenantDatabases($tenants, $event, $syncedAttributes); + } + + protected function getTenantsForCentralModel($centralModel) + { + if (! $centralModel instanceof SyncMaster) { + // If we're trying to use a tenant User model instead of the central User model, for example. + throw new ModelNotSyncMaster(get_class($centralModel)); + } + + /** @var SyncMaster|Model $centralModel */ + + // Since this model is "dirty" (taken by reference from the event), it might have the tenants + // relationship already loaded and cached. For this reason, we refresh the relationship. + $centralModel->load('tenants'); + + return $centralModel->tenants; + } + + protected function updateResourceInCentralDatabaseAndGetTenants($event, $syncedAttributes) + { + /** @var Model|SyncMaster $centralModel */ + $centralModel = $event->model->getCentralModelName() + ::where($event->model->getGlobalIdentifierKeyName(), $event->model->getGlobalIdentifierKey()) + ->first(); + + // We disable events for this call, to avoid triggering this event & listener again. + $event->model->getCentralModelName()::withoutEvents(function () use (&$centralModel, $syncedAttributes, $event) { + if ($centralModel) { + $centralModel->update($syncedAttributes); + event(new SyncedResourceChangedInForeignDatabase($event->model, null)); + } else { + // If the resource doesn't exist at all in the central DB,we create + // the record with all attributes, not just the synced ones. + $centralModel = $event->model->getCentralModelName()::create($event->model->getAttributes()); + event(new SyncedResourceChangedInForeignDatabase($event->model, null)); + } + }); + + // If the model was just created, the mapping of the tenant to the user likely doesn't exist, so we create it. + $currentTenantMapping = function ($model) use ($event) { + return ((string) $model->pivot->tenant_id) === ((string) $event->tenant->getTenantKey()); + }; + + $mappingExists = $centralModel->tenants->contains($currentTenantMapping); + + if (!$mappingExists) { + // Here we should call TenantPivot, but we call general Pivot, so that this works + // even if people use their own pivot model that is not based on our TenantPivot + Pivot::withoutEvents(function () use ($centralModel, $event) { + $centralModel->tenants()->attach($event->tenant->getTenantKey()); + }); + } + + return $centralModel->tenants->filter(function ($model) use ($currentTenantMapping) { + // Remove the mapping for the current tenant. + return !$currentTenantMapping($model); + }); + } + + protected function updateResourceInTenantDatabases($tenants, $event, $syncedAttributes) + { + tenancy()->runForMultiple($tenants, function ($tenant) use ($event, $syncedAttributes) { // Forget instance state and find the model, // again in the current tenant's context. @@ -84,13 +106,15 @@ class UpdateSyncedResource extends QueueableListener // why we're using Eloquent instead of direct DB queries. // We disable events for this call, to avoid triggering this event & listener again. - $localModelClass::withoutEvents(function () use ($localModelClass, $localModel, $syncedAttributes, $eventModel) { + $localModelClass::withoutEvents(function () use ($localModelClass, $localModel, $syncedAttributes, $eventModel, $tenant) { if ($localModel) { $localModel->update($syncedAttributes); } else { // When creating, we use all columns, not just the synced ones. - $localModelClass::create($eventModel->getAttributes()); + $localModel = $localModelClass::create($eventModel->getAttributes()); } + + event(new SyncedResourceChangedInForeignDatabase($localModel, $tenant)); }); }); } diff --git a/src/Events/SyncedResourceChangedInForeignDatabase.php b/src/Events/SyncedResourceChangedInForeignDatabase.php new file mode 100644 index 00000000..d672381a --- /dev/null +++ b/src/Events/SyncedResourceChangedInForeignDatabase.php @@ -0,0 +1,21 @@ +model = $model; + $this->tenant = $tenant; + } +} diff --git a/src/Events/SyncedResourceSaved.php b/src/Events/SyncedResourceSaved.php index d80bff2e..456861c5 100644 --- a/src/Events/SyncedResourceSaved.php +++ b/src/Events/SyncedResourceSaved.php @@ -4,17 +4,17 @@ namespace Stancl\Tenancy\Events; use Illuminate\Database\Eloquent\Model; use Stancl\Tenancy\Contracts\Syncable; -use Stancl\Tenancy\Contracts\Tenant; +use Stancl\Tenancy\Contracts\TenantWithDatabase; class SyncedResourceSaved { /** @var Syncable|Model */ public $model; - /** @var Tenant|Model|null */ + /** @var TenantWithDatabase|Model|null */ public $tenant; - public function __construct(Syncable $model, ?Tenant $tenant) + public function __construct(Syncable $model, ?TenantWithDatabase $tenant) { $this->model = $model; $this->tenant = $tenant; diff --git a/src/UniqueIDGenerators/UUIDGenerator.php b/src/UniqueIDGenerators/UUIDGenerator.php index 6f542ab9..ebb5cdad 100644 --- a/src/UniqueIDGenerators/UUIDGenerator.php +++ b/src/UniqueIDGenerators/UUIDGenerator.php @@ -4,13 +4,12 @@ declare(strict_types=1); namespace Stancl\Tenancy\UniqueIDGenerators; -use Illuminate\Database\Eloquent\Model; use Ramsey\Uuid\Uuid; use Stancl\Tenancy\Contracts\UniqueIdentifierGenerator; class UUIDGenerator implements UniqueIdentifierGenerator { - public static function generate(Model $model): string + public static function generate($resource): string { return Uuid::uuid4()->toString(); } 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 b7ad55e1..fb00b0d8 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 @@ -20,6 +20,8 @@ class CreateTenantUsersTable extends Migration $table->string('tenant_id'); $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/v3/ResourceSyncingTest.php b/tests/v3/ResourceSyncingTest.php index cc78c6eb..a866eecc 100644 --- a/tests/v3/ResourceSyncingTest.php +++ b/tests/v3/ResourceSyncingTest.php @@ -11,12 +11,13 @@ use Stancl\Tenancy\Contracts\Syncable; use Stancl\Tenancy\Contracts\SyncMaster; use Stancl\Tenancy\Database\Models\Concerns\CentralConnection; use Stancl\Tenancy\Database\Models\Concerns\ResourceSyncing; -use Stancl\Tenancy\Database\Models\Tenant; +use Stancl\Tenancy\Database\Models; use Stancl\Tenancy\Database\Models\TenantPivot; use Stancl\Tenancy\Events\Listeners\BootstrapTenancy; use Stancl\Tenancy\Events\Listeners\JobPipeline; use Stancl\Tenancy\Events\Listeners\RevertToCentralContext; use Stancl\Tenancy\Events\Listeners\UpdateSyncedResource; +use Stancl\Tenancy\Events\SyncedResourceChangedInForeignDatabase; use Stancl\Tenancy\Events\SyncedResourceSaved; use Stancl\Tenancy\Events\TenancyEnded; use Stancl\Tenancy\Events\TenancyInitialized; @@ -80,7 +81,7 @@ class ResourceSyncingTest extends TestCase return $event->model === $user; }); } - + /** @test */ public function only_the_synced_columns_are_updated_in_the_central_db() { @@ -211,6 +212,35 @@ class ResourceSyncingTest extends TestCase }); } + /** @test */ + public function attaching_users_to_tenants_DOES_NOT_DO_ANYTHING() + { + $centralUser = CentralUser::create([ + 'global_id' => 'acme', + 'name' => 'John Doe', + 'email' => 'john@localhost', + 'password' => 'secret', + 'role' => 'commenter', // unsynced + ]); + + $tenant = Tenant::create([ + 'id' => 't1', + ]); + $this->migrateTenants(); + + $tenant->run(function () { + $this->assertCount(0, User::all()); + }); + + // The child model is inaccessible in the Pivot Model, so we can't fire any events. + $tenant->users()->attach($centralUser); + + $tenant->run(function () { + // Still zero + $this->assertCount(0, User::all()); + }); + } + /** @test */ public function resources_are_synced_only_to_workspaces_that_have_the_resource() { @@ -439,6 +469,93 @@ class ResourceSyncingTest extends TestCase return $job->class === UpdateSyncedResource::class; }); } + + /** @test */ + public function an_event_is_fired_for_all_touched_resources() + { + Event::fake([SyncedResourceChangedInForeignDatabase::class]); + + // create shared resource + $centralUser = CentralUser::create([ + 'global_id' => 'acme', + 'name' => 'John Doe', + 'email' => 'john@localhost', + 'password' => 'secret', + 'role' => 'commenter', // unsynced + ]); + + $t1 = Tenant::create([ + 'id' => 't1', + ]); + $t2 = Tenant::create([ + 'id' => 't2', + ]); + $t3 = Tenant::create([ + 'id' => 't3', + ]); + $this->migrateTenants(); + + // Copy (cascade) user to t1 DB + $centralUser->tenants()->attach('t1'); + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return $event->tenant->getTenantKey() === 't1'; + }); + + $centralUser->tenants()->attach('t2'); + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return $event->tenant->getTenantKey() === 't2'; + }); + + $centralUser->tenants()->attach('t3'); + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return $event->tenant->getTenantKey() === 't3'; + }); + + // Assert no event for central + Event::assertNotDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return $event->tenant === null; + }); + + // Flush + Event::fake([SyncedResourceChangedInForeignDatabase::class]); + + $t3->run(function () { + User::first()->update([ + 'name' => 'John 3', + 'role' => 'employee', // unsynced + ]); + + $this->assertSame('employee', User::first()->role); + }); + + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return optional($event->tenant)->getTenantKey() === 't1'; + }); + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return optional($event->tenant)->getTenantKey() === 't2'; + }); + + // Assert NOT dispatched in t3 + Event::assertNotDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return optional($event->tenant)->getTenantKey() === 't3'; + }); + + // Assert dispatched in central + Event::assertDispatched(SyncedResourceChangedInForeignDatabase::class, function (SyncedResourceChangedInForeignDatabase $event) { + return $event->tenant === null; + }); + + // todo update in global + } +} + +class Tenant extends Models\Tenant +{ + public function users() + { + return $this->belongsToMany(CentralUser::class, 'tenant_users', 'tenant_id', 'global_user_id') + ->using(TenantPivot::class); + } } class CentralUser extends Model implements SyncMaster @@ -451,7 +568,7 @@ class CentralUser extends Model implements SyncMaster public function tenants(): BelongsToMany { - return $this->belongsToMany(Tenant::class, 'tenant_users', 'tenant_id', 'global_user_id') + return $this->belongsToMany(Tenant::class, 'tenant_users', 'global_user_id', 'tenant_id') ->using(TenantPivot::class); }