Skip to content

Inventory: software matching can silently attach new versions/installations to a trashed duplicate - #25456

Open
Megachip wants to merge 12 commits into
glpi-project:11.0/bugfixesfrom
Megachip:fix/inventory-software-prefer-active-duplicate
Open

Megachip wants to merge 12 commits into
glpi-project:11.0/bugfixesfrom
Megachip:fix/inventory-software-prefer-active-duplicate

Conversation

@Megachip

Copy link
Copy Markdown
Contributor

Bug description

When two glpi_softwares rows exist with identical name + manufacturer

  • entity (e.g. a leftover duplicate that was moved to the trashbin
    after a manual merge), Glpi\Inventory\Asset\Software::populateSoftware()
    can resolve future inventory imports to the trashed row instead of
    the active one, so new SoftwareVersion/Item_SoftwareVersion entries
    keep landing on a deleted item.

Root cause: the existing-software lookup has no ORDER BY and no
is_deleted filter, and the surrounding while ($row = ...) loop
overwrites the cached id on every row instead of taking the first
match — so the last row returned by MySQL wins, which is undefined
without an explicit order and in practice tends to be the highest id.

Steps to reproduce

  1. Let inventory create a software (e.g. via agent import).
  2. Manually create a second glpi_softwares row with the exact same
    name/manufacturer/entities_id/is_recursive, then move it to the
    trashbin (is_deleted = 1), so it has a higher id than the active
    one.
  3. Run a new inventory reporting that software name again.
  4. Observe: the new SoftwareVersion/Item_SoftwareVersion is attached
    to the trashed (higher-id) software, not the active one.

Fix

ORDER BY is_deleted ASC, id ASC LIMIT 1 on the lookup, single fetch
instead of the loop. Active software is always preferred; among rows
with the same is_deleted status, the lowest id wins deterministically.
If only a deleted duplicate matches, it is still reused (unchanged
behavior) — this PR only fixes the priority between active and
deleted, not whether deleted rows are eligible at all.

Test plan

Added testDuplicatedSoftPrefersActiveOverDeleted() in
SoftwareTest.php: creates an active software (lower id) and a
trashed duplicate (higher id), runs an inventory, asserts the new
version/installation lands on the active one only.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Powered by Qodo

@trasher trasher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix seem correct to me - waiting for tests suite to ends.

You'll have few CS issues to fix.

@trasher trasher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing

Fix duplicated-software test to seed the active entry via inventory
@Megachip
Megachip requested a review from trasher September 18, 2026 11:46
@trasher
trasher requested review from Rom1-B and stonebuzz September 18, 2026 11:53
$this->assertCount(
1,
$on_active,
'the new version/installation must attach to the active software, not the deleted duplicate'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions only look at glpi_softwareversions; the message claims "version/installation" but no Item_SoftwareVersion row is checked. Adding one assertion on the installation would cover the part of the bug that actually affects the asset sheet.

$installs = (new Item_SoftwareVersion())->find(['softwareversions_id' => (int) current($on_active)['id']]);
$this->assertCount(1, $installs);

$soft = new \Software();
$version = new SoftwareVersion();

$computers_id = $computer->add([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The computer is pre-created by hand, but the first inventory would create it anyway and nothing asserts that the inventory matched this row rather than creating a second one. Dropping the manual Computer::add() would make the fixture leaner without weakening the test.

@trasher trasher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address comments

Removed unused computer instantiation and related assertions.
Add assertions to verify installation links to active software version.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants