From bc7692ca3b287a236154a5e1dcebe021bdc505a3 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 18 Sep 2024 21:01:53 +1200 Subject: [PATCH] NEW Allow database read-only replicas --- cli-script.php | 3 + src/Control/Director.php | 20 ++ src/Core/CoreKernel.php | 117 +++++++-- src/Dev/SapphireTest.php | 5 + src/ORM/DB.php | 178 +++++++++++++- src/ORM/DataList.php | 4 +- src/ORM/DataObject.php | 8 + src/ORM/DataQuery.php | 41 +++- src/Security/Group.php | 2 + src/Security/LoginAttempt.php | 2 + src/Security/Member.php | 2 + src/Security/MemberPassword.php | 2 + src/Security/Permission.php | 80 +++--- src/Security/PermissionRole.php | 2 + src/Security/PermissionRoleCode.php | 2 + src/Security/RememberLoginHash.php | 2 + src/Security/Security.php | 6 + tests/php/Core/KernelTest.php | 32 +++ tests/php/ORM/DBReplicaTest.php | 230 ++++++++++++++++++ tests/php/ORM/DBReplicaTest.yml | 18 ++ .../php/ORM/DBReplicaTest/TestController.php | 18 ++ tests/php/ORM/DBReplicaTest/TestObject.php | 16 ++ 22 files changed, 712 insertions(+), 78 deletions(-) create mode 100644 tests/php/ORM/DBReplicaTest.php create mode 100644 tests/php/ORM/DBReplicaTest.yml create mode 100644 tests/php/ORM/DBReplicaTest/TestController.php create mode 100644 tests/php/ORM/DBReplicaTest/TestObject.php diff --git a/cli-script.php b/cli-script.php index 879b2de6546..93b5813ab1d 100755 --- a/cli-script.php +++ b/cli-script.php @@ -16,6 +16,9 @@ die(); } +// CLI scripts must only use the primary database connection and not replicas +DB::setMustUsePrimary(); + // Build request and detect flush $request = CLIRequestBuilder::createFromEnvironment(); diff --git a/src/Control/Director.php b/src/Control/Director.php index 119d4e746d0..518da5705d5 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -16,6 +16,7 @@ use SilverStripe\View\Requirements; use SilverStripe\View\Requirements_Backend; use SilverStripe\View\TemplateGlobalProvider; +use SilverStripe\ORM\DB; /** * Director is responsible for processing URLs, and providing environment information. @@ -83,6 +84,14 @@ class Director implements TemplateGlobalProvider */ private static $default_base_url = '`SS_BASE_URL`'; + /** + * List of rules that must only use the primary database and not a replica + */ + private static array $must_use_primary_db_rules = [ + 'dev', + 'Security', + ]; + public function __construct() { } @@ -295,6 +304,17 @@ public function handleRequest(HTTPRequest $request) { Injector::inst()->registerService($request, HTTPRequest::class); + // Check if primary database must be used based on request rules + // Note this check must happend before the rules are processed as + // $shiftOnSuccess is true during $request->match() below + $primaryDbOnlyRules = Director::config()->uninherited('must_use_primary_db_rules'); + foreach ($primaryDbOnlyRules as $rule) { + if ($request->match($rule)) { + DB::setMustUsePrimary(); + break; + } + } + $rules = Director::config()->uninherited('rules'); $this->extend('updateRules', $rules); diff --git a/src/Core/CoreKernel.php b/src/Core/CoreKernel.php index 7f968c8a3f4..5954868280b 100644 --- a/src/Core/CoreKernel.php +++ b/src/Core/CoreKernel.php @@ -66,31 +66,40 @@ protected function bootDatabaseGlobals() global $databaseConfig; global $database; - // Case 1: $databaseConfig global exists. Merge $database in as needed - if (!empty($databaseConfig)) { - if (!empty($database)) { - $databaseConfig['database'] = $this->getDatabasePrefix() . $database . $this->getDatabaseSuffix(); + for ($i = 0; $i <= 99; $i++) { + if ($i === 0) { + $key = 'default'; + } else { + $key = DB::getReplicaConfigKey($i); + if (!DB::hasConfig($key)) { + break; + } } - // Only set it if its valid, otherwise ignore $databaseConfig entirely - if (!empty($databaseConfig['database'])) { - DB::setConfig($databaseConfig); - - return; + // Case 1: $databaseConfig global exists. Merge $database in as needed + if (!empty($databaseConfig)) { + if (!empty($database)) { + $databaseConfig['database'] = $this->getDatabasePrefix() . $database . $this->getDatabaseSuffix(); + } + + // Only set it if its valid, otherwise ignore $databaseConfig entirely + if (!empty($databaseConfig['database'])) { + DB::setConfig($databaseConfig, $key); + return; + } } - } - // Case 2: $database merged into existing config - if (!empty($database)) { - $existing = DB::getConfig(); - $existing['database'] = $this->getDatabasePrefix() . $database . $this->getDatabaseSuffix(); - - DB::setConfig($existing); + // Case 2: $database merged into existing config + if (!empty($database)) { + $existing = DB::getConfig($key); + $existing['database'] = $this->getDatabasePrefix() . $database . $this->getDatabaseSuffix(); + DB::setConfig($existing, $key); + } } } /** - * Load default database configuration from environment variable + * Load default database configuration from environment variables */ protected function bootDatabaseEnvVars() { @@ -98,6 +107,17 @@ protected function bootDatabaseEnvVars() $databaseConfig = $this->getDatabaseConfig(); $databaseConfig['database'] = $this->getDatabaseName(); DB::setConfig($databaseConfig); + + // Set database replicas config + for ($i = 1; $i <= 99; $i++) { + $envKey = $this->getReplicaEnvKey('SS_DATABASE_SERVER', $i); + if (!Environment::hasEnv($envKey)) { + break; + } + $replicaDatabaseConfig = $this->getDatabaseReplicaConfig($i); + $configKey = DB::getReplicaConfigKey($i); + DB::setConfig($replicaDatabaseConfig, $configKey); + } } /** @@ -106,12 +126,56 @@ protected function bootDatabaseEnvVars() * @return array */ protected function getDatabaseConfig() + { + return $this->getSingleDataBaseConfig(0); + } + + private function getDatabaseReplicaConfig(int $replica) + { + return $this->getSingleDataBaseConfig($replica); + } + + /** + * Convert a database key to a replica key + * e.g. SS_DATABASE_SERVER -> SS_DATABASE_SERVER_REPLICA_01 + */ + private function getReplicaEnvKey(string $key, int $replica): string + { + // Do not allow replicas to define keys that could lead to unexpected behaviour if + // they do not match the primary database configuration + if (in_array($key, ['SS_DATABASE_CLASS', 'SS_DATABASE_NAME', 'SS_DATABASE_CHOOSE_NAME'])) { + return $key; + } + // Left pad replica number with a zero if it's less than 10 + return $key . '_REPLICA_' . str_pad($replica, 2, '0', STR_PAD_LEFT); + } + + /** + * Reads a single database configuration variable from the environment + * For replica databases, it will first attempt to find replica-specific configuration + * before falling back to the default configuration. + * + * Replicate specific configuration has `_REPLICA_01` appended to the key + * where 01 is the replica number. + */ + private function getDatabaseConfigVariable(string $key, int $replica): string + { + if ($replica > 0) { + $key = $this->getReplicaEnvKey($key, $replica); + } + if (Environment::hasEnv($key)) { + return Environment::getEnv($key); + } + return ''; + } + + private function getSingleDataBaseConfig(int $replica): array { $databaseConfig = [ - "type" => Environment::getEnv('SS_DATABASE_CLASS') ?: 'MySQLDatabase', - "server" => Environment::getEnv('SS_DATABASE_SERVER') ?: 'localhost', - "username" => Environment::getEnv('SS_DATABASE_USERNAME') ?: null, - "password" => Environment::getEnv('SS_DATABASE_PASSWORD') ?: null, + "type" => $this->getDatabaseConfigVariable('SS_DATABASE_CLASS', $replica) ?: 'MySQLDatabase', + "server" => $this->getDatabaseConfigVariable('SS_DATABASE_SERVER', $replica) ?: 'localhost', + "username" => $this->getDatabaseConfigVariable('SS_DATABASE_USERNAME', $replica) ?: null, + "password" => $this->getDatabaseConfigVariable('SS_DATABASE_PASSWORD', $replica) ?: null, ]; // Only add SSL keys in the array if there is an actual value associated with them @@ -122,7 +186,7 @@ protected function getDatabaseConfig() 'ssl_cipher' => 'SS_DATABASE_SSL_CIPHER', ]; foreach ($sslConf as $key => $envVar) { - $envValue = Environment::getEnv($envVar); + $envValue = $this->getDatabaseConfigVariable($envVar, $replica); if ($envValue) { $databaseConfig[$key] = $envValue; } @@ -138,25 +202,25 @@ protected function getDatabaseConfig() } // Set the port if called for - $dbPort = Environment::getEnv('SS_DATABASE_PORT'); + $dbPort = $this->getDatabaseConfigVariable('SS_DATABASE_PORT', $replica); if ($dbPort) { $databaseConfig['port'] = $dbPort; } // Set the timezone if called for - $dbTZ = Environment::getEnv('SS_DATABASE_TIMEZONE'); + $dbTZ = $this->getDatabaseConfigVariable('SS_DATABASE_TIMEZONE', $replica); if ($dbTZ) { $databaseConfig['timezone'] = $dbTZ; } // For schema enabled drivers: - $dbSchema = Environment::getEnv('SS_DATABASE_SCHEMA'); + $dbSchema = $this->getDatabaseConfigVariable('SS_DATABASE_SCHEMA', $replica); if ($dbSchema) { $databaseConfig["schema"] = $dbSchema; } // For SQlite3 memory databases (mainly for testing purposes) - $dbMemory = Environment::getEnv('SS_DATABASE_MEMORY'); + $dbMemory = $this->getDatabaseConfigVariable('SS_DATABASE_MEMORY', $replica); if ($dbMemory) { $databaseConfig["memory"] = $dbMemory; } @@ -184,6 +248,7 @@ protected function getDatabaseSuffix() /** * Get name of database + * Note that any replicas must have the same database name as the primary database * * @return string */ diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index b54a58e5554..11640b76e16 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -41,6 +41,7 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\Mailer\MailerInterface; use Symfony\Component\Mailer\Transport\NullTransport; +use SilverStripe\ORM\DB; /** * Test case class for the Silverstripe framework. @@ -395,6 +396,10 @@ protected function currentTestDisablesDatabase() */ public static function setUpBeforeClass(): void { + // Disallow the use of DB replicas in tests + // This prevents replicas from being used if there are environment variables set to use them + DB::setCanUseReplicas(false); + // Start tests static::start(); diff --git a/src/ORM/DB.php b/src/ORM/DB.php index 054b858d02f..172e4ce3d4e 100644 --- a/src/ORM/DB.php +++ b/src/ORM/DB.php @@ -3,6 +3,7 @@ namespace SilverStripe\ORM; use InvalidArgumentException; +use RunTimeException; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; use SilverStripe\Core\Config\Config; @@ -58,14 +59,18 @@ class DB */ protected static $configs = []; - - /** * The last SQL query run. * @var string */ public static $lastQuery; + /** + * The name of the last connection used. This is only used for unit-testing purposes. + * @interal + */ + private static string $lastConnectionName = ''; + /** * Internal flag to keep track of when db connection was attempted. */ @@ -96,10 +101,22 @@ public static function set_conn(Database $connection, $name = 'default') */ public static function get_conn($name = 'default') { + // Allow default to connect to replica if configured + if ($name === 'default') { + $name = DB::getDefaultConnectionName(); + } + if (isset(DB::$connections[$name])) { + DB::$lastConnectionName = $name; return DB::$connections[$name]; } + // Ensure that primary config is set, clone it from default config + if ($name === 'primary' && !static::hasConfig('primary')) { + $config = static::getConfig('default'); + static::setConfig($config, 'primary'); + } + // lazy connect $config = static::getConfig($name); if ($config) { @@ -109,6 +126,124 @@ public static function get_conn($name = 'default') return null; } + /** + * Only use the primary database connection for the rest of the current request + * + * @internal + */ + private static bool $mustUsePrimary = false; + + /** + * Used by DB::withPrimary() to count the number of times it has been called + * Uses an int instead of a bool to allow for nested calls * + * + * @internal + */ + private static int $withPrimaryCount = 0; + + /** + * The key of the previously choosen random replica config + * + * @internal + */ + private static string $randomReplicaConfigKey = ''; + + /** + * Flag to determine if replicas can be used + * + * @internal + */ + private static bool $canUseReplicas = true; + + /** + * Set the flag to allow or disallow the use of replica database connections + * For instance disable when running unit-tests + */ + public static function setCanUseReplicas(bool $canUseReplicas): void + { + DB::$canUseReplicas = $canUseReplicas; + } + + /** + * Set the flag to only use the primary database connection for the current request + * meaning that replia connections will no longer be used + * + * This intentioally does not have a parameter to set this back to false, as this it to prevent + * accidentally attempting writing to a replica, or reading from an out of date replica + * after a write + */ + public static function setMustUsePrimary(): void + { + DB::$mustUsePrimary = true; + } + + /** + * Only use the primary database connection when calling $callback + * Use this when doing non-mutable queries on the primary database where querying + * an out of sync replica could cause issues + * There's no need to use this with mutable queries, or after calling a mutable query + * as the primary database connection will be automatically used + */ + public static function withPrimary(callable $callback): mixed + { + DB::$withPrimaryCount++; + $result = $callback(); + DB::$withPrimaryCount--; + return $result; + } + + /** + * Get the name of the database connection to use for the given SQL query + * The 'default' connection can be either the primary or a replica connection if configured + */ + private static function getDefaultConnectionName(string $sql = ''): string + { + if (DB::$mustUsePrimary || DB::$withPrimaryCount > 0 || !DB::$canUseReplicas || !DB::hasReplicaConfig()) { + return 'primary'; + } + if ($sql) { + $dbClass = DB::getConfig('default')['type']; + // This must use getServiceSpec() and not Injector::get/create() followed by + // getConnector() as this can remove the dbConn from a different connection + // under edge case conditions + $dbSpec = Injector::inst()->getServiceSpec($dbClass); + $connectorService = $dbSpec['properties']['connector']; + $connector = Injector::inst()->convertServiceProperty($connectorService); + if ($connector->isQueryMutable($sql)) { + DB::$mustUsePrimary = true; + return 'primary'; + } + } + if (DB::$randomReplicaConfigKey) { + return DB::$randomReplicaConfigKey; + } + $name = DB::getRandomReplicaConfigKey(); + DB::$randomReplicaConfigKey = $name; + return $name; + } + + /** + * Get a random replica database configuration key from the available replica configurations + * The replica choosen will be used for the rest of the request, unless the primary connection + * is forced + */ + private static function getRandomReplicaConfigKey(): string + { + $replicaNumbers = []; + for ($i = 1; $i < 99; $i++) { + $replicaKey = DB::getReplicaConfigKey($i); + if (DB::hasConfig($replicaKey)) { + $replicaNumbers[] = $i; + } + } + if (count($replicaNumbers) === 0) { + throw new RunTimeException('No replica configurations found'); + } + // Choose a random replica + $index = rand(0, count($replicaNumbers) - 1); + return DB::getReplicaConfigKey($replicaNumbers[$index]); + } + /** * Retrieves the schema manager for the current database * @@ -288,6 +423,7 @@ public static function connect($databaseConfig, $label = 'default') $conn = Injector::inst()->create($dbClass); DB::set_conn($conn, $label); $conn->connect($databaseConfig); + DB::$lastConnectionName = $label; return $conn; } @@ -311,11 +447,39 @@ public static function setConfig($databaseConfig, $name = 'default') */ public static function getConfig($name = 'default') { - if (isset(static::$configs[$name])) { + if (static::hasConfig($name)) { return static::$configs[$name]; } } + /** + * Check if a named connection config exists + */ + public static function hasConfig($name = 'default'): bool + { + return array_key_exists($name, static::$configs); + } + + /** + * Get a replica database configuration key + * e.g. replica_01 + */ + public static function getReplicaConfigKey(int $replica): string + { + return 'replica_' . str_pad($replica, 2, '0', STR_PAD_LEFT); + } + + /** + * Check if there are any replica configurations + */ + public static function hasReplicaConfig(): bool + { + $configKeys = array_keys(static::$configs); + return !empty(array_filter($configKeys, function (string $key) { + return (bool) preg_match('#^replica_[0-9]+$#', $key); + })); + } + /** * Returns true if a database connection has been attempted. * In particular, it lets the caller know if we're still so early in the execution pipeline that @@ -335,8 +499,8 @@ public static function connection_attempted() public static function query($sql, $errorLevel = E_USER_ERROR) { DB::$lastQuery = $sql; - - return DB::get_conn()->query($sql, $errorLevel); + $name = DB::getDefaultConnectionName($sql); + return DB::get_conn($name)->query($sql, $errorLevel); } /** @@ -427,8 +591,8 @@ public static function inline_parameters($sql, $parameters) public static function prepared_query($sql, $parameters, $errorLevel = E_USER_ERROR) { DB::$lastQuery = $sql; - - return DB::get_conn()->preparedQuery($sql, $parameters, $errorLevel); + $name = DB::getDefaultConnectionName($sql); + return DB::get_conn($name)->preparedQuery($sql, $parameters, $errorLevel); } /** diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 72ccacd7113..03ef9ad6a9e 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -232,7 +232,7 @@ public function setDataQueryParam($keyOrArray, $val = null) */ public function sql(&$parameters = []) { - return $this->dataQuery->query()->sql($parameters); + return $this->dataQuery->sql($parameters); } /** @@ -1035,7 +1035,7 @@ private function getEagerLoadVariables(string $relationChain, string $relationNa private function executeQuery(): Query { - $query = $this->dataQuery->query()->execute(); + $query = $this->dataQuery->execute(); $this->fetchEagerLoadRelations($query); return $query; } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index c496f631d9f..c30fc7ae6c2 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -141,6 +141,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity */ private static $default_classname = null; + /** + * Whether this DataObject class must only use the primary database and not a replica + * Note that this will be only be enforced when using DataQuery::execute() or + * another method that uses calls DataQuery::execute() internally e.g. as DataObject::get() + * This will not be enforced when using another method to query data e.g. SQLSelect or DB::query() + */ + private static bool $must_use_primary_db = false; + /** * Data stored in this objects database record. An array indexed by fieldname. * diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index c9230986947..27c2bf044d5 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -10,6 +10,7 @@ use SilverStripe\ORM\Queries\SQLConditionGroup; use SilverStripe\ORM\Queries\SQLSelect; use InvalidArgumentException; +use SilverStripe\Core\Config\Config; /** * An object representing a query of data from the DataObject's supporting database. @@ -442,6 +443,18 @@ protected function ensureSelectContainsOrderbyColumns($query, $originalSelect = } } + /** + * Calls a callback on either the primary database or a replica, with respect to the configured + * value of `must_use_primary_db` on the current dataClass + */ + private function withCorrectDatabase(callable $callback): mixed + { + if (Config::inst()->get($this->dataClass(), 'must_use_primary_db')) { + return DB::withPrimary($callback); + } + return $callback(); + } + /** * Execute the query and return the result as {@link SS_Query} object. * @@ -449,7 +462,9 @@ protected function ensureSelectContainsOrderbyColumns($query, $originalSelect = */ public function execute() { - return $this->getFinalisedQuery()->execute(); + return $this->withCorrectDatabase( + fn() => $this->getFinalisedQuery()->execute() + ); } /** @@ -472,7 +487,9 @@ public function sql(&$parameters = []) public function count() { $quotedColumn = DataObject::getSchema()->sqlColumnForField($this->dataClass(), 'ID'); - return $this->getFinalisedQuery()->count("DISTINCT {$quotedColumn}"); + return $this->withCorrectDatabase( + fn() => $this->getFinalisedQuery()->count("DISTINCT {$quotedColumn}") + ); } /** @@ -501,7 +518,9 @@ public function exists(): bool // Wrap the whole thing in an "EXISTS" $sql = 'SELECT CASE WHEN EXISTS(' . $statement->sql($params) . ') THEN 1 ELSE 0 END'; - $result = DB::prepared_query($sql, $params); + $result = $this->withCorrectDatabase( + fn() => DB::prepared_query($sql, $params) + ); $row = $result->record(); $result = reset($row); @@ -582,7 +601,9 @@ public function sum($field) */ public function aggregate($expression) { - return $this->getFinalisedQuery()->aggregate($expression)->execute()->value(); + return $this->withCorrectDatabase( + fn() => $this->getFinalisedQuery()->aggregate($expression)->execute()->value() + ); } /** @@ -593,7 +614,9 @@ public function aggregate($expression) */ public function firstRow() { - return $this->getFinalisedQuery()->firstRow(); + return $this->withCorrectDatabase( + fn() => $this->getFinalisedQuery()->firstRow() + ); } /** @@ -604,7 +627,9 @@ public function firstRow() */ public function lastRow() { - return $this->getFinalisedQuery()->lastRow(); + return $this->withCorrectDatabase( + fn() => $this->getFinalisedQuery()->lastRow() + ); } /** @@ -1344,7 +1369,9 @@ public function column($field = 'ID') $query->selectField($fieldExpression, $field); $this->ensureSelectContainsOrderbyColumns($query, $originalSelect); - return $query->execute()->column($field); + return $this->withCorrectDatabase( + fn() => $query->execute()->column($field) + ); } /** diff --git a/src/Security/Group.php b/src/Security/Group.php index dff5e81b84e..69efee2c45d 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -85,6 +85,8 @@ class Group extends DataObject private static $table_name = "Group"; + private static bool $must_use_primary_db = true; + private static $indexes = [ 'Title' => true, 'Code' => true, diff --git a/src/Security/LoginAttempt.php b/src/Security/LoginAttempt.php index 9ff1b31ce4e..b5142073c66 100644 --- a/src/Security/LoginAttempt.php +++ b/src/Security/LoginAttempt.php @@ -50,6 +50,8 @@ class LoginAttempt extends DataObject private static $table_name = "LoginAttempt"; + private static bool $must_use_primary_db = true; + /** * @param bool $includerelations Indicate if the labels returned include relation fields * @return array diff --git a/src/Security/Member.php b/src/Security/Member.php index 2003e8f05cd..df2d4035475 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -98,6 +98,8 @@ class Member extends DataObject private static $table_name = "Member"; + private static bool $must_use_primary_db = true; + private static $default_sort = '"Surname", "FirstName"'; private static $indexes = [ diff --git a/src/Security/MemberPassword.php b/src/Security/MemberPassword.php index 8488671f6c6..cebb2cce116 100644 --- a/src/Security/MemberPassword.php +++ b/src/Security/MemberPassword.php @@ -27,6 +27,8 @@ class MemberPassword extends DataObject private static $table_name = "MemberPassword"; + private static bool $must_use_primary_db = true; + /** * Log a password change from the given member. * Call MemberPassword::log($this) from within Member whenever the password is changed. diff --git a/src/Security/Permission.php b/src/Security/Permission.php index ce57f81bbf3..9142cadeb6e 100644 --- a/src/Security/Permission.php +++ b/src/Security/Permission.php @@ -46,6 +46,8 @@ class Permission extends DataObject implements TemplateGlobalProvider, Resettabl private static $table_name = "Permission"; + private static bool $must_use_primary_db = true; + /** * This is the value to use for the "Type" field if a permission should be * granted. @@ -233,15 +235,15 @@ public static function checkMember($member, $code, $arg = "any", $strict = true) } // Raw SQL for efficiency - $permission = DB::prepared_query( + $permission = DB::withPrimary(fn() => DB::prepared_query( "SELECT \"ID\" - FROM \"Permission\" - WHERE ( - \"Code\" IN ($codeClause $adminClause) - AND \"Type\" = ? - AND \"GroupID\" IN ($groupClause) - $argClause - )", + FROM \"Permission\" + WHERE ( + \"Code\" IN ($codeClause $adminClause) + AND \"Type\" = ? + AND \"GroupID\" IN ($groupClause) + $argClause + )", array_merge( $codeParams, $adminParams, @@ -249,7 +251,7 @@ public static function checkMember($member, $code, $arg = "any", $strict = true) $groupParams, $argParams ) - )->value(); + )->value()); if ($permission) { return $permission; @@ -257,15 +259,15 @@ public static function checkMember($member, $code, $arg = "any", $strict = true) // Strict checking disabled? if (!static::config()->strict_checking || !$strict) { - $hasPermission = DB::prepared_query( + $hasPermission = DB::withPrimary(fn() => DB::prepared_query( "SELECT COUNT(*) - FROM \"Permission\" - WHERE ( - \"Code\" IN ($codeClause) AND - \"Type\" = ? - )", + FROM \"Permission\" + WHERE ( + \"Code\" IN ($codeClause) AND + \"Type\" = ? + )", array_merge($codeParams, [Permission::GRANT_PERMISSION]) - )->value(); + )->value()); if (!$hasPermission) { return false; @@ -288,25 +290,29 @@ public static function permissions_for_member($memberID) if ($groupList) { $groupCSV = implode(", ", $groupList); - $allowed = array_unique(DB::query(" - SELECT \"Code\" - FROM \"Permission\" - WHERE \"Type\" = " . Permission::GRANT_PERMISSION . " AND \"GroupID\" IN ($groupCSV) - - UNION - - SELECT \"Code\" - FROM \"PermissionRoleCode\" PRC - INNER JOIN \"PermissionRole\" PR ON PRC.\"RoleID\" = PR.\"ID\" - INNER JOIN \"Group_Roles\" GR ON GR.\"PermissionRoleID\" = PR.\"ID\" - WHERE \"GroupID\" IN ($groupCSV) - ")->column() ?? []); - - $denied = array_unique(DB::query(" - SELECT \"Code\" - FROM \"Permission\" - WHERE \"Type\" = " . Permission::DENY_PERMISSION . " AND \"GroupID\" IN ($groupCSV) - ")->column() ?? []); + $allowed = array_unique( + DB::withPrimary(fn() => DB::query(" + SELECT \"Code\" + FROM \"Permission\" + WHERE \"Type\" = " . Permission::GRANT_PERMISSION . " AND \"GroupID\" IN ($groupCSV) + + UNION + + SELECT \"Code\" + FROM \"PermissionRoleCode\" PRC + INNER JOIN \"PermissionRole\" PR ON PRC.\"RoleID\" = PR.\"ID\" + INNER JOIN \"Group_Roles\" GR ON GR.\"PermissionRoleID\" = PR.\"ID\" + WHERE \"GroupID\" IN ($groupCSV) + "))->column() ?? [] + ); + + $denied = array_unique( + DB::withPrimary(fn() => DB::query(" + SELECT \"Code\" + FROM \"Permission\" + WHERE \"Type\" = " . Permission::DENY_PERMISSION . " AND \"GroupID\" IN ($groupCSV) + "))->column() ?? [] + ); return array_diff($allowed ?? [], $denied); } @@ -584,7 +590,9 @@ public static function get_codes($grouped = true) $flatCodeArray[] = $code; } } - $otherPerms = DB::query("SELECT DISTINCT \"Code\" From \"Permission\" WHERE \"Code\" != ''")->column(); + $otherPerms = DB::withPrimary( + fn() => DB::query("SELECT DISTINCT \"Code\" From \"Permission\" WHERE \"Code\" != ''")->column() + ); if ($otherPerms) { foreach ($otherPerms as $otherPerm) { diff --git a/src/Security/PermissionRole.php b/src/Security/PermissionRole.php index 5f96f877024..1e3d88e103d 100644 --- a/src/Security/PermissionRole.php +++ b/src/Security/PermissionRole.php @@ -40,6 +40,8 @@ class PermissionRole extends DataObject private static $table_name = "PermissionRole"; + private static bool $must_use_primary_db = true; + private static $default_sort = '"Title"'; private static $singular_name = 'Role'; diff --git a/src/Security/PermissionRoleCode.php b/src/Security/PermissionRoleCode.php index 8d7e2979bd0..8893382419e 100644 --- a/src/Security/PermissionRoleCode.php +++ b/src/Security/PermissionRoleCode.php @@ -23,6 +23,8 @@ class PermissionRoleCode extends DataObject ]; private static $table_name = "PermissionRoleCode"; + + private static bool $must_use_primary_db = true; private static $indexes = [ "Code" => true, diff --git a/src/Security/RememberLoginHash.php b/src/Security/RememberLoginHash.php index 8af22057353..1895445b226 100644 --- a/src/Security/RememberLoginHash.php +++ b/src/Security/RememberLoginHash.php @@ -44,6 +44,8 @@ class RememberLoginHash extends DataObject private static $table_name = "RememberLoginHash"; + private static bool $must_use_primary_db = true; + /** * Determines if logging out on one device also clears existing login tokens * on all other devices owned by the member. diff --git a/src/Security/Security.php b/src/Security/Security.php index 214dbcb47d8..81ff66e1a09 100644 --- a/src/Security/Security.php +++ b/src/Security/Security.php @@ -437,6 +437,12 @@ public static function permissionFailure($controller = null, $messageSet = null) */ public static function setCurrentUser($currentUser = null) { + // Always use the primary database and not a replica if a CMS user is logged in + // This is to ensure that when viewing content on the frontend it is always + // up to date i.e. not from an unsynced replica + if ($currentUser && Permission::checkMember($currentUser, 'CMS_ACCESS')) { + DB::setMustUsePrimary(); + } Security::$currentUser = $currentUser; } diff --git a/tests/php/Core/KernelTest.php b/tests/php/Core/KernelTest.php index 42f05c2714e..fd38d43df1a 100644 --- a/tests/php/Core/KernelTest.php +++ b/tests/php/Core/KernelTest.php @@ -11,6 +11,10 @@ use SilverStripe\Core\Injector\InjectorLoader; use SilverStripe\Core\Kernel; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Core\Environment; +use ReflectionClass; +use SilverStripe\ORM\DB; +use ReflectionObject; class KernelTest extends SapphireTest { @@ -81,4 +85,32 @@ public function testInvalidConfigDetection() $kernel->getConfigLoader()->getManifest(); } + + public function testReplicaDatabaseVarsLoaded() + { + // Set environment variables for a fake replica database + Environment::setEnv('SS_DATABASE_SERVER_REPLICA_01', 'the-moon'); + Environment::setEnv('SS_DATABASE_USERNAME_REPLICA_01', 'alien'); + Environment::setEnv('SS_DATABASE_PASSWORD_REPLICA_01', 'hi_people'); + // Get the CoreKernel + /** @var Kernel $kernel */ + $kernel = Injector::inst()->get(Kernel::class); + /** @var CoreKernel $coreKernel */ + $coreKernel = $kernel->nest(); + $this->assertTrue(is_a($coreKernel, CoreKernel::class)); + // Boot the database environment variables + $reflector = new ReflectionObject($coreKernel); + $method = $reflector->getMethod('bootDatabaseEnvVars'); + $method->setAccessible(true); + $method->invoke($coreKernel); + // Assert DB config was updated + $default = DB::getConfig('default'); + $configs = (new ReflectionClass(DB::class))->getStaticPropertyValue('configs'); + $this->assertSame([ + 'type' => $default['type'], + 'server' => 'the-moon', + 'username' => 'alien', + 'password' => 'hi_people', + ], $configs['replica_01']); + } } diff --git a/tests/php/ORM/DBReplicaTest.php b/tests/php/ORM/DBReplicaTest.php new file mode 100644 index 00000000000..148d036ffec --- /dev/null +++ b/tests/php/ORM/DBReplicaTest.php @@ -0,0 +1,230 @@ +setupConfigsAndConnections(true); + // Allow using replicas - this is disabled by default in SapphireTest::setUpBeforeClass() + DB::setCanUseReplicas(true); + // Reset mustUsePrimary after using mutable sql to create yml fixtures + // and also because by default an ADMIN user is logged in when using fixtures in SapphireTest::setUp() + // and also prevent tests from affecting subsequent tests + (new ReflectionClass(DB::class))->setStaticPropertyValue('mustUsePrimary', false); + } + + protected function tearDown(): void + { + $this->setupConfigsAndConnections(false); + DB::setCanUseReplicas(false); + parent::tearDown(); + } + + /** + * Using reflection, set DB::configs and DB::connections with a fake a replica connection + * that points to the same connection as the primary connection. + */ + private function setupConfigsAndConnections($includeReplica = true) + { + $reflector = new ReflectionClass(DB::class); + $defaultConfig = DB::getConfig('default'); + $configs = ['default' => $defaultConfig]; + if ($includeReplica) { + $configs['primary'] = $defaultConfig; + $configs['replica_01'] = $defaultConfig; + } + $reflector->setStaticPropertyValue('configs', $configs); + // Make a call to the database to ensure the connection is established + TestObject::get()->count(); + // Create connections + $defaultConnection = DB::get_conn('default'); + $connections = ['default' => $defaultConnection]; + if ($includeReplica) { + $connections['primary'] = $defaultConnection; + $connections['replica_01'] = $defaultConnection; + } + $reflector->setStaticPropertyValue('connections', $connections); + } + + /** + * Get the last connection name used by the DB class. This shows if a replica was used. + */ + private function getLastConnectionName() + { + return (new ReflectionClass(DB::class))->getStaticPropertyValue('lastConnectionName'); + } + + public function testUsesReplica() + { + // Assert uses replica by default + TestObject::get()->count(); + $this->assertSame('replica_01', $this->getLastConnectionName()); + // Assert uses primary when using withPrimary() + DB::withPrimary(fn() => TestObject::get()->count()); + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that withPrimary() was only temporary + TestObject::get()->count(); + $this->assertSame('replica_01', $this->getLastConnectionName()); + // Assert DB::setMustUsePrimary() forces primary from now on + DB::setMustUsePrimary(); + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testMutableSql() + { + // Assert that using mutable sql in an ORM method with a dataclass uses primary + TestObject::create(['Title' => 'testing'])->write(); + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that now all subsequent queries use primary + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testMutableSqlDbQuery() + { + // Assert that using mutable sql in DB::query() uses primary + DB::query('INSERT INTO "DBReplicaTest_TestObject" ("Title") VALUES (\'testing\')'); + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that now all subsequent queries use primary + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testMutableSqlDbPreparedQuery() + { + // Assert that using mutable sql in DB::prepared_query() uses primary + DB::prepared_query('INSERT INTO "DBReplicaTest_TestObject" ("Title") VALUES (?)', ['testing']); + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that now all subsequent queries use primary + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testSetCurrentUser() + { + // Assert non cms user logged in uses replica + $member = Member::get()->find('FirstName', 'random'); + Security::setCurrentUser($member); + TestObject::get()->count(); + $this->assertSame('replica_01', $this->getLastConnectionName()); + // Assert cms user logged in uses primary + $member = Member::get()->find('FirstName', 'cmsuser'); + Security::setCurrentUser($member); + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testDataObjectMustUsePrimaryDb() + { + // Assert that DataList::getIterator() respect DataObject.must_use_primary_db + foreach (TestObject::get() as $object) { + $object->Title = 'test2'; + } + $this->assertSame('replica_01', $this->getLastConnectionName()); + foreach (Group::get() as $group) { + $group->Title = 'test2'; + } + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that DataQuery methods without params respect DataObject.must_use_primary_db + $methods = [ + 'count', + 'exists', + 'firstRow', + 'lastRow' + ]; + foreach ($methods as $method) { + (new DataQuery(TestObject::class))->$method(); + $this->assertSame('replica_01', $this->getLastConnectionName(), "method is $method"); + (new DataQuery(Group::class))->$method(); + $this->assertSame('primary', $this->getLastConnectionName(), "method is $method"); + } + // Assert that DataQuery methods with a param respect DataObject.must_use_primary_db + $methods = [ + 'max', + 'min', + 'avg', + 'sum', + 'column', + ]; + foreach ($methods as $method) { + (new DataQuery(TestObject::class))->$method('ID'); + $this->assertSame('replica_01', $this->getLastConnectionName(), "method is $method"); + (new DataQuery(Group::class))->$method('ID'); + $this->assertSame('primary', $this->getLastConnectionName(), "method is $method"); + } + } + + public function testRoutes() + { + // Create a custom rule to test our controller that should default to using a replica + $rules = Config::inst()->get(Director::class, 'rules'); + $rules['test'] = TestController::class; + // Ensure that routes staring with '$' are at the bottom of the assoc array index and don't override + // our new 'test' route + uksort($rules, fn($a, $b) => str_starts_with($a, '$') ? 1 : (str_starts_with($b, '$') ? -1 : 0)); + // Assert that normal routes use replica + $this->get('/test'); + $this->assertSame('replica_01', $this->getLastConnectionName()); + // Assert that Security route is forced to use primary + $this->get('/Security/login'); + $this->assertSame('primary', $this->getLastConnectionName()); + // Assert that dev route is forced to use primary + $this->get('/dev/tasks'); + $this->assertSame('primary', $this->getLastConnectionName()); + } + + public function testSetCanUseReplicas() + { + DB::setCanUseReplicas(false); + TestObject::get()->count(); + $this->assertSame('primary', $this->getLastConnectionName()); + DB::setCanUseReplicas(true); + TestObject::get()->count(); + $this->assertSame('replica_01', $this->getLastConnectionName()); + } + + public function testConfigMethods() + { + // Test DB::hasConfig() + $this->assertFalse(DB::hasConfig('lorem')); + DB::setConfig(['type' => 'lorem'], 'lorem'); + $this->assertTrue(DB::hasConfig('lorem')); + // Test DB::hasReplicaConfig() + $this->assertTrue(DB::hasReplicaConfig()); + $default = DB::getConfig('default'); + (new ReflectionClass(DB::class))->setStaticPropertyValue('configs', ['default' => $default]); + $this->assertFalse(DB::hasReplicaConfig()); + // Test DB::getReplicaConfigKey() + $this->assertSame('replica_03', DB::getReplicaConfigKey(3)); + $this->assertSame('replica_58', DB::getReplicaConfigKey(58)); + } +} diff --git a/tests/php/ORM/DBReplicaTest.yml b/tests/php/ORM/DBReplicaTest.yml new file mode 100644 index 00000000000..4f7dd7f8c90 --- /dev/null +++ b/tests/php/ORM/DBReplicaTest.yml @@ -0,0 +1,18 @@ +SilverStripe\ORM\Tests\DBReplicaTest\TestObject: + test: + Title: 'test' +SilverStripe\Security\Permission: + test: + Code: 'CMS_ACCESS_Something' +SilverStripe\Security\Group: + test: + Title: 'test' + Permissions: + - =>SilverStripe\Security\Permission.test +SilverStripe\Security\Member: + cmsuser: + FirstName: 'CmsUser' + Groups: + - =>SilverStripe\Security\Group.test + random: + FirstName: 'Random' diff --git a/tests/php/ORM/DBReplicaTest/TestController.php b/tests/php/ORM/DBReplicaTest/TestController.php new file mode 100644 index 00000000000..56cdc92eade --- /dev/null +++ b/tests/php/ORM/DBReplicaTest/TestController.php @@ -0,0 +1,18 @@ +count(); + $response = $this->getResponse(); + $response->setBody('DB_REPLICA_TEST_CONTROLLER'); + return $response; + } +} diff --git a/tests/php/ORM/DBReplicaTest/TestObject.php b/tests/php/ORM/DBReplicaTest/TestObject.php new file mode 100644 index 00000000000..aac3f295c74 --- /dev/null +++ b/tests/php/ORM/DBReplicaTest/TestObject.php @@ -0,0 +1,16 @@ + 'Varchar', + ]; + + private static $table_name = 'DBReplicaTest_TestObject'; +}