From 750fdced131a8d2fd3ed30ba06b19506651f4258 Mon Sep 17 00:00:00 2001 From: caddoo Date: Sat, 7 Sep 2024 00:57:56 +1200 Subject: [PATCH] Fix loose typing of constructor for transaction level, and remove public property. (#22535) * Fix loose typing of constructor for transaction level, and remove public property. * Change interface to support mix of static vs non-static methods * Add retry logic back for transactions * reintroduce exception variable for php72 * reintroduce exception variable for php72 * fix broken integration test * fix broken integration test * fix broken integration test * fix broken integration test * Reorder methods * Move duplicate logic into traits --- core/Db.php | 8 +++-- core/Db/Adapter/Mysqli.php | 6 ++-- core/Db/Adapter/Pdo/Mysql.php | 6 ++-- core/Db/AdapterInterface.php | 2 +- core/Db/TransactionLevel.php | 35 ++++++++----------- core/Db/TransactionalDatabaseDynamicTrait.php | 32 +++++++++++++++++ core/Db/TransactionalDatabaseInterface.php | 11 ++++++ core/Db/TransactionalDatabaseStaticTrait.php | 32 +++++++++++++++++ core/Tracker/Db.php | 9 ++--- .../DataAccess/LogAggregatorTest.php | 4 +-- 10 files changed, 106 insertions(+), 39 deletions(-) create mode 100644 core/Db/TransactionalDatabaseDynamicTrait.php create mode 100644 core/Db/TransactionalDatabaseInterface.php create mode 100644 core/Db/TransactionalDatabaseStaticTrait.php diff --git a/core/Db.php b/core/Db.php index c0b58bfecd2..6df549b5fc7 100644 --- a/core/Db.php +++ b/core/Db.php @@ -12,6 +12,8 @@ use Exception; use Piwik\Db\Adapter; use Piwik\Db\Schema; +use Piwik\Db\TransactionalDatabaseInterface; +use Piwik\Db\TransactionalDatabaseStaticTrait; /** * Contains SQL related helper functions for Piwik's MySQL database. @@ -32,8 +34,10 @@ * * @api */ -class Db +class Db implements TransactionalDatabaseInterface { + use TransactionalDatabaseStaticTrait; + public const SQL_MODE = 'NO_AUTO_VALUE_ON_ZERO'; private static $connection = null; @@ -41,8 +45,6 @@ class Db private static $logQueries = true; - // this is used for indicate TransactionLevel Cache - public $supportsUncommitted; /** * Returns the database connection and creates it if it hasn't been already. * diff --git a/core/Db/Adapter/Mysqli.php b/core/Db/Adapter/Mysqli.php index 5dca6138b40..cfc838abee1 100644 --- a/core/Db/Adapter/Mysqli.php +++ b/core/Db/Adapter/Mysqli.php @@ -21,15 +21,13 @@ */ class Mysqli extends Zend_Db_Adapter_Mysqli implements AdapterInterface { + use Db\TransactionalDatabaseDynamicTrait; + /** * Constructor * * @param array|Zend_Config $config database configuration */ - - // this is used for indicate TransactionLevel Cache - public $supportsUncommitted; - public function __construct($config) { // Enable LOAD DATA INFILE diff --git a/core/Db/Adapter/Pdo/Mysql.php b/core/Db/Adapter/Pdo/Mysql.php index b36927e4e80..e6664bacbe3 100644 --- a/core/Db/Adapter/Pdo/Mysql.php +++ b/core/Db/Adapter/Pdo/Mysql.php @@ -25,15 +25,13 @@ */ class Mysql extends Zend_Db_Adapter_Pdo_Mysql implements AdapterInterface { + use Db\TransactionalDatabaseDynamicTrait; + /** * Constructor * * @param array|Zend_Config $config database configuration */ - - // this is used for indicate TransactionLevel Cache - public $supportsUncommitted; - public function __construct($config) { // Enable LOAD DATA INFILE diff --git a/core/Db/AdapterInterface.php b/core/Db/AdapterInterface.php index 18debd257e4..9b54fdc4a3a 100644 --- a/core/Db/AdapterInterface.php +++ b/core/Db/AdapterInterface.php @@ -13,7 +13,7 @@ /** */ -interface AdapterInterface +interface AdapterInterface extends TransactionalDatabaseInterface { /** * Reset the configuration variables in this adapter. diff --git a/core/Db/TransactionLevel.php b/core/Db/TransactionLevel.php index cb704804fa3..6ff4ca681dd 100644 --- a/core/Db/TransactionLevel.php +++ b/core/Db/TransactionLevel.php @@ -19,16 +19,13 @@ class TransactionLevel private $statusBackup; /** - * @var \Piwik\Tracker\Db|\Piwik\Db\AdapterInterface|\Piwik\Db $db + * @var TransactionalDatabaseInterface $transactionalDatabase */ - private $db; + private $transactionalDatabase; - /** - * @param \Piwik\Tracker\Db|\Piwik\Db\AdapterInterface|\Piwik\Db $db - */ - public function __construct($db) + public function __construct(TransactionalDatabaseInterface $transactionalDatabase) { - $this->db = $db; + $this->transactionalDatabase = $transactionalDatabase; } public function canLikelySetTransactionLevel() @@ -48,38 +45,34 @@ public function setUncommitted() public function setTransactionLevelForNonLockingReads(): bool { - if ($this->db->supportsUncommitted === false) { + if ($this->transactionalDatabase->getSupportsTransactionLevelForNonLockingReads() === false) { // we know "Uncommitted" transaction level is not supported, we don't need to do anything as it won't work to set the status return false; } try { - $backup = $this->db->fetchOne('SELECT @@TX_ISOLATION'); + $backup = $this->transactionalDatabase->getCurrentTransactionIsolationLevelForSession(); } catch (\Exception $e) { - try { - $backup = $this->db->fetchOne('SELECT @@transaction_isolation'); - } catch (\Exception $e) { - $this->db->supportsUncommitted = false; - return false; - } + $this->transactionalDatabase->setSupportsTransactionLevelForNonLockingReads(false); + return false; } try { - $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL ' . Schema::getInstance()->getSupportedReadIsolationTransactionLevel()); + $this->transactionalDatabase->setTransactionIsolationLevel(Schema::getInstance()->getSupportedReadIsolationTransactionLevel()); $this->statusBackup = $backup; - if ($this->db->supportsUncommitted === null) { + if ($this->transactionalDatabase->getSupportsTransactionLevelForNonLockingReads() === null) { // the first time we need to check if the transaction level actually works by // trying to set something w/ the new transaction isolation level Option::set(self::TEST_OPTION_NAME, '1'); } - $this->db->supportsUncommitted = true; + $this->transactionalDatabase->setSupportsTransactionLevelForNonLockingReads(true); } catch (\Exception $e) { // setting the transaction level status did not work // catch eg 1665 Cannot execute statement: impossible to write to binary log since BINLOG_FORMAT = STATEMENT and at least one table uses a storage engine limited to row-based logging. InnoDB is limited to row-logging when transaction isolation level is READ COMMITTED or READ UNCOMMITTED - $this->db->supportsUncommitted = false; + $this->transactionalDatabase->setSupportsTransactionLevelForNonLockingReads(false); $this->restorePreviousStatus(); return false; } @@ -95,9 +88,9 @@ public function restorePreviousStatus() $value = str_replace('-', ' ', $value); if (in_array($value, array('REPEATABLE READ', 'READ COMMITTED', 'SERIALIZABLE'))) { - $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL ' . $value); + $this->transactionalDatabase->setTransactionIsolationLevel($value); } elseif ($value !== 'READ UNCOMMITTED') { - $this->db->query('SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ'); + $this->transactionalDatabase->setTransactionIsolationLevel('REPEATABLE READ'); } } } diff --git a/core/Db/TransactionalDatabaseDynamicTrait.php b/core/Db/TransactionalDatabaseDynamicTrait.php new file mode 100644 index 00000000000..fa158b0c244 --- /dev/null +++ b/core/Db/TransactionalDatabaseDynamicTrait.php @@ -0,0 +1,32 @@ +fetchOne('SELECT @@TX_ISOLATION'); + } catch (Exception $e) { + return $this->fetchOne('SELECT @@transaction_isolation'); + } + } + + public function setTransactionIsolationLevel(string $level): void + { + $this->query("SET SESSION TRANSACTION ISOLATION LEVEL $level"); + } + + public function getSupportsTransactionLevelForNonLockingReads(): ?bool + { + return $this->supportsTransactionLevelForNonLockingReads; + } + + public function setSupportsTransactionLevelForNonLockingReads(bool $supports = null): void + { + $this->supportsTransactionLevelForNonLockingReads = $supports; + } +} diff --git a/core/Db/TransactionalDatabaseInterface.php b/core/Db/TransactionalDatabaseInterface.php new file mode 100644 index 00000000000..64b41ec368f --- /dev/null +++ b/core/Db/TransactionalDatabaseInterface.php @@ -0,0 +1,11 @@ +supportsTransactionLevelForNonLockingReads = $supports; + } + + public function getSupportsTransactionLevelForNonLockingReads(): ?bool + { + return $this->supportsTransactionLevelForNonLockingReads; + } +} diff --git a/core/Tracker/Db.php b/core/Tracker/Db.php index a1b52d3deaf..b6888cd1112 100644 --- a/core/Tracker/Db.php +++ b/core/Tracker/Db.php @@ -13,6 +13,8 @@ use Piwik\Common; use Piwik\Config; use Piwik\Db\Adapter; +use Piwik\Db\TransactionalDatabaseInterface; +use Piwik\Db\TransactionalDatabaseStaticTrait; use Piwik\Piwik; use Piwik\Timer; use Piwik\Tracker\Db\DbException; @@ -23,17 +25,16 @@ * We wrote this simple class * */ -abstract class Db +abstract class Db implements TransactionalDatabaseInterface { + use TransactionalDatabaseStaticTrait; + protected static $profiling = false; protected $queriesProfiling = array(); protected $connection = null; - // this is used for indicate TransactionLevel Cache - public $supportsUncommitted = null; - /** * Enables the SQL profiling. * For each query, saves in the DB the time spent on this query. diff --git a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php index cb2700acbc2..2834541d745 100644 --- a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php +++ b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php @@ -421,13 +421,13 @@ public function testGenerateQuerySwitchesSupportsUncommittedToTrueWhenSupports() $db = Db::get(); - $db->supportsUncommitted = null; + $db->setSupportsTransactionLevelForNonLockingReads(null); $this->logAggregator->generateQuery('test, test2', 'log_visit', '1=1', false, '5'); $this->setSqlRequirePrimaryKeySetting(0); - $this->assertTrue($db->supportsUncommitted); + $this->assertTrue($db->getSupportsTransactionLevelForNonLockingReads()); } public function testGetSegmentTmpTableName()