From 5a84804530722ab037b19cbeed0539ce3c16faeb Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 22 Jul 2026 22:37:34 +0200 Subject: [PATCH 1/2] =?UTF-8?q?N=C2=B09831=20-=20Fix=20DataCleanupService?= =?UTF-8?q?=20so=20it=20handles=20external=20keys=20the=20same=20way=20as?= =?UTF-8?q?=20\Combodo\iTop\DataFeatureRemoval\Service\StaticDeletionPlan:?= =?UTF-8?q?:DeletionPlanForReferencingClasses=20and=20\DBObject::MakeDelet?= =?UTF-8?q?ionPlan?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Check if ext. key is nullable first - Then check if its a DEL_MANUAL option --- .../src/Service/DataCleanupService.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php b/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php index e2d3956b7a..180ef7af22 100644 --- a/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php +++ b/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php @@ -134,10 +134,6 @@ private function RecursiveDeletion(DBObject $oObjectToClean): bool /** @var DBObject $oDependentObj */ while ($oDependentObj = $oSet->Fetch()) { $iDeletePropagationOption = $oExtKeyAttDef->GetDeletionPropagationOption(); - if ($iDeletePropagationOption == DEL_MANUAL) { - $this->oObjectService->SetIssue(get_class($oDependentObj)); - continue; - } if ($oExtKeyAttDef->IsNullAllowed()) { // Optional external key, list to reset @@ -152,6 +148,12 @@ private function RecursiveDeletion(DBObject $oObjectToClean): bool return false; } } else { + // Mandatory external key + if ($iDeletePropagationOption == DEL_MANUAL) { + // Cannot be deleted automatically, must be handled manually + $this->oObjectService->SetIssue(get_class($oDependentObj)); + continue; + } // Propagate deletion only if not visited if ($this->IsVisited($oDependentObj)) { continue; From 0770d89f29d17ccf8b19a4b8f242b564a29000e8 Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 22 Jul 2026 22:42:27 +0200 Subject: [PATCH 2/2] =?UTF-8?q?N=C2=B09831=20-=20Fix=20ext.=20management?= =?UTF-8?q?=20deletion=20plan=20analysis=20by=20avoiding=20overwriting=20e?= =?UTF-8?q?ntity=20for=20a=20previously=20processed=20class?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/Service/StaticDeletionPlan.php | 8 ++- .../DataCleanupServiceTest.php | 52 ++++++++++++++++ .../StaticDeletionPlanTest.php | 61 ++++++++++++++++++- .../data_cleanup_delta.xml | 60 ++++++++++++++++++ 4 files changed, 177 insertions(+), 4 deletions(-) diff --git a/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php b/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php index 82cf651570..6c3b790c41 100644 --- a/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php +++ b/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php @@ -59,9 +59,11 @@ public function GetStaticDeletionPlan(array $aClasses): array { foreach ($aClasses as $sClass) { $oDeletionPlanItem = $this->GetInitialClassDeletionPlan($sClass); - $oDeletionPlanEntity = new DeletionPlanEntity(); - $oDeletionPlanEntity->oDelete->Merge($oDeletionPlanItem); - $this->aDeletionPlan[$sClass] = $oDeletionPlanEntity; + // N°9831 Do not overwrite existing entity as it may already exist for this class if a previously processed class references it (issues/updates already accumulated must be kept) + if (false === array_key_exists($sClass, $this->aDeletionPlan)) { + $this->aDeletionPlan[$sClass] = new DeletionPlanEntity(); + } + $this->aDeletionPlan[$sClass]->oDelete->Merge($oDeletionPlanItem); $this->DeletionPlanForReferencingClasses($sClass); } diff --git a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php index cbf3b54f2e..9df032c777 100644 --- a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php +++ b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php @@ -163,6 +163,58 @@ public function testGetCleanupSummary_ManualDeleteShouldFail() $this->AssertSummaryEquals($aExpected, $aRes); } + /** + * N°9831 + * + * A nullable external key configured as DEL_MANUAL must NOT be treated as an issue: it is simply + * reset (set to null), exactly like core DBObject::MakeDeletionPlan does. DEL_MANUAL only blocks + * for mandatory keys. This mirrors the real "Hypervisor.farm_id -> Farm" case (nullable + DEL_MANUAL). + * + * Regression guard for the ordering of the IsNullAllowed()/DEL_MANUAL checks in RecursiveDeletion: + * reverting that change would make ExecuteCleanup throw here instead of resetting the object. + */ + public function testExecuteCleanup_NullableManualShouldResetAndNotThrow() + { + $this->GivenDFRTreeInDB(<<ExecuteCleanup($aClasses); + + $aExpected = [ + ['DFRManualNullable', 1, 0 ], + ['DFRToRemoveLeaf', 0, 1 ], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + + /** + * N°9831 + * + * Summary counterpart of the test above: a nullable DEL_MANUAL external key is reported as an + * update (reset), with an issue count of 0. Reverting the RecursiveDeletion change would report + * it as an issue instead. + */ + public function testGetCleanupSummary_NullableManualShouldBeUpdateNotIssue() + { + $this->GivenDFRTreeInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + ['DFRManualNullable', 1, 0, 0 ], + ['DFRToRemoveLeaf', 0, 1, 0 ], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + private function AssertSummaryEquals(array $expected, $actual, $sMessage = '') { $aExpected = []; diff --git a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php index bd0e00a470..ca292f3ca1 100644 --- a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php +++ b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php @@ -9,7 +9,6 @@ use Combodo\iTop\DataFeatureRemoval\Entity\DataCleanupSummaryEntity; use Combodo\iTop\DataFeatureRemoval\Service\StaticDeletionPlan; -use MetaModel; require_once __DIR__."/AbstractCleanup.php"; class StaticDeletionPlanTest extends \AbstractCleanup @@ -150,6 +149,66 @@ public function testGetCleanupSummaryWithIssues() $this->AssertSummaryEquals($aExpected, $aRes); } + /** + * N°9831 + * + * A DEL_MANUAL issue must still be reported when the referencing class (DFRManual) is itself part + * of the classes to remove AND is processed AFTER the class it points to (DFRToRemoveLeaf). + * + * This mirrors the real "VirtualMachine -> VirtualHost" case: the setup audit sorts the removed + * classes alphabetically and drops abstract classes, so a referencing class such as VirtualMachine + * ends up processed after its concrete targets (Farm/Hypervisor/Cloud). Here DFRToRemove is abstract + * and DFRToRemoveLeaf is its concrete child, while DFRManual points to DFRToRemove with a mandatory + * DEL_MANUAL external key. + * + * Regression: the plan entity accumulating the issue used to be overwritten when the referencing + * class was processed as a top-level class, silently dropping the issue (no blocking message on the + * summary page, then a crash at execution time). + */ + public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedAfterTarget(): void + { + $this->GivenDFRObjectsInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + 'DFRToRemoveLeaf' => ['iDeleteCount' => 1], + 'DFRManual' => ['iDeleteCount' => 1, 'iIssueCount' => 1], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + + /** + * N°9831 + * + * Control case: the same issue must also be reported when the referencing class is listed BEFORE + * its target (this order already worked, e.g. "Enclosure -> Rack"). Together with the test above, + * this locks in that issue detection is independent of the processing order. + */ + public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedBeforeTarget(): void + { + $this->GivenDFRObjectsInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + 'DFRToRemoveLeaf' => ['iDeleteCount' => 1], + 'DFRManual' => ['iDeleteCount' => 1, 'iIssueCount' => 1], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + public function testCircularRefsShouldNotRunInfinitely() { $this->GivenDFRObjectsInDB(<< cmdbAbstractObject + + + bizmodel,searchable + false + dfrmanualnullable + + + + + + + + + + name + + true + + + all + + + extkey_id + + + true + DFRToRemove + DEL_MANUAL + all + + + + + + + + + + +
+ + + 10 + + + 20 + + +
+ + cmdbAbstractObject + bizmodel,searchable @@ -718,6 +770,14 @@ + + + + + + + +