From aa1437fb5e1c672fb7a50fb1ee60832d63a88dcf Mon Sep 17 00:00:00 2001 From: lukinovec Date: Sat, 10 Feb 2024 18:38:05 +0100 Subject: [PATCH] Resolve misc to-dos (#26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Resolve delete tenant storage todo * Delete outdated todo (resolved in #25) * Delete resource syncing todo (resolved in #11) * Make it clear that getArgsForTenant() is used during cache invalidation * Delete redundant __call() and __callStatic() annotations * Fix code style (php-cs-fixer) * Revert %tenant_id% to-do removal * Test all cached resolvers instead of just the domain one * Make docblock more concise, delete renaming to-do (the name seems fine) * Fix method in tests * If route is the only resolver arg, use the tenant as the cache key instead of encoded route instance * Resolve to-do * make docblock more clear * Add comments to getResolverArgument() * Rename $id to $tenantKey * Fix code style (php-cs-fixer) * Add regression test for forgetting tenant parameters of cached tenants * Forget route parameter when tenant gets resolved * Add parameter type * Simplify getCacheKey() * Resolvers wip * Resolvers wip * Fix code style (php-cs-fixer) * Bring back the route instance check to getCacheKey, fix test * add todo * add assertion --------- Co-authored-by: PHP CS Fixer Co-authored-by: Samuel Štancl Co-authored-by: Samuel Štancl --- src/Contracts/Domain.php | 2 - src/Database/DatabaseConfig.php | 7 +- src/Listeners/DeleteTenantStorage.php | 3 - src/Listeners/UpdateSyncedResource.php | 2 - .../Contracts/CachedTenantResolver.php | 6 +- src/Resolvers/PathTenantResolver.php | 21 +++ tests/CachedTenantResolverTest.php | 155 +++++++++++++----- 7 files changed, 140 insertions(+), 56 deletions(-) diff --git a/src/Contracts/Domain.php b/src/Contracts/Domain.php index cfe89f43..60747fb7 100644 --- a/src/Contracts/Domain.php +++ b/src/Contracts/Domain.php @@ -11,8 +11,6 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo; * * @see \Stancl\Tenancy\Database\Models\Domain * - * @method __call(string $method, array $parameters) IDE support. This will be a model. // todo check if we can remove these now - * @method static __callStatic(string $method, array $parameters) IDE support. This will be a model. * @mixin \Illuminate\Database\Eloquent\Model */ interface Domain diff --git a/src/Database/DatabaseConfig.php b/src/Database/DatabaseConfig.php index b0bda3e5..f47d171e 100644 --- a/src/Database/DatabaseConfig.php +++ b/src/Database/DatabaseConfig.php @@ -162,10 +162,7 @@ class DatabaseConfig } /** - * Purge host database connection. - * - * It's possible database has previous tenant connection. - * This will clean up the previous connection before creating it for the current tenant. + * Purge the previous tenant connection before opening it for another tenant. */ public function purgeHostConnection(): void { @@ -205,7 +202,7 @@ class DatabaseConfig public function manager(): Contracts\TenantDatabaseManager { // Laravel caches the previous PDO connection, so we purge it to be able to change the connection details - $this->purgeHostConnection(); // todo come up with a better name + $this->purgeHostConnection(); // Create the tenant host connection config $tenantHostConnectionName = $this->getTenantHostConnectionName(); diff --git a/src/Listeners/DeleteTenantStorage.php b/src/Listeners/DeleteTenantStorage.php index 9cc1daae..ce1a4203 100644 --- a/src/Listeners/DeleteTenantStorage.php +++ b/src/Listeners/DeleteTenantStorage.php @@ -11,9 +11,6 @@ class DeleteTenantStorage { public function handle(DeletingTenant $event): void { - // todo@lukas since this is using the 'File' facade instead of low-level PHP functions, Tenancy might affect this? - // Therefore, when Tenancy is initialized, this might look INSIDE the tenant's storage, instead of the main storage dir? - // The DeletingTenant event will be fired in the central context in 99% of cases, but sometimes it might run in the tenant context (from another tenant) so we want to make sure this works well in all contexts. File::deleteDirectory($event->tenant->run(fn () => storage_path())); } } diff --git a/src/Listeners/UpdateSyncedResource.php b/src/Listeners/UpdateSyncedResource.php index 38245a80..595aaf7e 100644 --- a/src/Listeners/UpdateSyncedResource.php +++ b/src/Listeners/UpdateSyncedResource.php @@ -16,8 +16,6 @@ use Stancl\Tenancy\Events\SyncedResourceSaved; use Stancl\Tenancy\Exceptions\ModelNotSyncMasterException; use Stancl\Tenancy\Tenancy; -// todo@v4 review all code related to resource syncing - class UpdateSyncedResource extends QueueableListener { public static bool $shouldQueue = false; diff --git a/src/Resolvers/Contracts/CachedTenantResolver.php b/src/Resolvers/Contracts/CachedTenantResolver.php index dfdd2cf4..e6588718 100644 --- a/src/Resolvers/Contracts/CachedTenantResolver.php +++ b/src/Resolvers/Contracts/CachedTenantResolver.php @@ -62,11 +62,13 @@ abstract class CachedTenantResolver implements TenantResolver } /** - * Get all the arg combinations for resolve() that can be used to find this tenant. + * Get all possible argument combinations for resolve() which can be used for caching the tenant. + * + * This is used during tenant cache invalidation. * * @return array[] */ - abstract public function getArgsForTenant(Tenant $tenant): array; // todo@v4 make it clear that this is only used for cache *invalidation* + abstract public function getArgsForTenant(Tenant $tenant): array; public static function shouldCache(): bool { diff --git a/src/Resolvers/PathTenantResolver.php b/src/Resolvers/PathTenantResolver.php index e7b4ebcb..d5742294 100644 --- a/src/Resolvers/PathTenantResolver.php +++ b/src/Resolvers/PathTenantResolver.php @@ -7,6 +7,7 @@ namespace Stancl\Tenancy\Resolvers; use Illuminate\Routing\Route; use Stancl\Tenancy\Contracts\Tenant; use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedByPathException; +use Stancl\Tenancy\PathIdentificationManager; class PathTenantResolver extends Contracts\CachedTenantResolver { @@ -37,6 +38,26 @@ class PathTenantResolver extends Contracts\CachedTenantResolver ]; } + public function resolved(Tenant $tenant, mixed ...$args): void + { + /** @var Route $route */ + $route = $args[0]; + + $route->forgetParameter(PathIdentificationManager::getTenantParameterName()); + } + + public function getCacheKey(mixed ...$args): string + { + // todo@samuel: fix the coupling here. when this is called from the cachedresolver, $args are the tenant key. when it's called from within this class, $args are a Route instance + // the logic shouldn't have to be coupled to where it's being called from + + // $args[0] can be either a Route instance with the tenant key as a parameter + // Or the tenant key + $args = [$args[0] instanceof Route ? $args[0]->parameter(static::tenantParameterName()) : $args[0]]; + + return '_tenancy_resolver:' . static::class . ':' . json_encode($args); + } + public static function tenantParameterName(): string { return config('tenancy.identification.resolvers.' . static::class . '.tenant_parameter_name') ?? 'tenant'; diff --git a/tests/CachedTenantResolverTest.php b/tests/CachedTenantResolverTest.php index fa624b04..94a555aa 100644 --- a/tests/CachedTenantResolverTest.php +++ b/tests/CachedTenantResolverTest.php @@ -2,73 +2,88 @@ declare(strict_types=1); +use Illuminate\Routing\Route; use Illuminate\Support\Facades\DB; -use Stancl\Tenancy\Resolvers\DomainTenantResolver; use Stancl\Tenancy\Tests\Etc\Tenant; +use Stancl\Tenancy\Resolvers\PathTenantResolver; +use Stancl\Tenancy\Resolvers\DomainTenantResolver; +use Illuminate\Support\Facades\Route as RouteFacade; +use Stancl\Tenancy\Middleware\InitializeTenancyByPath; +use Stancl\Tenancy\PathIdentificationManager; +use Stancl\Tenancy\Resolvers\RequestDataTenantResolver; -// todo@v4 test this with other resolvers as well? +test('tenants can be resolved using cached resolvers', function (string $resolver) { + $tenant = Tenant::create(['id' => $tenantKey = 'acme']); -test('tenants can be resolved using the cached resolver', function () { - $tenant = Tenant::create(); - $tenant->domains()->create([ - 'domain' => 'acme', - ]); + $tenant->domains()->create(['domain' => $tenantKey]); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue()->toBeTrue(); -}); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); +})->with([ + DomainTenantResolver::class, + PathTenantResolver::class, + RequestDataTenantResolver::class, +]); -test('the underlying resolver is not touched when using the cached resolver', function () { - $tenant = Tenant::create(); - $tenant->domains()->create([ - 'domain' => 'acme', - ]); +test('the underlying resolver is not touched when using the cached resolver', function (string $resolver) { + $tenant = Tenant::create(['id' => $tenantKey = 'acme']); + + $tenant->createDomain($tenantKey); DB::enableQueryLog(); - config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => false]); + config(['tenancy.identification.resolvers.' . $resolver . '.cache' => false]); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); DB::flushQueryLog(); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); + pest()->assertNotEmpty(DB::getQueryLog()); // not empty - config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => true]); + config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); DB::flushQueryLog(); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); expect(DB::getQueryLog())->toBeEmpty(); // empty -}); +})->with([ + DomainTenantResolver::class, + PathTenantResolver::class, + RequestDataTenantResolver::class, +]); -test('cache is invalidated when the tenant is updated', function () { - $tenant = Tenant::create(); - $tenant->createDomain([ - 'domain' => 'acme', - ]); +test('cache is invalidated when the tenant is updated', function (string $resolver) { + $tenant = Tenant::create(['id' => $tenantKey = 'acme']); + $tenant->createDomain($tenantKey); DB::enableQueryLog(); - config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => true]); + config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); - DB::flushQueryLog(); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); - expect(DB::getQueryLog())->toBeEmpty(); // empty - - $tenant->update([ - 'foo' => 'bar', - ]); + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); + expect(DB::getQueryLog())->not()->toBeEmpty(); DB::flushQueryLog(); - expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue(); - pest()->assertNotEmpty(DB::getQueryLog()); // not empty -}); + + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); + expect(DB::getQueryLog())->toBeEmpty(); + + // Tenant cache gets invalidated when the tenant is updated + $tenant->touch(); + + DB::flushQueryLog(); + + expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue(); + + expect(DB::getQueryLog())->not()->toBeEmpty(); // Cache was invalidated, so the tenant was retrievevd from the DB +})->with([ + DomainTenantResolver::class, + PathTenantResolver::class, + RequestDataTenantResolver::class, +]); test('cache is invalidated when a tenants domain is changed', function () { - $tenant = Tenant::create(); - $tenant->createDomain([ - 'domain' => 'acme', - ]); + $tenant = Tenant::create(['id' => $tenantKey = 'acme']); + $tenant->createDomain($tenantKey); DB::enableQueryLog(); @@ -91,3 +106,59 @@ test('cache is invalidated when a tenants domain is changed', function () { expect($tenant->is(app(DomainTenantResolver::class)->resolve('bar')))->toBeTrue(); pest()->assertNotEmpty(DB::getQueryLog()); // not empty }); + +test('PathTenantResolver forgets the tenant route parameter when the tenant is resolved from cache', function() { + config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.cache' => true]); + + Tenant::create(['id' => 'foo']); + + RouteFacade::get('/', fn () => request()->route()->parameter('tenant') ? 'Tenant parameter present' : 'No tenant parameter') + ->name('tenant-route') + ->prefix('{tenant}') + ->middleware(InitializeTenancyByPath::class); + + // Tenant gets cached on first request + pest()->get("/foo")->assertSee('No tenant parameter'); + + // Tenant is resolved from cache on second request + // The tenant parameter should be forgotten + DB::flushQueryLog(); + pest()->get("/foo")->assertSee('No tenant parameter'); + pest()->assertEmpty(DB::getQueryLog()); // resolved from cache +}); + +/** + * Return the argument for the resolver – tenant key, or a route instance with the tenant parameter. + * + * PathTenantResolver uses a route instance with the tenant parameter as the argument, + * unlike other resolvers which use a tenant key as the argument. + * + * This method is used in the tests where we test all the resolvers + * to make getting the resolver arguments less repetitive (primarily because of the PathTenantResolver). + */ +function getResolverArgument(string $resolver, string $tenantKey): string|Route +{ + // PathTenantResolver uses a route instance as the argument + if ($resolver === PathTenantResolver::class) { + $routeName = 'tenant-route'; + + // Find or create a route instance for the resolver + $route = RouteFacade::getRoutes()->getByName($routeName) ?? RouteFacade::get('/', fn () => null) + ->name($routeName) + ->prefix('{tenant}') + ->middleware(InitializeTenancyByPath::class); + + // To make the tenant available on the route instance + // Make the 'tenant' route parameter the tenant key + // Setting the parameter on the $route->parameters property is required + // Because $route->setParameter() throws an exception when $route->parameters is not set yet + $route->parameters[PathIdentificationManager::getTenantParameterName()] = $tenantKey; + + // Return the route instance with the tenant key as the 'tenant' parameter + return $route; + } + + // Resolvers other than PathTenantResolver use the tenant key as the argument + // Return the tenant key + return $tenantKey; +}