N°3513 - ObjectFormManager : remove transaction

core/cmdbsource.class.inc.php

@@ -671,6 +671,11 @@ class CMDBSource
 	 */
 	private static function DBQuery($sSql)
 	{
+		$sShortSQL = str_replace("\n", " ", substr($sSql, 0, 120));
+		if (substr_compare($sShortSQL, "SELECT", 0, strlen("SELECT")) !== 0) {
+			IssueLog::Trace("$sShortSQL", 'cmdbsource');
+		}
+
 		$oKPI = new ExecutionKPI();
 		try
 		{
@@ -754,10 +759,15 @@ class CMDBSource
 	 */
 	private static function StartTransaction()
 	{
+		$aStackTrace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT , 3);
+		$sCaller = 'From '.$aStackTrace[1]['file'].'('.$aStackTrace[1]['line'].'): '.$aStackTrace[2]['class'].'->'.$aStackTrace[2]['function'].'()';
 		$bHasExistingTransactions = self::IsInsideTransaction();
 		if (!$bHasExistingTransactions)
 		{
+			IssueLog::Trace("START TRANSACTION $sCaller", 'cmdbsource');
 			self::DBQuery('START TRANSACTION');
+		} else {
+			IssueLog::Trace("Ignore nested (".self::$m_iTransactionLevel.") START TRANSACTION $sCaller", 'cmdbsource');
 		}
 
 		self::AddTransactionLevel();
@@ -775,9 +785,12 @@ class CMDBSource
 	 */
 	private static function Commit()
 	{
+		$aStackTrace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT , 3);
+		$sCaller = 'From '.$aStackTrace[1]['file'].'('.$aStackTrace[1]['line'].'): '.$aStackTrace[2]['class'].'->'.$aStackTrace[2]['function'].'()';
 		if (!self::IsInsideTransaction())
 		{
 			// should not happen !
+			IssueLog::Error("No Transaction COMMIT $sCaller", 'cmdbsource');
 			throw new MySQLNoTransactionException('Trying to commit transaction whereas none have been started !', null);
 		}
 
@@ -785,8 +798,10 @@ class CMDBSource
 
 		if (self::IsInsideTransaction())
 		{
+			IssueLog::Trace("Ignore nested (".self::$m_iTransactionLevel.") COMMIT $sCaller", 'cmdbsource');
 			return;
 		}
+		IssueLog::Trace("COMMIT $sCaller", 'cmdbsource');
 		self::DBQuery('COMMIT');
 	}
 
@@ -805,17 +820,22 @@ class CMDBSource
 	 */
 	private static function Rollback()
 	{
+		$aStackTrace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT , 3);
+		$sCaller = 'From '.$aStackTrace[1]['file'].'('.$aStackTrace[1]['line'].'): '.$aStackTrace[2]['class'].'->'.$aStackTrace[2]['function'].'()';
 		if (!self::IsInsideTransaction())
 		{
 			// should not happen !
+			IssueLog::Error("No Transaction ROLLBACK $sCaller", 'cmdbsource');
 			throw new MySQLNoTransactionException('Trying to commit transaction whereas none have been started !', null);
 		}
 		self::RemoveLastTransactionLevel();
 		if (self::IsInsideTransaction())
 		{
+			IssueLog::Trace("Ignore nested (".self::$m_iTransactionLevel.") ROLLBACK $sCaller", 'cmdbsource');
 			return;
 		}
 
+		IssueLog::Trace("ROLLBACK $sCaller", 'cmdbsource');
 		self::DBQuery('ROLLBACK');
 	}
 
@@ -865,6 +885,17 @@ class CMDBSource
 		self::$m_iTransactionLevel = 0;
 	}
 
+	public static function IsDeadlockException(Exception $e)
+	{
+		while ($e instanceof Exception) {
+			if (($e instanceof MySQLException) && ($e->getCode() == 1213)) {
+				return true;
+			}
+			$e = $e->getPrevious();
+		}
+		return false;
+	}
+
 	/**
 	 *
 	 * @deprecated 2.7.0 N°1627 use ItopCounter instead

core/coreexception.class.inc.php

@@ -149,14 +149,14 @@ class CoreCannotSaveObjectException extends CoreException
 	 *
 	 * @param array $aContextData containing at least those keys : issues, id, class
 	 */
-	public function __construct($aContextData)
+	public function __construct($aContextData, $oPrevious = null)
 	{
 		$this->aIssues = $aContextData['issues'];
 		$this->iObjectId = $aContextData['id'];
 		$this->sObjectClass = $aContextData['class'];
 
 		$sIssues = implode(', ', $this->aIssues);
-		parent::__construct($sIssues, $aContextData);
+		parent::__construct($sIssues, $aContextData, '', $oPrevious);
 	}
 
 	/**

core/dbobject.class.php

@@ -2739,11 +2739,20 @@ abstract class DBObject implements iDisplay
 			}
 		}
 
+		$iTransactionRetry = 1;
 		$bIsTransactionEnabled = MetaModel::GetConfig()->Get('db_core_transactions_enabled');
-		try
-		{
 		if ($bIsTransactionEnabled)
 		{
+			// TODO Deep clone this object before the transaction (to use it in case of rollback)
+			// $iTransactionRetryCount = MetaModel::GetConfig()->Get('db_core_transactions_retry_count');
+			$iTransactionRetryCount = 1;
+			$iTransactionRetryDelay = MetaModel::GetConfig()->Get('db_core_transactions_retry_delay_ms');
+			$iTransactionRetry = $iTransactionRetryCount;
+		}
+		while ($iTransactionRetry > 0) {
+			try {
+				$iTransactionRetry--;
+				if ($bIsTransactionEnabled) {
 					CMDBSource::Query('START TRANSACTION');
 				}
 
@@ -2751,16 +2760,13 @@ abstract class DBObject implements iDisplay
 				$this->m_iKey = $this->DBInsertSingleTable($sRootClass);
 
 				// Then do the leaf class, if different from the root class
-			if ($sClass != $sRootClass)
-			{
+				if ($sClass != $sRootClass) {
 					$this->DBInsertSingleTable($sClass);
 				}
 
 				// Then do the other classes
-			foreach (MetaModel::EnumParentClasses($sClass) as $sParentClass)
-			{
-				if ($sParentClass == $sRootClass)
-				{
+				foreach (MetaModel::EnumParentClasses($sClass) as $sParentClass) {
+					if ($sParentClass == $sRootClass) {
 						continue;
 					}
 					$this->DBInsertSingleTable($sParentClass);
@@ -2771,19 +2777,35 @@ abstract class DBObject implements iDisplay
 				$this->DBWriteLinks();
 				$this->WriteExternalAttributes();
 
-			if ($bIsTransactionEnabled)
-			{
+				if ($bIsTransactionEnabled) {
 					CMDBSource::Query('COMMIT');
 				}
+				break;
 			}
-		catch (Exception $e)
-		{
+			catch (Exception $e) {
+				IssueLog::Error($e->getMessage());
 				if ($bIsTransactionEnabled)
 				{
 					CMDBSource::Query('ROLLBACK');
+					if (!CMDBSource::IsInsideTransaction() && CMDBSource::IsDeadlockException($e))
+					{
+						// Deadlock found when trying to get lock; try restarting transaction (only in main transaction)
+						if ($iTransactionRetry > 0)
+						{
+							// wait and retry
+							IssueLog::Error("Insert TRANSACTION Retrying...");
+							usleep(random_int(1, 5) * 1000 * $iTransactionRetryDelay * ($iTransactionRetryCount - $iTransactionRetry));
+							continue;
+						}
+						else
+						{
+							IssueLog::Error("Insert Deadlock TRANSACTION prevention failed.");
+						}
+					}
 				}
 				throw $e;
 			}
+		}
 
 		$this->m_bIsInDB = true;
 		$this->m_bDirty = false;
@@ -3156,9 +3178,11 @@ abstract class DBObject implements iDisplay
 			$bIsTransactionEnabled = MetaModel::GetConfig()->Get('db_core_transactions_enabled');
 			if ($bIsTransactionEnabled)
 			{
-				$iIsTransactionRetryCount = MetaModel::GetConfig()->Get('db_core_transactions_retry_count');
+				// TODO Deep clone this object before the transaction (to use it in case of rollback)
+				// $iTransactionRetryCount = MetaModel::GetConfig()->Get('db_core_transactions_retry_count');
+				$iTransactionRetryCount = 1;
 				$iIsTransactionRetryDelay = MetaModel::GetConfig()->Get('db_core_transactions_retry_delay_ms');
-				$iTransactionRetry = $iIsTransactionRetryCount;
+				$iTransactionRetry = $iTransactionRetryCount;
 			}
 			while ($iTransactionRetry > 0)
 			{
@@ -3246,18 +3270,18 @@ abstract class DBObject implements iDisplay
 				}
 				catch (MySQLException $e)
 				{
+					IssueLog::Error($e->getMessage());
 					if ($bIsTransactionEnabled)
 					{
 						CMDBSource::Query('ROLLBACK');
-						if ($e->getCode() == 1213)
+						if (!CMDBSource::IsInsideTransaction() && CMDBSource::IsDeadlockException($e))
 						{
-							// Deadlock found when trying to get lock; try restarting transaction
-							IssueLog::Error($e->getMessage());
+							// Deadlock found when trying to get lock; try restarting transaction (only in main transaction)
 							if ($iTransactionRetry > 0)
 							{
 								// wait and retry
 								IssueLog::Error("Update TRANSACTION Retrying...");
-								usleep(random_int(1, 5) * 1000 * $iIsTransactionRetryDelay * ($iIsTransactionRetryCount - $iTransactionRetry));
+								usleep(random_int(1, 5) * 1000 * $iIsTransactionRetryDelay * ($iTransactionRetryCount - $iTransactionRetry));
 								continue;
 							}
 							else
@@ -3271,10 +3295,11 @@ abstract class DBObject implements iDisplay
 						'id' => $this->GetKey(),
 						'class' => get_class($this),
 						'issues' => $aErrors
-					));
+					), $e);
 				}
 				catch (CoreCannotSaveObjectException $e)
 				{
+					IssueLog::Error($e->getMessage());
 					if ($bIsTransactionEnabled)
 					{
 						CMDBSource::Query('ROLLBACK');
@@ -3283,6 +3308,7 @@ abstract class DBObject implements iDisplay
 				}
 				catch (Exception $e)
 				{
+					IssueLog::Error($e->getMessage());
 					if ($bIsTransactionEnabled)
 					{
 						CMDBSource::Query('ROLLBACK');
@@ -3494,9 +3520,11 @@ abstract class DBObject implements iDisplay
 		$bIsTransactionEnabled = MetaModel::GetConfig()->Get('db_core_transactions_enabled');
 		if ($bIsTransactionEnabled)
 		{
-			$iIsTransactionRetryCount = MetaModel::GetConfig()->Get('db_core_transactions_retry_count');
-			$iIsTransactionRetryDelay = MetaModel::GetConfig()->Get('db_core_transactions_retry_delay_ms');
-			$iTransactionRetry = $iIsTransactionRetryCount;
+			// TODO Deep clone this object before the transaction (to use it in case of rollback)
+			// $iTransactionRetryCount = MetaModel::GetConfig()->Get('db_core_transactions_retry_count');
+			$iTransactionRetryCount = 1;
+			$iTransactionRetryDelay = MetaModel::GetConfig()->Get('db_core_transactions_retry_delay_ms');
+			$iTransactionRetry = $iTransactionRetryCount;
 		}
 		while ($iTransactionRetry > 0)
 		{
@@ -3519,18 +3547,18 @@ abstract class DBObject implements iDisplay
 			}
 			catch (MySQLException $e)
 			{
+				IssueLog::Error($e->getMessage());
 				if ($bIsTransactionEnabled)
 				{
 					CMDBSource::Query('ROLLBACK');
-					if ($e->getCode() == 1213)
+					if (!CMDBSource::IsInsideTransaction() && CMDBSource::IsDeadlockException($e))
 					{
 						// Deadlock found when trying to get lock; try restarting transaction
-						IssueLog::Error($e->getMessage());
 						if ($iTransactionRetry > 0)
 						{
 							// wait and retry
 							IssueLog::Error("Delete TRANSACTION Retrying...");
-							usleep(random_int(1, 5) * 1000 * $iIsTransactionRetryDelay * ($iIsTransactionRetryCount - $iTransactionRetry));
+							usleep(random_int(1, 5) * 1000 * $iTransactionRetryDelay * ($iTransactionRetryCount - $iTransactionRetry));
 							continue;
 						}
 						else

core/log.class.inc.php

@@ -632,6 +632,8 @@ abstract class LogAPI
 			}
 		}
 
+		$sUser = UserRights::GetUserId();
+		$sMessage = str_pad($sUser, 5)." | $sMessage";
 		static::$m_oFileLog->$sLevel($sMessage, $sChannel, $aContext);
 	}
 

datamodels/2.x/itop-portal-base/en.dict.itop-portal-base.php

@@ -63,6 +63,8 @@ Dict::Add('EN US', 'English', 'English', array(
 	'Portal:File:DisplayInfo+' => '%1$s (%2$s) <a href="%3$s" class="file_open_link" target="_blank">Open</a> / <a href="%4$s" class="file_download_link">Download</a>',
 	'Portal:Calendar-FirstDayOfWeek' => 'en-us', //work with moment.js locales
 	'Portal:Form:Close:Warning' => 'Do you want to leave this form ? Data entered may be lost',
+	'Portal:Error:ObjectCannotBeCreated' => 'Error: object cannot be created. Check associated objects and attachments before submitting again this form.',
+	'Portal:Error:ObjectCannotBeUpdated' => 'Error: object cannot be updated. Check associated objects and attachments before submitting again this form.',
 ));
 
 // UserProfile brick

datamodels/2.x/itop-portal-base/fr.dict.itop-portal-base.php

@@ -62,6 +62,8 @@ Dict::Add('FR FR', 'French', 'Français', array(
 	'Portal:File:DisplayInfo+' => '%1$s (%2$s) <a href="%3$s" class="file_open_link" target="_blank">Ouvrir</a> / <a href="%4$s" class="file_download_link">Télécharger</a>',
 	'Portal:Calendar-FirstDayOfWeek' => 'fr', //work with moment.js locales
 	'Portal:Form:Close:Warning' => 'Voulez-vous quitter ce formulaire ? Les données saisies seront perdues',
+	'Portal:Error:ObjectCannotBeCreated' => 'Erreur: L\'objet n\'a pas été créé. Vérifiez les objets liés et les attachements avant de soumettre à nouveau le formulaire.',
+	'Portal:Error:ObjectCannotBeUpdated' => 'Erreur: L\'objet n\'a pas été modifié. Vérifiez les objets liés et les attachements avant de soumettre à nouveau le formulaire.',
 ));
 
 // UserProfile brick

datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php

@@ -25,14 +25,12 @@ use AttributeDateTime;
 use AttributeTagSet;
 use CMDBChangeOpAttachmentAdded;
 use CMDBChangeOpAttachmentRemoved;
-use CMDBSource;
 use Combodo\iTop\Form\Field\Field;
 use Combodo\iTop\Form\Field\FileUploadField;
 use Combodo\iTop\Form\Field\LabelField;
 use Combodo\iTop\Form\Form;
 use Combodo\iTop\Form\FormManager;
 use Combodo\iTop\Portal\Helper\ApplicationHelper;
-use CoreCannotSaveObjectException;
 use DBObject;
 use DBObjectSearch;
 use DBObjectSet;
@@ -1108,30 +1106,28 @@ class ObjectFormManager extends FormManager
 			return $aData;
 		}
 
-		// The try catch is essentially to start a MySQL transaction in order to ensure that all or none objects are persisted when creating an object with links
-		try
-		{
 		$sObjectClass = get_class($this->oObject);
 
-			// Starting transaction
-			CMDBSource::Query('START TRANSACTION');
+		try {
 			// Forcing allowed writing on the object if necessary. This is used in some particular cases.
 			$bAllowWrite = ($sObjectClass === 'Person' && $this->oObject->GetKey() == UserRights::GetContactId());
-			if ($bAllowWrite)
-			{
+			if ($bAllowWrite) {
 				$this->oObject->AllowWrite(true);
 			}
 
 			// Writing object to DB
-			$bActivateTriggers = (!$this->oObject->IsNew() && $this->oObject->IsModified());
+			$bIsNew = $this->oObject->IsNew();
 			$bWasModified = $this->oObject->IsModified();
+			$bActivateTriggers = (!$bIsNew && $bWasModified);
 			try
 			{
 				$this->oObject->DBWrite();
 			}
-			catch (CoreCannotSaveObjectException $e)
-			{
-				throw new Exception($e->getHtmlMessage());
+			catch (Exception $e) {
+				if ($bIsNew) {
+					throw new Exception(Dict::S('Portal:Error:ObjectCannotBeCreated'));
+				}
+				throw new Exception(Dict::S('Portal:Error:ObjectCannotBeUpdated'));
 			}
 			// Finalizing images link to object, otherwise it will be cleaned by the GC
 			InlineImage::FinalizeInlineImages($this->oObject);
@@ -1142,9 +1138,6 @@ class ObjectFormManager extends FormManager
 				$this->FinalizeAttachments($aArgs['attachmentIds']);
 			}
 
-			// Ending transaction with a commit as everything was fine
-			CMDBSource::Query('COMMIT');
-
 			// Checking if we have to apply a stimulus
 			if (isset($aArgs['applyStimulus']))
 			{
@@ -1192,14 +1185,11 @@ class ObjectFormManager extends FormManager
 		}
 		catch (Exception $e)
 		{
-			// End transaction with a rollback as something failed
-			CMDBSource::Query('ROLLBACK');
 			$aData['valid'] = false;
 			$aData['messages']['error'] += array('_main' => array($e->getMessage()));
-			IssueLog::Error(__METHOD__.' at line '.__LINE__.' : Rollback during submit ('.$e->getMessage().')');
+			IssueLog::Error(__METHOD__.' at line '.__LINE__.' : '.$e->getMessage());
 		}
 
-
 		return $aData;
 	}