Allow multiple packages - migration#48596
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/28108-multiple-custom-packages #48596 +/- ##
=======================================================================
+ Coverage 67.99% 68.03% +0.03%
=======================================================================
Files 3678 3679 +1
Lines 233791 233925 +134
Branches 12305 12305
=======================================================================
+ Hits 158968 159146 +178
+ Misses 60496 60460 -36
+ Partials 14327 14319 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Replace the version-unique key on software_installers with a regime-aware unique key. A new VIRTUAL dedup_token column resolves to storage_id for custom packages (dedupe by content hash, so Arm and Intel builds of one version coexist while identical bytes are rejected) and to version for FMA packages (version-uniqueness unchanged, so the auto-update cron can cache several versions backed by the same bytes). The migration keeps the first-added row per (global_or_team_id, title_id, dedup_token), re-points policies off the removed rows, and deletes the rest before adding UNIQUE KEY idx_software_installers_dedup.
…48396) Allow multiple custom packages per title in MatchOrCreateSoftwareInstaller. checkSoftwareConflictsByIdentifier keeps the cross-type VPP/in-house checks and the FMA-vs-custom single-regime guard, then rejects a custom package whose dedup_token (content hash) already backs the title and enforces a 10-package limit. The conflict copy now distinguishes VPP apps, Fleet-maintained apps, and software packages. The cross-type and FMA checks run before getOrGenerateSoftwareInstallerTitleID so a rejected upload never triggers its title-create/rename side effects.
…ion fixes (#48396) GetSoftwareInstallerMetadataByTeamAndTitleID now returns the first-added active package. Add GetSoftwarePackagesByTeamAndTitleID returning every active package on a title with per-package label scope, for the API and precedence sub-issues. Fix the regressions that multiple active rows cause: SoftwareTitleByID counts use COUNT(DISTINCT) so the installer/VPP/in-house cross-join no longer inflates them, both ListSoftwareTitles query paths join only the first-added active installer so a title appears once (hash and package-name filters use EXISTS over all packages), and the edit path no longer rejects a title with more than one package.
127e5ff to
798d435
Compare
|
I think the test failure is unrelated, and looks like PR 48613 will fix it. |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughChangesThis PR adds support for multiple custom packages per software title. A new migration adds a Sequence Diagram(s)Diagrams included in the hidden review stack artifact illustrate the migration flow, conflict detection flow, and package listing flow. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)ee/server/service/software_installers.go[{"text":"tx.Exec(fmt.Sprintf( ... [truncated 38263 characters] ... style":"secondary"},{"text":"Sprintf","range":{"byteOffset":{"start":125221,"end":125228},"start":{"line":3504,"column":37},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf","range":{"byteOffset":{"start":125217,"end":125228},"start":{"line":3504,"column":33},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues)","range":{"byteOffset":{"start":125217,"end":125279},"start":{"line":3504,"column":33},"end":{"line":3504,"column":95}},"style":"secondary"},{"text":"(ctx, fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues), upsertCategoriesArgs...)","range":{"byteOffset":{"start":125211,"end":125305},"start":{"line":3504,"column":27},"end":{"line":3504,"column":121}},"style":"secondary"}]} server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.go[{"text":"tx.Exec(fmt.Sprintf( ... [truncated 38263 characters] ... style":"secondary"},{"text":"Sprintf","range":{"byteOffset":{"start":125221,"end":125228},"start":{"line":3504,"column":37},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf","range":{"byteOffset":{"start":125217,"end":125228},"start":{"line":3504,"column":33},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues)","range":{"byteOffset":{"start":125217,"end":125279},"start":{"line":3504,"column":33},"end":{"line":3504,"column":95}},"style":"secondary"},{"text":"(ctx, fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues), upsertCategoriesArgs...)","range":{"byteOffset":{"start":125211,"end":125305},"start":{"line":3504,"column":27},"end":{"line":3504,"column":121}},"style":"secondary"}]} server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle_test.go[{"text":"tx.Exec(fmt.Sprintf( ... [truncated 38263 characters] ... style":"secondary"},{"text":"Sprintf","range":{"byteOffset":{"start":125221,"end":125228},"start":{"line":3504,"column":37},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf","range":{"byteOffset":{"start":125217,"end":125228},"start":{"line":3504,"column":33},"end":{"line":3504,"column":44}},"style":"secondary"},{"text":"fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues)","range":{"byteOffset":{"start":125217,"end":125279},"start":{"line":3504,"column":33},"end":{"line":3504,"column":95}},"style":"secondary"},{"text":"(ctx, fmt.Sprintf(upsertInstallerCategories, upsertCategoriesValues), upsertCategoriesArgs...)","range":{"byteOffset":{"start":125211,"end":125305},"start":{"line":3504,"column":27},"end":{"line":3504,"column":121}},"style":"secondary"}]}
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle_test.go (1)
72-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for mixed active/inactive duplicate groups.
All duplicate-group fixtures here use
is_active = 1for every row. Consider adding a case where the lower-id row is inactive and the higher-id row is active (samededup_token), and assert the active row survives — this would catch the survivor-selection issue flagged in the migration file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle_test.go` around lines 72 - 92, Add a test fixture in MultipleCustomPackagesPerTitle_test that covers a duplicate group with mixed active/inactive rows sharing the same dedup_token. Create the group so the lower-id row is inactive and the higher-id row is active, then run the migration path that deduplicates installers. Assert the active installer is the one retained and any references are repointed to it, using the existing helpers like insertInstaller, execNoErr, and the policy re-point checks to locate the relevant assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.go`:
- Around line 26-57: The duplicate-collapsing logic in the migration currently
uses dupGroups with MIN(id) as keep_id, which can preserve an inactive
software_installers row and delete the active one. Update the dedup selection in
this migration so the retained row prefers the active installer (is_active = 1)
when duplicates share global_or_team_id, title_id, and dedup_token, and only
falls back to the lowest id if no active row exists; keep the subsequent
policies re-pointing and delete statements using the chosen keep_id.
In `@server/datastore/mysql/software_installers.go`:
- Around line 493-520: The fallback lookup in softwareInstallerTitleSelect can
still match non-plain software titles because it only filters by name, source,
and extension_for. Update the default branch used by
getExistingSoftwareInstallerTitleID to restrict matches to plain titles only, so
sqlx.GetContext does not return an arbitrary bundle/app/upgrade-code-backed
SoftwareTitle with the same name. Use the softwareInstallerTitleSelect and
getExistingSoftwareInstallerTitleID symbols to locate the change.
---
Nitpick comments:
In
`@server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle_test.go`:
- Around line 72-92: Add a test fixture in MultipleCustomPackagesPerTitle_test
that covers a duplicate group with mixed active/inactive rows sharing the same
dedup_token. Create the group so the lower-id row is inactive and the higher-id
row is active, then run the migration path that deduplicates installers. Assert
the active installer is the one retained and any references are repointed to it,
using the existing helpers like insertInstaller, execNoErr, and the policy
re-point checks to locate the relevant assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cee9533-bf90-4bf5-8c5e-95ed606771e0
⛔ Files ignored due to path filters (1)
server/datastore/mysql/testdata/select_software_titles_sql_fixture.gzis excluded by!**/*.gz
📒 Files selected for processing (15)
ee/server/service/software_installers.goserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.goserver/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/software_titles_test.goserver/fleet/datastore.goserver/fleet/errors.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/service/integration_enterprise_test.goserver/service/integration_vpp_install_test.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/in_house_apps.go
| // Collapse rows that would violate the new key: keep the lowest id per group and delete | ||
| // the rest. Re-point policies off the deleted rows first, since | ||
| // policies.software_installer_id is RESTRICT. Keep policies.updated_at so this | ||
| // content-identical swap doesn't read as a policy edit. | ||
| const dupGroups = ` | ||
| SELECT global_or_team_id, title_id, dedup_token, MIN(id) AS keep_id | ||
| FROM software_installers | ||
| WHERE title_id IS NOT NULL | ||
| GROUP BY global_or_team_id, title_id, dedup_token | ||
| HAVING COUNT(*) > 1` | ||
|
|
||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| UPDATE policies p | ||
| JOIN software_installers si ON si.id = p.software_installer_id | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| SET p.software_installer_id = dup.keep_id, p.updated_at = p.updated_at | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("re-pointing policies off duplicate installers: %w", err) | ||
| } | ||
|
|
||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| DELETE si FROM software_installers si | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("deleting duplicate installers: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the migration file
ast-grep outline server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.go --view expanded
echo '--- MIGRATION ---'
sed -n '1,220p' server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.go
echo '--- TESTS ---'
fd -a '20260629163945_MultipleCustomPackagesPerTitle*' server/datastore/mysql | while read -r f; do
echo "### $f"
sed -n '1,260p' "$f"
done
echo '--- is_active READ PATHS ---'
rg -n "is_active\s*=\s*1|GetSoftwareInstallerMetadataByTeamAndTitleID|GetSoftwarePackagesByTeamAndTitleID" server/datastore/mysqlRepository: fleetdm/fleet
Length of output: 21345
Prefer the active installer when collapsing duplicates.
MIN(id) can keep an inactive row and delete the active one if a duplicate pair has mixed is_active values. That leaves the title with no row visible to lookups that require is_active = 1. Pick the active row as keep_id instead of always taking the lowest id.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 37-45: SQL query built via fmt.Sprintf or string concatenation passed to a database method. Use parameterized queries with placeholder arguments.
(coderabbit.sql-injection.go-query-format)
[ERROR] 49-55: SQL query built via fmt.Sprintf or string concatenation passed to a database method. Use parameterized queries with placeholder arguments.
(coderabbit.sql-injection.go-query-format)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@server/datastore/mysql/migrations/tables/20260629163945_MultipleCustomPackagesPerTitle.go`
around lines 26 - 57, The duplicate-collapsing logic in the migration currently
uses dupGroups with MIN(id) as keep_id, which can preserve an inactive
software_installers row and delete the active one. Update the dedup selection in
this migration so the retained row prefers the active installer (is_active = 1)
when duplicates share global_or_team_id, title_id, and dedup_token, and only
falls back to the lowest id if no active row exists; keep the subsequent
policies re-pointing and delete statements using the chosen keep_id.
There was a problem hiding this comment.
claude:
Finding 1 (migration keeps inactive over active)** — VALID, will fix. keep_id = MIN(id) ignores is_active, but the reads (GetSoftwareInstallerMetadataByTeamAndTitleID) filter is_active = 1 ORDER BY id ASC. So if a same-dedup_token group ever has a lower-id inactive row and higher-id active row, the migration would delete the active one and the read would then return nothing. Cheap, strictly-better fix that keeps the migration consistent with the reads.
| func softwareInstallerTitleSelect(payload *fleet.UploadSoftwareInstallerPayload) (string, []any) { | ||
| switch { | ||
| case payload.BundleIdentifier != "": | ||
| // match by bundle identifier and source first, or standard matching if we don't have a bundle identifier match | ||
| return `SELECT id FROM software_titles WHERE (bundle_identifier = ? AND source = ?) OR (name = ? AND source = ? AND extension_for = '') ORDER BY bundle_identifier = ? DESC LIMIT 1`, | ||
| []any{payload.BundleIdentifier, payload.Source, payload.Title, payload.Source, payload.BundleIdentifier} | ||
| case payload.Source == "programs" && payload.UpgradeCode != "": | ||
| // select by either name or upgrade code, preferring upgrade code | ||
| return `SELECT id FROM software_titles WHERE (name = ? AND source = ? AND extension_for = '' AND upgrade_code = '') OR upgrade_code = ? ORDER BY upgrade_code = ? DESC LIMIT 1`, | ||
| []any{payload.Title, payload.Source, payload.UpgradeCode, payload.UpgradeCode} | ||
| default: | ||
| return `SELECT id FROM software_titles WHERE name = ? AND source = ? AND extension_for = ''`, | ||
| []any{payload.Title, payload.Source} | ||
| } | ||
| } | ||
|
|
||
| func (ds *Datastore) getExistingSoftwareInstallerTitleID(ctx context.Context, payload *fleet.UploadSoftwareInstallerPayload) (uint, error) { | ||
| stmt, args := softwareInstallerTitleSelect(payload) | ||
| var titleID uint | ||
| switch err := sqlx.GetContext(ctx, ds.reader(ctx), &titleID, stmt, args...); { | ||
| case err == nil: | ||
| return titleID, nil | ||
| case errors.Is(err, sql.ErrNoRows): | ||
| return 0, notFound("SoftwareTitle") | ||
| default: | ||
| return 0, ctxerr.Wrap(ctx, err, "get existing software installer title id") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "CREATE TABLE \`software_titles\`" -A 40 server/datastore/mysql/schema.sqlRepository: fleetdm/fleet
Length of output: 3276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate software_titles insert/upsert paths and package-limit query.
rg -n "software_titles|software_installers|is_active = 1|COUNT\(\*\) FROM software_installers|10 packages|package limit" server/datastore/mysql -g'*.go' -g'*.sql'
# Map the relevant file for targeted reading.
ast-grep outline server/datastore/mysql/software_installers.go --view expanded || trueRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "software_titles|software_installers|is_active = 1|COUNT\(\*\) FROM software_installers|10 packages|package limit" server/datastore/mysql -g'*.go' -g'*.sql'
ast-grep outline server/datastore/mysql/software_installers.go --view expanded || trueRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant parts of the datastore code.
sed -n '300,520p' server/datastore/mysql/software_installers.go
# Find how software_titles rows are inserted/upserted elsewhere.
rg -n "INSERT INTO software_titles|ON DUPLICATE KEY UPDATE|software_titles" server/datastore/mysql -g'*.go'Repository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact code around the title lookup and package limit.
sed -n '480,530p' server/datastore/mysql/software_installers.go
sed -n '4230,4270p' server/datastore/mysql/software_installers.go
# Find any code that deactivates software installer rows.
rg -n "is_active\s*=\s*false|SET is_active|deactivate.*software_installers|soft.*delete.*software_installers" server/datastore/mysql -g'*.go'Repository: fleetdm/fleet
Length of output: 6694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the package-limit code around the reported lines.
sed -n '4228,4268p' server/datastore/mysql/software_installers.go
# Read the code that toggles active/inactive state.
sed -n '660,710p' server/datastore/mysql/software_installers.go
sed -n '3368,3392p' server/datastore/mysql/software_installers.goRepository: fleetdm/fleet
Length of output: 4833
Scope the fallback title lookup to plain titles
software_titles is only unique on unique_identifier, source, extension_for, so WHERE name = ? AND source = ? AND extension_for = '' can still match a bundle/app/upgrade-code-backed title with the same name. sqlx.GetContext will then return an arbitrary row; narrow the fallback predicate to plain titles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/software_installers.go` around lines 493 - 520, The
fallback lookup in softwareInstallerTitleSelect can still match non-plain
software titles because it only filters by name, source, and extension_for.
Update the default branch used by getExistingSoftwareInstallerTitleID to
restrict matches to plain titles only, so sqlx.GetContext does not return an
arbitrary bundle/app/upgrade-code-backed SoftwareTitle with the same name. Use
the softwareInstallerTitleSelect and getExistingSoftwareInstallerTitleID symbols
to locate the change.
Source: Path instructions
There was a problem hiding this comment.
ignoring this, we don't want to change this logic. claude:
Finding 2 (default title-select matches non-plain titles)** — SKIP. That default query is main's unchanged title-matching logic, and it's shared with getOrGenerateSoftwareInstallerTitleID (the create path). getExisting must resolve the same title the create path will match/create, or the hash guard would check the wrong title. Restricting only the read breaks that invariant; restricting the shared query changes main's create behavior and risks duplicate titles — out of scope, and the collision (a plain package vs a bundle/upgrade-code title sharing name+source+extension_for='') predates this PR.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token |
There was a problem hiding this comment.
Should this re-point setup_experience_software_installers rows onto the surviving installer before deleting duplicates, since that FK is ON DELETE CASCADE (unlike policies, which you already addrewss here) and would otherwise silently drop a setup experience selection that lived only on a deleted row?
| WHERE si.global_or_team_id = ? AND st.source = ? AND st.bundle_identifier = ? | ||
| AND (si.fleet_maintained_app_id IS NOT NULL) = ? | ||
| )` | ||
| args = []any{ptr.ValOrZero(payload.TeamID), payload.Source, payload.BundleIdentifier, wantFMA} |
There was a problem hiding this comment.
Should we add an explicit non-empty BundleIdentifier guard here, just to be defensive?
Related issue: Resolves #48396
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Bug Fixes
Chores