Fixed issue where different variations of the same software weren't linked to the same software title. - #38926
Conversation
…en't linked to the same software title.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #38926 +/- ##
==========================================
+ Coverage 66.10% 66.11% +0.01%
==========================================
Files 2420 2421 +1
Lines 193439 193874 +435
Branches 8434 8434
==========================================
+ Hits 127865 128186 +321
- Misses 53970 54043 +73
- Partials 11604 11645 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
getvictor
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughCollects non-empty Windows MSI upgrade_code values during pre-insert and extends software_title lookup/mapping to match titles by upgrade_code in addition to name/bundle_identifier/source/extension_for, allowing different-named variants sharing an upgrade_code to link to the same software_title. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/software.go (1)
996-1071: Make upgrade_code matching case‑insensitive to avoid orphaned titles.The new upgrade_code path compares raw strings. If hosts report GUIDs with different casing, MySQL’s case‑insensitive collation will still return the title, but the Go‑side comparison will fail and the software can be inserted with a
NULLtitle_id (reintroducing the log noise this PR aims to fix). Normalize upgrade_code before comparison (and consider the same normalization inreconcileExistingTitleEmptyWindowsUpgradeCodesconflict checks).🛠️ Suggested fix (normalize upgrade_code before comparing)
- var titleSummaryUpgradeCode string - if titleSummary.UpgradeCode != nil { - titleSummaryUpgradeCode = *titleSummary.UpgradeCode - } + var titleSummaryUpgradeCode string + if titleSummary.UpgradeCode != nil { + titleSummaryUpgradeCode = strings.ToLower(*titleSummary.UpgradeCode) + } for checksum, title := range newTitlesNeeded { var titleBundleID string if title.BundleIdentifier != nil { titleBundleID = *title.BundleIdentifier } - var titleUpgradeCode string - if title.UpgradeCode != nil { - titleUpgradeCode = *title.UpgradeCode - } + var titleUpgradeCode string + if title.UpgradeCode != nil { + titleUpgradeCode = strings.ToLower(*title.UpgradeCode) + } // For apps with bundle_identifier, match by bundle_identifier (since we may have picked a different name) // For Windows programs with upgrade_code, match by upgrade_code (names may differ between versions) // For others, match by name (case-insensitive to match MySQL collation) // We also trim whitespace to handle data quality issues (e.g., trailing spaces). nameMatches := strings.EqualFold(strings.TrimSpace(titleSummary.Name), strings.TrimSpace(title.Name)) if bundleID != "" && titleBundleID != "" { // Both have bundle_identifier - match by bundle_identifier instead of name nameMatches = true } - if titleUpgradeCode != "" && titleSummaryUpgradeCode != "" && titleUpgradeCode == titleSummaryUpgradeCode { + if titleUpgradeCode != "" && titleSummaryUpgradeCode != "" && titleUpgradeCode == titleSummaryUpgradeCode { // Both have non-empty upgrade_code and they match: consider it a match // This handles the case where different versions of Windows software have // different names (e.g., "7-Zip 24.08 (x64)" vs "7-Zip 24.09 (x64 edition)") but share the same upgrade_code nameMatches = true }
🧹 Nitpick comments (1)
server/datastore/mysql/software_upgrade_code_test.go (1)
465-551: Consider covering case‑only upgrade_code differences.This test validates identical upgrade_code values with the same casing. Adding a quick variant where the second host reports a differently cased GUID would guard against case‑sensitivity regressions.
| // no-op code change | ||
| // TODO(jacob) - this var name is shadowing the one in the outer scope. Is this successfully | ||
| // adding titles-by-checksum for software with bundle ids? | ||
| incomingChecksumsToTitleSummaries = make(map[string]fleet.SoftwareTitleSummary, len(newSoftwareChecksums)) |
There was a problem hiding this comment.
This was clearing the values populated above.
2 participants
Related issue: Resolves #36494
Fixes:
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.