N°3513 - ObjectFormManager : remove transaction

* Add automatic tests * Fix object consistency
2026-05-19 23:32:17 +02:00 · 2021-06-22 14:29:10 +02:00
parent 58e315d7f6
commit b1ca1f2630
5 changed files with 344 additions and 22 deletions
--- a/core/cmdbsource.class.inc.php
+++ b/core/cmdbsource.class.inc.php
@@ -154,6 +154,7 @@ class CMDBSource

 	/** @var mysqli $m_oMysqli */
 	protected static $m_oMysqli;
+	protected static $oMySQLiForQuery;

 	/**
 	 * @var int number of level for nested transactions : 0 if no transaction was ever opened, +1 for each 'START TRANSACTION' sent
@@ -220,6 +221,7 @@ class CMDBSource
 		self::$m_sDBTlsCA = empty($sTlsCA) ? null : $sTlsCA;

 		self::$m_oMysqli = self::GetMysqliInstance($sServer, $sUser, $sPwd, $sSource, $bTlsEnabled, $sTlsCA, true);
+		self::SetMySQLiForQuery(self::$m_oMysqli);
 	}

 	/**
@@ -237,8 +239,6 @@ class CMDBSource
 	public static function GetMysqliInstance(
 		$sDbHost, $sUser, $sPwd, $sSource = '', $bTlsEnabled = false, $sTlsCa = null, $bCheckTlsAfterConnection = false
 	) {
-		$oMysqli = null;
-
 		$sServer = null;
 		$iPort = null;
 		self::InitServerAndPort($sDbHost, $sServer, $iPort);
@@ -536,6 +536,24 @@ class CMDBSource
 		return self::$m_oMysqli;
 	}

+	/**
+	 * @return
+	 */
+	private static function GetMySQLiForQuery()
+	{
+		return self::$oMySQLiForQuery;
+	}
+
+
+	/**
+	 * Used for test purpose (mysqli mock)
+	 * @param $oMySQLi
+	 */
+	private static function SetMySQLiForQuery($oMySQLi)
+	{
+		self::$oMySQLiForQuery = $oMySQLi;
+	}
+
 	public static function GetErrNo()
 	{
 		if (self::$m_oMysqli->errno != 0)
@@ -671,7 +689,7 @@ class CMDBSource
 	 */
 	private static function DBQuery($sSql)
 	{
-		$sShortSQL = str_replace("\n", " ", substr($sSql, 0, 120));
+		$sShortSQL = substr(preg_replace("/\s+/", " ", substr($sSql, 0, 180)), 0, 150);
 		if (substr_compare($sShortSQL, "SELECT", 0, strlen("SELECT")) !== 0) {
 			IssueLog::Trace("$sShortSQL", 'cmdbsource');
 		}
@@ -679,7 +697,7 @@ class CMDBSource
 		$oKPI = new ExecutionKPI();
 		try
 		{
-			$oResult = self::$m_oMysqli->query($sSql);
+			$oResult = self::GetMySQLiForQuery()->query($sSql);
 		}
 		catch (mysqli_sql_exception $e)
 		{
@@ -959,7 +977,7 @@ class CMDBSource
 		$oKPI = new ExecutionKPI();
 		try
 		{
-			$oResult = self::$m_oMysqli->query($sSql);
+			$oResult = self::GetMySQLiForQuery()->query($sSql);
 		}
 		catch(mysqli_sql_exception $e)
 		{
@@ -999,7 +1017,7 @@ class CMDBSource
 		$oKPI = new ExecutionKPI();
 		try
 		{
-			$oResult = self::$m_oMysqli->query($sSql);
+			$oResult = self::GetMySQLiForQuery()->query($sSql);
 		}
 		catch(mysqli_sql_exception $e)
 		{
@@ -1081,7 +1099,7 @@ class CMDBSource
 	{
 		try
 		{
-			$oResult = self::$m_oMysqli->query($sSql);
+			$oResult = self::GetMySQLiForQuery()->query($sSql);
 		}
 		catch(mysqli_sql_exception $e)
 		{
@@ -1568,7 +1586,7 @@ class CMDBSource
 		$sSql = "SELECT * FROM `$sTable`";
 		try
 		{
-			$oResult = self::$m_oMysqli->query($sSql);
+			$oResult = self::GetMySQLiForQuery()->query($sSql);
 		}
 		catch(mysqli_sql_exception $e)
 		{
--- a/core/dbobject.class.php
+++ b/core/dbobject.class.php
@@ -2776,6 +2776,7 @@ abstract class DBObject implements iDisplay

 				$this->DBWriteLinks();
 				$this->WriteExternalAttributes();
+				$this->RecordObjCreation();

 				if ($bIsTransactionEnabled) {
 					CMDBSource::Query('COMMIT');
@@ -2837,8 +2838,6 @@ abstract class DBObject implements iDisplay
 			}
 		}

-		$this->RecordObjCreation();
-
 		return $this->m_iKey;
 	}

@@ -3250,13 +3249,6 @@ abstract class DBObject implements iDisplay
 					$this->DBWriteLinks();
 					$this->WriteExternalAttributes();

-					// following lines are resetting changes (so after this {@see DBObject::ListChanges()} won't return changes anymore)
-					// new values are already in the object (call {@see DBObject::Get()} to get them)
-					// call {@see DBObject::ListPreviousValuesForUpdatedAttributes()} to get changed fields and previous values
-					$this->m_bDirty = false;
-					$this->m_aTouchedAtt = array();
-					$this->m_aModifiedAtt = array();
-
 					if (count($aChanges) != 0)
 					{
 						$this->RecordAttChanges($aChanges, $aOriginalValues);
@@ -3322,6 +3314,13 @@ abstract class DBObject implements iDisplay
 				}
 			}

+			// following lines are resetting changes (so after this {@see DBObject::ListChanges()} won't return changes anymore)
+			// new values are already in the object (call {@see DBObject::Get()} to get them)
+			// call {@see DBObject::ListPreviousValuesForUpdatedAttributes()} to get changed fields and previous values
+			$this->m_bDirty = false;
+			$this->m_aTouchedAtt = array();
+			$this->m_aModifiedAtt = array();
+
 			try
 			{
 				$this->AfterUpdate();
--- a/test/core/CMDBSource/CMDBSourceTest.php
+++ b/test/core/CMDBSource/CMDBSourceTest.php
@@ -113,11 +113,6 @@ class CMDBSourceTest extends ItopTestCase
 				"ENUM('value 1 (with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci",
 				"enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci",
 			),
-			'ENUM with different values, containing parenthesis' => array(
-				false,
-				"ENUM('value 1 ) with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci",
-				"enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci",
-			),
 		);
 	}
 }
--- a/test/core/CMDBSource/DeadLockInjection.php
+++ b/test/core/CMDBSource/DeadLockInjection.php
@@ -0,0 +1,63 @@
+<?php
+/**
+ * @copyright   Copyright (C) 2010-2021 Combodo SARL
+ * @license     http://opensource.org/licenses/AGPL-3.0
+ */
+
+
+namespace Combodo\iTop\Test\UnitTest\Core;
+
+
+use CMDBSource;
+use Exception;
+use IssueLog;
+use MySQLException;
+use utils;
+
+class DeadLockInjection
+{
+	private $bMustFail = false;
+	private $iRequestCount = 0;
+	private $iFailAt = 0;
+	private $bShowRequest = true;
+
+	/**
+	 *
+	 * @param int $iFailAt
+	 */
+	public function SetFailAt($iFailAt)
+	{
+		$this->bMustFail = true;
+		$this->iRequestCount = 0;
+		$this->iFailAt = $iFailAt;
+		$this->bShowRequest = true;
+	}
+
+	/**
+	 * @param bool $bShowRequest
+	 */
+	public function SetShowRequest($bShowRequest)
+	{
+		$this->bShowRequest = $bShowRequest;
+	}
+
+
+	public function query($sSQL)
+	{
+		if (utils::StartsWith($sSQL, "SELECT")) {
+			return;
+		}
+		if ($this->bShowRequest) {
+			$sShortSQL = substr(preg_replace("/\s+/", " ", substr($sSQL, 0, 180)), 0, 150);
+			echo "$sShortSQL\n";
+		}
+		if (CMDBSource::IsInsideTransaction() && $this->bMustFail) {
+			$this->iRequestCount++;
+			if ($this->iRequestCount == $this->iFailAt) {
+				echo "Generating a FAKE DEADLOCK\n";
+				IssueLog::Trace("Generating a FAKE DEADLOCK", 'cmdbsource');
+				throw new MySQLException("FAKE DEADLOCK", [], new Exception("FAKE DEADLOCK", 1213));
+			}
+		}
+	}
+}
--- a/test/core/CMDBSource/TransactionsTest.php
+++ b/test/core/CMDBSource/TransactionsTest.php
@@ -0,0 +1,247 @@
+<?php
+/**
+ * @copyright   Copyright (C) 2010-2021 Combodo SARL
+ * @license     http://opensource.org/licenses/AGPL-3.0
+ */
+
+namespace Combodo\iTop\Test\UnitTest\Core;
+
+use CMDBSource;
+use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use Exception;
+use MetaModel;
+
+/**
+ *
+ * @group itopRequestMgmt
+ * Class TransactionsTest
+ *
+ * @package Combodo\iTop\Test\UnitTest\Core
+ */
+class TransactionsTest extends ItopTestCase
+{
+	/** @var DeadLockInjection */
+	private $oMySQLiMock;
+
+	protected function setUp()
+	{
+		parent::setUp();
+		require_once ('DeadLockInjection.php');
+		require_once(APPROOT.'/core/cmdbsource.class.inc.php');
+		$sEnv = 'production';
+		$sConfigFile = APPCONF.$sEnv.'/config-itop.php';
+
+		MetaModel::Startup($sConfigFile, false, true, false, $sEnv);
+
+		$oInitialMysqli = CMDBSource::GetMysqli();
+		$this->oMySQLiMock = new DeadLockInjection();
+
+		$oMockMysqli = $this->getMockBuilder('mysqli')
+			->setMethods(['query'])
+			->getMock();
+		$oMockMysqli->expects($this->any())
+			->method('query')
+			->will($this->returnCallback(
+				function ($sSql) use ($oInitialMysqli) {
+					$this->oMySQLiMock->query($sSql);
+					return $oInitialMysqli->query($sSql);
+				}
+			));
+
+		$this->InvokeNonPublicStaticMethod('CMDBSource', 'SetMySQLiForQuery', [$oMockMysqli]);
+	}
+
+	/**
+	 * Test DBInsertNoReload database transaction by provoking deadlock exceptions
+	 *
+	 * @dataProvider DBInsertProvider
+	 *
+	 * @param int $iFailAt  Specify the request occurrence that fails
+	 * @param bool $bIsInDB Indicates if the object must have been created or not
+	 *
+	 * @throws \ArchivedObjectException
+	 * @throws \CoreCannotSaveObjectException
+	 * @throws \CoreException
+	 * @throws \CoreUnexpectedValue
+	 * @throws \DeleteException
+	 * @throws \MySQLException
+	 * @throws \MySQLHasGoneAwayException
+	 * @throws \OQLException
+	 */
+	public function testDBInsert($iFailAt, $bIsInDB)
+	{
+		// Create a UserRequest with 2 contacts
+		$oTicket = MetaModel::NewObject('UserRequest', [
+			'ref' => 'Test Ticket',
+			'title' => 'Create OK',
+			'description' => 'Create OK',
+			'caller_id' => 15,
+			'org_id' => 3,
+		]);
+		$oLinkSet = $oTicket->Get('contacts_list');
+		$oLinkSet->AddItem(MetaModel::NewObject('lnkContactToTicket', ['contact_id' => 6]));
+		$oLinkSet->AddItem(MetaModel::NewObject('lnkContactToTicket', ['contact_id' => 7]));
+
+		$this->oMySQLiMock->SetFailAt($iFailAt);
+		$this->debug("---> DBInsert()");
+		try {
+			$oTicket->DBWrite();
+		}
+		catch (Exception $e) {
+			// If an exception occurs must be a deadlock
+			$this->assertTrue(CMDBSource::IsDeadlockException($e), $e->getMessage());
+		}
+
+		// Verify if the ticket is considered as saved in the database
+		$this->assertEquals($bIsInDB, !$oTicket->IsNew());
+
+		if (!$oTicket->IsNew()) {
+			$this->oMySQLiMock->SetShowRequest(false);
+			// Delete created objects
+			$oTicket->DBDelete();
+		}
+	}
+
+	public function DBInsertProvider()
+	{
+		return [
+			"Normal case" => ['iFailAt' => -1, 'bIsInDB' => true],
+			"ticket" => ['iFailAt' => 1, 'bIsInDB' => false],
+			"ticket_request" => ['iFailAt' => 2, 'bIsInDB' => false],
+			"priv_change" => ['iFailAt' => 3, 'bIsInDB' => false],
+			"priv_changeop" => ['iFailAt' => 4, 'bIsInDB' => false],
+			"priv_changeop_create" => ['iFailAt' => 5, 'bIsInDB' => false],
+			"History 4" => ['iFailAt' => 6, 'bIsInDB' => false],
+			"History 5" => ['iFailAt' => 7, 'bIsInDB' => false],
+			"History 6" => ['iFailAt' => 8, 'bIsInDB' => false],
+			"History 7" => ['iFailAt' => 9, 'bIsInDB' => false],
+			"History 8" => ['iFailAt' => 10, 'bIsInDB' => false],
+			"History 9" => ['iFailAt' => 11, 'bIsInDB' => false],
+			"History 10" => ['iFailAt' => 12, 'bIsInDB' => false],
+			"History 11" => ['iFailAt' => 13, 'bIsInDB' => false],
+			"History 12" => ['iFailAt' => 14, 'bIsInDB' => false],
+			"History 13" => ['iFailAt' => 15, 'bIsInDB' => false],
+			"History 14" => ['iFailAt' => 16, 'bIsInDB' => false],
+			"History 15" => ['iFailAt' => 17, 'bIsInDB' => false],
+			"History 16" => ['iFailAt' => 18, 'bIsInDB' => false],
+			"History 17" => ['iFailAt' => 19, 'bIsInDB' => false],
+			"History 18" => ['iFailAt' => 20, 'bIsInDB' => false],
+			"History 19" => ['iFailAt' => 21, 'bIsInDB' => false],
+			"History 20" => ['iFailAt' => 22, 'bIsInDB' => false],
+			"History 21" => ['iFailAt' => 23, 'bIsInDB' => false],
+			"History 22" => ['iFailAt' => 24, 'bIsInDB' => false],
+			"History 23" => ['iFailAt' => 25, 'bIsInDB' => false],
+			"History 24" => ['iFailAt' => 26, 'bIsInDB' => false],
+			"History 25" => ['iFailAt' => 27, 'bIsInDB' => false],
+			"History 26" => ['iFailAt' => 28, 'bIsInDB' => false],
+			"History 27" => ['iFailAt' => 29, 'bIsInDB' => false],
+			"History 28" => ['iFailAt' => 30, 'bIsInDB' => false],
+			"History 29" => ['iFailAt' => 31, 'bIsInDB' => false],
+			"History 30" => ['iFailAt' => 32, 'bIsInDB' => false],
+			"History 31" => ['iFailAt' => 33, 'bIsInDB' => false],
+			"History 32" => ['iFailAt' => 34, 'bIsInDB' => false],
+			"History 33" => ['iFailAt' => 35, 'bIsInDB' => false],
+			"History 34" => ['iFailAt' => 36, 'bIsInDB' => false],
+			"History 35" => ['iFailAt' => 37, 'bIsInDB' => false],
+			"History 36" => ['iFailAt' => 38, 'bIsInDB' => false],
+			"History 37" => ['iFailAt' => 39, 'bIsInDB' => false],
+			"History 38" => ['iFailAt' => 40, 'bIsInDB' => false],
+		];
+	}
+
+	/**
+ 	 * Test DBUpdate database transaction by provoking deadlock exceptions
+	 *
+	 * @dataProvider DBUpdateProvider
+	 * @param $iFailAt
+	 *
+	 * @throws \ArchivedObjectException
+	 * @throws \CoreCannotSaveObjectException
+	 * @throws \CoreException
+	 * @throws \CoreUnexpectedValue
+	 * @throws \DeleteException
+	 * @throws \MySQLException
+	 * @throws \MySQLHasGoneAwayException
+	 * @throws \OQLException
+	 */
+	public function testDBUpdate($iFailAt, $bIsModified)
+	{
+		// Create a UserRequest into the database with 2 contacts
+		$oTicket = MetaModel::NewObject('UserRequest', [
+			'ref' => 'Test Ticket',
+			'title' => 'Create OK',
+			'description' => 'Create OK',
+			'solution' => 'Create OK',
+			'caller_id' => 15,
+			'org_id' => 3,
+		]);
+		$oLinkSet = $oTicket->Get('contacts_list');
+		$oLinkSet->AddItem(MetaModel::NewObject('lnkContactToTicket', ['contact_id' => 6]));
+		$oLinkSet->AddItem(MetaModel::NewObject('lnkContactToTicket', ['contact_id' => 7]));
+		//$oTicket->Set('contacts_list', $oLinkSet);
+
+		$this->oMySQLiMock->SetShowRequest(false);
+		$oTicket->DBWrite();
+
+		// Verify that the object is considered as saved in the database
+		$this->assertEquals(true, !$oTicket->IsNew());
+
+		// Reload from db
+		$oTicket = MetaModel::GetObject('UserRequest', $oTicket->GetKey());
+		$oTicket->Set('description', 'Update OK');
+		$oTicket->Set('solution', 'Test OK');
+		$oLinkSet = $oTicket->Get('contacts_list');
+		$oLinkSet->AddItem(MetaModel::NewObject('lnkContactToTicket', ['contact_id' => 8]));
+		$oTicket->Set('contacts_list', $oLinkSet);
+
+		// Provoke an error during the update
+		$this->oMySQLiMock->SetFailAt($iFailAt);
+		$this->debug("---> DBUpdate()");
+		try {
+			$oTicket->DBWrite();
+		}
+		catch (Exception $e) {
+			// If an exception occurs must be a deadlock
+			$this->assertTrue(CMDBSource::IsDeadlockException($e));
+		}
+
+		// Verify if the ticket is considered as saved in the database
+		$this->assertEquals($bIsModified, $oTicket->IsModified());
+
+		// Reload from db after the update to check the value present in the database
+		$oTicket = MetaModel::GetObject('UserRequest', $oTicket->GetKey());
+		if ($bIsModified) {
+			$this->assertEquals('Create OK', $oTicket->Get('solution'));
+		} else {
+			$this->assertEquals('Test OK', $oTicket->Get('solution'));
+		}
+
+		if (!$oTicket->IsNew()) {
+			$this->oMySQLiMock->SetShowRequest(false);
+			// Delete created objects
+			$oTicket->DBDelete();
+		}
+	}
+
+	public function DBUpdateProvider()
+	{
+		return [
+			"Normal case" => ['iFailAt' => -1, 'bIsModified' => false],
+			"ticket_request" => ['iFailAt' => 1, 'bIsModified' => true],
+			"lnkcontacttoticket" => ['iFailAt' => 2, 'bIsModified' => true],
+			"History 1" => ['iFailAt' => 3, 'bIsModified' => true],
+			"History 2" => ['iFailAt' => 4, 'bIsModified' => true],
+			"History 3" => ['iFailAt' => 5, 'bIsModified' => true],
+			"History 4" => ['iFailAt' => 6, 'bIsModified' => true],
+			"History 5" => ['iFailAt' => 7, 'bIsModified' => true],
+			"History 6" => ['iFailAt' => 8, 'bIsModified' => true],
+			"History 7" => ['iFailAt' => 9, 'bIsModified' => true],
+			"History 8" => ['iFailAt' => 10, 'bIsModified' => true],
+			"History 9" => ['iFailAt' => 11, 'bIsModified' => true],
+			"History 10" => ['iFailAt' => 12, 'bIsModified' => true],
+			"History 11" => ['iFailAt' => 13, 'bIsModified' => true],
+			"History 12" => ['iFailAt' => 14, 'bIsModified' => true],
+			"History 13" => ['iFailAt' => 15, 'bIsModified' => true],
+		];
+	}
+}