Skip to content

N°9831 - Fix setup error when trying to remove class with a DEL_MANUAL ext. key #980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Molkobain merged 2 commits into from
Jul 23, 2026
Merged

N°9831 - Fix setup error when trying to remove class with a DEL_MANUAL ext. key #980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a84804
N°9831 - Fix DataCleanupService so it handles external keys the same …
Molkobain Jul 22, 2026
0770d89
N°9831 - Fix ext. management deletion plan analysis by avoiding overw…
Molkobain Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -152,6 +148,12 @@ private function RecursiveDeletion(DBObject $oObjectToClean): bool
return false;
}
} else {
// Mandatory external key
if ($iDeletePropagationOption == DEL_MANUAL) {
Comment thread
Molkobain marked this conversation as resolved.
// 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;
Expand Down
8 changes: 5 additions & 3 deletions datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
52 changes: 52 additions & 0 deletions ...ests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<<<EOF
DFRToRemoveLeaf_1 <- DFRManualNullable_1
EOF);

$aClasses = [ 'DFRToRemoveLeaf' ];
$oService = new DataCleanupService();
// Must NOT throw: the nullable external key is reset, not reported as an issue
$aRes = $oService->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(<<<EOF
DFRToRemoveLeaf_1 <- DFRManualNullable_1
EOF);

$aClasses = [ 'DFRToRemoveLeaf' ];
$oService = new DataCleanupService();
$aRes = $oService->GetCleanupSummary($aClasses);

$aExpected = [
['DFRManualNullable', 1, 0, 0 ],
['DFRToRemoveLeaf', 0, 1, 0 ],
];
$this->AssertSummaryEquals($aExpected, $aRes);
}

private function AssertSummaryEquals(array $expected, $actual, $sMessage = '')
{
$aExpected = [];
Expand Down
61 changes: 60 additions & 1 deletion ...ests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(<<<EOF
create DFRToRemoveLeaf (name = DFRToRemoveLeaf_1)
create DFRManual (name = DFRManual_1, extkey_id = DFRToRemoveLeaf_1)
EOF);

// Target listed BEFORE the referencing class, as produced by the alphabetically sorted audit
$aClasses = ['DFRToRemoveLeaf', 'DFRManual'];
$oService = new StaticDeletionPlan();
$aRes = $oService->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(<<<EOF
create DFRToRemoveLeaf (name = DFRToRemoveLeaf_1)
create DFRManual (name = DFRManual_1, extkey_id = DFRToRemoveLeaf_1)
EOF);

$aClasses = ['DFRManual', 'DFRToRemoveLeaf'];
$oService = new StaticDeletionPlan();
$aRes = $oService->GetCleanupSummary($aClasses);

$aExpected = [
'DFRToRemoveLeaf' => ['iDeleteCount' => 1],
'DFRManual' => ['iDeleteCount' => 1, 'iIssueCount' => 1],
];
$this->AssertSummaryEquals($aExpected, $aRes);
}

public function testCircularRefsShouldNotRunInfinitely()
{
$this->GivenDFRObjectsInDB(<<<EOF
Expand Down
60 changes: 60 additions & 0 deletions ...it-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/data_cleanup_delta.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,58 @@
</presentation>
<parent>cmdbAbstractObject</parent>
</class>
<class id="DFRManualNullable" _created_in="itop-structure" _delta="define">
<properties>
<category>bizmodel,searchable</category>
<abstract>false</abstract>
<db_table>dfrmanualnullable</db_table>
<naming>
<attributes/>
</naming>
<reconciliation>
<attributes/>
</reconciliation>
</properties>
<fields>
<field id="name" xsi:type="AttributeString">
<sql>name</sql>
<default_value/>
<is_null_allowed>true</is_null_allowed>
<validation_pattern/>
<dependencies/>
<tracking_level>all</tracking_level>
</field>
<field id="extkey_id" xsi:type="AttributeExternalKey">
<sql>extkey_id</sql>
<filter/>
<dependencies/>
<is_null_allowed>true</is_null_allowed>
<target_class>DFRToRemove</target_class>
<on_target_delete>DEL_MANUAL</on_target_delete>
<tracking_level>all</tracking_level>
</field>
</fields>
<methods/>
<presentation>
<list>
<items/>
</list>
<search>
<items/>
</search>
<details>
<items>
<item id="name">
<rank>10</rank>
</item>
<item id="extkey_id">
<rank>20</rank>
</item>
</items>
</details>
</presentation>
<parent>cmdbAbstractObject</parent>
</class>
<class id="DFRLeafNotToRemove" _created_in="itop-structure" _delta="define">
<properties>
<category>bizmodel,searchable</category>
Expand Down Expand Up @@ -718,6 +770,14 @@
<entry id="Class:DFRManual/Attribute:name+" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManual/Attribute:extkey_id" _delta="define"><![CDATA[Dfrtoremove id]]></entry>
<entry id="Class:DFRManual/Attribute:extkey_id+" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManualNullable/Name" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManualNullable/ComplementaryName" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManualNullable" _delta="define"><![CDATA[DFRManualNullable]]></entry>
<entry id="Class:DFRManualNullable+" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManualNullable/Attribute:name" _delta="define"><![CDATA[Name]]></entry>
<entry id="Class:DFRManualNullable/Attribute:name+" _delta="define"><![CDATA[]]></entry>
<entry id="Class:DFRManualNullable/Attribute:extkey_id" _delta="define"><![CDATA[Dfrtoremove id]]></entry>
<entry id="Class:DFRManualNullable/Attribute:extkey_id+" _delta="define"><![CDATA[]]></entry>
</entries>
</dictionary>
</dictionaries>
Expand Down