Fix loose typing of constructor for transaction level, and remove pub…

…lic 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
matomo-org · Sep 6, 2024 · 750fdce · 750fdce
1 parent 330596d
commit 750fdce
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 39 deletions.
diff --git 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,17 +34,17 @@
  *
  * @api
  */
-class Db
+class Db implements TransactionalDatabaseInterface
 {
+    use TransactionalDatabaseStaticTrait;
+
     public const SQL_MODE = 'NO_AUTO_VALUE_ON_ZERO';
 
     private static $connection = null;
     private static $readerConnection = null;
 
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,32 @@
+<?php
+
+namespace Piwik\Db;
+
+trait TransactionalDatabaseDynamicTrait
+{
+    private $supportsTransactionLevelForNonLockingReads;
+
+    public function getCurrentTransactionIsolationLevelForSession(): string
+    {
+        try {
+            return $this->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
@@ -0,0 +1,11 @@
+<?php
+
+namespace Piwik\Db;
+
+interface TransactionalDatabaseInterface
+{
+    public function getCurrentTransactionIsolationLevelForSession(): string;
+    public function setTransactionIsolationLevel(string $level): void;
+    public function getSupportsTransactionLevelForNonLockingReads(): ?bool;
+    public function setSupportsTransactionLevelForNonLockingReads(bool $supports = null): void;
+}
diff --git a/core/Db/TransactionalDatabaseStaticTrait.php b/core/Db/TransactionalDatabaseStaticTrait.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace Piwik\Db;
+
+trait TransactionalDatabaseStaticTrait
+{
+    private $supportsTransactionLevelForNonLockingReads;
+
+    public function setTransactionIsolationLevel(string $level): void
+    {
+        static::query("SET SESSION TRANSACTION ISOLATION LEVEL $level");
+    }
+
+    public function getCurrentTransactionIsolationLevelForSession(): string
+    {
+        try {
+            return static::fetchOne('SELECT @@TX_ISOLATION');
+        } catch (Exception $e) {
+            return static::fetchOne('SELECT @@transaction_isolation');
+        }
+    }
+
+    public function setSupportsTransactionLevelForNonLockingReads(bool $supports = null): void
+    {
+        $this->supportsTransactionLevelForNonLockingReads = $supports;
+    }
+
+    public function getSupportsTransactionLevelForNonLockingReads(): ?bool
+    {
+        return $this->supportsTransactionLevelForNonLockingReads;
+    }
+}
diff --git 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
@@ -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()