From 00c58bb245274b5412054c987af8cf10d454ddba Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Thu, 17 Jun 2021 17:11:30 +0200 Subject: [PATCH 1/6] :bulb: updateLicenses.php : copy phpdoc from develop branch --- .make/license/updateLicenses.php | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/.make/license/updateLicenses.php b/.make/license/updateLicenses.php index b2ef0b631..706378247 100644 --- a/.make/license/updateLicenses.php +++ b/.make/license/updateLicenses.php @@ -1,7 +1,26 @@ C:\Dev\wamp64\www\itop-dev\.make\license/../..//lib/symfony/console` + * + * Licenses sources : + * * `composer licenses --format json` (see https://getcomposer.org/doc/03-cli.md#licenses) + * * keep every existing nodes with `/licenses/license[11]/product/@scope` not in ['lib', 'datamodels'] + * ⚠ If licenses were added manually, they might be removed by this tool ! Be very careful to check for the result before pushing ! + * + * To launch, check requirements and run `php updateLicenses.php` + * The target license file path is in `$xmlFilePath` */ $iTopFolder = __DIR__ . "/../../" ; From 8f84c3b84b150e1c6d98a6272eb7081383a2e806 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Fri, 18 Jun 2021 10:18:54 +0200 Subject: [PATCH 2/6] :art: LogAPI code formatting --- core/log.class.inc.php | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/core/log.class.inc.php b/core/log.class.inc.php index e90af34ed..38a4a5e40 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -604,36 +604,29 @@ abstract class LogAPI public static function Log($sLevel, $sMessage, $sChannel = null, $aContext = array()) { - if (! static::$m_oFileLog) - { + if (!static::$m_oFileLog) { return; } - if (! isset(self::$aLevelsPriority[$sLevel])) - { + if (!isset(self::$aLevelsPriority[$sLevel])) { IssueLog::Error("invalid log level '{$sLevel}'"); + return; } - if (is_null($sChannel)) - { + if (is_null($sChannel)) { $sChannel = static::CHANNEL_DEFAULT; } $sMinLogLevel = self::GetMinLogLevel($sChannel); - if ($sMinLogLevel === false || $sMinLogLevel === 'false') - { + if ($sMinLogLevel === false || $sMinLogLevel === 'false') { return; } - if (is_string($sMinLogLevel)) - { - if (! isset(self::$aLevelsPriority[$sMinLogLevel])) - { + if (is_string($sMinLogLevel)) { + if (!isset(self::$aLevelsPriority[$sMinLogLevel])) { throw new Exception("invalid configuration for log_level '{$sMinLogLevel}' is not within the list: ".implode(',', array_keys(self::$aLevelsPriority))); - } - elseif (self::$aLevelsPriority[$sLevel] < self::$aLevelsPriority[$sMinLogLevel]) - { + } elseif (self::$aLevelsPriority[$sLevel] < self::$aLevelsPriority[$sMinLogLevel]) { //priority too low regarding the conf, do not log this return; } From dd63f2b817cb4b0b71df6595be10803b8a09091a Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Fri, 18 Jun 2021 09:46:30 +0200 Subject: [PATCH 3/6] =?UTF-8?q?N=C2=B04012=20Debug=20trace=20for=20objects?= =?UTF-8?q?=20lists=20in=20portal=20:=20ManageBrick=20and=20BrowseBrick=20?= =?UTF-8?q?'portal'=20channel,=20debug=20level?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/Controller/BrowseBrickController.php | 25 ++++++++------ .../src/Controller/ManageBrickController.php | 34 +++++++++---------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/BrowseBrickController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/BrowseBrickController.php index de55c9a26..79b62448f 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/BrowseBrickController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/BrowseBrickController.php @@ -30,6 +30,7 @@ use DBObjectSearch; use DBObjectSet; use DBSearch; use FieldExpression; +use IssueLog; use MetaModel; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; @@ -65,6 +66,8 @@ class BrowseBrickController extends BrickController */ public function DisplayAction(Request $oRequest, $sBrickId, $sBrowseMode = null, $sDataLoading = null) { + $sPortalId = $this->getParameter('combodo.portal.instance.id'); + /** @var \Combodo\iTop\Portal\Helper\BrowseBrickHelper $oBrowseBrickHelper */ $oBrowseBrickHelper = $this->get('browse_brick'); /** @var \Combodo\iTop\Portal\Helper\RequestManipulatorHelper $oRequestManipulator */ @@ -266,8 +269,7 @@ class BrowseBrickController extends BrickController // Note : This could be way more simpler if we had a SetInternalParam($sParam, $value) verb $aQueryParams = $aLevelsProperties[$aLevelsPropertiesKeys[$i]]['search']->GetInternalParams(); // Note : $iSearchloopMax was initialized on the previous loop - for ($j = 0; $j <= $iSearchLoopMax; $j++) - { + for ($j = 0; $j <= $iSearchLoopMax; $j++) { $aQueryParams['search_value_'.$j] = '%'.$aSearchValues[$j].'%'; } $aLevelsProperties[$aLevelsPropertiesKeys[$i]]['search']->SetInternalParams($aQueryParams); @@ -277,12 +279,11 @@ class BrowseBrickController extends BrickController $oQuery = $aLevelsProperties[$aLevelsPropertiesKeys[0]]['search']; // Testing appropriate data loading mode if we are in auto - if ($sDataLoading === AbstractBrick::ENUM_DATA_LOADING_AUTO) - { + if ($sDataLoading === AbstractBrick::ENUM_DATA_LOADING_AUTO) { // - Check how many records there is. // - Update $sDataLoading with its new value regarding the number of record and the threshold $oCountSet = new DBObjectSet($oQuery); - $fThreshold = (float)MetaModel::GetModuleSetting($this->getParameter('combodo.portal.instance.id'), + $fThreshold = (float)MetaModel::GetModuleSetting($sPortalId, 'lazy_loading_threshold'); $sDataLoading = ($oCountSet->Count() > $fThreshold) ? AbstractBrick::ENUM_DATA_LOADING_LAZY : AbstractBrick::ENUM_DATA_LOADING_FULL; unset($oCountSet); @@ -440,17 +441,21 @@ class BrowseBrickController extends BrickController } } + IssueLog::Debug('Portal BrowseBrick query', 'portal', array( + 'portalId' => $sPortalId, + 'brickId' => $sBrickId, + 'oql' => $oSet->GetFilter()->ToOQL(), + )); + + // Preparing response - if ($oRequest->isXmlHttpRequest()) - { + if ($oRequest->isXmlHttpRequest()) { $aData = $aData + array( 'data' => $aItems, 'levelsProperties' => $aLevelsProperties, ); $oResponse = new JsonResponse($aData); - } - else - { + } else { $aData = $aData + array( 'oBrick' => $oBrick, 'sBrickId' => $sBrickId, diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php index 147d8ebe7..5f1468651 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php @@ -41,13 +41,13 @@ use Dict; use Exception; use FieldExpression; use iPopupMenuExtension; +use IssueLog; use JSButtonItem; use MetaModel; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; use UnaryExpression; use URLButtonItem; -use VariableExpression; /** * Class ManageBrickController @@ -89,10 +89,10 @@ class ManageBrickController extends BrickController /** @var \Combodo\iTop\Portal\Brick\ManageBrick $oBrick */ $oBrick = $oBrickCollection->GetBrickById($sBrickId); - if (is_null($sDisplayMode)) - { + if (is_null($sDisplayMode)) { $sDisplayMode = $oBrick->GetDefaultDisplayMode(); } + $aData = $this->GetData($oRequest, $sBrickId, $sGroupingTab, $oBrick::AreDetailsNeededForDisplayMode($sDisplayMode)); $aExportFields = $oBrick->GetExportFields(); @@ -102,8 +102,7 @@ class ManageBrickController extends BrickController 'iDefaultListLength' => $oBrick->GetDefaultListLength(), ); // Preparing response - if ($oRequest->isXmlHttpRequest()) - { + if ($oRequest->isXmlHttpRequest()) { $oResponse = new JsonResponse($aData); } else @@ -814,30 +813,31 @@ class ManageBrickController extends BrickController 'aColumnsDefinition' => $aColumnsDefinition, ); } - } - else - { + + IssueLog::Debug('Portal ManageBrick query', 'portal', array( + 'portalId' => $sPortalId, + 'brickId' => $sBrickId, + 'groupingTab' => $sGroupingTab, + 'oql' => $oSet->GetFilter()->ToOQL(), + 'aGroupingTabs' => $aGroupingTabs, + )); + } else { $aGroupingAreasData = array(); $sGroupingArea = null; } // Preparing response - if ($oRequest->isXmlHttpRequest()) - { + if ($oRequest->isXmlHttpRequest()) { $aData = $aData + array( 'data' => $aGroupingAreasData[$sGroupingArea]['aItems'], ); - } - else - { + } else { $aDisplayValues = array(); $aUrls = array(); $aColumns = array(); $aNames = array(); - if ($bHasScope) - { - foreach ($aGroupingTabsValues as $aValues) - { + if ($bHasScope) { + foreach ($aGroupingTabsValues as $aValues) { $aDisplayValues[] = array( 'value' => $aValues['count'], 'label' => $aValues['label'], From 0d40235791886894eaf6aaf8b3c191ec28e55286 Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 21 Jun 2021 12:33:15 +0200 Subject: [PATCH 4/6] =?UTF-8?q?:card=5Ffile=5Fbox:=20N=C2=B03968=20Fix=20m?= =?UTF-8?q?utex=20being=20silently=20released=20after=20connection=20timeo?= =?UTF-8?q?ut=20(#209)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutex are using their own DB connection Because the `wait_timeout` isn't specified when opening the connection, it could be closed before we released the lock : if so the lock is silently released ! We are now setting this variable directly when opening the connection to avoid such case (setting 86400s, so 1 day : this should be enough !) --- core/mutex.class.inc.php | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/core/mutex.class.inc.php b/core/mutex.class.inc.php index b743dcaf9..f9580a963 100644 --- a/core/mutex.class.inc.php +++ b/core/mutex.class.inc.php @@ -242,6 +242,8 @@ class iTopMutex * * @throws \Exception * @throws \MySQLException + * + * @since 2.7.5 3.0.0 N°3968 specify `wait_timeout` for the mutex dedicated connection */ public function InitMySQLSession() { @@ -254,10 +256,36 @@ class iTopMutex $this->hDBLink = CMDBSource::GetMysqliInstance($sServer, $sUser, $sPwd, $sSource, $bTlsEnabled, $sTlsCA, false); - if (!$this->hDBLink) - { + if (!$this->hDBLink) { throw new Exception("Could not connect to the DB server (host=$sServer, user=$sUser): ".mysqli_connect_error().' (mysql errno: '.mysqli_connect_errno().')'); } + + // Make sure that the server variable `wait_timeout` is at least 86400 seconds for this connection, + // since the lock will be released if/when the connection times out. + // Source https://dev.mysql.com/doc/refman/5.7/en/locking-functions.html : + // > A lock obtained with GET_LOCK() is released explicitly by executing RELEASE_LOCK() or implicitly when your session terminates + // + // BEWARE: If you want to check the value of this variable, when run from an interactive console `SHOW VARIABLES LIKE 'wait_timeout'` + // will actually returns the value of the variable `interactive_timeout` which may be quite different. + $sSql = "SHOW VARIABLES LIKE 'wait_timeout'"; + $result = mysqli_query($this->hDBLink, $sSql); + if (!$result) { + throw new Exception("Failed to issue MySQL query '".$sSql."': ".mysqli_error($this->hDBLink).' (mysql errno: '.mysqli_errno($this->hDBLink).')'); + } + if ($aRow = mysqli_fetch_array($result, MYSQLI_BOTH)) { + $iTimeout = (int)$aRow[1]; + } else { + mysqli_free_result($result); + throw new Exception("No result for query '".$sSql."'"); + } + mysqli_free_result($result); + + if ($iTimeout < 86400) { + $result = mysqli_query($this->hDBLink, 'SET SESSION wait_timeout=86400'); + if ($result === false) { + throw new Exception("Failed to issue MySQL query '".$sSql."': ".mysqli_error($this->hDBLink).' (mysql errno: '.mysqli_errno($this->hDBLink).')'); + } + } } From a1271da74a9bfc384b88a1284b06eab21ad5cdf4 Mon Sep 17 00:00:00 2001 From: Eric Date: Mon, 21 Jun 2021 15:03:17 +0200 Subject: [PATCH 5/6] =?UTF-8?q?N=C2=B03513=20-=20ObjectFormManager=20:=20r?= =?UTF-8?q?emove=20transaction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbsource.class.inc.php | 31 ++++ core/coreexception.class.inc.php | 4 +- core/dbobject.class.php | 134 +++++++++++------- core/log.class.inc.php | 2 + .../en.dict.itop-portal-base.php | 2 + .../fr.dict.itop-portal-base.php | 2 + .../portal/src/Form/ObjectFormManager.php | 32 ++--- 7 files changed, 131 insertions(+), 76 deletions(-) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index f52c6a69b..8b6cd837a 100644 --- a/core/cmdbsource.class.inc.php +++ b/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 diff --git a/core/coreexception.class.inc.php b/core/coreexception.class.inc.php index cf6469807..a85417786 100644 --- a/core/coreexception.class.inc.php +++ b/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); } /** diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 46efb0a4d..6a4d23193 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -2739,50 +2739,72 @@ abstract class DBObject implements iDisplay } } + $iTransactionRetry = 1; $bIsTransactionEnabled = MetaModel::GetConfig()->Get('db_core_transactions_enabled'); - try + if ($bIsTransactionEnabled) { - if ($bIsTransactionEnabled) - { - CMDBSource::Query('START TRANSACTION'); - } - - // First query built upon on the root class, because the ID must be created first - $this->m_iKey = $this->DBInsertSingleTable($sRootClass); - - // Then do the leaf class, if different from the root class - if ($sClass != $sRootClass) - { - $this->DBInsertSingleTable($sClass); - } - - // Then do the other classes - foreach (MetaModel::EnumParentClasses($sClass) as $sParentClass) - { - if ($sParentClass == $sRootClass) - { - continue; - } - $this->DBInsertSingleTable($sParentClass); - } - - $this->OnObjectKeyReady(); - - $this->DBWriteLinks(); - $this->WriteExternalAttributes(); - - if ($bIsTransactionEnabled) - { - CMDBSource::Query('COMMIT'); - } + // 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; } - catch (Exception $e) - { - if ($bIsTransactionEnabled) - { - CMDBSource::Query('ROLLBACK'); + while ($iTransactionRetry > 0) { + try { + $iTransactionRetry--; + if ($bIsTransactionEnabled) { + CMDBSource::Query('START TRANSACTION'); + } + + // First query built upon on the root class, because the ID must be created first + $this->m_iKey = $this->DBInsertSingleTable($sRootClass); + + // Then do the leaf class, if different from the root class + if ($sClass != $sRootClass) { + $this->DBInsertSingleTable($sClass); + } + + // Then do the other classes + foreach (MetaModel::EnumParentClasses($sClass) as $sParentClass) { + if ($sParentClass == $sRootClass) { + continue; + } + $this->DBInsertSingleTable($sParentClass); + } + + $this->OnObjectKeyReady(); + + $this->DBWriteLinks(); + $this->WriteExternalAttributes(); + + if ($bIsTransactionEnabled) { + CMDBSource::Query('COMMIT'); + } + break; + } + 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; } - throw $e; } $this->m_bIsInDB = true; @@ -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 diff --git a/core/log.class.inc.php b/core/log.class.inc.php index 38a4a5e40..c8754dc1e 100644 --- a/core/log.class.inc.php +++ b/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); } diff --git a/datamodels/2.x/itop-portal-base/en.dict.itop-portal-base.php b/datamodels/2.x/itop-portal-base/en.dict.itop-portal-base.php index aa767f8b2..ae1243d5b 100644 --- a/datamodels/2.x/itop-portal-base/en.dict.itop-portal-base.php +++ b/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) Open / Download', '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 diff --git a/datamodels/2.x/itop-portal-base/fr.dict.itop-portal-base.php b/datamodels/2.x/itop-portal-base/fr.dict.itop-portal-base.php index ccab6bcdf..db2104434 100644 --- a/datamodels/2.x/itop-portal-base/fr.dict.itop-portal-base.php +++ b/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) Ouvrir / Télécharger', '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 diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index d93f3debd..0944c0bbd 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/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); + $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; } From 0f5130611dfbd7a0804f0a76eedce152604426a6 Mon Sep 17 00:00:00 2001 From: Eric Date: Mon, 21 Jun 2021 16:07:36 +0200 Subject: [PATCH 6/6] :white_check_mark: Fix log API tests --- core/log.class.inc.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/log.class.inc.php b/core/log.class.inc.php index c8754dc1e..f4e7a10cf 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -502,6 +502,7 @@ class FileLog protected function Write($sText, $sLevel = '', $sChannel = '', $aContext = array()) { $sTextPrefix = empty($sLevel) ? '' : (str_pad($sLevel, 7).' | '); + $sTextPrefix .= str_pad(UserRights::GetUserId(), 5)." | "; $sTextSuffix = empty($sChannel) ? '' : " | $sChannel"; $sText = "{$sTextPrefix}{$sText}{$sTextSuffix}"; $sLogFilePath = $this->oFileNameBuilder->GetLogFilePath(); @@ -632,8 +633,6 @@ abstract class LogAPI } } - $sUser = UserRights::GetUserId(); - $sMessage = str_pad($sUser, 5)." | $sMessage"; static::$m_oFileLog->$sLevel($sMessage, $sChannel, $aContext); }