Skip to content

Fix bulk modify timeout on AttributeLinkedSetIndirect#923

Open
larhip wants to merge 1 commit into
Combodo:developfrom
itomig-de:fix/bulkmodify_linkedset
Open

Fix bulk modify timeout on AttributeLinkedSetIndirect#923
larhip wants to merge 1 commit into
Combodo:developfrom
itomig-de:fix/bulkmodify_linkedset

Conversation

@larhip
Copy link
Copy Markdown
Contributor

@larhip larhip commented May 29, 2026

Base information

Question Answer
Related to a SourceForge thread / Another PR / A GitHub Issue / Combodo ticket? n/a
Type of change? Bug fix

Symptom (bug)

Bulk-modifying records that contain large AttributeLinkedSetIndirect fields times out before the bulk modify form is rendered.

Field report: bulk-modifying 4 ProviderContract records with ~5400 linked CIs each (≈21,600 link materialisations + HTML renders per request) reliably exceeds the PHP request timeout. Profiling shows the time is spent inside DisplayBulkModifyForm() materialising the indirect link sets of every selected object.

The defect was introduced by N°3190 commit fb1ceeba, which added AttributeLinkedSetIndirect::IsBulkModifyCompatible() returning true unconditionally to support the new display_style=property (tagset-like) widget. The same return value, however, also enables bulk processing for the default tab display style, which is what triggers the timeout.

Reproduction procedure (bug)

  1. On iTop 3.1.x or 3.2.x (any version including the N°3190 fix)
  2. With PHP 7.4+ (any supported version)
  3. Pick any class with an indirect link set rendered as a tab — e.g. UserRequest::contacts_list, or ProviderContract::functionalcis_list from a populated dataset. For a reliable reproduction, populate at least 4 objects with ≈5000 links each.
  4. List view → select multiple objects → "Other actions" → "Modify all..."
  5. Observe: the bulk modify form takes very long to render and eventually triggers a PHP request timeout (HTTP 504 / blank page).

Workaround that confirms the cause: temporarily change AttributeLinkedSetIndirect::IsBulkModifyCompatible() to return false; — the bulk modify form renders immediately.

Cause (bug)

IsBulkModifyCompatible() is checked in application/cmdbabstract.class.inc.php::DisplayBulkModifyForm() for every attribute of every selected object (lines 4544, 4550, 4613):

while ($oObj = $oSet->Fetch()) {
    foreach ($aList as $sAttCode => $oAttDef) {
        if ($oAttDef->IsBulkModifyCompatible() && $oAttDef->IsWritable()) {
            $currValue   = $oObj->Get($sAttCode);              // materialises ormLinkSet
            ...
            $sHtmlValue  = $oAttDef->GetAsHTML($currValue);    // renders the whole widget
            $editValue   = $oAttDef->GetEditValue($currValue, $oObj);
        }
    }
}

For tab-style indirect link sets this work is entirely wasted: cmdbabstract.class.inc.php:2134 and :4065 already gate the bulk-context rendering path on LINKSET_DISPLAY_STYLE_PROPERTY, so a tab-style indirect link set is not editable in the bulk form at all.

IsBulkModifyCompatible() is declared static, which is why the N°3190 override could not read the per-instance display_style XML property. As a consequence the override returns a class-level constant — true for all indirect link sets, regardless of the actual rendering mode.

A repository-wide check confirms that all four call sites (cmdbabstract.class.inc.php:1854, 4544, 4550, 4613) already use instance dispatch (\$oAttDef->IsBulkModifyCompatible()); there is no static caller. The static modifier is therefore historical, not load-bearing.

Proposed solution

Make IsBulkModifyCompatible() a regular instance method across the AttributeDefinition hierarchy. AttributeLinkedSetIndirect then gates on the per-instance display_style:

  • sources/Core/AttributeDefinition/AttributeDefinition.php — drop static from the base declaration; document the migration in the docblock.
  • sources/Core/AttributeDefinition/AttributeLinkedSet.php — drop static from the override (return value unchanged: false).
  • sources/Core/AttributeDefinition/AttributeLinkedSetIndirect.php — drop static, return $this->GetDisplayStyle() === LINKSET_DISPLAY_STYLE_PROPERTY.

The property-style path (the original N°3190 feature) is preserved; the tab-style path is excluded from the bulk form with the "IncompatibleAttribute" notice, matching pre-fb1ceeba behaviour.

Breaking change for extensions: overrides of IsBulkModifyCompatible() must drop the static modifier from the override declaration, otherwise PHP raises a fatal error (Cannot make non-static method ... static in class ...). The method is not marked @api and is not part of any documented extension API; expected blast radius is small. A note has been added to the base class docblock to document the migration.

Regression coverage

tests/php-unit-tests/unitary-tests/core/AttributeDefinitionTest.php::testIsBulkModifyCompatible (data-provider, 4 cases):

  • scalar (UserRequest::title) → true
  • AttributeLinkedSet (UserRequest::workorders_list) → false
  • AttributeLinkedSetIndirect, default tab style (UserRequest::contacts_list) → false (regression case)
  • AttributeLinkedSetIndirect, display_style=property (FunctionalCI::groups_list) → true (preserves N°3190)

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand without digging in the code?

…rect on display_style

Since N°3190 (commit fb1ceeb), AttributeLinkedSetIndirect::IsBulkModifyCompatible()
returned true unconditionally. For the default (tab) display style this caused
DisplayBulkModifyForm() to eagerly materialise and render the full link set of
every selected object, leading to timeouts when bulk modifying records with
thousands of links each.

Bulk modify of an indirect link set is only meaningful for the property-style
(tagset-like) widget introduced by N°3190 — the tab-style widget cannot be
rendered inside the bulk modify form at all.

IsBulkModifyCompatible() is changed from static to a regular instance method
across the AttributeDefinition hierarchy so it can depend on the per-instance
display_style XML property. All four call sites in cmdbabstract.class.inc.php
already use instance dispatch, so no caller changes are required.

Breaking change for extensions: overrides of IsBulkModifyCompatible() must
drop the \`static\` modifier to avoid a PHP fatal error.

Adds a regression test covering the four relevant paths (scalar,
AttributeLinkedSet, AttributeLinkedSetIndirect with default tab style,
AttributeLinkedSetIndirect with display_style=property).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending review

Development

Successfully merging this pull request may close these issues.

2 participants