From a0a9b8598204d629a9fcbef961ab125ef02a4824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Mon, 1 Sep 2025 21:09:47 +0200 Subject: [PATCH] Refactor DatabaseConfig, minor DB manager improvements, resolve todos Notable changes: - CreateUserWithRLSPolicies: Clarify why we're creating a custom DatabaseConfing instance - HasDatabase: Clarify why we're ignoring tenancy_db_connection - DatabaseConfig: General refactor, clarify the role of the host conn - SQLiteDatabaseManager: Handle trailing DIRECTORY_SEPARATOR in static::$path - DisallowSqliteAttach: Don't throw any exceptions, just silently fail since the class isn't 100% portable - Clean up todos that are no longer relevant - Clean up dead code or comments in some database managers --- .../DatabaseTenancyBootstrapper.php | 2 +- src/Commands/CreateUserWithRLSPolicies.php | 11 ++++++-- src/Database/Concerns/HasDatabase.php | 3 +- src/Database/DatabaseConfig.php | 28 +++++++++---------- ...rmissionControlledMySQLDatabaseManager.php | 1 - ...ssionControlledPostgreSQLSchemaManager.php | 4 --- .../SQLiteDatabaseManager.php | 11 +++----- src/Features/DisallowSqliteAttach.php | 5 ++-- 8 files changed, 33 insertions(+), 32 deletions(-) diff --git a/src/Bootstrappers/DatabaseTenancyBootstrapper.php b/src/Bootstrappers/DatabaseTenancyBootstrapper.php index 33ff7b29..7f0bce0a 100644 --- a/src/Bootstrappers/DatabaseTenancyBootstrapper.php +++ b/src/Bootstrappers/DatabaseTenancyBootstrapper.php @@ -35,7 +35,7 @@ class DatabaseTenancyBootstrapper implements TenancyBootstrapper // Better debugging, but breaks cached lookup, so we disable this in prod if (app()->environment('local') || app()->environment('testing')) { $database = $tenant->database()->getName(); - if (! $tenant->database()->manager()->databaseExists($database)) { // todo@dbRefactor does this call correctly use the host connection? + if (! $tenant->database()->manager()->databaseExists($database)) { throw new TenantDatabaseDoesNotExistException($database); } } diff --git a/src/Commands/CreateUserWithRLSPolicies.php b/src/Commands/CreateUserWithRLSPolicies.php index 420df935..3998dc48 100644 --- a/src/Commands/CreateUserWithRLSPolicies.php +++ b/src/Commands/CreateUserWithRLSPolicies.php @@ -81,12 +81,19 @@ class CreateUserWithRLSPolicies extends Command #[\SensitiveParameter] string $password, ): DatabaseConfig { + // This is a bit of a hack. We want to use our existing createUser() logic. + // That logic needs a DatabaseConfig instance. However, we aren't really working + // with any specific tenant here. We also *don't* want to use anything tenant-specific + // here. We are creating the SHARED "RLS user". Therefore, we need a custom DatabaseConfig + // instance for this purpose. The easiest way to do that is to grab an empty Tenant model + // (we use TenantWithDatabase in RLS) and manually create the host connection, just like + // DatabaseConfig::manager() would. We don't call that method since we want to use our existing + // PermissionControlledPostgreSQLSchemaManager $manager instance, rather than the "tenant's manager". + /** @var TenantWithDatabase $tenantModel */ $tenantModel = tenancy()->model(); - // Use a temporary DatabaseConfig instance to set the host connection $temporaryDbConfig = $tenantModel->database(); - $temporaryDbConfig->purgeHostConnection(); $tenantHostConnectionName = $temporaryDbConfig->getTenantHostConnectionName(); diff --git a/src/Database/Concerns/HasDatabase.php b/src/Database/Concerns/HasDatabase.php index e1f4a55f..9388f168 100644 --- a/src/Database/Concerns/HasDatabase.php +++ b/src/Database/Concerns/HasDatabase.php @@ -28,7 +28,8 @@ trait HasDatabase } if ($key === $this->internalPrefix() . 'db_connection') { - // Remove DB connection because that's not used here + // Remove DB connection because that's not used for the connection *contents*. + // Instead the code uses getInternal('db_connection'). continue; } diff --git a/src/Database/DatabaseConfig.php b/src/Database/DatabaseConfig.php index 7dbbc577..bd167761 100644 --- a/src/Database/DatabaseConfig.php +++ b/src/Database/DatabaseConfig.php @@ -13,7 +13,6 @@ use Stancl\Tenancy\Database\Contracts\TenantWithDatabase as Tenant; use Stancl\Tenancy\Database\Exceptions\DatabaseManagerNotRegisteredException; use Stancl\Tenancy\Database\Exceptions\NoConnectionSetException; -// todo@dbRefactor refactor host connection logic to make customizing the host connection easier class DatabaseConfig { /** The tenant whose database we're dealing with. */ @@ -115,7 +114,7 @@ class DatabaseConfig { $this->tenant->setInternal('db_name', $this->getName()); - if ($this->connectionDriverManager($this->getTemplateConnectionDriver()) instanceof Contracts\ManagesDatabaseUsers) { + if ($this->managerForDriver($this->getTemplateConnectionDriver()) instanceof Contracts\ManagesDatabaseUsers) { $this->tenant->setInternal('db_username', $this->getUsername() ?? (static::$usernameGenerator)($this->tenant, $this)); $this->tenant->setInternal('db_password', $this->getPassword() ?? (static::$passwordGenerator)($this->tenant, $this)); } @@ -137,7 +136,9 @@ class DatabaseConfig } if ($template = config('tenancy.database.template_tenant_connection')) { - return is_array($template) ? array_merge($this->getCentralConnection(), $template) : config("database.connections.{$template}"); + return is_array($template) + ? array_merge($this->getCentralConnection(), $template) + : config("database.connections.{$template}"); } return $this->getCentralConnection(); @@ -176,10 +177,10 @@ class DatabaseConfig $config = $this->tenantConfig; $templateConnection = $this->getTemplateConnection(); - if ($this->connectionDriverManager($this->getTemplateConnectionDriver()) instanceof Contracts\ManagesDatabaseUsers) { - // We're removing the username and password because user with these credentials is not created yet - // If you need to provide username and password when using PermissionControlledMySQLDatabaseManager, - // consider creating a new connection and use it as `tenancy_db_connection` tenant config key + if ($this->managerForDriver($this->getTemplateConnectionDriver()) instanceof Contracts\ManagesDatabaseUsers) { + // We remove the username and password because the user with these credentials is not yet created. + // If you need to provide a username and a password when using a permission controlled database manager, + // consider creating a new connection and use it as `tenancy_db_connection`. unset($config['username'], $config['password']); } @@ -191,7 +192,7 @@ class DatabaseConfig } /** - * Purge the previous tenant connection before opening it for another tenant. + * Purge the previous host connection before opening it for another tenant. */ public function purgeHostConnection(): void { @@ -199,20 +200,20 @@ class DatabaseConfig } /** - * Get the TenantDatabaseManager for this tenant's connection. + * Get the TenantDatabaseManager for this tenant's host connection. * * @throws NoConnectionSetException|DatabaseManagerNotRegisteredException */ public function manager(): Contracts\TenantDatabaseManager { - // Laravel caches the previous PDO connection, so we purge it to be able to change the connection details + // Laravel persists the PDO connection, so we purge it to be able to change the connection details $this->purgeHostConnection(); // Create the tenant host connection config $tenantHostConnectionName = $this->getTenantHostConnectionName(); config(["database.connections.{$tenantHostConnectionName}" => $this->hostConnection()]); - $manager = $this->connectionDriverManager(config("database.connections.{$tenantHostConnectionName}.driver")); + $manager = $this->managerForDriver(config("database.connections.{$tenantHostConnectionName}.driver")); if ($manager instanceof Contracts\StatefulTenantDatabaseManager) { $manager->setConnection($tenantHostConnectionName); @@ -222,12 +223,11 @@ class DatabaseConfig } /** - * todo@dbRefactor come up with a better name - * Get database manager class from the given connection config's driver. + * Get the TenantDatabaseManager for a given database driver. * * @throws DatabaseManagerNotRegisteredException */ - protected function connectionDriverManager(string $driver): Contracts\TenantDatabaseManager + protected function managerForDriver(string $driver): Contracts\TenantDatabaseManager { $databaseManagers = config('tenancy.database.managers'); diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php index 8ea3e631..47ec11a2 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php @@ -23,7 +23,6 @@ class PermissionControlledMySQLDatabaseManager extends MySQLDatabaseManager impl { $database = $databaseConfig->getName(); $username = $databaseConfig->getUsername(); - $hostname = $databaseConfig->connection()['host']; $password = $databaseConfig->getPassword(); $this->connection()->statement("CREATE USER `{$username}`@`%` IDENTIFIED BY '{$password}'"); diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php index eca2ef87..b528d4e3 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php @@ -30,10 +30,6 @@ class PermissionControlledPostgreSQLSchemaManager extends PostgreSQLSchemaManage $tables = $this->connection()->select("SELECT table_name FROM information_schema.tables WHERE table_schema = '{$schema}' AND table_type = 'BASE TABLE'"); // Grant permissions to any existing tables. This is used with RLS - // todo@dbRefactor refactor this along with the todo in TenantDatabaseManager - // and move the grantPermissions() call inside the condition in `ManagesPostgresUsers::createUser()` - // but maybe moving it inside $createUser is wrong because some central user may migrate new tables - // while the RLS user should STILL get access to those tables foreach ($tables as $table) { $tableName = $table->table_name; diff --git a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php index b792f228..34ad394d 100644 --- a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php @@ -14,7 +14,9 @@ use Throwable; class SQLiteDatabaseManager implements TenantDatabaseManager { /** - * SQLite Database path without ending slash. + * SQLite database directory path. + * + * Defaults to database_path(). */ public static string|null $path = null; @@ -132,15 +134,10 @@ class SQLiteDatabaseManager implements TenantDatabaseManager return $baseConfig; } - public function setConnection(string $connection): void - { - // - } - public function getPath(string $name): string { if (static::$path) { - return static::$path . DIRECTORY_SEPARATOR . $name; + return rtrim(static::$path, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $name; } return database_path($name); diff --git a/src/Features/DisallowSqliteAttach.php b/src/Features/DisallowSqliteAttach.php index d7c57ac2..3d4d977e 100644 --- a/src/Features/DisallowSqliteAttach.php +++ b/src/Features/DisallowSqliteAttach.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Stancl\Tenancy\Features; -use Exception; use Illuminate\Database\Connectors\ConnectionFactory; use Illuminate\Database\SQLiteConnection; use Illuminate\Support\Facades\DB; @@ -48,9 +47,11 @@ class DisallowSqliteAttach implements Feature 'Linux' => 'so', 'Windows' => 'dll', 'Darwin' => 'dylib', - default => throw new Exception("The DisallowSqliteAttach feature doesn't support your operating system: " . PHP_OS_FAMILY), + default => 'error', }; + if ($suffix === 'error') return false; + $arch = php_uname('m'); $arm = $arch === 'aarch64' || $arch === 'arm64';