From e2680a787c2dee9854533a2111b880d281fd8d0f Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 4 Feb 2025 15:59:15 +0100 Subject: [PATCH 1/6] =?UTF-8?q?N=C2=B08162=20-=20Issue=20when=20synchroniz?= =?UTF-8?q?ing=20read-only=20notify=5Fcontact=5Fid=5Farchive=5Fflag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/collector.class.inc.php | 70 ++++++++++++++++------------------- core/restclient.class.inc.php | 9 ++--- 2 files changed, 34 insertions(+), 45 deletions(-) diff --git a/core/collector.class.inc.php b/core/collector.class.inc.php index 3b4d624..95cb283 100644 --- a/core/collector.class.inc.php +++ b/core/collector.class.inc.php @@ -41,6 +41,15 @@ abstract class Collector * @var string TABLENAME_PATTERN used to validate data synchro table name */ const TABLENAME_PATTERN = '/^[A-Za-z0-9_]*$/'; + const READONLY_FIELDS = [ + 'friendlyname', + 'user_id_friendlyname', + 'user_id_finalclass_recall', + 'notify_contact_id_friendlyname', + 'notify_contact_id_finalclass_recall', + 'notify_contact_id_obsolescence_flag', + 'notify_contact_id_archive_flag', + ]; protected $sProjectName; protected $sSynchroDataSourceDefinitionFile; @@ -486,7 +495,7 @@ public function InitSynchroDataSource($aPlaceholders) } else { $iCount = ($aResult['objects'] !== null) ? count($aResult['objects']) : 0; switch ($iCount) { - case 0: + ²0: // not found, need to create the Source Utils::Log(LOG_INFO, "There is no Synchro Data Source named '{$this->sSourceName}' in iTop. Let's create it."); $key = $this->CreateSynchroDataSource($aExpectedSourceDefinition, $this->GetName()); @@ -824,12 +833,10 @@ protected function CreateSynchroDataSource($aSourceDefinition, $sComment) $ret = false; // Ignore read-only fields - unset($aSourceDefinition['friendlyname']); - unset($aSourceDefinition['user_id_friendlyname']); - unset($aSourceDefinition['user_id_finalclass_recall']); - unset($aSourceDefinition['notify_contact_id_friendlyname']); - unset($aSourceDefinition['notify_contact_id_finalclass_recall']); - unset($aSourceDefinition['notify_contact_id_obsolescence_flag']); + foreach (self::READONLY_FIELDS as $sField){ + unset($aSourceDefinition[$sField]); + } + // SynchroAttributes will be processed one by one, below $aSynchroAttr = $aSourceDefinition['attribute_list']; unset($aSourceDefinition['attribute_list']); @@ -857,12 +864,10 @@ protected function UpdateSynchroDataSource($aSourceDefinition, $sComment) $oClient = new RestClient(); // Ignore read-only fields - unset($aSourceDefinition['friendlyname']); - unset($aSourceDefinition['user_id_friendlyname']); - unset($aSourceDefinition['user_id_finalclass_recall']); - unset($aSourceDefinition['notify_contact_id_friendlyname']); - unset($aSourceDefinition['notify_contact_id_finalclass_recall']); - unset($aSourceDefinition['notify_contact_id_obsolescence_flag']); + foreach (Collector::READONLY_FIELDS as $sField){ + unset($aSourceDefinition[$sField]); + } + // SynchroAttributes will be processed one by one, below $aSynchroAttr = $aSourceDefinition['attribute_list']; unset($aSourceDefinition['attribute_list']); @@ -952,19 +957,14 @@ protected function FindAttr($sAttCode, $aExpectedAttrDef) protected function DataSourcesAreEquivalent($aDS1, $aDS2) { foreach ($aDS1 as $sKey => $value) { - switch ($sKey) { - case 'friendlyname': - case 'user_id_friendlyname': - case 'user_id_finalclass_recall': - case 'notify_contact_id_friendlyname': - case 'notify_contact_id_finalclass_recall': - case 'notify_contact_id_obsolescence_flag': - case 'notify_contact_id_archive_flag': - // Ignore all read-only attributes - break; + if (in_array($sKey, Collector::READONLY_FIELDS)){ + // Ignore all read-only attributes + break; + } + switch ($sKey) { case 'attribute_list': - foreach ($value as $sKey => $aDef) { + foreach ($value as $sKey2 => $aDef) { $sAttCode = $aDef['attcode']; $aDef2 = $this->FindAttr($sAttCode, $aDS2['attribute_list']); if ($aDef2 === false) { @@ -1012,23 +1012,15 @@ protected function DataSourcesAreEquivalent($aDS1, $aDS2) } //Check the other way around foreach ($aDS2 as $sKey => $value) { - switch ($sKey) { - case 'friendlyname': - case 'user_id_friendlyname': - case 'user_id_finalclass_recall': - case 'notify_contact_id_friendlyname': - case 'notify_contact_id_finalclass_recall': - case 'notify_contact_id_obsolescence_flag': - case 'notify_contact_id_archive_flag': - // Ignore all read-only attributes - break; + if (in_array($sKey, Collector::READONLY_FIELDS)){ + // Ignore all read-only attributes + break; + } - default: - if (!array_key_exists($sKey, $aDS1)) { - Utils::Log(LOG_DEBUG, "Comparison: Found an extra property '$sKey' in iTop. Data sources differ."); + if (!array_key_exists($sKey, $aDS1)) { + Utils::Log(LOG_DEBUG, "Comparison: Found an extra property '$sKey' in iTop. Data sources differ."); - return false; - } + return false; } } diff --git a/core/restclient.class.inc.php b/core/restclient.class.inc.php index e54cbf8..8539fd3 100644 --- a/core/restclient.class.inc.php +++ b/core/restclient.class.inc.php @@ -196,12 +196,9 @@ public static function GetFullSynchroDataSource(&$aSource, $iSourceId) } // Don't care about these read-only fields - unset($aSource['friendlyname']); - unset($aSource['user_id_friendlyname']); - unset($aSource['user_id_finalclass_recall']); - unset($aSource['notify_contact_id_friendlyname']); - unset($aSource['notify_contact_id_finalclass_recall']); - unset($aSource['notify_contact_id_obsolescence_flag']); + foreach (Collector::READONLY_FIELDS as $sField){ + unset($aSource[$sField]); + } return $bResult; } From 44728484e74ddfe426e3d4fe0330d1f1481d11e0 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 4 Feb 2025 16:01:43 +0100 Subject: [PATCH 2/6] =?UTF-8?q?N=C2=B08162=20-=20fix=20typo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/collector.class.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/collector.class.inc.php b/core/collector.class.inc.php index 95cb283..53929d1 100644 --- a/core/collector.class.inc.php +++ b/core/collector.class.inc.php @@ -495,7 +495,7 @@ public function InitSynchroDataSource($aPlaceholders) } else { $iCount = ($aResult['objects'] !== null) ? count($aResult['objects']) : 0; switch ($iCount) { - ²0: + case 0: // not found, need to create the Source Utils::Log(LOG_INFO, "There is no Synchro Data Source named '{$this->sSourceName}' in iTop. Let's create it."); $key = $this->CreateSynchroDataSource($aExpectedSourceDefinition, $this->GetName()); From c7a110e9c00efc03945ce967191b92b58e6489bc Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 4 Feb 2025 18:06:45 +0100 Subject: [PATCH 3/6] =?UTF-8?q?N=C2=B08162=20-=20add=20tests=20mainly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/collector.class.inc.php | 40 +++++++--- core/restclient.class.inc.php | 16 ++-- test/CollectorTest.php | 108 +++++++++++++++++++++++-- test/RestTest.php | 143 ++++++++++++++++++++++++++++++++++ toolkit/dump_tasks.php | 3 +- 5 files changed, 289 insertions(+), 21 deletions(-) diff --git a/core/collector.class.inc.php b/core/collector.class.inc.php index 53929d1..85be031 100644 --- a/core/collector.class.inc.php +++ b/core/collector.class.inc.php @@ -260,6 +260,17 @@ public function GetName() return get_class($this); } + public function GetSourceId() + { + return $this->iSourceId; + } + + protected function SetSourceId($iSourceId) + { + $this->iSourceId = $iSourceId; + } + + public function GetVersion() { if ($this->sVersion == null) { @@ -517,7 +528,7 @@ public function InitSynchroDataSource($aPlaceholders) } else { $iKey = (int)$aData['key']; } - $this->iSourceId = $iKey; + $this->SetSourceId($iKey); RestClient::GetFullSynchroDataSource($aCurrentSourceDefinition, $this->iSourceId); if ($this->DataSourcesAreEquivalent($aExpectedSourceDefinition, $aCurrentSourceDefinition)) { Utils::Log(LOG_INFO, "Ok, the Synchro Data Source '{$this->sSourceName}' exists in iTop and is up to date"); @@ -827,14 +838,20 @@ public static function CallItopViaHttp($sUri, $aAdditionalData, $iTimeOut = -1) return static::$oCallItopService->CallItopViaHttp($sUri, $aAdditionalData, $iTimeOut); } - protected function CreateSynchroDataSource($aSourceDefinition, $sComment) + protected function CreateSynchroDataSource($aSourceDefinition, $sComment, RestClient $oClient = null) { - $oClient = new RestClient(); + if ($oClient === null) + { + $oClient = new RestClient(); + } + $ret = false; // Ignore read-only fields foreach (self::READONLY_FIELDS as $sField){ - unset($aSourceDefinition[$sField]); + if (array_key_exists($sField, $aSourceDefinition)){ + unset($aSourceDefinition[$sField]); + } } // SynchroAttributes will be processed one by one, below @@ -846,7 +863,7 @@ protected function CreateSynchroDataSource($aSourceDefinition, $sComment) $aCreatedObj = reset($aResult['objects']); $aExpectedAttrDef = $aCreatedObj['fields']['attribute_list']; $iKey = (int)$aCreatedObj['key']; - $this->iSourceId = $iKey; + $this->SetSourceId($iKey); if ($this->UpdateSDSAttributes($aExpectedAttrDef, $aSynchroAttr, $sComment)) { $ret = $this->iSourceId; @@ -858,14 +875,17 @@ protected function CreateSynchroDataSource($aSourceDefinition, $sComment) return $ret; } - protected function UpdateSynchroDataSource($aSourceDefinition, $sComment) + protected function UpdateSynchroDataSource($aSourceDefinition, $sComment, RestClient $oClient = null) { - $bRet = true; - $oClient = new RestClient(); - + if ($oClient === null) + { + $oClient = new RestClient(); + } // Ignore read-only fields foreach (Collector::READONLY_FIELDS as $sField){ - unset($aSourceDefinition[$sField]); + if (array_key_exists($sField, $aSourceDefinition)){ + unset($aSourceDefinition[$sField]); + } } // SynchroAttributes will be processed one by one, below diff --git a/core/restclient.class.inc.php b/core/restclient.class.inc.php index 8539fd3..8f46488 100644 --- a/core/restclient.class.inc.php +++ b/core/restclient.class.inc.php @@ -153,20 +153,24 @@ public static function GetNewestKnownVersion() * * @param hash $aSource The definition of 'fields' the Synchro DataSource, as retrieved by Get * @param integer $iSourceId The identifier (key) of the Synchro Data Source + * @param RestClient|null $oClient : used for tests only */ - public static function GetFullSynchroDataSource(&$aSource, $iSourceId) + public static function GetFullSynchroDataSource(&$aSource, $iSourceId, RestClient $oRestClient = null) { $bResult = true; - $aAttributes = array(); + $aAttributes = []; // Optimize the calls to the REST API: one call per finalclass foreach ($aSource['attribute_list'] as $aAttr) { if (!array_key_exists($aAttr['finalclass'], $aAttributes)) { - $aAttributes[$aAttr['finalclass']] = array(); + $aAttributes[$aAttr['finalclass']] = []; } $aAttributes[$aAttr['finalclass']][] = $aAttr['attcode']; } - $oRestClient = new RestClient(); + if ($oRestClient === null) + { + $oRestClient = new RestClient(); + } foreach ($aAttributes as $sFinalClass => $aAttCodes) { Utils::Log(LOG_DEBUG, "RestClient::Get SELECT $sFinalClass WHERE attcode IN ('".implode("','", $aAttCodes)."') AND sync_source_id = $iSourceId"); $aResult = $oRestClient->Get($sFinalClass, "SELECT $sFinalClass WHERE attcode IN ('".implode("','", $aAttCodes)."') AND sync_source_id = $iSourceId"); @@ -197,7 +201,9 @@ public static function GetFullSynchroDataSource(&$aSource, $iSourceId) // Don't care about these read-only fields foreach (Collector::READONLY_FIELDS as $sField){ - unset($aSource[$sField]); + if (array_key_exists($sField, $aSource)){ + unset($aSource[$sField]); + } } return $bResult; diff --git a/test/CollectorTest.php b/test/CollectorTest.php index 67004b5..2220cd3 100644 --- a/test/CollectorTest.php +++ b/test/CollectorTest.php @@ -157,13 +157,21 @@ public function testUpdateSDSAttributes($aExpectedAttrDef, $aSynchroAttrDef, boo require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; $oCollector = new iTopPersonCollector(); $oMockClient = $this->CreateMock('RestClient'); - $oMockClient->expects($this->exactly($bWillUpdate ? 1 : 0))->method("Update")->willReturn(['code' => 0]); - + + if ($bWillUpdate==0){ + $oMockClient->expects($this->never())->method("Update"); + } else { + $oMockClient->expects($this->once()) + ->method("Update") + ->with('SynchroAttribute') + ->willReturn(['code' => 0]); + } + $bRet = $this->InvokeNonPublicMethod(get_class($oCollector), 'UpdateSDSAttributes', $oCollector, [$aExpectedAttrDef, $aSynchroAttrDef, '', $oMockClient]); - + $this->assertTrue($bRet); } - + public function providerUpdateSDSAttributes() { return [ @@ -271,7 +279,97 @@ public function InvokeNonPublicMethod($sObjectClass, $sMethodName, $oObject, $aA $class = new \ReflectionClass($sObjectClass); $method = $class->getMethod($sMethodName); $method->setAccessible(true); - + return $method->invokeArgs($oObject, $aArgs); } + + public function CreateSynchroDataSourceProvider() { + return [ + 'all readonly fields' => [ + 'aSourceDefinition' => [ + 'attribute_list' => [], + 'friendlyname' => [], + 'user_id_friendlyname' => [], + 'user_id_finalclass_recall' => [], + 'notify_contact_id_friendlyname' => [], + 'notify_contact_id_finalclass_recall' => [], + 'notify_contact_id_obsolescence_flag' => [], + 'notify_contact_id_archive_flag' => [], + 'name' => [], + ], + 'aExpectedSourceDefinition' => [ + 'name' => [], + ], + ], + 'subset of all readonly fields' => [ + 'aSourceDefinition' => [ + 'attribute_list' => [], + 'friendlyname' => [], + 'user_id_friendlyname' => [], + 'user_id_finalclass_recall' => [], + 'notify_contact_id_friendlyname' => [], + 'notify_contact_id_archive_flag' => [], + 'name' => [], + ], + 'aExpectedSourceDefinition' => [ + 'name' => [], + ], + ], + ]; + } + + /** + * @dataProvider CreateSynchroDataSourceProvider + * @param array $aSourceDefinition + * @param array $aExpectedSourceDefinition + */ + public function testCreateSynchroDataSource($aSourceDefinition, $aExpectedSourceDefinition) + { + + $this->copy(APPROOT."/test/collector/attribute_isnullified/*"); + require_once APPROOT."/core/restclient.class.inc.php"; + require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; + $oCollector = new iTopPersonCollector(); + $oMockClient = $this->CreateMock('RestClient'); + + $sComment="COMMENT"; + + $oMockClient->expects($this->once()) + ->method("Create") + ->with('SynchroDataSource',$aExpectedSourceDefinition, $sComment) + ->willReturn(['code' => 0, 'objects' => [ ['key' => '123', 'fields'=> ['attribute_list' => []], ]]]); + + + $bRet = $this->InvokeNonPublicMethod(get_class($oCollector), 'CreateSynchroDataSource', $oCollector, [$aSourceDefinition, $sComment, $oMockClient]); + + $this->assertEquals('123', $bRet); + $this->assertEquals('123', $oCollector->GetSourceId()); + } + + /** + * @dataProvider CreateSynchroDataSourceProvider + * @param array $aSourceDefinition + * @param array $aExpectedSourceDefinition + */ + public function testUpdateSynchroDataSource($aSourceDefinition, $aExpectedSourceDefinition) + { + + $this->copy(APPROOT."/test/collector/attribute_isnullified/*"); + require_once APPROOT."/core/restclient.class.inc.php"; + require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; + $oCollector = new iTopPersonCollector(); + $oMockClient = $this->CreateMock('RestClient'); + + $sComment="COMMENT"; + + $oMockClient->expects($this->once()) + ->method("Update") + ->with('SynchroDataSource', "123", $aExpectedSourceDefinition, $sComment) + ->willReturn(['code' => 0, 'objects' => [ ['key' => '123', 'fields'=> ['attribute_list' => []], ]]]); + + $this->InvokeNonPublicMethod(get_class($oCollector), 'SetSourceId', $oCollector, ['123']); + $bRet = $this->InvokeNonPublicMethod(get_class($oCollector), 'UpdateSynchroDataSource', $oCollector, [$aSourceDefinition, $sComment, $oMockClient]); + + $this->assertEquals('123', $bRet); + } } diff --git a/test/RestTest.php b/test/RestTest.php index 0c8cfe5..9952044 100644 --- a/test/RestTest.php +++ b/test/RestTest.php @@ -116,4 +116,147 @@ function($sKey, $aDefaultValue) use ($aParameters) { $this->assertEquals(['retcode' => 0], $oRestClient->ListOperations()); } + public function testGetFullSynchroDataSource(){ + require_once(APPROOT.'core/collector.class.inc.php'); + $oMockClient = $this->CreateMock('RestClient'); + + $aFields = [ + 'name' => 'SynchroAttribute', + 'name2' => 'SynchroAttribute', + 'cis_list' => 'SynchroAttLinkSet', + ]; + + $aAttributeList = []; + foreach ($aFields as $sField => $sClass){ + $aAttributeList []= [ + 'attcode' => $sField, + 'finalclass' => $sClass, + ]; + } + $aSource = [ + 'attribute_list' => $aAttributeList, + 'friendlyname' => [], //should be removed from output response + 'user_id_friendlyname' => [], //should be removed from output response + 'user_id_finalclass_recall' => [], //should be removed from output response + 'notify_contact_id_friendlyname' => [], //should be removed from output response + 'notify_contact_id_finalclass_recall' => [], //should be removed from output response + 'notify_contact_id_obsolescence_flag' => [], //should be removed from output response + 'notify_contact_id_archive_flag' => [], //should be removed from output response + ]; + + $sExpectedOql1 = << [ + 'attcode' => 'name', + 'update' => '1', + 'reconcile' => '1', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + 'finalclass' => 'SynchroAttribute', + 'friendlyname' => 'name', //should be removed + "sync_source_id" => 'sync_source_id', //should be removed + "sync_source_name" => 'sync_source_name', //should be removed + "sync_source_id_friendlyname" => 'sync_source_id_friendlyname', //should be removed + "fake_field" => 'fake_field' + ] + ], + [ + 'fields' => [ + 'attcode' => 'name2', + 'update' => '1', + 'reconcile' => '0', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + 'finalclass' => 'SynchroAttribute', + 'friendlyname' => 'name2', //should be removed + "sync_source_id" => 'sync_source_id', //should be removed + "sync_source_name" => 'sync_source_name', //should be removed + "sync_source_id_friendlyname" => 'sync_source_id_friendlyname', //should be removed + "fake_field2" => 'fake_field2', + ] + ], + ]; + $aReturnObjects2 = [ + [ + 'fields' => [ + 'attcode' => 'cis_list', + 'update' => '1', + 'reconcile' => '1', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + 'finalclass' => 'SynchroAttLinkSet', + 'friendlyname' => 'cis_list', //should be removed + "sync_source_id" => 'sync_source_id', //should be removed + "sync_source_name" => 'sync_source_name', //should be removed + "sync_source_id_friendlyname" => 'sync_source_id_friendlyname', //should be removed + "fake_field3" => 'fake_field3' + ] + ], + ]; + $oMockClient->expects($this->exactly(2)) + ->method("Get") + ->withConsecutive(['SynchroAttribute', $sExpectedOql1], ['SynchroAttLinkSet', $sExpectedOql2]) + ->willReturnOnConsecutiveCalls(['code' => 0, 'objects' => $aReturnObjects1], ['code' => 0, 'objects' => $aReturnObjects2]); + + RestClient::GetFullSynchroDataSource($aSource, "123", $oMockClient); + $aExpected = [ + 'attribute_list' => [ + [ + 'attcode' => 'name', + 'finalclass' => "SynchroAttribute", + 'update' => '1', + 'reconcile' => '1', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + "fake_field" => 'fake_field', + ], + [ + 'attcode' => 'name2', + 'finalclass' => "SynchroAttribute", + 'update' => '1', + 'reconcile' => '0', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + "fake_field2" => 'fake_field2', + ], + [ + 'attcode' => 'cis_list', + 'finalclass' => "SynchroAttLinkSet", + 'update' => '1', + 'reconcile' => '1', + 'update_policy' => 'master_locked', + 'row_separator' => '|', + 'attribute_separator' => ';', + 'value_separator' => ':', + 'attribute_qualifier' => "'", + "fake_field3" => 'fake_field3', + ], + ] + ]; + $this->assertEquals($aExpected, $aSource); + } + } diff --git a/toolkit/dump_tasks.php b/toolkit/dump_tasks.php index 8419b7b..ac95af4 100644 --- a/toolkit/dump_tasks.php +++ b/toolkit/dump_tasks.php @@ -1,7 +1,7 @@ Date: Wed, 5 Feb 2025 11:15:58 +0100 Subject: [PATCH 4/6] =?UTF-8?q?N=C2=B08162=20-=20enhance=20DataSourcesAreE?= =?UTF-8?q?quivalent=20behaviour=20+=20add=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/collector.class.inc.php | 19 +++--- test/CollectorTest.php | 47 ++++++++++++++- .../iTopPersonCollector.class.inc.php | 29 +++++++++- test/collector/datasources/ds1.json | 50 ++++++++++++++++ .../datasources/ds1_oneadditional_field.json | 51 ++++++++++++++++ ...s1_oneadditionnal_attributelist_field.json | 58 +++++++++++++++++++ .../ds1_onefieldvalue_different.json | 50 ++++++++++++++++ .../ds1_readonly_fields_different.json | 50 ++++++++++++++++ ...ttributelist_fields_onedifferentvalue.json | 50 ++++++++++++++++ 9 files changed, 391 insertions(+), 13 deletions(-) create mode 100644 test/collector/datasources/ds1.json create mode 100644 test/collector/datasources/ds1_oneadditional_field.json create mode 100644 test/collector/datasources/ds1_oneadditionnal_attributelist_field.json create mode 100644 test/collector/datasources/ds1_onefieldvalue_different.json create mode 100644 test/collector/datasources/ds1_readonly_fields_different.json create mode 100644 test/collector/datasources/ds1_same_attributelist_fields_onedifferentvalue.json diff --git a/core/collector.class.inc.php b/core/collector.class.inc.php index 85be031..acb8ac9 100644 --- a/core/collector.class.inc.php +++ b/core/collector.class.inc.php @@ -979,7 +979,7 @@ protected function DataSourcesAreEquivalent($aDS1, $aDS2) foreach ($aDS1 as $sKey => $value) { if (in_array($sKey, Collector::READONLY_FIELDS)){ // Ignore all read-only attributes - break; + continue; } switch ($sKey) { @@ -993,13 +993,13 @@ protected function DataSourcesAreEquivalent($aDS1, $aDS2) Utils::Log(LOG_DEBUG, "Comparison: ignoring the missing, but optional, attribute: '$sAttCode'."); $this->aSkippedAttributes[] = $sAttCode; continue; - } else { - // Missing non-optional attribute - Utils::Log(LOG_DEBUG, "Comparison: The definition of the non-optional attribute '$sAttCode' is missing. Data sources differ."); - - return false; } + // Missing non-optional attribute + Utils::Log(LOG_DEBUG, "Comparison: The definition of the non-optional attribute '$sAttCode' is missing. Data sources differ."); + + return false; + } else { if (($aDef != $aDef2) && (!$this->AttributeIsOptional($sAttCode))) { // Definitions are different @@ -1034,13 +1034,12 @@ protected function DataSourcesAreEquivalent($aDS1, $aDS2) foreach ($aDS2 as $sKey => $value) { if (in_array($sKey, Collector::READONLY_FIELDS)){ // Ignore all read-only attributes - break; + continue; } - if (!array_key_exists($sKey, $aDS1)) { + if (! array_key_exists($sKey, $aDS1)) { + //locally unknown fields can NOT be updated. so it is useless to update datasynchro on itop Utils::Log(LOG_DEBUG, "Comparison: Found an extra property '$sKey' in iTop. Data sources differ."); - - return false; } } diff --git a/test/CollectorTest.php b/test/CollectorTest.php index 2220cd3..5cce172 100644 --- a/test/CollectorTest.php +++ b/test/CollectorTest.php @@ -353,7 +353,6 @@ public function testCreateSynchroDataSource($aSourceDefinition, $aExpectedSource */ public function testUpdateSynchroDataSource($aSourceDefinition, $aExpectedSourceDefinition) { - $this->copy(APPROOT."/test/collector/attribute_isnullified/*"); require_once APPROOT."/core/restclient.class.inc.php"; require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; @@ -372,4 +371,50 @@ public function testUpdateSynchroDataSource($aSourceDefinition, $aExpectedSource $this->assertEquals('123', $bRet); } + + public function DataSourcesAreEquivalentProvider() { + return [ + 'exactly same ds' => [ "ds1.json", "ds1.json", true ], + + 'ds1 vs ds1_oneadditional_field' => [ "ds1.json", "ds1_oneadditional_field.json", true ], + 'ds1_oneadditional_field vs ds1' => [ "ds1_oneadditional_field.json", "ds1.json", false ], + + 'ds1 vs ds1_oneadditionnal_attributelist_field' => [ "ds1.json", "ds1_oneadditionnal_attributelist_field.json", true], + 'ds1_oneadditionnal_attributelist_field vs ds1' => [ "ds1_oneadditionnal_attributelist_field.json", "ds1.json", false], + + 'optional attribute case: ds1 vs ds1_oneadditionnal_attributelist_field' => [ "ds1.json", "ds1_oneadditionnal_attributelist_field.json", true, ['name'] ], + 'optional attribute case: ds1_oneadditionnal_attributelist_field vs ds1' => [ "ds1_oneadditionnal_attributelist_field.json", "ds1.json", true, ['name']], + + 'ds1 vs ds1_onefieldvalue_different' => [ "ds1.json", "ds1_onefieldvalue_different.json", false ], + 'ds1_onefieldvalue_different vs ds1' => [ "ds1_onefieldvalue_different.json", "ds1.json", false ], + + 'ds1 vs ds1_same_attributelist_fields_onedifferentvalue' => [ "ds1.json", "ds1_same_attributelist_fields_onedifferentvalue.json", false ], + 'ds1_same_attributelist_fields_onedifferentvalue vs ds1' => [ "ds1_same_attributelist_fields_onedifferentvalue.json", "ds1.json", false ], + + 'optional attribute case: ds1 vs ds1_same_attributelist_fields_onedifferentvalue' => [ "ds1.json", "ds1_same_attributelist_fields_onedifferentvalue.json", true, ['team_list'] ], + 'optional attribute case: ds1_same_attributelist_fields_onedifferentvalue vs ds1' => [ "ds1_same_attributelist_fields_onedifferentvalue.json", "ds1.json", true, ['team_list'] ], + ]; + } + + /** + * @dataProvider DataSourcesAreEquivalentProvider + * + * @param string $sDS1Path + * @param string $sDS2Path + * @param bool $bExpected + */ + public function testDataSourcesAreEquivalent($sDS1Path, $sDS2Path, $bExpected, $aOptionalAttributes=[]) { + $this->copy(APPROOT."/test/collector/attribute_isnullified/*"); + require_once APPROOT."/core/restclient.class.inc.php"; + require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; + $oCollector = new iTopPersonCollector(); + $oCollector->SetOptionalAttributes($aOptionalAttributes); + + $sResourceDir = __DIR__ . '/collector/datasources/'; + $sDS1 = json_decode(file_get_contents($sResourceDir . $sDS1Path), true); + $sDS2 = json_decode(file_get_contents($sResourceDir . $sDS2Path), true); + $bRet = $this->InvokeNonPublicMethod(get_class($oCollector), 'DataSourcesAreEquivalent', $oCollector, [ $sDS1, $sDS2 ]); + + $this->assertEquals($bExpected, $bRet, "$sDS1Path vs $sDS2Path"); + } } diff --git a/test/collector/attribute_isnullified/iTopPersonCollector.class.inc.php b/test/collector/attribute_isnullified/iTopPersonCollector.class.inc.php index eb87c65..23f2c6b 100644 --- a/test/collector/attribute_isnullified/iTopPersonCollector.class.inc.php +++ b/test/collector/attribute_isnullified/iTopPersonCollector.class.inc.php @@ -3,6 +3,7 @@ class iTopPersonCollector extends Collector { private $bFetched; + private $aOptionalAttributes; protected function Fetch() { @@ -24,13 +25,37 @@ protected function Fetch() return null; } - + + /** + * @return array + */ + public function GetOptionalAttributes() { + if (! isset($this->aOptionalAttributes)){ + return ['optional']; + } + return $this->aOptionalAttributes; + } + + /** + * @param array $aOptionalAttributes + */ + public function SetOptionalAttributes($aOptionalAttributes) { + $this->aOptionalAttributes = $aOptionalAttributes; + } + + /** * {@inheritDoc} * @see Collector::AttributeIsOptional() */ public function AttributeIsOptional($sAttCode) { - return ($sAttCode === 'optional'); + foreach ($this->GetOptionalAttributes() as $sAttOptionalCode){ + if ($sAttCode === $sAttOptionalCode){ + return true; + } + } + + return false; } } diff --git a/test/collector/datasources/ds1.json b/test/collector/datasources/ds1.json new file mode 100644 index 0000000..b241571 --- /dev/null +++ b/test/collector/datasources/ds1.json @@ -0,0 +1,50 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "0", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} diff --git a/test/collector/datasources/ds1_oneadditional_field.json b/test/collector/datasources/ds1_oneadditional_field.json new file mode 100644 index 0000000..ca3142e --- /dev/null +++ b/test/collector/datasources/ds1_oneadditional_field.json @@ -0,0 +1,51 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "0", + "delete_policy_retention2": "0", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} diff --git a/test/collector/datasources/ds1_oneadditionnal_attributelist_field.json b/test/collector/datasources/ds1_oneadditionnal_attributelist_field.json new file mode 100644 index 0000000..75e1ef8 --- /dev/null +++ b/test/collector/datasources/ds1_oneadditionnal_attributelist_field.json @@ -0,0 +1,58 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "0", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "name", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} diff --git a/test/collector/datasources/ds1_onefieldvalue_different.json b/test/collector/datasources/ds1_onefieldvalue_different.json new file mode 100644 index 0000000..b1c70a7 --- /dev/null +++ b/test/collector/datasources/ds1_onefieldvalue_different.json @@ -0,0 +1,50 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "1", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} diff --git a/test/collector/datasources/ds1_readonly_fields_different.json b/test/collector/datasources/ds1_readonly_fields_different.json new file mode 100644 index 0000000..a9f1eb7 --- /dev/null +++ b/test/collector/datasources/ds1_readonly_fields_different.json @@ -0,0 +1,50 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "0", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "&", + "user_id_friendlyame": "1", + "user_id_finalclass_recall": "2", + "notify_contact_id_friendlyname": 3, + "notify_contact_id_finalclass_recall": 4, + "notify_contact_id_archive_flag": "5", + "notify_contact_id_obsolescence_flag": "6" +} diff --git a/test/collector/datasources/ds1_same_attributelist_fields_onedifferentvalue.json b/test/collector/datasources/ds1_same_attributelist_fields_onedifferentvalue.json new file mode 100644 index 0000000..be11b44 --- /dev/null +++ b/test/collector/datasources/ds1_same_attributelist_fields_onedifferentvalue.json @@ -0,0 +1,50 @@ +{ + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "0", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "1", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} From 79cfc816d3bc365acef77293e4c03ba543e898ee Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 5 Feb 2025 13:58:27 +0100 Subject: [PATCH 5/6] =?UTF-8?q?N=C2=B08162=20-=20enhance=20DataSourcesAreE?= =?UTF-8?q?quivalent=20behaviour=20+=20add=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/CollectorTest.php | 44 ++++++++++++++++ ...1_oneadditional_field_readonlytesting.json | 51 +++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 test/collector/datasources/ds1_oneadditional_field_readonlytesting.json diff --git a/test/CollectorTest.php b/test/CollectorTest.php index 5cce172..5c47b90 100644 --- a/test/CollectorTest.php +++ b/test/CollectorTest.php @@ -417,4 +417,48 @@ public function testDataSourcesAreEquivalent($sDS1Path, $sDS2Path, $bExpected, $ $this->assertEquals($bExpected, $bRet, "$sDS1Path vs $sDS2Path"); } + + public function DataSourcesAreEquivalent_READONLY_FIELDS_Provider() { + $aUseCases = []; + + foreach (\Collector::READONLY_FIELDS as $sField){ + $sMessage = "same datasources: additional field ($sField) should not impact comparison"; + $aUseCases[$sMessage] = [ + $sMessage, $sField, "0", true + ]; + + $sMessage = "distinct datasources: additional field ($sField) should not impact comparison"; + $aUseCases[$sMessage] = [ + $sMessage, $sField, "1", false + ]; + } + + return $aUseCases; + } + /** + * @dataProvider DataSourcesAreEquivalent_READONLY_FIELDS_Provider + * + * @param string $sMessage + * @param string $sFieldToTest + * @param string $delete_policy_retention_value + * @param bool $bExpected + */ + public function testDataSourcesAreEquivalent_READONLY_FIELDS($sMessage, $sFieldToTest, $delete_policy_retention_value, $bExpected) { + $this->copy(APPROOT."/test/collector/attribute_isnullified/*"); + require_once APPROOT."/core/restclient.class.inc.php"; + require_once self::$sCollectorPath."iTopPersonCollector.class.inc.php"; + $oCollector = new iTopPersonCollector(); + + $sResourceDir = __DIR__ . '/collector/datasources/'; + $sContent = file_get_contents($sResourceDir."ds1_oneadditional_field_readonlytesting.json"); + $sContent = str_replace("ADDITIONAL_FIELD_NAME", "$sFieldToTest", $sContent); + $sContent = str_replace("delete_policy_retention_value", $delete_policy_retention_value, $sContent); + $sDS1 = json_decode($sContent, true); + $sDS2 = json_decode(file_get_contents($sResourceDir . "ds1.json"), true); + $bRet = $this->InvokeNonPublicMethod(get_class($oCollector), 'DataSourcesAreEquivalent', $oCollector, [ $sDS1, $sDS2 ]); + + var_dump($sDS1); + var_dump($sDS2); + $this->assertEquals($bExpected, $bRet, $sMessage); + } } diff --git a/test/collector/datasources/ds1_oneadditional_field_readonlytesting.json b/test/collector/datasources/ds1_oneadditional_field_readonlytesting.json new file mode 100644 index 0000000..5d36e18 --- /dev/null +++ b/test/collector/datasources/ds1_oneadditional_field_readonlytesting.json @@ -0,0 +1,51 @@ +{ + "ADDITIONAL_FIELD_NAME" : "", + "name": "$prefix$Synchro CSV Person", + "description": "Synchronization of persons from CSV", + "status": "$synchro_status$", + "user_id": "$synchro_user$", + "notify_contact_id": "$contact_to_notify$", + "scope_class": "Person", + "database_table_name": "$persons_data_table$", + "scope_restriction": "", + "full_load_periodicity": "$full_load_interval$", + "reconciliation_policy": "use_primary_key", + "action_on_zero": "create", + "action_on_one": "update", + "action_on_multiple": "error", + "delete_policy": "ignore", + "delete_policy_update": "", + "delete_policy_retention": "delete_policy_retention_value", + "attribute_list": [ + { + "attcode": "status", + "update": "1", + "reconcile": "0", + "update_policy": "master_locked", + "finalclass": "SynchroAttribute", + "friendlyname": "status" + }, + { + "attcode": "team_list", + "update": "0", + "reconcile": "0", + "update_policy": "master_locked", + "row_separator": "|", + "attribute_separator": ";", + "value_separator": ":", + "attribute_qualifier": "'", + "finalclass": "SynchroAttLinkSet", + "friendlyname": "team_list" + } + ], + "user_delete_policy": "nobody", + "url_icon": "", + "url_application": "", + "friendlyname": "", + "user_id_friendlyname": "", + "user_id_finalclass_recall": "", + "notify_contact_id_friendlyname": "", + "notify_contact_id_finalclass_recall": "", + "notify_contact_id_archive_flag": "", + "notify_contact_id_obsolescence_flag": "" +} From be938e1c78e64b8dc7cf91c53cc36b9b62650c8d Mon Sep 17 00:00:00 2001 From: odain-cbd <56586767+odain-cbd@users.noreply.github.com> Date: Thu, 6 Feb 2025 17:53:05 +0100 Subject: [PATCH 6/6] Update core/collector.class.inc.php Co-authored-by: Thomas Casteleyn --- core/collector.class.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/collector.class.inc.php b/core/collector.class.inc.php index acb8ac9..d63e998 100644 --- a/core/collector.class.inc.php +++ b/core/collector.class.inc.php @@ -984,7 +984,7 @@ protected function DataSourcesAreEquivalent($aDS1, $aDS2) switch ($sKey) { case 'attribute_list': - foreach ($value as $sKey2 => $aDef) { + foreach ($value as $aDef) { $sAttCode = $aDef['attcode']; $aDef2 = $this->FindAttr($sAttCode, $aDS2['attribute_list']); if ($aDef2 === false) {