From 8ba28adf68cb86bb6bac04beda0576f9712fa359 Mon Sep 17 00:00:00 2001 From: Eric Espie Date: Fri, 8 Apr 2022 15:10:58 +0200 Subject: [PATCH] Refactor --- core/dbobject.class.php | 95 +++++++++++++++----------------------- test/ItopDataTestCase.php | 7 ++- test/core/DBObjectTest.php | 82 +++++++++++++++++++++++++++++--- 3 files changed, 118 insertions(+), 66 deletions(-) diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 6474cf83d..7fc733e8a 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -766,89 +766,68 @@ abstract class DBObject implements iDisplay */ public function GetStrict($sAttCode) { - if ($sAttCode == 'id') - { + if ($sAttCode == 'id') { return $this->m_iKey; } $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode); - if (!$oAttDef->LoadInObject()) - { + if (!$oAttDef->LoadInObject()) { $value = $oAttDef->GetValue($this); - } - else - { - if (isset($this->m_aLoadedAtt[$sAttCode])) - { - // Standard case... we have the information directly - } - elseif ($this->m_bIsInDB && !$this->m_bFullyLoaded && !$this->m_bDirty) - { + } elseif (!isset($this->m_aLoadedAtt[$sAttCode])) { + if ($this->m_bIsInDB && !$this->m_bFullyLoaded && !$this->m_bDirty) { // Lazy load (polymorphism): complete by reloading the entire object $oKPI = new ExecutionKPI(); $this->Reload(); $oKPI->ComputeStats('Reload', get_class($this).'/'.$sAttCode); - } - elseif ($oAttDef->IsBasedOnOQLExpression()) - { + } elseif ($oAttDef->IsBasedOnOQLExpression()) { // Recompute -which is likely to call Get() // /** @var AttributeFriendlyName|\AttributeObsolescenceFlag $oAttDef */ $this->m_aCurrValues[$sAttCode] = $this->EvaluateExpression($oAttDef->GetOQLExpression()); $this->m_aLoadedAtt[$sAttCode] = true; - } - else - { - // Not loaded... is it related to an external key? - if ($oAttDef->IsExternalField()) - { - // Let's get the object and compute all of the corresponding attributes - // (i.e not only the requested attribute) - // - /** @var \AttributeExternalField $oAttDef */ - $sExtKeyAttCode = $oAttDef->GetKeyAttCode(); + } elseif ($oAttDef->IsExternalField()) { + // Let's get the object and compute all the corresponding attributes + // (i.e. not only the requested attribute) + // + /** @var \AttributeExternalField $oAttDef */ + $sExtKeyAttCode = $oAttDef->GetKeyAttCode(); - if (($iRemote = $this->Get($sExtKeyAttCode)) && ($iRemote > 0)) // Objects in memory have negative IDs - { - $oExtKeyAttDef = MetaModel::GetAttributeDef(get_class($this), $sExtKeyAttCode); - // Note: "allow all data" must be enabled because the external fields are always visible - // to the current user even if this is not the case for the remote object - // This is consistent with the behavior of the lists - /** @var \AttributeExternalKey $oExtKeyAttDef */ - $oRemote = MetaModel::GetObject($oExtKeyAttDef->GetTargetClass(), $iRemote, true, true); - } - else - { - $oRemote = null; - } - - foreach(MetaModel::ListAttributeDefs(get_class($this)) as $sCode => $oDef) - { - /** @var \AttributeExternalField $oDef */ - if ($oDef->IsExternalField() && ($oDef->GetKeyAttCode() == $sExtKeyAttCode)) - { - if ($oRemote) - { - $this->m_aCurrValues[$sCode] = $oRemote->Get($oDef->GetExtAttCode()); - } - else - { - $this->m_aCurrValues[$sCode] = $this->GetDefaultValue($sCode); - } - $this->m_aLoadedAtt[$sCode] = true; + if (($iRemote = $this->Get($sExtKeyAttCode)) && ($iRemote > 0)) // Objects in memory have negative IDs + { + $oExtKeyAttDef = MetaModel::GetAttributeDef(get_class($this), $sExtKeyAttCode); + // Note: "allow all data" must be enabled because the external fields are always visible + // to the current user even if this is not the case for the remote object + // This is consistent with the behavior of the lists + /** @var \AttributeExternalKey $oExtKeyAttDef */ + $oRemote = MetaModel::GetObject($oExtKeyAttDef->GetTargetClass(), $iRemote, true, true); + } else { + $oRemote = null; + } + + foreach (MetaModel::ListAttributeDefs(get_class($this)) as $sCode => $oDef) { + /** @var \AttributeExternalField $oDef */ + if ($oDef->IsExternalField() && ($oDef->GetKeyAttCode() == $sExtKeyAttCode)) { + if ($oRemote) { + $this->m_aCurrValues[$sCode] = $oRemote->Get($oDef->GetExtAttCode()); + } else { + $this->m_aCurrValues[$sCode] = $this->GetDefaultValue($sCode); } + $this->m_aLoadedAtt[$sCode] = true; } } } $value = $this->m_aCurrValues[$sAttCode]; + } else { + $value = $this->m_aCurrValues[$sAttCode]; } - if ($value instanceof ormLinkSet) - { + + if ($value instanceof ormLinkSet) { $value->Rewind(); } - return $value; + + return $value; } /** diff --git a/test/ItopDataTestCase.php b/test/ItopDataTestCase.php index ee4e44f3d..a804ee4ba 100644 --- a/test/ItopDataTestCase.php +++ b/test/ItopDataTestCase.php @@ -831,10 +831,15 @@ class ItopDataTestCase extends ItopTestCase $oObject = $oData->Get('object'); $sClass = get_class($oObject); $sKey = $oObject->GetKey(); - $iCount = $this->aReloadCount[$sClass][$sKey] ?? 0; + $iCount = $this->GetObjectReloadCount($sClass, $sKey); $this->aReloadCount[$sClass][$sKey] = 1 + $iCount; } + public function GetObjectReloadCount($sClass, $sKey) + { + return $this->aReloadCount[$sClass][$sKey] ?? 0; + } + /** * Assert that a series of operations will trigger a given number of MySL queries * diff --git a/test/core/DBObjectTest.php b/test/core/DBObjectTest.php index 63569095f..0ef84a690 100644 --- a/test/core/DBObjectTest.php +++ b/test/core/DBObjectTest.php @@ -29,6 +29,7 @@ namespace Combodo\iTop\Test\UnitTest\Core; use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use CoreException; use DBObject; +use lnkPersonToTeam; use MetaModel; @@ -114,7 +115,7 @@ class DBObjectTest extends ItopDataTestCase $oOrg->DBUpdate(); $this->assertCount(0, $oOrg->ListChanges()); - $this->assertCount(1, $oOrg->ListPreviousValuesForUpdatedAttributes()); + $this->assertCount(0, $oOrg->ListPreviousValuesForUpdatedAttributes()); $oOrg->DBDelete(); @@ -124,9 +125,7 @@ class DBObjectTest extends ItopDataTestCase $oOrg->Set('code', strtoupper('testListPreviousValuesForUpdatedAttributes')); $oOrg->DBUpdate(); $oOrg->DBUpdate(); - $this->markTestIncomplete('This test has not been implemented yet. wait for N°4967 fix'); - $this->debug("ERROR: N°4967 - 'Previous Values For Updated Attributes' not updated if DBUpdate is called without modifying the object"); - //$this->assertCount(0, $oOrg->ListPreviousValuesForUpdatedAttributes()); + $this->assertCount(0, $oOrg->ListPreviousValuesForUpdatedAttributes()); } /** @@ -253,11 +252,14 @@ class DBObjectTest extends ItopDataTestCase $this->ResetReloadCount(); static::assertDBQueryCount(0, function() use (&$oObject){ - $oObject = \MetaModel::NewObject('Person', array('name' => 'Foo', 'first_name' => 'John', 'org_id' => 3, 'location_id' => 2)); + $oObject = \MetaModel::NewObject('Person', ['name' => 'Foo', 'first_name' => 'John', 'org_id' => 3, 'location_id' => 2]); }); static::assertDBQueryCount(28, function() use (&$oObject) { $oObject->DBInsertNoReload(); }); + $sClass = get_class($oObject); + $sKey = $oObject->GetKey(); + $this->debug("Created $sClass::$sKey"); $this->DebugReloadCount("Person::DBInsertNoReload()"); static::assertDBQueryCount(0, function() use (&$oObject){ @@ -271,11 +273,12 @@ class DBObjectTest extends ItopDataTestCase $oObject->Set('org_id', 2); static::assertEquals('IT Department', $oObject->Get('org_id_friendlyname')); }); - $this->DebugReloadCount("Set('org_id', 2) andGet('org_id_friendlyname')"); + $this->assertEquals(1, $this->GetObjectReloadCount($sClass, $sKey)); + $this->DebugReloadCount("Set('org_id', 2) and Get('org_id_friendlyname')"); // External key given as an object static::assertDBQueryCount(1, function() use (&$oBordeaux){ - $oBordeaux = \MetaModel::GetObject('Location', 1); + $oBordeaux = MetaModel::GetObject('Location', 1); }); $this->DebugReloadCount("GetObject('Location', 1)"); @@ -288,6 +291,71 @@ class DBObjectTest extends ItopDataTestCase $this->DebugReloadCount("Set('location_id',...) Get('org_id_friendlyname') Get('org_name') Get('location_id_friendlyname')"); } + + /** + * @covers DBObject::NewObject + * @covers DBObject::Get + * @covers DBObject::Set + */ + public function testInsertNoReloadAttributeLinkSet() + { + $this->ResetReloadCount(); + + static::assertDBQueryCount(0, function() use (&$oPerson){ + $oPerson = MetaModel::NewObject('Person', ['name' => 'Foo', 'first_name' => 'John', 'org_id' => 3, 'location_id' => 2]); + }); + static::assertDBQueryCount(28, function() use (&$oPerson) { + $oPerson->DBInsertNoReload(); + }); + $sPersonClass = get_class($oPerson); + $sPersonKey = $oPerson->GetKey(); + $this->debug("Created $sPersonClass::$sPersonKey"); + $this->DebugReloadCount("Person::DBInsertNoReload()"); + + static::assertDBQueryCount(1, function() use (&$oTeam, &$oPerson){ + $oTeam = MetaModel::NewObject('Team', ['name' => 'Team Foo', 'org_id' => 3]); + // Add person to team + $oNewLink = new lnkPersonToTeam(); + $oNewLink->Set('person_id', $oPerson->GetKey()); + $oPersons = $oTeam->Get('persons_list'); + $oPersons->AddItem($oNewLink); + $oTeam->Set('persons_list', $oPersons); + }); + +// global $fItopStarted; +// $fItopStarted = microtime(true); +// ExecutionKPI::EnableDuration(2); +// $oKPI = new ExecutionKPI(); + static::assertDBQueryCount(92, function() use (&$oTeam) { + $oTeam->DBInsertNoReload(); + }); +// $oKPI->ComputeAndReport('Team DBInsertNoReload'); +// $_SERVER['REQUEST_URI'] = $this->GetTestId(); +// $_SERVER['REQUEST_METHOD'] = ''; +// ExecutionKPI::ReportStats(); + $this->assertCount(0, $oTeam->ListChanges()); + + $oTeam->DBUpdate(); + $this->assertCount(0, $oTeam->ListChanges()); + $this->DebugReloadCount("Team::DBUpdate()"); + + $sTeamClass = get_class($oTeam); + $sTeamKey = $oTeam->GetKey(); + $this->debug("Created $sTeamClass::$sTeamKey"); + $this->DebugReloadCount("Team::DBInsertNoReload()"); + + $this->assertCount(0, $oTeam->ListChanges()); + + // External key given as an id + static::assertDBQueryCount(2, function() use (&$oTeam){ + $oTeam->Set('org_id', 2); + static::assertEquals('IT Department', $oTeam->Get('org_id_friendlyname')); + }); + $this->DebugReloadCount("Set('org_id', 2) and Get('org_id_friendlyname')"); + $this->assertCount(1, $oTeam->ListChanges()); + } + + public function testSetExtKeyUnsetDependentAttribute() { $oObject = \MetaModel::NewObject('Person', array('name' => 'Foo', 'first_name' => 'John', 'org_id' => 3, 'location_id' => 2));